Repository: kubernetes-sigs/cluster-api-provider-aws
Pull request #5321 (2025-01-24 07:50:14)

[release-2.6] 🐛: Tags defined in subnet spec should be applied
This is an automated cherry-pick of #5175

/assign damdo

```release-note
Tags defined in subnet spec should be applied
```

k8s-ci-robot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by:
**Once this PR has been reviewed and has the lgtm label**, please assign [dlipovetsky](https://github.com/dlipovetsky) for approval. For more information see [the Code Review Process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process).

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=kubernetes-sigs%2Fcluster-api-provider-aws).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/kubernetes-sigs/cluster-api-provider-aws/blob/release-2.6/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["dlipovetsky"]} -->
damdo: Hey @fiunchinho it looks like this automated backport of your #5175 PR is failing linting,
would you be able to have a look? Thanks!
damdo: /assign @fiunchinho 

Patch:
diff --git a/pkg/cloud/services/network/subnets.go b/pkg/cloud/services/network/subnets.go
index 62277cd9f..4cd826a87 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))


Test patch:
diff --git a/pkg/cloud/services/network/subnets_test.go b/pkg/cloud/services/network/subnets_test.go
index 447b1713b..7a97fc4f9 100644
--- a/pkg/cloud/services/network/subnets_test.go
+++ b/pkg/cloud/services/network/subnets_test.go
@@ -63,9 +63,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(stubSubnetsAvailabilityZone, stubSubnetsLocalZone...)
 	stubSubnetsAllZones = append(stubSubnetsAllZones, stubSubnetsWavelengthZone...)
 
 	// NetworkSpec with subnets in zone type availability-zone
@@ -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("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().