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

Jump to bottom

🐛 fix: update network subnets prio tag failures #4917

Merged
merged 1 commit into from
Apr 11, 2024
Merged

🐛 fix: update network subnets prio tag failures #4917

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
24 changes: 11 additions & 13 deletions pkg/cloud/services/network/subnets.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,8 +133,17 @@ func (s *Service) reconcileSubnets() error {
sub := &subnets[i]
existingSubnet := existing.FindEqual(sub)
if existingSubnet != nil {
subnetTags := sub.Tags
if len(sub.ID) > 0 {
// NOTE: Describing subnets assumes the subnet.ID is the same as the subnet's identifier (i.e. subnet-<xyz>),
// if we have a subnet ID specified in the spec, we need to restore it.
existingSubnet.ID = sub.ID
}

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

// Make sure tags are up-to-date.
subnetTags := sub.Tags
Comment on lines +142 to +146
Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed that tags were not being applied when running CAPA v2.5.2, and I found this PR changing the subnet tagging behaviour.

Shouldn't subnetTags := sub.Tags run before existingSubnet.DeepCopyInto(sub)? Otherwise, we basically ignore the tags specified in the subnet spec, because they are overwritten by the tags found in the AWS Subnet. /cc @mtulio @vincepri @damdo

Copy link
Member

Choose a reason for hiding this comment

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

Yeah looks that way. Although not looked too deeply into the code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Created this to fix it #5175

if err := wait.WaitForWithRetryable(wait.NewBackoff(), func() (bool, error) {
buildParams := s.getSubnetTagParams(unmanagedVPC, existingSubnet.GetResourceID(), existingSubnet.IsPublic, existingSubnet.AvailabilityZone, subnetTags)
tagsBuilder := tags.New(&buildParams, tags.WithEC2(s.EC2Client))
Expand All @@ -151,19 +160,8 @@ func (s *Service) reconcileSubnets() error {
// We may not have a permission to tag unmanaged subnets.
// When tagging unmanaged subnet fails, record an event and proceed.
record.Warnf(s.scope.InfraCluster(), "FailedTagSubnet", "Failed tagging unmanaged Subnet %q: %v", existingSubnet.GetResourceID(), err)
break
}

// TODO(vincepri): check if subnet needs to be updated.

if len(sub.ID) > 0 {
// NOTE: Describing subnets assumes the subnet.ID is the same as the subnet's identifier (i.e. subnet-<xyz>),
// if we have a subnet ID specified in the spec, we need to restore it.
existingSubnet.ID = sub.ID
continue
}

// Update subnet spec with the existing subnet details
existingSubnet.DeepCopyInto(sub)
} else if unmanagedVPC {
// If there is no existing subnet and we have an umanaged vpc report an error
record.Warnf(s.scope.InfraCluster(), "FailedMatchSubnet", "Using unmanaged VPC and failed to find existing subnet for specified subnet id %d, cidr %q", sub.GetResourceID(), sub.CidrBlock)
Expand Down
235 changes: 232 additions & 3 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 @@ -47,7 +47,9 @@ func TestReconcileSubnets(t *testing.T) {
input ScopeBuilder
expect func(m *mocks.MockEC2APIMockRecorder)
errorExpected bool
errorMessageExpected string
tagUnmanagedNetworkResources bool
optionalExpectSubnets infrav1.Subnets
}{
{
name: "Unmanaged VPC, disable TagUnmanagedNetworkResources, 2 existing subnets in vpc, 2 subnet in spec, subnets match, with routes, should succeed",
Expand Down Expand Up @@ -486,6 +488,194 @@ func TestReconcileSubnets(t *testing.T) {
},
{
name: "Unmanaged VPC, 2 existing matching subnets, subnet tagging fails, should succeed",
input: NewClusterScope().WithNetwork(&infrav1.NetworkSpec{
VPC: infrav1.VPCSpec{
ID: subnetsVPCID,
},
Subnets: []infrav1.SubnetSpec{
{
ID: "subnet-1",
},
},
}).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),
},
},
}, nil)

m.DescribeRouteTablesWithContext(context.TODO(), gomock.AssignableToTypeOf(&ec2.DescribeRouteTablesInput{})).
Return(&ec2.DescribeRouteTablesOutput{
RouteTables: []*ec2.RouteTable{
{
VpcId: aws.String(subnetsVPCID),
Associations: []*ec2.RouteTableAssociation{
{
SubnetId: aws.String("subnet-1"),
RouteTableId: aws.String("rt-12345"),
},
},
Routes: []*ec2.Route{
{
GatewayId: aws.String("igw-12345"),
},
},
},
},
}, 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)

m.CreateTagsWithContext(context.TODO(), gomock.Eq(&ec2.CreateTagsInput{
Resources: aws.StringSlice([]string{"subnet-1"}),
Tags: []*ec2.Tag{
{
Key: aws.String("kubernetes.io/cluster/test-cluster"),
Value: aws.String("shared"),
},
{
Key: aws.String("kubernetes.io/role/elb"),
Value: aws.String("1"),
},
},
})).
Return(&ec2.CreateTagsOutput{}, fmt.Errorf("tagging failed"))
},
tagUnmanagedNetworkResources: true,
},
{
name: "Unmanaged VPC, 2 existing matching subnets, subnet tagging fails with subnet update, should succeed",
input: NewClusterScope().WithNetwork(&infrav1.NetworkSpec{
VPC: infrav1.VPCSpec{
ID: subnetsVPCID,
},
Subnets: []infrav1.SubnetSpec{
{
ID: "subnet-1",
},
},
}).WithTagUnmanagedNetworkResources(true),
optionalExpectSubnets: infrav1.Subnets{
{
ID: "subnet-1",
ResourceID: "subnet-1",
AvailabilityZone: "us-east-1a",
CidrBlock: "10.0.10.0/24",
IsPublic: true,
Tags: infrav1.Tags{},
},
},
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),
},
},
}, nil)

