Skip to content

Commit 11adba6

Browse files
CecileRobertMichonk8s-infra-cherrypick-robot
CecileRobertMichon
authored and
k8s-infra-cherrypick-robot
committed
Don't default NAT Gateway for existing node subnet
1 parent 61be879 commit 11adba6

File tree

4 files changed

+228
-24
lines changed

4 files changed

+228
-24
lines changed

‎api/v1beta1/azurecluster_default.go‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -125,9 +125,10 @@ func (c *AzureCluster) setSubnetDefaults() {
125125
subnet.RouteTable.Name = generateNodeRouteTableName(c.ObjectMeta.Name)
126126
}
127127

128-
if !subnet.IsIPv6Enabled() {
129-
// NAT gateway supports the use of IPv4 public IP addresses for outbound connectivity.
130-
// So default use the NAT gateway for outbound traffic in IPv4 cluster instead of loadbalancer.
128+
// NAT gateway only supports the use of IPv4 public IP addresses for outbound connectivity.
129+
// So default use the NAT gateway for outbound traffic in IPv4 cluster instead of loadbalancer.
130+
// We assume that if the ID is set, the subnet already exists so we shouldn't add a NAT gateway.
131+
if !subnet.IsIPv6Enabled() && subnet.ID == "" {
131132
if subnet.NatGateway.Name == "" {
132133
subnet.NatGateway.Name = withIndex(generateNatGatewayName(c.ObjectMeta.Name), nodeSubnetCounter)
133134
}

‎api/v1beta1/azurecluster_default_test.go‎

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -724,6 +724,73 @@ func TestSubnetDefaults(t *testing.T) {
724724
},
725725
},
726726
},
727+
{
728+
name: "don't default NAT Gateway if subnet already exists",
729+
cluster: &AzureCluster{
730+
ObjectMeta: metav1.ObjectMeta{
731+
Name: "cluster-test",
732+
},
733+
Spec: AzureClusterSpec{
734+
NetworkSpec: NetworkSpec{
735+
Subnets: Subnets{
736+
{
737+
SubnetClassSpec: SubnetClassSpec{
738+
Role: SubnetControlPlane,
739+
Name: "cluster-test-controlplane-subnet",
740+
},
741+
ID: "my-subnet-id",
742+
},
743+
{
744+
SubnetClassSpec: SubnetClassSpec{
745+
Role: SubnetNode,
746+
Name: "cluster-test-node-subnet",
747+
},
748+
ID: "my-subnet-id-2",
749+
},
750+
},
751+
},
752+
},
753+
},
754+
output: &AzureCluster{
755+
ObjectMeta: metav1.ObjectMeta{
756+
Name: "cluster-test",
757+
},
758+
Spec: AzureClusterSpec{
759+
NetworkSpec: NetworkSpec{
760+
Subnets: Subnets{
761+
{
762+
SubnetClassSpec: SubnetClassSpec{
763+
Role: SubnetControlPlane,
764+
CIDRBlocks: []string{DefaultControlPlaneSubnetCIDR},
765+
Name: "cluster-test-controlplane-subnet",
766+
},
767+
ID: "my-subnet-id",
768+
SecurityGroup: SecurityGroup{Name: "cluster-test-controlplane-nsg"},
769+
RouteTable: RouteTable{},
770+
},
771+
{
772+
SubnetClassSpec: SubnetClassSpec{
773+
Role: SubnetNode,
774+
CIDRBlocks: []string{DefaultNodeSubnetCIDR},
775+
Name: "cluster-test-node-subnet",
776+
},
777+
ID: "my-subnet-id-2",
778+
SecurityGroup: SecurityGroup{Name: "cluster-test-node-nsg"},
779+
RouteTable: RouteTable{Name: "cluster-test-node-routetable"},
780+
NatGateway: NatGateway{
781+
NatGatewayClassSpec: NatGatewayClassSpec{
782+
Name: "",
783+
},
784+
NatGatewayIP: PublicIPSpec{
785+
Name: "",
786+
},
787+
},
788+
},
789+
},
790+
},
791+
},
792+
},
793+
},
727794
}
728795

