Skip to content

Commit

Permalink
Merge pull request #3711 from k8s-infra-cherrypick-robot/cherry-pick-…
Browse files Browse the repository at this point in the history 
…3700-to-release-1.9

[release-1.9] Don't default NAT Gateway for existing node subnet
  • Loading branch information
@k8s-ci-robot
k8s-ci-robot authored Jul 12, 2023
2 parents 61be879 + 11adba6 commit a020dae
Show file tree
Hide file tree
Showing 4 changed files with 228 additions and 24 deletions.
7 changes: 4 additions & 3 deletions api/v1beta1/azurecluster_default.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -125,9 +125,10 @@ func (c *AzureCluster) setSubnetDefaults() {
subnet.RouteTable.Name = generateNodeRouteTableName(c.ObjectMeta.Name)
}

if !subnet.IsIPv6Enabled() {
// NAT gateway supports the use of IPv4 public IP addresses for outbound connectivity.
// So default use the NAT gateway for outbound traffic in IPv4 cluster instead of loadbalancer.
// NAT gateway only supports the use of IPv4 public IP addresses for outbound connectivity.
// So default use the NAT gateway for outbound traffic in IPv4 cluster instead of loadbalancer.
// We assume that if the ID is set, the subnet already exists so we shouldn't add a NAT gateway.
if !subnet.IsIPv6Enabled() && subnet.ID == "" {
if subnet.NatGateway.Name == "" {
subnet.NatGateway.Name = withIndex(generateNatGatewayName(c.ObjectMeta.Name), nodeSubnetCounter)
}
Expand Down
67 changes: 67 additions & 0 deletions api/v1beta1/azurecluster_default_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -724,6 +724,73 @@ func TestSubnetDefaults(t *testing.T) {
},
},
},
{
name: "don't default NAT Gateway if subnet already exists",
cluster: &AzureCluster{
ObjectMeta: metav1.ObjectMeta{
Name: "cluster-test",
},
Spec: AzureClusterSpec{
NetworkSpec: NetworkSpec{
Subnets: Subnets{
{
SubnetClassSpec: SubnetClassSpec{
Role: SubnetControlPlane,
Name: "cluster-test-controlplane-subnet",
},
ID: "my-subnet-id",
},
{
SubnetClassSpec: SubnetClassSpec{
Role: SubnetNode,
Name: "cluster-test-node-subnet",
},
ID: "my-subnet-id-2",
},
},
},
},
},
output: &AzureCluster{
ObjectMeta: metav1.ObjectMeta{
Name: "cluster-test",
},
Spec: AzureClusterSpec{
NetworkSpec: NetworkSpec{
Subnets: Subnets{
{
SubnetClassSpec: SubnetClassSpec{
Role: SubnetControlPlane,
CIDRBlocks: []string{DefaultControlPlaneSubnetCIDR},
Name: "cluster-test-controlplane-subnet",
},
ID: "my-subnet-id",
SecurityGroup: SecurityGroup{Name: "cluster-test-controlplane-nsg"},
RouteTable: RouteTable{},
},
{
SubnetClassSpec: SubnetClassSpec{
Role: SubnetNode,
CIDRBlocks: []string{DefaultNodeSubnetCIDR},
Name: "cluster-test-node-subnet",
},
ID: "my-subnet-id-2",
SecurityGroup: SecurityGroup{Name: "cluster-test-node-nsg"},
RouteTable: RouteTable{Name: "cluster-test-node-routetable"},
NatGateway: NatGateway{
NatGatewayClassSpec: NatGatewayClassSpec{
Name: "",
},
NatGatewayIP: PublicIPSpec{
Name: "",
},
},
},
},
},
},
},
},
}

for _, c := range cases {
Expand Down
54 changes: 33 additions & 21 deletions azure/services/subnets/spec.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,27 +66,7 @@ func (s *SubnetSpec) Parameters(ctx context.Context, existing interface{}) (para
return nil, errors.Errorf("%T is not a network.Subnet", existing)
}

// No modifications for non-managed subnets
if !s.IsVNetManaged {
return nil, nil
}

var existingServiceEndpoints []network.ServiceEndpointPropertiesFormat
if existingSubnet.ServiceEndpoints != nil {
for _, se := range *existingSubnet.ServiceEndpoints {
existingServiceEndpoints = append(existingServiceEndpoints, network.ServiceEndpointPropertiesFormat{Service: se.Service, Locations: se.Locations})
}
}
var newServiceEndpoints []network.ServiceEndpointPropertiesFormat
for _, se := range s.ServiceEndpoints {
se := se
newServiceEndpoints = append(newServiceEndpoints, network.ServiceEndpointPropertiesFormat{Service: pointer.String(se.Service), Locations: &se.Locations})
}

// Right now only serviceEndpoints are allowed to be updated. More to come later
diff := cmp.Diff(newServiceEndpoints, existingServiceEndpoints)
if diff == "" {
// up to date, nothing to do
if !s.shouldUpdate(existingSubnet) {
return nil, nil
}
}
Expand Down Expand Up @@ -135,3 +115,35 @@ func (s *SubnetSpec) Parameters(ctx context.Context, existing interface{}) (para
SubnetPropertiesFormat: &subnetProperties,
}, nil
}

