Skip to content

Commit 9b14493

Browse files
fiunchinhofiunchinho
committed
Don't put tags from AWS subnet into the subnetSpec
1 parent 8c2168b commit 9b14493

File tree

2 files changed

+27
-111
lines changed

2 files changed

+27
-111
lines changed

‎pkg/cloud/services/network/subnets.go‎

Lines changed: 15 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -58,26 +58,16 @@ func (s *Service) reconcileSubnets() error {
5858
existing infrav1.Subnets
5959
)
6060

61-
// Describing the VPC Subnets tags the resources.
62-
if s.scope.TagUnmanagedNetworkResources() {
63-
// Describe subnets in the vpc.
64-
if existing, err = s.describeVpcSubnets(); err != nil {
65-
return err
66-
}
67-
}
68-
6961
unmanagedVPC := s.scope.VPC().IsUnmanaged(s.scope.Name())
7062

71-
if len(subnets) == 0 {
72-
if unmanagedVPC {
73-
// If we have a unmanaged VPC then subnets must be specified
74-
errMsg := "no subnets specified, you must specify the subnets when using an umanaged vpc"
75-
record.Warnf(s.scope.InfraCluster(), "FailedNoSubnets", errMsg)
76-
return errors.New(errMsg)
77-
}
78-
79-
// If we a managed VPC and have no subnets then create subnets. There will be 1 public and 1 private subnet
80-
// for each az in a region up to a maximum of 3 azs
63+
// If we have a unmanaged VPC then subnets must be specified
64+
if unmanagedVPC && len(subnets) == 0 {
65+
errMsg := "no subnets specified, you must specify the subnets when using an umanaged vpc"
66+
record.Warnf(s.scope.InfraCluster(), "FailedNoSubnets", errMsg)
67+
return errors.New(errMsg)
68+
} else if len(subnets) == 0 {
69+
// If we have a managed VPC and have no subnets then create some default subnets.
70+
// There will be 1 public and 1 private subnet for each az in a region up to a maximum of 3 azs.
8171
s.scope.Info("no subnets specified, setting defaults")
8272

8373
subnets, err = s.getDefaultSubnets()
@@ -93,12 +83,9 @@ func (s *Service) reconcileSubnets() error {
9383
}
9484
}
9585

96-
// Describing the VPC Subnets tags the resources.
97-
if !s.scope.TagUnmanagedNetworkResources() {
98-
// Describe subnets in the vpc.
99-
if existing, err = s.describeVpcSubnets(); err != nil {
100-
return err
101-
}
86+
// Describe subnets in the vpc.
87+
if existing, err = s.describeVpcSubnets(); err != nil {
88+
return err
10289
}
10390

10491
if s.scope.SecondaryCidrBlock() != nil {
@@ -133,13 +120,10 @@ func (s *Service) reconcileSubnets() error {
133120
sub := &subnets[i]
134121
existingSubnet := existing.FindEqual(sub)
135122
if existingSubnet != nil {
136-
if len(sub.ID) > 0 {
137-
// NOTE: Describing subnets assumes the subnet.ID is the same as the subnet's identifier (i.e. subnet-<xyz>),
138-
// if we have a subnet ID specified in the spec, we need to restore it.
139-
existingSubnet.ID = sub.ID
140-
}
141-
142-
// Update subnet spec with the existing subnet details
123+
// Update subnet spec with the existing subnet details, but we want to keep the subnet ID and tags defined in the spec.
124+
// We don't want to mess with tags that exist only on AWS.
125+
existingSubnet.ID = sub.ID
126+
existingSubnet.Tags = sub.Tags
143127
existingSubnet.DeepCopyInto(sub)
144128

145129
// Make sure tags are up-to-date.
@@ -169,12 +153,6 @@ func (s *Service) reconcileSubnets() error {
169153
}
170154
}
171155

172-
// If we have an unmanaged VPC, require that the user has specified at least 1 subnet.
173-
if unmanagedVPC && len(subnets) < 1 {
174-
record.Warnf(s.scope.InfraCluster(), "FailedNoSubnet", "Expected at least 1 subnet but got 0")
175-
return errors.New("expected at least 1 subnet but got 0")
176-
}
177-
178156
// Reconciling the zone information for the subnets. Subnets are grouped
179157
// by regular zones (availability zones) or edge zones (local zones or wavelength zones)
180158
// based in the zone-type attribute for zone.

‎pkg/cloud/services/network/subnets_test.go‎

Lines changed: 12 additions & 74 deletions
Original file line numberDiff line numberDiff line change
@@ -18,9 +18,9 @@ package network
1818

1919
import (
2020
"context"
21-
"encoding/json"
2221
"fmt"
2322
"reflect"
23+
"slices"
2424
"testing"
2525

2626
"github.com/aws/aws-sdk-go/aws"
@@ -63,10 +63,7 @@ func TestReconcileSubnets(t *testing.T) {
6363
{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},
6464
{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},
6565
}
66-
// TODO(mtulio): replace by slices.Concat(...) on go 1.22+
67-
stubSubnetsAllZones := stubSubnetsAvailabilityZone
68-
stubSubnetsAllZones = append(stubSubnetsAllZones, stubSubnetsLocalZone...)
69-
stubSubnetsAllZones = append(stubSubnetsAllZones, stubSubnetsWavelengthZone...)
66+
stubSubnetsAllZones := slices.Concat(stubSubnetsAvailabilityZone, stubSubnetsLocalZone, stubSubnetsWavelengthZone)
7067

7168
// NetworkSpec with subnets in zone type availability-zone
7269
stubNetworkSpecWithSubnets := &infrav1.NetworkSpec{
@@ -678,7 +675,6 @@ func TestReconcileSubnets(t *testing.T) {
678675
AvailabilityZone: "us-east-1a",
679676
CidrBlock: "10.0.10.0/24",
680677
IsPublic: true,
681-
Tags: infrav1.Tags{},
682678
},
683679
},
684680
expect: func(m *mocks.MockEC2APIMockRecorder) {
@@ -702,6 +698,12 @@ func TestReconcileSubnets(t *testing.T) {
702698
AvailabilityZone: aws.String("us-east-1a"),
703699
CidrBlock: aws.String("10.0.10.0/24"),
704700
MapPublicIpOnLaunch: aws.Bool(false),
701+
Tags: []*ec2.Tag{
702+
{
703+
Key: aws.String("company-policy"),
704+
Value: aws.String("enabled"),
705+
},
706+
},
705707
},
706708
},
707709
}, nil)
@@ -784,15 +786,13 @@ func TestReconcileSubnets(t *testing.T) {
784786
AvailabilityZone: "us-east-1a",
785787
CidrBlock: "10.0.10.0/24",
786788
IsPublic: true,
787-
Tags: infrav1.Tags{},
788789
},
789790
{
790791
ID: "subnet-2",
791792
ResourceID: "subnet-2",
792793
AvailabilityZone: "us-east-1b",
793794
CidrBlock: "10.0.11.0/24",
794795
IsPublic: true,
795-
Tags: infrav1.Tags{},
796796
},
797797
},
798798
expect: func(m *mocks.MockEC2APIMockRecorder) {
@@ -1057,55 +1057,7 @@ func TestReconcileSubnets(t *testing.T) {
10571057
},
10581058
Subnets: []infrav1.SubnetSpec{},
10591059
}).WithTagUnmanagedNetworkResources(true),
1060-
expect: func(m *mocks.MockEC2APIMockRecorder) {
1061-
m.DescribeSubnetsWithContext(context.TODO(), gomock.Eq(&ec2.DescribeSubnetsInput{
1062-
Filters: []*ec2.Filter{
1063-
{
1064-
Name: aws.String("state"),
1065-
Values: []*string{aws.String("pending"), aws.String("available")},
1066-
},
1067-
{
1068-
Name: aws.String("vpc-id"),
1069-
Values: []*string{aws.String(subnetsVPCID)},
1070-
},
1071-
},
1072-
})).
1073-
Return(&ec2.DescribeSubnetsOutput{
1074-
Subnets: []*ec2.Subnet{
1075-
{
1076-
VpcId: aws.String(subnetsVPCID),
1077-
SubnetId: aws.String("subnet-1"),
1078-
AvailabilityZone: aws.String("us-east-1a"),
1079-
CidrBlock: aws.String("10.0.10.0/24"),
1080-
MapPublicIpOnLaunch: aws.Bool(false),
1081-
},
1082-
{
1083-
VpcId: aws.String(subnetsVPCID),
1084-
SubnetId: aws.String("subnet-2"),
1085-
AvailabilityZone: aws.String("us-east-1a"),
1086-
CidrBlock: aws.String("10.0.20.0/24"),
1087-
MapPublicIpOnLaunch: aws.Bool(false),
1088-
},
1089-
},
1090-
}, nil)
1091-
m.DescribeRouteTablesWithContext(context.TODO(), gomock.AssignableToTypeOf(&ec2.DescribeRouteTablesInput{})).
1092-
Return(&ec2.DescribeRouteTablesOutput{}, nil)
1093-
1094-
m.DescribeNatGatewaysPagesWithContext(context.TODO(),
1095-
gomock.Eq(&ec2.DescribeNatGatewaysInput{
1096-
Filter: []*ec2.Filter{
1097-
{
1098-
Name: aws.String("vpc-id"),
1099-
Values: []*string{aws.String(subnetsVPCID)},
1100-
},
1101-
{
1102-
Name: aws.String("state"),
1103-
Values: []*string{aws.String("pending"), aws.String("available")},
1104-
},
1105-
},
1106-
}),
1107-
gomock.Any()).Return(nil)
1108-
},
1060+
expect: func(m *mocks.MockEC2APIMockRecorder) {},
11091061
errorExpected: true,
11101062
tagUnmanagedNetworkResources: true,
11111063
},
@@ -4017,10 +3969,7 @@ func TestDiscoverSubnets(t *testing.T) {
40173969
CidrBlock: "10.0.10.0/24",
40183970
IsPublic: true,
40193971
RouteTableID: aws.String("rtb-1"),
4020-
Tags: infrav1.Tags{
4021-
"Name": "provided-subnet-public",
4022-
},
4023-
ZoneType: ptr.To[infrav1.ZoneType]("availability-zone"),
3972+
ZoneType: ptr.To[infrav1.ZoneType]("availability-zone"),
40243973
},
40253974
{
40263975
ID: "subnet-2",
@@ -4029,10 +3978,7 @@ func TestDiscoverSubnets(t *testing.T) {
40293978
CidrBlock: "10.0.11.0/24",
40303979
IsPublic: false,
40313980
RouteTableID: aws.String("rtb-2"),
4032-
Tags: infrav1.Tags{
4033-
"Name": "provided-subnet-private",
4034-
},
4035-
ZoneType: ptr.To[infrav1.ZoneType]("availability-zone"),
3981+
ZoneType: ptr.To[infrav1.ZoneType]("availability-zone"),
40363982
},
40373983
},
40383984
},
@@ -4084,15 +4030,7 @@ func TestDiscoverSubnets(t *testing.T) {
40844030
}
40854031

40864032
if !cmp.Equal(sn, exp) {
4087-
expected, err := json.MarshalIndent(exp, "", "\t")
4088-
if err != nil {
4089-
t.Fatalf("got an unexpected error: %v", err)
4090-
}
4091-
actual, err := json.MarshalIndent(sn, "", "\t")
4092-
if err != nil {
4093-
t.Fatalf("got an unexpected error: %v", err)
4094-
}
4095-
t.Errorf("Expected %s, got %s", string(expected), string(actual))
4033+
t.Errorf("Expected subnets to be equal. Diff %s", cmp.Diff(sn, exp))
40964034
}
40974035
delete(out, exp.ID)
40984036
}

0 commit comments

Comments
 (0)