Skip to content

Commit

Permalink
🐛: Tags defined in subnet spec should be applied
Browse files Browse the repository at this point in the history
  • Loading branch information
@fiunchinho
fiunchinho committed Oct 23, 2024
1 parent b99a49c commit c8eb479
Show file tree
Hide file tree
Showing 2 changed files with 186 additions and 69 deletions.
22 changes: 6 additions & 16 deletions pkg/cloud/services/network/subnets.go
View file
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.
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

// 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
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",
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

0 comments on commit c8eb479

Please sign in to comment.