// shouldUpdate returns true if an existing subnet should be updated.
func (s *SubnetSpec) shouldUpdate(existingSubnet network.Subnet) bool {
// No modifications for non-managed subnets
if !s.IsVNetManaged {
return false
}

// Update the subnet a NAT Gateway was added for backwards compatibility.
if s.NatGatewayName != "" && existingSubnet.SubnetPropertiesFormat.NatGateway == nil {
return true
}

// Update the subnet if the service endpoints changed.
if existingSubnet.ServiceEndpoints != nil || len(s.ServiceEndpoints) > 0 {
var existingServiceEndpoints []network.ServiceEndpointPropertiesFormat
if existingSubnet.ServiceEndpoints != nil {
for _, se := range *existingSubnet.ServiceEndpoints {
existingServiceEndpoints = append(existingServiceEndpoints, network.ServiceEndpointPropertiesFormat{Service: se.Service, Locations: se.Locations})
}
}
newServiceEndpoints := make([]network.ServiceEndpointPropertiesFormat, len(s.ServiceEndpoints))
for _, se := range s.ServiceEndpoints {
se := se
newServiceEndpoints = append(newServiceEndpoints, network.ServiceEndpointPropertiesFormat{Service: pointer.String(se.Service), Locations: &se.Locations})
}

diff := cmp.Diff(newServiceEndpoints, existingServiceEndpoints)
return diff != ""
}
return false
}
124 changes: 124 additions & 0 deletions azure/services/subnets/spec_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -186,3 +186,127 @@ func TestParameters(t *testing.T) {
})
}
}

func TestSubnetSpec_shouldUpdate(t *testing.T) {
type fields struct {
Name string
ResourceGroup string
SubscriptionID string
CIDRs []string
VNetName string
VNetResourceGroup string
IsVNetManaged bool
RouteTableName string
SecurityGroupName string
Role infrav1.SubnetRole
NatGatewayName string
ServiceEndpoints infrav1.ServiceEndpoints
}
type args struct {
existingSubnet network.Subnet
}
tests := []struct {
name string
fields fields
args args
want bool
}{
{
name: "subnet should not be updated when the VNet is not managed",
fields: fields{
Name: "my-subnet",
ResourceGroup: "my-rg",
SubscriptionID: "123",
IsVNetManaged: false,
},
args: args{
existingSubnet: network.Subnet{
Name: pointer.String("my-subnet"),
},
},
want: false,
},
{
name: "subnet should be updated when NAT Gateway gets added",
fields: fields{
Name: "my-subnet",
ResourceGroup: "my-rg",
SubscriptionID: "123",
IsVNetManaged: true,
NatGatewayName: "my-nat-gateway",
},
args: args{
existingSubnet: network.Subnet{
Name: pointer.String("my-subnet"),
SubnetPropertiesFormat: &network.SubnetPropertiesFormat{
NatGateway: nil,
},
},
},
want: true,
},
{
name: "subnet should be updated if service endpoints changed",
fields: fields{
Name: "my-subnet",
ResourceGroup: "my-rg",
SubscriptionID: "123",
IsVNetManaged: true,
ServiceEndpoints: infrav1.ServiceEndpoints{
{
Service: "Microsoft.Storage",
},
},
},
args: args{
existingSubnet: network.Subnet{
Name: pointer.String("my-subnet"),
SubnetPropertiesFormat: &network.SubnetPropertiesFormat{
ServiceEndpoints: nil,
},
},
},
want: true,
},
{
name: "subnet should not be updated if other properties change",
fields: fields{
Name: "my-subnet",
ResourceGroup: "my-rg",
SubscriptionID: "123",
IsVNetManaged: true,
CIDRs: []string{"10.1.0.0/16"},
},
args: args{
existingSubnet: network.Subnet{
Name: pointer.String("my-subnet"),
SubnetPropertiesFormat: &network.SubnetPropertiesFormat{
AddressPrefixes: &[]string{"10.1.0.0/8"},
},
},
},
want: false,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
s := &SubnetSpec{
Name: tt.fields.Name,
ResourceGroup: tt.fields.ResourceGroup,
SubscriptionID: tt.fields.SubscriptionID,
CIDRs: tt.fields.CIDRs,
VNetName: tt.fields.VNetName,
VNetResourceGroup: tt.fields.VNetResourceGroup,
IsVNetManaged: tt.fields.IsVNetManaged,
RouteTableName: tt.fields.RouteTableName,
SecurityGroupName: tt.fields.SecurityGroupName,
Role: tt.fields.Role,
NatGatewayName: tt.fields.NatGatewayName,
ServiceEndpoints: tt.fields.ServiceEndpoints,
}
if got := s.shouldUpdate(tt.args.existingSubnet); got != tt.want {
t.Errorf("SubnetSpec.shouldUpdate() = %v, want %v", got, tt.want)
}
})
}
}

0 comments on commit a020dae

Please sign in to comment.