m.DescribeRouteTablesWithContext(context.TODO(), gomock.AssignableToTypeOf(&ec2.DescribeRouteTablesInput{})).
Return(&ec2.DescribeRouteTablesOutput{
RouteTables: []*ec2.RouteTable{
{
VpcId: aws.String(subnetsVPCID),
Associations: []*ec2.RouteTableAssociation{
{
SubnetId: aws.String("subnet-1"),
RouteTableId: aws.String("rt-12345"),
},
},
Routes: []*ec2.Route{
{
GatewayId: aws.String("igw-12345"),
},
},
},
},
}, 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)

m.CreateTagsWithContext(context.TODO(), gomock.Eq(&ec2.CreateTagsInput{
Resources: aws.StringSlice([]string{"subnet-1"}),
Tags: []*ec2.Tag{
{
Key: aws.String("kubernetes.io/cluster/test-cluster"),
Value: aws.String("shared"),
},
{
Key: aws.String("kubernetes.io/role/elb"),
Value: aws.String("1"),
},
},
})).
Return(&ec2.CreateTagsOutput{}, fmt.Errorf("tagging failed"))
},
tagUnmanagedNetworkResources: true,
},
{
name: "Unmanaged VPC, 2 existing matching subnets, subnet tagging fails second call, should succeed",
input: NewClusterScope().WithNetwork(&infrav1.NetworkSpec{
VPC: infrav1.VPCSpec{
ID: subnetsVPCID,
Expand Down Expand Up @@ -524,7 +714,7 @@ func TestReconcileSubnets(t *testing.T) {
{
VpcId: aws.String(subnetsVPCID),
SubnetId: aws.String("subnet-2"),
AvailabilityZone: aws.String("us-east-1a"),
AvailabilityZone: aws.String("us-east-1b"),
CidrBlock: aws.String("10.0.20.0/24"),
MapPublicIpOnLaunch: aws.Bool(false),
},
Expand All @@ -548,6 +738,20 @@ func TestReconcileSubnets(t *testing.T) {
},
},
},
{
VpcId: aws.String(subnetsVPCID),
Associations: []*ec2.RouteTableAssociation{
{
SubnetId: aws.String("subnet-2"),
RouteTableId: aws.String("rt-22222"),
},
},
Routes: []*ec2.Route{
{
GatewayId: aws.String("igw-12345"),
},
},
},
},
}, nil)

Expand All @@ -566,7 +770,7 @@ func TestReconcileSubnets(t *testing.T) {
}),
gomock.Any()).Return(nil)

m.CreateTagsWithContext(context.TODO(), gomock.Eq(&ec2.CreateTagsInput{
secondSubnetTag := m.CreateTagsWithContext(context.TODO(), gomock.Eq(&ec2.CreateTagsInput{
Resources: aws.StringSlice([]string{"subnet-1"}),
Tags: []*ec2.Tag{
{
Expand All @@ -579,7 +783,22 @@ func TestReconcileSubnets(t *testing.T) {
},
},
})).
Return(&ec2.CreateTagsOutput{}, fmt.Errorf("tagging failed"))
Return(&ec2.CreateTagsOutput{}, nil)

m.CreateTagsWithContext(context.TODO(), gomock.Eq(&ec2.CreateTagsInput{
Resources: aws.StringSlice([]string{"subnet-2"}),
Tags: []*ec2.Tag{
{
Key: aws.String("kubernetes.io/cluster/test-cluster"),
Value: aws.String("shared"),
},
{
Key: aws.String("kubernetes.io/role/elb"),
Value: aws.String("1"),
},
},
})).
Return(&ec2.CreateTagsOutput{}, fmt.Errorf("tagging failed")).After(secondSubnetTag)
},
tagUnmanagedNetworkResources: true,
},
Expand Down Expand Up @@ -2341,6 +2560,16 @@ func TestReconcileSubnets(t *testing.T) {
if !tc.errorExpected && err != nil {
t.Fatalf("got an unexpected error: %v", err)
}
if tc.errorExpected && err != nil && len(tc.errorMessageExpected) > 0 {
if err.Error() != tc.errorMessageExpected {
t.Fatalf("got an unexpected error message: %v", err)
}
}
if len(tc.optionalExpectSubnets) > 0 {
if !cmp.Equal(s.scope.Subnets(), tc.optionalExpectSubnets) {
t.Errorf("got unexpect Subnets():\n%v", cmp.Diff(s.scope.Subnets(), tc.optionalExpectSubnets))
}
}
})
}
}
Expand Down
Loading