Merge pull request #3961 from nawazkh/update_security_rules_sources

Add ability to specify multiple sources for a security rule
kubernetes-sigs · Dec 28, 2023 · 1792fd4 · 1792fd4
2 parents f505a66 + dee2344
commit 1792fd4
Show file tree

Hide file tree

Showing 8 changed files with 78 additions and 7 deletions.
diff --git a/api/v1beta1/azurecluster_validation.go b/api/v1beta1/azurecluster_validation.go
@@ -239,7 +239,7 @@ func validateSubnets(subnets Subnets, vnet VnetSpec, fldPath *field.Path) field.
 				rule,
 				fldPath.Index(i).Child("securityGroup").Child("securityRules").Index(i),
 			); err != nil {
-				allErrs = append(allErrs, err)
+				allErrs = append(allErrs, err...)
 			}
 		}
 		allErrs = append(allErrs, validateSubnetCIDR(subnet.CIDRBlocks, vnet.CIDRBlocks, fldPath.Index(i).Child("cidrBlocks"))...)
@@ -356,12 +356,16 @@ func validateInternalLBIPAddress(address string, cidrs []string, fldPath *field.
 }
 
 // validateSecurityRule validates a SecurityRule.
-func validateSecurityRule(rule SecurityRule, fldPath *field.Path) *field.Error {
+func validateSecurityRule(rule SecurityRule, fldPath *field.Path) (allErrs field.ErrorList) {
 	if rule.Priority < minRulePriority || rule.Priority > maxRulePriority {
-		return field.Invalid(fldPath, rule.Priority, fmt.Sprintf("security rule priorities should be between %d and %d", minRulePriority, maxRulePriority))
+		allErrs = append(allErrs, field.Invalid(fldPath, rule.Priority, fmt.Sprintf("security rule priorities should be between %d and %d", minRulePriority, maxRulePriority)))
 	}
 
-	return nil
+	if rule.Source != nil && rule.Sources != nil {
+		allErrs = append(allErrs, field.Invalid(fldPath, rule.Source, "security rule cannot have both source and sources"))
+	}
+
+	return allErrs
 }
 
 func validateAPIServerLB(lb LoadBalancerSpec, old LoadBalancerSpec, cidrs []string, fldPath *field.Path) field.ErrorList {

diff --git a/api/v1beta1/azurecluster_validation_test.go b/api/v1beta1/azurecluster_validation_test.go
@@ -663,6 +663,33 @@ func TestValidateSecurityRule(t *testing.T) {
 			},
 			wantErr: true,
 		},
+		{
+			name: "security rule - invalid sources priority",
+			validRule: SecurityRule{
+				Name:        "allow_apiserver",
+				Description: "Allow K8s API Server",
+				Priority:    4000,
+				Source:      ptr.To("*"),
+				Sources: []*string{
+					ptr.To("*"),
+					ptr.To("unknown"),
+				},
+			},
+			wantErr: true,
+		},
+		{
+			name: "security rule - valid sources",
+			validRule: SecurityRule{
+				Name:        "allow_apiserver",
+				Description: "Allow K8s API Server",
+				Priority:    4000,
+				Sources: []*string{
+					ptr.To("*"),
+					ptr.To("unknown"),
+				},
+			},
+			wantErr: false,
+		},
 	}
 	for _, testCase := range tests {
 		testCase := testCase
@@ -674,9 +701,11 @@ func TestValidateSecurityRule(t *testing.T) {
 				field.NewPath("spec").Child("networkSpec").Child("subnets").Index(0).Child("securityGroup").Child("securityRules").Index(0),
 			)
 			if testCase.wantErr {
-				g.Expect(err).To(HaveOccurred())
+				g.Expect(err).NotTo(BeNil())
+				g.Expect(len(err)).To(Equal(1))
 			} else {
-				g.Expect(err).NotTo(HaveOccurred())
+				g.Expect(err).To(BeNil())
+				g.Expect(len(err)).To(Equal(0))
 			}
 		})
 	}

