Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🐛: Tags defined in subnet spec should be applied #5175

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

🐛: Tags defined in subnet spec should be applied #5175

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 6 additions & 16 deletions pkg/cloud/services/network/subnets.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,14 +58,6 @@ func (s *Service) reconcileSubnets() error {
existing infrav1.Subnets
)

// Describing the VPC Subnets tags the resources.
if s.scope.TagUnmanagedNetworkResources() {
// Describe subnets in the vpc.
if existing, err = s.describeVpcSubnets(); err != nil {
return err
}
}

unmanagedVPC := s.scope.VPC().IsUnmanaged(s.scope.Name())

if len(subnets) == 0 {
Expand Down Expand Up @@ -93,12 +85,9 @@ func (s *Service) reconcileSubnets() error {
}
}

// Describing the VPC Subnets tags the resources.
if !s.scope.TagUnmanagedNetworkResources() {
// Describe subnets in the vpc.
if existing, err = s.describeVpcSubnets(); err != nil {
return err
}
// Describe subnets in the vpc.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We had two blocks to describe the subnets in the VPC. One was executed when the feature flag for tagging unmanaged network resources was enabled, and the other block was executed when the feature flag was disabled. So it was always executed anyway.

if existing, err = s.describeVpcSubnets(); err != nil {
return err
}

if s.scope.SecondaryCidrBlock() != nil {
Expand Down Expand Up @@ -139,11 +128,12 @@ func (s *Service) reconcileSubnets() error {
existingSubnet.ID = sub.ID
}

// Make sure tags are up-to-date.
subnetTags := sub.Tags

Comment on lines +131 to +133
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the key change that fixes the issue. By storing the subnet tags from the spec to a local variable, we won't overwrite them when using DeepCopyInto.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @fiunchinho - is this behavior considering managed and unmanaged scenarios?

Checking the documentation about BYO/unmanaged and tags, it states:

Note: All the tagging of resources should be the responsibility of the users and are not managed by CAPA controllers.

My understanding is this block will be reached at once when unmanaged (existingSubnets!=nil), and later interactions in the reconcile loop when managed (with subnets created here , preserving subnets tags defined on spec), considering the documentation that unmanaged should not create additional tags, than required from k8s, the tags from spec should be skipped. Is it make sense? (+ @richardcase @nrb would love to hear your thoughts too)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey @mtulio thanks for the comment. If I understand it correctly, with this approach, when reconciling an unmanaged vpc, the tags in the subnet on AWS are applied to the AWSCluster spec, and new tags in the AWSCluster spec are ignored/never applied into AWS. When reconciling a managed VPC, the tags in the AWSCluster spec are applied to the subnet on AWS.

In the subnets.go file, line 626, we only take into consideration the tags defined in the AWSCluster spec (called manualTags in the function) if the vpc is managed.

So I think this approach is not going against current documentation of BYOI/unmanaged infra.

Also, I think finding the subnet in AWS (existingSubnets!=nil) doesn't mean it's unmanaged. When CAPA creates the subnets, on following reconciliations it will find the same subnets on AWS, and they are managed by CAPA. A managed VPC is defined as a VPC with an id set, and the sigs.k8s.io/cluster-api-provider-aws/cluster/$clusterName set.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey @mtulio thanks for the comment. If I understand it correctly, with this approach, when reconciling an unmanaged vpc, the tags in the subnet on AWS are applied to the AWSCluster spec, and new tags in the AWSCluster spec are ignored/never applied into AWS. [...]

Yep, that's the current behavior, isn't it aligned with the documentation for BYOI/unmanaged infra?

The change wasn't intentionally added, but revisiting the docs with your findings I am a little bit confused what would be the expected behavior for unmanaged on CAPA. @richardcase @nrb thoughts?

Just sharing some thoughts how we are using: we, on OpenShift, consider tags are managed by the user when they opt to provide their own network/subnets, and we don't touch on it when installing a cluster. So this change won't affect our automation as we are not setting the subnet tags for unmanaged VPC.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For unmanaged, I'd say:

  • Only apply required k8s tags to the subnets in AWS
  • No other custom tags to be added to the subnets in AWS

As for syncing existing tags on the subnets back to AWSCluster, I'm not strongly opinionated on this.

Every time i look at the subnets code, it makes me 😢 IMHO, it's historically one of the most error-prone and horrible bits of code in CAPA.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getSubnetTagParams still does the right thing for unmanaged VPCs (= don't use additional tags unless TagUnmanagedNetworkResources()), so I think the behavior is fine even with this PR. If tags are directly defined on AWSCluster.spec.network.subnets.tags, it's a conscious decision by the admin and those tags should be added even for unmanaged ones – why else have a field for tags?

The field description could be more precise, though (out of scope for the PR):

$ kubectl explain awscluster.spec.network.subnets.tags
GROUP:      infrastructure.cluster.x-k8s.io
KIND:       AWSCluster
VERSION:    v1beta2

FIELD: tags <map[string]string>


DESCRIPTION:
    Tags is a collection of tags describing the resource.

// Update subnet spec with the existing subnet details
existingSubnet.DeepCopyInto(sub)

// Make sure tags are up-to-date.
subnetTags := sub.Tags
if err := wait.WaitForWithRetryable(wait.NewBackoff(), func() (bool, error) {
buildParams := s.getSubnetTagParams(unmanagedVPC, existingSubnet.GetResourceID(), existingSubnet.IsPublic, existingSubnet.AvailabilityZone, subnetTags, existingSubnet.IsEdge())
tagsBuilder := tags.New(&buildParams, tags.WithEC2(s.EC2Client))
Expand Down
233 changes: 180 additions & 53 deletions pkg/cloud/services/network/subnets_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"encoding/json"
"fmt"
"reflect"
"slices"
"testing"

"github.com/aws/aws-sdk-go/aws"
Expand Down Expand Up @@ -63,10 +64,7 @@ func TestReconcileSubnets(t *testing.T) {
{ID: "subnet-private-us-east-1-wl1-nyc-wlz-1", AvailabilityZone: "us-east-1-wl1-nyc-wlz-1", CidrBlock: "10.0.7.0/24", IsPublic: false},
{ID: "subnet-public-us-east-1-wl1-nyc-wlz-1", AvailabilityZone: "us-east-1-wl1-nyc-wlz-1", CidrBlock: "10.0.8.0/24", IsPublic: true},
}
// TODO(mtulio): replace by slices.Concat(...) on go 1.22+
stubSubnetsAllZones := stubSubnetsAvailabilityZone
stubSubnetsAllZones = append(stubSubnetsAllZones, stubSubnetsLocalZone...)
stubSubnetsAllZones = append(stubSubnetsAllZones, stubSubnetsWavelengthZone...)
stubSubnetsAllZones := slices.Concat(stubSubnetsAvailabilityZone, stubSubnetsLocalZone, stubSubnetsWavelengthZone)

// NetworkSpec with subnets in zone type availability-zone
stubNetworkSpecWithSubnets := &infrav1.NetworkSpec{
Expand Down Expand Up @@ -1057,55 +1055,7 @@ func TestReconcileSubnets(t *testing.T) {
},
Subnets: []infrav1.SubnetSpec{},
}).WithTagUnmanagedNetworkResources(true),
expect: func(m *mocks.MockEC2APIMockRecorder) {
m.DescribeSubnetsWithContext(context.TODO(), gomock.Eq(&ec2.DescribeSubnetsInput{
Filters: []*ec2.Filter{
{
Name: aws.String("state"),
Values: []*string{aws.String("pending"), aws.String("available")},
},
{
Name: aws.String("vpc-id"),
Values: []*string{aws.String(subnetsVPCID)},
},
},
})).
Return(&ec2.DescribeSubnetsOutput{
Subnets: []*ec2.Subnet{
{
VpcId: aws.String(subnetsVPCID),
SubnetId: aws.String("subnet-1"),
AvailabilityZone: aws.String("us-east-1a"),
CidrBlock: aws.String("10.0.10.0/24"),
MapPublicIpOnLaunch: aws.Bool(false),
},
{
VpcId: aws.String(subnetsVPCID),
SubnetId: aws.String("subnet-2"),
AvailabilityZone: aws.String("us-east-1a"),
CidrBlock: aws.String("10.0.20.0/24"),
MapPublicIpOnLaunch: aws.Bool(false),
},
},
}, nil)
m.DescribeRouteTablesWithContext(context.TODO(), gomock.AssignableToTypeOf(&ec2.DescribeRouteTablesInput{})).
Return(&ec2.DescribeRouteTablesOutput{}, nil)

m.DescribeNatGatewaysPagesWithContext(context.TODO(),
gomock.Eq(&ec2.DescribeNatGatewaysInput{
Filter: []*ec2.Filter{
{
Name: aws.String("vpc-id"),
Values: []*string{aws.String(subnetsVPCID)},
},
{
Name: aws.String("state"),
Values: []*string{aws.String("pending"), aws.String("available")},
},
},
}),
gomock.Any()).Return(nil)
},
expect: func(m *mocks.MockEC2APIMockRecorder) {},
errorExpected: true,
tagUnmanagedNetworkResources: true,
},
Expand Down Expand Up @@ -2625,6 +2575,183 @@ func TestReconcileSubnets(t *testing.T) {
}, nil).AnyTimes()
},
},
{
name: "Managed VPC, existing public and private subnets, 2 subnets in spec, custom tags in spec should be created",
Copy link
Contributor

@mtulio mtulio Oct 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just making sure if I understood the scenario: is "Managed VPC" covers "existing subnets" on CAPA?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In BYO infra / Configuring the AWSCluster Specification it states:

Specifying existing infrastructure for Cluster API to use takes place in the specification for the AWSCluster object. Specifically, you will need to add an entry with the VPC ID and the IDs of all applicable subnets into the network field.

So, should it be "unmanaged VPC" when you specify the VPC ID and subnet IDs?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried explaining here. Let me know your thoughts, please!

input: NewClusterScope().WithNetwork(&infrav1.NetworkSpec{
VPC: infrav1.VPCSpec{
ID: subnetsVPCID,
Tags: infrav1.Tags{
infrav1.ClusterTagKey("test-cluster"): "owned",
},
},
Subnets: []infrav1.SubnetSpec{
{
ID: "subnet-1",
AvailabilityZone: "us-east-1a",
IsPublic: true,
Tags: map[string]string{"this-tag-is-in-the-spec": "but-its-not-on-aws"},
},
{
ID: "subnet-2",
AvailabilityZone: "us-east-1a",
IsPublic: false,
Tags: map[string]string{"subnet-2-this-tag-is-in-the-spec": "subnet-2-but-its-not-on-aws"},
},
},
}),
expect: func(m *mocks.MockEC2APIMockRecorder) {
tagsOnSubnet1 := []*ec2.Tag{
{
Key: aws.String("Name"),
Value: aws.String("test-cluster-subnet-public"),
},
{
Key: aws.String("kubernetes.io/cluster/test-cluster"),
Value: aws.String("owned"),
},
{
Key: aws.String("sigs.k8s.io/cluster-api-provider-aws/role"),
Value: aws.String("public"),
},
}
tagsOnSubnet2 := []*ec2.Tag{
{
Key: aws.String("Name"),
Value: aws.String("test-cluster-subnet-private"),
},
{
Key: aws.String("kubernetes.io/cluster/test-cluster"),
Value: aws.String("owned"),
},
{
Key: aws.String("sigs.k8s.io/cluster-api-provider-aws/role"),
Value: aws.String("private"),
},
}
m.DescribeSubnetsWithContext(context.TODO(), gomock.Eq(&ec2.DescribeSubnetsInput{
Filters: []*ec2.Filter{
{
Name: aws.String("state"),
Values: []*string{aws.String("pending"), aws.String("available")},
},
{
Name: aws.String("vpc-id"),
Values: []*string{aws.String(subnetsVPCID)},
},
},
})).
Return(&ec2.DescribeSubnetsOutput{
Subnets: []*ec2.Subnet{
{
VpcId: aws.String(subnetsVPCID),
SubnetId: aws.String("subnet-1"),
AvailabilityZone: aws.String("us-east-1a"),
CidrBlock: aws.String("10.0.0.0/17"),
Tags: tagsOnSubnet1,
},
{
VpcId: aws.String(subnetsVPCID),
SubnetId: aws.String("subnet-2"),
AvailabilityZone: aws.String("us-east-1a"),
CidrBlock: aws.String("10.0.128.0/17"),
Tags: tagsOnSubnet2,
},
},
}, nil)

m.DescribeRouteTablesWithContext(context.TODO(), gomock.AssignableToTypeOf(&ec2.DescribeRouteTablesInput{})).
Return(&ec2.DescribeRouteTablesOutput{}, nil)

m.DescribeNatGatewaysPagesWithContext(context.TODO(),
gomock.Eq(&ec2.DescribeNatGatewaysInput{
Filter: []*ec2.Filter{
{
Name: aws.String("vpc-id"),
Values: []*string{aws.String(subnetsVPCID)},
},
{
Name: aws.String("state"),
Values: []*string{aws.String("pending"), aws.String("available")},
},
},
}),
gomock.Any()).Return(nil)

// Public subnet
expectedAppliedAwsTagsForSubnet1 := []*ec2.Tag{
{
Key: aws.String("Name"),
Value: aws.String("test-cluster-subnet-public-us-east-1a"),
},
{
Key: aws.String("kubernetes.io/cluster/test-cluster"),
Value: aws.String("owned"),
},
{
Key: aws.String("kubernetes.io/role/elb"),
Value: aws.String("1"),
},
{
Key: aws.String("sigs.k8s.io/cluster-api-provider-aws/cluster/test-cluster"),
Value: aws.String("owned"),
},
{
Key: aws.String("sigs.k8s.io/cluster-api-provider-aws/role"),
Value: aws.String("public"),
},
{
Key: aws.String("this-tag-is-in-the-spec"),
Value: aws.String("but-its-not-on-aws"),
}}
m.CreateTagsWithContext(context.TODO(), gomock.Eq(&ec2.CreateTagsInput{
Resources: aws.StringSlice([]string{"subnet-1"}),
Tags: expectedAppliedAwsTagsForSubnet1,
})).
Return(nil, nil)

// Private subnet
expectedAppliedAwsTagsForSubnet2 := []*ec2.Tag{
{
Key: aws.String("Name"),
Value: aws.String("test-cluster-subnet-private-us-east-1a"),
},
{
Key: aws.String("kubernetes.io/cluster/test-cluster"),
Value: aws.String("owned"),
},
{
Key: aws.String("kubernetes.io/role/internal-elb"),
Value: aws.String("1"),
},
{
Key: aws.String("sigs.k8s.io/cluster-api-provider-aws/cluster/test-cluster"),
Value: aws.String("owned"),
},
{
Key: aws.String("sigs.k8s.io/cluster-api-provider-aws/role"),
Value: aws.String("private"),
},
{
Key: aws.String("subnet-2-this-tag-is-in-the-spec"),
Value: aws.String("subnet-2-but-its-not-on-aws"),
}}
m.CreateTagsWithContext(context.TODO(), gomock.Eq(&ec2.CreateTagsInput{
Resources: aws.StringSlice([]string{"subnet-2"}),
Tags: expectedAppliedAwsTagsForSubnet2,
})).
Return(nil, nil)

m.DescribeAvailabilityZonesWithContext(context.TODO(), gomock.Any()).
Return(&ec2.DescribeAvailabilityZonesOutput{
AvailabilityZones: []*ec2.AvailabilityZone{
{
ZoneName: aws.String("us-east-1a"),
ZoneType: aws.String("availability-zone"),
},
},
}, nil).AnyTimes()
},
},
{
name: "With ManagedControlPlaneScope, Managed VPC, no existing subnets exist, two az's, expect two private and two public from default, created with tag including eksClusterName not a name of Cluster resource",
input: NewManagedControlPlaneScope().
Expand Down