729796
for _, c := range cases {

‎azure/services/subnets/spec.go‎

Lines changed: 33 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -66,27 +66,7 @@ func (s *SubnetSpec) Parameters(ctx context.Context, existing interface{}) (para
6666
return nil, errors.Errorf("%T is not a network.Subnet", existing)
6767
}
6868

69-
// No modifications for non-managed subnets
70-
if !s.IsVNetManaged {
71-
return nil, nil
72-
}
73-
74-
var existingServiceEndpoints []network.ServiceEndpointPropertiesFormat
75-
if existingSubnet.ServiceEndpoints != nil {
76-
for _, se := range *existingSubnet.ServiceEndpoints {
77-
existingServiceEndpoints = append(existingServiceEndpoints, network.ServiceEndpointPropertiesFormat{Service: se.Service, Locations: se.Locations})
78-
}
79-
}
80-
var newServiceEndpoints []network.ServiceEndpointPropertiesFormat
81-
for _, se := range s.ServiceEndpoints {
82-
se := se
83-
newServiceEndpoints = append(newServiceEndpoints, network.ServiceEndpointPropertiesFormat{Service: pointer.String(se.Service), Locations: &se.Locations})
84-
}
85-
86-
// Right now only serviceEndpoints are allowed to be updated. More to come later
87-
diff := cmp.Diff(newServiceEndpoints, existingServiceEndpoints)
88-
if diff == "" {
89-
// up to date, nothing to do
69+
if !s.shouldUpdate(existingSubnet) {
9070
return nil, nil
9171
}
9272
}
@@ -135,3 +115,35 @@ func (s *SubnetSpec) Parameters(ctx context.Context, existing interface{}) (para
135115
SubnetPropertiesFormat: &subnetProperties,
136116
}, nil
137117
}
118+
119+
// shouldUpdate returns true if an existing subnet should be updated.
120+
func (s *SubnetSpec) shouldUpdate(existingSubnet network.Subnet) bool {
121+
// No modifications for non-managed subnets
122+
if !s.IsVNetManaged {
123+
return false
124+
}
125+
126+
// Update the subnet a NAT Gateway was added for backwards compatibility.
127+
if s.NatGatewayName != "" && existingSubnet.SubnetPropertiesFormat.NatGateway == nil {
128+
return true
129+
}
130+
131+
// Update the subnet if the service endpoints changed.
132+
if existingSubnet.ServiceEndpoints != nil || len(s.ServiceEndpoints) > 0 {
133+
var existingServiceEndpoints []network.ServiceEndpointPropertiesFormat
134+
if existingSubnet.ServiceEndpoints != nil {
135+
for _, se := range *existingSubnet.ServiceEndpoints {
136+
existingServiceEndpoints = append(existingServiceEndpoints, network.ServiceEndpointPropertiesFormat{Service: se.Service, Locations: se.Locations})
137+
}
138+
}
139+
newServiceEndpoints := make([]network.ServiceEndpointPropertiesFormat, len(s.ServiceEndpoints))
140+
for _, se := range s.ServiceEndpoints {
141+
se := se
142+
newServiceEndpoints = append(newServiceEndpoints, network.ServiceEndpointPropertiesFormat{Service: pointer.String(se.Service), Locations: &se.Locations})
143+
}
144+
145+
diff := cmp.Diff(newServiceEndpoints, existingServiceEndpoints)
146+
return diff != ""
147+
}
148+
return false
149+
}

‎azure/services/subnets/spec_test.go‎