diff --git a/api/v1beta1/azureclustertemplate_validation.go b/api/v1beta1/azureclustertemplate_validation.go
@@ -109,7 +109,7 @@ func validateSubnetTemplates(subnets SubnetTemplatesSpec, vnet VnetTemplateSpec,
 				rule,
 				fld.Index(i).Child("securityGroup").Child("securityGroup").Child("securityRules").Index(j),
 			); err != nil {
-				allErrs = append(allErrs, err)
+				allErrs = append(allErrs, err...)
 			}
 		}
 		allErrs = append(allErrs, validateSubnetCIDR(subnet.CIDRBlocks, vnet.CIDRBlocks, fld.Index(i).Child("cidrBlocks"))...)

diff --git a/api/v1beta1/types.go b/api/v1beta1/types.go
@@ -305,6 +305,8 @@ type SecurityRule struct {
 	// Source specifies the CIDR or source IP range. Asterix '*' can also be used to match all source IPs. Default tags such as 'VirtualNetwork', 'AzureLoadBalancer' and 'Internet' can also be used. If this is an ingress rule, specifies where network traffic originates from.
 	// +optional
 	Source *string `json:"source,omitempty"`
+	// Sources specifies The CIDR or source IP ranges.
+	Sources []*string `json:"sources,omitempty"`
 	// Destination is the destination address prefix. CIDR or destination IP range. Asterix '*' can also be used to match all source IPs. Default tags such as 'VirtualNetwork', 'AzureLoadBalancer' and 'Internet' can also be used.
 	// +optional
 	Destination *string `json:"destination,omitempty"`

diff --git a/api/v1beta1/zz_generated.deepcopy.go b/api/v1beta1/zz_generated.deepcopy.go
diff --git a/azure/converters/rules.go b/azure/converters/rules.go
@@ -29,6 +29,7 @@ func SecurityRuleToSDK(rule infrav1.SecurityRule) *armnetwork.SecurityRule {
 		Properties: &armnetwork.SecurityRulePropertiesFormat{
 			Description:              ptr.To(rule.Description),
 			SourceAddressPrefix:      rule.Source,
+			SourceAddressPrefixes:    rule.Sources,
 			SourcePortRange:          rule.SourcePorts,
 			DestinationAddressPrefix: rule.Destination,
 			DestinationPortRange:     rule.DestinationPorts,

diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_azureclusters.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_azureclusters.yaml
@@ -394,6 +394,12 @@ spec:
                                         Asterix '*' can also be used to match all
                                         ports.
                                       type: string
+                                    sources:
+                                      description: Sources specifies The CIDR or source
+                                        IP ranges.
+                                      items:
+                                        type: string
+                                      type: array
                                   required:
                                   - description
                                   - direction
@@ -1127,6 +1133,12 @@ spec:
                                       or range. Integer or range between 0 and 65535.
                                       Asterix '*' can also be used to match all ports.
                                     type: string
+                                  sources:
+                                    description: Sources specifies The CIDR or source
+                                      IP ranges.
+                                    items:
+                                      type: string
+                                    type: array
                                 required:
                                 - description
                                 - direction

diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_azureclustertemplates.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_azureclustertemplates.yaml
@@ -284,6 +284,12 @@ spec:
                                                 0 and 65535. Asterix '*' can also
                                                 be used to match all ports.
                                               type: string
+                                            sources:
+                                              description: Sources specifies The CIDR
+                                                or source IP ranges.
+                                              items:
+                                                type: string
+                                              type: array
                                           required:
                                           - description
                                           - direction
@@ -774,6 +780,12 @@ spec:
                                               0 and 65535. Asterix '*' can also be
                                               used to match all ports.
                                             type: string
+                                          sources:
+                                            description: Sources specifies The CIDR
+                                              or source IP ranges.
+                                            items:
+                                              type: string
+                                            type: array
                                         required:
                                         - description
                                         - direction