Skip to content

Commit d3c0f03

Browse files
nawazkhnawazkh
authored
fix AzCluster_default and AzCluster_validation webhooks (#5603)
* remove unused defaulters * fix code * fix tests
1 parent 86ea1f6 commit d3c0f03

File tree

4 files changed

+64
-150
lines changed

4 files changed

+64
-150
lines changed

‎api/v1beta1/azurecluster_default.go

Lines changed: 4 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -200,7 +200,7 @@ func (s *SubnetSpec) setClusterSubnetDefaults(clusterName string) {
200200
s.SecurityGroup.Name = generateClusterSecurityGroupName(clusterName)
201201
}
202202
if s.RouteTable.Name == "" {
203-
s.RouteTable.Name = generateClustereRouteTableName(clusterName)
203+
s.RouteTable.Name = generateClusterRouteTableName(clusterName)
204204
}
205205
if s.NatGateway.Name == "" {
206206
s.NatGateway.Name = generateClusterNatGatewayName(clusterName)
@@ -456,25 +456,6 @@ func (lb *LoadBalancerClassSpec) setOutboundLBDefaults() {
456456
}
457457
}
458458

459-
func setControlPlaneOutboundLBDefaults(lb *LoadBalancerClassSpec, apiserverLBType LBType) {
460-
// public clusters don't need control plane outbound lb
461-
if apiserverLBType == Public {
462-
return
463-
}
464-
465-
// private clusters can disable control plane outbound lb by setting it to nil.
466-
if lb == nil {
467-
return
468-
}
469-
470-
lb.Type = Public
471-
lb.SKU = SKUStandard
472-
473-
if lb.IdleTimeoutInMinutes == nil {
474-
lb.IdleTimeoutInMinutes = ptr.To[int32](DefaultOutboundRuleIdleTimeoutInMinutes)
475-
}
476-
}
477-
478459
// generateVnetName generates a virtual network name, based on the cluster name.
479460
func generateVnetName(clusterName string) string {
480461
return fmt.Sprintf("%s-%s", clusterName, "vnet")
@@ -520,8 +501,8 @@ func generateNodeSecurityGroupName(clusterName string) string {
520501
return fmt.Sprintf("%s-%s", clusterName, "node-nsg")
521502
}
522503

523-
// generateClustereRouteTableName generates a route table name, based on the cluster name.
524-
func generateClustereRouteTableName(clusterName string) string {
504+
// generateClusterRouteTableName generates a route table name, based on the cluster name.
505+
func generateClusterRouteTableName(clusterName string) string {
525506
return fmt.Sprintf("%s-%s", clusterName, "routetable")
526507
}
527508

@@ -555,7 +536,7 @@ func generateFrontendIPConfigName(lbName string) string {
555536
return fmt.Sprintf("%s-%s", lbName, "frontEnd")
556537
}
557538

558-
// generateFrontendIPConfigName generates a load balancer frontend IP config name.
539+
// generatePrivateIPConfigName generates a load balancer frontend private IP config name.
559540
func generatePrivateIPConfigName(lbName string) string {
560541
return fmt.Sprintf("%s-%s", lbName, "frontEnd-internal-ip")
561542
}

‎api/v1beta1/azurecluster_validation.go

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -232,7 +232,6 @@ func validateSubnets(controlPlaneEnabled bool, subnets Subnets, vnet VnetSpec, f
232232
requiredSubnetRoles["control-plane"] = false
233233
}
234234
clusterSubnet := false
235-
numberofClusterSubnets := 0
236235
for i, subnet := range subnets {
237236
if err := validateSubnetName(subnet.Name, fldPath.Index(i).Child("name")); err != nil {
238237
allErrs = append(allErrs, err)
@@ -243,7 +242,6 @@ func validateSubnets(controlPlaneEnabled bool, subnets Subnets, vnet VnetSpec, f
243242
subnetNames[subnet.Name] = true
244243
if subnet.Role == SubnetCluster {
245244
clusterSubnet = true
246-
numberofClusterSubnets++
247245
} else {
248246
for role := range requiredSubnetRoles {
249247
if role == string(subnet.Role) {
@@ -252,10 +250,10 @@ func validateSubnets(controlPlaneEnabled bool, subnets Subnets, vnet VnetSpec, f
252250
}
253251
}
254252

255-
for _, rule := range subnet.SecurityGroup.SecurityRules {
253+
for j, rule := range subnet.SecurityGroup.SecurityRules {
256254
if err := validateSecurityRule(
257255
rule,
258-
fldPath.Index(i).Child("securityGroup").Child("securityRules").Index(i),
256+
fldPath.Index(i).Child("securityGroup").Child("securityRules").Index(j),
259257
); err != nil {
260258
allErrs = append(allErrs, err...)
261259
}
@@ -615,7 +613,7 @@ func validateClassSpecForAPIServerLB(lb LoadBalancerClassSpec, old *LoadBalancer
615613

616614
if lb.IdleTimeoutInMinutes != nil && (*lb.IdleTimeoutInMinutes < MinLBIdleTimeoutInMinutes || *lb.IdleTimeoutInMinutes > MaxLBIdleTimeoutInMinutes) {
617615
allErrs = append(allErrs, field.Invalid(apiServerLBPath.Child("idleTimeoutInMinutes"), *lb.IdleTimeoutInMinutes,
618-
fmt.Sprintf("Node outbound idle timeout should be between %d and %d minutes", MinLBIdleTimeoutInMinutes, MaxLoadBalancerOutboundIPs)))
616+
fmt.Sprintf("API Server load balancer idle timeout should be between %d and %d minutes", MinLBIdleTimeoutInMinutes, MaxLBIdleTimeoutInMinutes)))
619617
}
620618

621619
return allErrs
@@ -648,7 +646,7 @@ func validateClassSpecForNodeOutboundLB(lb *LoadBalancerClassSpec, old *LoadBala
648646

649647
if lb.IdleTimeoutInMinutes != nil && (*lb.IdleTimeoutInMinutes < MinLBIdleTimeoutInMinutes || *lb.IdleTimeoutInMinutes > MaxLBIdleTimeoutInMinutes) {
650648
allErrs = append(allErrs, field.Invalid(fldPath.Child("idleTimeoutInMinutes"), *lb.IdleTimeoutInMinutes,
651-
fmt.Sprintf("Node outbound idle timeout should be between %d and %d minutes", MinLBIdleTimeoutInMinutes, MaxLoadBalancerOutboundIPs)))
649+
fmt.Sprintf("Node outbound idle timeout should be between %d and %d minutes", MinLBIdleTimeoutInMinutes, MaxLBIdleTimeoutInMinutes)))
652650
}
653651

654652
return allErrs
@@ -670,7 +668,7 @@ func validateClassSpecForControlPlaneOutboundLB(lb *LoadBalancerClassSpec, apise
670668

671669
if lb.IdleTimeoutInMinutes != nil && (*lb.IdleTimeoutInMinutes < MinLBIdleTimeoutInMinutes || *lb.IdleTimeoutInMinutes > MaxLBIdleTimeoutInMinutes) {
672670
allErrs = append(allErrs, field.Invalid(fldPath.Child("idleTimeoutInMinutes"), *lb.IdleTimeoutInMinutes,
673-
fmt.Sprintf("Control plane outbound idle timeout should be between %d and %d minutes", MinLBIdleTimeoutInMinutes, MaxLoadBalancerOutboundIPs)))
671+
fmt.Sprintf("Control plane outbound idle timeout should be between %d and %d minutes", MinLBIdleTimeoutInMinutes, MaxLBIdleTimeoutInMinutes)))
674672
}
675673
}
676674

‎api/v1beta1/azurecluster_validation_test.go

Lines changed: 55 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -131,29 +131,40 @@ func TestClusterWithPreexistingVnetValid(t *testing.T) {
131131
}
132132

133133
func TestClusterWithPreexistingVnetInvalid(t *testing.T) {
134-
type test struct {
134+
tests := []struct {
135135
name string
136136
cluster *AzureCluster
137-
}
138-
139-
testCase := test{
140-
name: "azurecluster with pre-existing vnet - invalid",
141-
cluster: createValidCluster(),
142-
}
143-
144-
// invalid because it doesn't specify a controlplane subnet
145-
testCase.cluster.Spec.NetworkSpec.Subnets[0] = SubnetSpec{
146-
SubnetClassSpec: SubnetClassSpec{
147-
Role: "random",
148-
Name: "random-subnet",
137+
wantErr bool
138+
}{
139+
{
140+
name: "azurecluster with pre-existing vnet - invalid",
141+
cluster: func() *AzureCluster {
142+
cluster := createValidCluster()
143+
// invalid because it doesn't specify a controlplane subnet
144+
cluster.Spec.NetworkSpec.Subnets[0] = SubnetSpec{
145+
SubnetClassSpec: SubnetClassSpec{
146+
Role: "random",
147+
Name: "random-subnet",
148+
},
149+
}
150+
return cluster
151+
}(),
152+
wantErr: true,
149153
},
150154
}
151155

152-
t.Run(testCase.name, func(t *testing.T) {
153-
g := NewWithT(t)
154-
_, err := testCase.cluster.validateCluster(nil)
155-
g.Expect(err).To(HaveOccurred())
156-
})
156+
for _, tc := range tests {
157+
t.Run(tc.name, func(t *testing.T) {
158+
t.Parallel()
159+
g := NewWithT(t)
160+
_, err := tc.cluster.validateCluster(nil)
161+
if tc.wantErr {
162+
g.Expect(err).To(HaveOccurred())
163+
} else {
164+
g.Expect(err).NotTo(HaveOccurred())
165+
}
166+
})
167+
}
157168
}
158169

159170
func TestClusterWithoutPreexistingVnetValid(t *testing.T) {
@@ -2050,24 +2061,35 @@ func TestServiceEndpointsLackRequiredFieldLocations(t *testing.T) {
20502061
}
20512062

20522063
func TestClusterWithExtendedLocationInvalid(t *testing.T) {
2053-
type test struct {
2064+
tests := []struct {
20542065
name string
20552066
cluster *AzureCluster
2067+
wantErr bool
2068+
}{
2069+
{
2070+
name: "azurecluster spec with extended location but not enable EdgeZone feature gate flag",
2071+
cluster: func() *AzureCluster {
2072+
cluster := createValidCluster()
2073+
cluster.Spec.ExtendedLocation = &ExtendedLocationSpec{
2074+
Name: "rr4",
2075+
Type: "EdgeZone",
2076+
}
2077+
return cluster
2078+
}(),
2079+
wantErr: true,
2080+
},
20562081
}
20572082

2058-
testCase := test{
2059-
name: "azurecluster spec with extended location but not enable EdgeZone feature gate flag",
2060-
cluster: createValidCluster(),
2061-
}
2062-
2063-
testCase.cluster.Spec.ExtendedLocation = &ExtendedLocationSpec{
2064-
Name: "rr4",
2065-
Type: "EdgeZone",
2083+
for _, tc := range tests {
2084+
t.Run(tc.name, func(t *testing.T) {
2085+
t.Parallel()
2086+
g := NewWithT(t)
2087+
err := tc.cluster.validateClusterSpec(nil)
2088+
if tc.wantErr {
2089+
g.Expect(err).NotTo(BeNil())
2090+
} else {
2091+
g.Expect(err).To(BeNil())
2092+
}
2093+
})
20662094
}
2067-
2068-
t.Run(testCase.name, func(t *testing.T) {
2069-
g := NewWithT(t)
2070-
err := testCase.cluster.validateClusterSpec(nil)
2071-
g.Expect(err).NotTo(BeNil())
2072-
})
20732095
}

‎api/v1beta1/azureclustertemplate_default_test.go

Lines changed: 0 additions & 87 deletions
Original file line numberDiff line numberDiff line change
@@ -1169,93 +1169,6 @@ func TestNodeOutboundLBClassDefaults(t *testing.T) {
11691169
}
11701170
}
11711171

1172-
func TestControlPlaneOutboundLBTemplateDefaults(t *testing.T) {
1173-
cases := []struct {
1174-
name string
1175-
clusterTemplate *AzureClusterTemplate
1176-
outputTemplate *AzureClusterTemplate
1177-
}{
1178-
{
1179-
name: "no cp lb for public clusters",
1180-
clusterTemplate: &AzureClusterTemplate{
1181-
ObjectMeta: metav1.ObjectMeta{
1182-
Name: "test-cluster-template",
1183-
},
1184-
Spec: AzureClusterTemplateSpec{
1185-
Template: AzureClusterTemplateResource{
1186-
Spec: AzureClusterTemplateResourceSpec{
1187-
NetworkSpec: NetworkTemplateSpec{
1188-
APIServerLB: LoadBalancerClassSpec{Type: Public},
1189-
},
1190-
},
1191-
},
1192-
},
1193-
},
1194-
outputTemplate: &AzureClusterTemplate{
1195-
ObjectMeta: metav1.ObjectMeta{
1196-
Name: "test-cluster-template",
1197-
},
1198-
Spec: AzureClusterTemplateSpec{
1199-
Template: AzureClusterTemplateResource{
1200-
Spec: AzureClusterTemplateResourceSpec{
1201-
NetworkSpec: NetworkTemplateSpec{
1202-
APIServerLB: LoadBalancerClassSpec{Type: Public},
1203-
},
1204-
},
1205-
},
1206-
},
1207-
},
1208-
},
1209-
{
1210-
name: "no cp lb for private clusters",
1211-
clusterTemplate: &AzureClusterTemplate{
1212-
ObjectMeta: metav1.ObjectMeta{
1213-
Name: "test-cluster-template",
1214-
},
1215-
Spec: AzureClusterTemplateSpec{
1216-
Template: AzureClusterTemplateResource{
1217-
Spec: AzureClusterTemplateResourceSpec{
1218-
NetworkSpec: NetworkTemplateSpec{
1219-
APIServerLB: LoadBalancerClassSpec{Type: Internal},
1220-
},
1221-
},
1222-
},
1223-
},
1224-
},
1225-
outputTemplate: &AzureClusterTemplate{
1226-
ObjectMeta: metav1.ObjectMeta{
1227-
Name: "test-cluster-template",
1228-
},
1229-
Spec: AzureClusterTemplateSpec{
1230-
Template: AzureClusterTemplateResource{
1231-
Spec: AzureClusterTemplateResourceSpec{
1232-
NetworkSpec: NetworkTemplateSpec{
1233-
APIServerLB: LoadBalancerClassSpec{Type: Internal},
1234-
},
1235-
},
1236-
},
1237-
},
1238-
},
1239-
},
1240-
}
1241-
1242-
for _, c := range cases {
1243-
tc := c
1244-
t.Run(tc.name, func(t *testing.T) {
1245-
t.Parallel()
1246-
setControlPlaneOutboundLBDefaults(
1247-
tc.clusterTemplate.Spec.Template.Spec.NetworkSpec.ControlPlaneOutboundLB,
1248-
tc.clusterTemplate.Spec.Template.Spec.NetworkSpec.APIServerLB.Type,
1249-
)
1250-
if !reflect.DeepEqual(tc.clusterTemplate, tc.outputTemplate) {
1251-
expected, _ := json.MarshalIndent(tc.outputTemplate, "", "\t")
1252-
actual, _ := json.MarshalIndent(tc.clusterTemplate, "", "\t")
1253-
t.Errorf("Expected %s, got %s", string(expected), string(actual))
1254-
}
1255-
})
1256-
}
1257-
}
1258-
12591172
func TestBastionTemplateDefaults(t *testing.T) {
12601173
cases := []struct {
12611174
name string

0 commit comments

Comments
 (0)