From c8e12ac293024866de1da0083bcfb890a0fd7752 Mon Sep 17 00:00:00 2001 From: Jose Armesto Date: Wed, 23 Oct 2024 00:05:55 +0200 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B:=20Tags=20defined=20in=20subnet=20?= =?UTF-8?q?spec=20should=20be=20applied?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- pkg/cloud/services/network/subnets.go | 22 +- pkg/cloud/services/network/subnets_test.go | 233 ++++++++++++++++----- 2 files changed, 186 insertions(+), 69 deletions(-) diff --git a/pkg/cloud/services/network/subnets.go b/pkg/cloud/services/network/subnets.go index 62277cd9fb..4cd826a879 100644 --- a/pkg/cloud/services/network/subnets.go +++ b/pkg/cloud/services/network/subnets.go @@ -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 { @@ -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. + if existing, err = s.describeVpcSubnets(); err != nil { + return err } if s.scope.SecondaryCidrBlock() != nil { @@ -139,11 +128,12 @@ func (s *Service) reconcileSubnets() error { existingSubnet.ID = sub.ID } + // Make sure tags are up-to-date. + subnetTags := sub.Tags + // 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)) diff --git a/pkg/cloud/services/network/subnets_test.go b/pkg/cloud/services/network/subnets_test.go index 447b1713b8..dd079cfd4f 100644 --- a/pkg/cloud/services/network/subnets_test.go +++ b/pkg/cloud/services/network/subnets_test.go @@ -21,6 +21,7 @@ import ( "encoding/json" "fmt" "reflect" + "slices" "testing" "github.com/aws/aws-sdk-go/aws" @@ -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{ @@ -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, }, @@ -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", + 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("shared"), + }, + { + 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("shared"), + }, + { + 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("shared"), + }, + { + 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("shared"), + }, + { + 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().