Lines changed: 124 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -186,3 +186,127 @@ func TestParameters(t *testing.T) {
186186
})
187187
}
188188
}
189+
190+
func TestSubnetSpec_shouldUpdate(t *testing.T) {
191+
type fields struct {
192+
Name string
193+
ResourceGroup string
194+
SubscriptionID string
195+
CIDRs []string
196+
VNetName string
197+
VNetResourceGroup string
198+
IsVNetManaged bool
199+
RouteTableName string
200+
SecurityGroupName string
201+
Role infrav1.SubnetRole
202+
NatGatewayName string
203+
ServiceEndpoints infrav1.ServiceEndpoints
204+
}
205+
type args struct {
206+
existingSubnet network.Subnet
207+
}
208+
tests := []struct {
209+
name string
210+
fields fields
211+
args args
212+
want bool
213+
}{
214+
{
215+
name: "subnet should not be updated when the VNet is not managed",
216+
fields: fields{
217+
Name: "my-subnet",
218+
ResourceGroup: "my-rg",
219+
SubscriptionID: "123",
220+
IsVNetManaged: false,
221+
},
222+
args: args{
223+
existingSubnet: network.Subnet{
224+
Name: pointer.String("my-subnet"),
225+
},
226+
},
227+
want: false,
228+
},
229+
{
230+
name: "subnet should be updated when NAT Gateway gets added",
231+
fields: fields{
232+
Name: "my-subnet",
233+
ResourceGroup: "my-rg",
234+
SubscriptionID: "123",
235+
IsVNetManaged: true,
236+
NatGatewayName: "my-nat-gateway",
237+
},
238+
args: args{
239+
existingSubnet: network.Subnet{
240+
Name: pointer.String("my-subnet"),
241+
SubnetPropertiesFormat: &network.SubnetPropertiesFormat{
242+
NatGateway: nil,
243+
},
244+
},
245+
},
246+
want: true,
247+
},
248+
{
249+
name: "subnet should be updated if service endpoints changed",
250+
fields: fields{
251+
Name: "my-subnet",
252+
ResourceGroup: "my-rg",
253+
SubscriptionID: "123",
254+
IsVNetManaged: true,
255+
ServiceEndpoints: infrav1.ServiceEndpoints{
256+
{
257+
Service: "Microsoft.Storage",
258+
},
259+
},
260+
},
261+
args: args{
262+
existingSubnet: network.Subnet{
263+
Name: pointer.String("my-subnet"),
264+
SubnetPropertiesFormat: &network.SubnetPropertiesFormat{
265+
ServiceEndpoints: nil,
266+
},
267+
},
268+
},
269+
want: true,
270+
},
271+
{
272+
name: "subnet should not be updated if other properties change",
273+
fields: fields{
274+
Name: "my-subnet",
275+
ResourceGroup: "my-rg",
276+
SubscriptionID: "123",
277+
IsVNetManaged: true,
278+
CIDRs: []string{"10.1.0.0/16"},
279+
},
280+
args: args{
281+
existingSubnet: network.Subnet{
282+
Name: pointer.String("my-subnet"),
283+
SubnetPropertiesFormat: &network.SubnetPropertiesFormat{
284+
AddressPrefixes: &[]string{"10.1.0.0/8"},
285+
},
286+
},
287+
},
288+
want: false,
289+
},
290+
}
291+
for _, tt := range tests {
292+
t.Run(tt.name, func(t *testing.T) {
293+
s := &SubnetSpec{
294+
Name: tt.fields.Name,
295+
ResourceGroup: tt.fields.ResourceGroup,
296+
SubscriptionID: tt.fields.SubscriptionID,
297+
CIDRs: tt.fields.CIDRs,
298+
VNetName: tt.fields.VNetName,
299+
VNetResourceGroup: tt.fields.VNetResourceGroup,
300+
IsVNetManaged: tt.fields.IsVNetManaged,
301+
RouteTableName: tt.fields.RouteTableName,
302+
SecurityGroupName: tt.fields.SecurityGroupName,
303+
Role: tt.fields.Role,
304+
NatGatewayName: tt.fields.NatGatewayName,
305+
ServiceEndpoints: tt.fields.ServiceEndpoints,
306+
}
307+
if got := s.shouldUpdate(tt.args.existingSubnet); got != tt.want {
308+
t.Errorf("SubnetSpec.shouldUpdate() = %v, want %v", got, tt.want)
309+
}
310+
})
311+
}
312+
}

0 commit comments

Comments
 (0)