Merge pull request #5147 from krasoffski/default_node_port_security_group

k8s-ci-robot · web-flow · commit 4143c989c5fa · 2024-10-15T14:20:22.000+01:00
✨ CIDR blocks for default Security Group rule "Node Port Services"
diff --git a/api/v1beta1/awscluster_conversion.go b/api/v1beta1/awscluster_conversion.go
@@ -85,6 +85,7 @@ func (src *AWSCluster) ConvertTo(dstRaw conversion.Hub) error {
 	}
 
 	dst.Spec.NetworkSpec.AdditionalControlPlaneIngressRules = restored.Spec.NetworkSpec.AdditionalControlPlaneIngressRules
+	dst.Spec.NetworkSpec.NodePortIngressRuleCidrBlocks = restored.Spec.NetworkSpec.NodePortIngressRuleCidrBlocks
 
 	if restored.Spec.NetworkSpec.VPC.IPAMPool != nil {
 		if dst.Spec.NetworkSpec.VPC.IPAMPool == nil {
diff --git a/api/v1beta1/zz_generated.conversion.go b/api/v1beta1/zz_generated.conversion.go
diff --git a/api/v1beta2/awscluster_webhook.go b/api/v1beta2/awscluster_webhook.go
@@ -18,6 +18,7 @@ package v1beta2
 
 import (
 	"fmt"
+	"net"
 	"strings"
 
 	"github.com/google/go-cmp/cmp"
@@ -267,6 +268,12 @@ func (r *AWSCluster) validateNetwork() field.ErrorList {
 		allErrs = append(allErrs, r.validateIngressRule(rule)...)
 	}
 
+	for cidrBlockIndex, cidrBlock := range r.Spec.NetworkSpec.NodePortIngressRuleCidrBlocks {
+		if _, _, err := net.ParseCIDR(cidrBlock); err != nil {
+			allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "network", fmt.Sprintf("nodePortIngressRuleCidrBlocks[%d]", cidrBlockIndex)), r.Spec.NetworkSpec.NodePortIngressRuleCidrBlocks, "CIDR block is invalid"))
+		}
+	}
+
 	if r.Spec.NetworkSpec.VPC.ElasticIPPool != nil {
 		eipp := r.Spec.NetworkSpec.VPC.ElasticIPPool
 		if eipp.PublicIpv4Pool != nil {
diff --git a/api/v1beta2/awscluster_webhook_test.go b/api/v1beta2/awscluster_webhook_test.go
@@ -638,6 +638,28 @@ func TestAWSClusterValidateCreate(t *testing.T) {
 			},
 			wantErr: false,
 		},
+		{
+			name: "accepts cidrBlock for default node port ingress rule",
+			cluster: &AWSCluster{
+				Spec: AWSClusterSpec{
+					NetworkSpec: NetworkSpec{
+						NodePortIngressRuleCidrBlocks: []string{"10.0.0.0/16"},
+					},
+				},
+			},
+			wantErr: false,
+		},
+		{
+			name: "reject invalid cidrBlock for default node port ingress rule",
+			cluster: &AWSCluster{
+				Spec: AWSClusterSpec{
+					NetworkSpec: NetworkSpec{
+						NodePortIngressRuleCidrBlocks: []string{"10.0.0.0"},
+					},
+				},
+			},
+			wantErr: true,
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
diff --git a/api/v1beta2/network_types.go b/api/v1beta2/network_types.go
@@ -351,6 +351,11 @@ type NetworkSpec struct {
 	// AdditionalControlPlaneIngressRules is an optional set of ingress rules to add to the control plane
 	// +optional
 	AdditionalControlPlaneIngressRules []IngressRule `json:"additionalControlPlaneIngressRules,omitempty"`
+
+	// NodePortIngressRuleCidrBlocks is an optional set of CIDR blocks to allow traffic to nodes' NodePort services.
+	// If none are specified here, all IPs are allowed to connect.
+	// +optional
+	NodePortIngressRuleCidrBlocks []string `json:"nodePortIngressRuleCidrBlocks,omitempty"`
 }
 
 // IPv6 contains ipv6 specific settings for the network.
diff --git a/api/v1beta2/zz_generated.deepcopy.go b/api/v1beta2/zz_generated.deepcopy.go
diff --git a/config/crd/bases/controlplane.cluster.x-k8s.io_awsmanagedcontrolplanes.yaml b/config/crd/bases/controlplane.cluster.x-k8s.io_awsmanagedcontrolplanes.yaml
@@ -474,6 +474,13 @@ spec:
                           type: object
                         type: array
                     type: object
+                  nodePortIngressRuleCidrBlocks:
+                    description: |-
+                      NodePortIngressRuleCidrBlocks is an optional set of CIDR blocks to allow traffic to nodes' NodePort services.
+                      If none are specified here, all IPs are allowed to connect.
+                    items:
+                      type: string
+                    type: array
                   securityGroupOverrides:
                     additionalProperties:
                       type: string
@@ -2500,6 +2507,13 @@ spec:
                           type: object
                         type: array
                     type: object
+                  nodePortIngressRuleCidrBlocks:
+                    description: |-
+                      NodePortIngressRuleCidrBlocks is an optional set of CIDR blocks to allow traffic to nodes' NodePort services.
+                      If none are specified here, all IPs are allowed to connect.
+                    items:
+                      type: string
+                    type: array
                   securityGroupOverrides:
                     additionalProperties:
                       type: string
diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_awsclusters.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_awsclusters.yaml
@@ -1414,6 +1414,13 @@ spec:
                           type: object
                         type: array
                     type: object
+                  nodePortIngressRuleCidrBlocks:
+                    description: |-
+                      NodePortIngressRuleCidrBlocks is an optional set of CIDR blocks to allow traffic to nodes' NodePort services.
+                      If none are specified here, all IPs are allowed to connect.
+                    items:
+                      type: string
+                    type: array
                   securityGroupOverrides:
                     additionalProperties:
                       type: string
diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_awsclustertemplates.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_awsclustertemplates.yaml
@@ -1011,6 +1011,13 @@ spec:
                                   type: object
                                 type: array
                             type: object
+                          nodePortIngressRuleCidrBlocks:
+                            description: |-
+                              NodePortIngressRuleCidrBlocks is an optional set of CIDR blocks to allow traffic to nodes' NodePort services.
+                              If none are specified here, all IPs are allowed to connect.
+                            items:
+                              type: string
+                            type: array
                           securityGroupOverrides:
                             additionalProperties:
                               type: string
diff --git a/pkg/cloud/scope/cluster.go b/pkg/cloud/scope/cluster.go
@@ -418,3 +418,8 @@ func (s *ClusterScope) AdditionalControlPlaneIngressRules() []infrav1.IngressRul
 func (s *ClusterScope) UnstructuredControlPlane() (*unstructured.Unstructured, error) {
 	return getUnstructuredControlPlane(context.TODO(), s.client, s.Cluster)
 }
+
+// NodePortIngressRuleCidrBlocks returns the CIDR blocks for the node NodePort ingress rules.
+func (s *ClusterScope) NodePortIngressRuleCidrBlocks() []string {
+	return s.AWSCluster.Spec.NetworkSpec.DeepCopy().NodePortIngressRuleCidrBlocks
+}
diff --git a/pkg/cloud/scope/managedcontrolplane.go b/pkg/cloud/scope/managedcontrolplane.go
@@ -480,3 +480,8 @@ func (s *ManagedControlPlaneScope) AdditionalControlPlaneIngressRules() []infrav
 func (s *ManagedControlPlaneScope) UnstructuredControlPlane() (*unstructured.Unstructured, error) {
 	return getUnstructuredControlPlane(context.TODO(), s.Client, s.Cluster)
 }
+
+// NodePortIngressRuleCidrBlocks returns the CIDR blocks for the node NodePort ingress rules.
+func (s *ManagedControlPlaneScope) NodePortIngressRuleCidrBlocks() []string {
+	return nil
+}
diff --git a/pkg/cloud/scope/sg.go b/pkg/cloud/scope/sg.go
@@ -59,4 +59,7 @@ type SGScope interface {
 	// ControlPlaneLoadBalancers returns both the ControlPlaneLoadBalancer and SecondaryControlPlaneLoadBalancer AWSLoadBalancerSpecs.
 	// The control plane load balancers should always be returned in the above order.
 	ControlPlaneLoadBalancers() []*infrav1.AWSLoadBalancerSpec
+
+	// NodePortIngressRuleCidrBlocks returns the CIDR blocks for the node NodePort ingress rules.
+	NodePortIngressRuleCidrBlocks() []string
 }
diff --git a/pkg/cloud/services/securitygroup/securitygroups.go b/pkg/cloud/services/securitygroup/securitygroups.go
@@ -591,7 +591,6 @@ func (s *Service) getSecurityGroupIngressRules(role infrav1.SecurityGroupRole) (
 			},
 		}
 	}
-	cidrBlocks := []string{services.AnyIPv4CidrBlock}
 	switch role {
 	case infrav1.SecurityGroupBastion:
 		return infrav1.IngressRules{
@@ -645,6 +644,10 @@ func (s *Service) getSecurityGroupIngressRules(role infrav1.SecurityGroupRole) (
 		return append(cniRules, rules...), nil
 
 	case infrav1.SecurityGroupNode:
+		cidrBlocks := []string{services.AnyIPv4CidrBlock}
+		if scopeCidrBlocks := s.scope.NodePortIngressRuleCidrBlocks(); len(scopeCidrBlocks) > 0 {
+			cidrBlocks = scopeCidrBlocks
+		}
 		rules := infrav1.IngressRules{
 			{
 				Description: "Node Port Services",
diff --git a/pkg/cloud/services/securitygroup/securitygroups_test.go b/pkg/cloud/services/securitygroup/securitygroups_test.go
@@ -2237,3 +2237,97 @@ func TestExpandIngressRules(t *testing.T) {
 		})
 	}
 }
+
+func TestNodePortServicesIngressRules(t *testing.T) {
+	scheme := runtime.NewScheme()
+	_ = infrav1.AddToScheme(scheme)
+
+	testCases := []struct {
+		name                string
+		cidrBlocks          []string
+		expectedIngresRules infrav1.IngressRules
+	}{
+		{
+			name:       "default node ports services ingress rules, no node port cidr block provided",
+			cidrBlocks: nil,
+			expectedIngresRules: infrav1.IngressRules{
+				{
+					Description: "Node Port Services",
+					Protocol:    infrav1.SecurityGroupProtocolTCP,
+					FromPort:    30000,
+					ToPort:      32767,
+					CidrBlocks:  []string{services.AnyIPv4CidrBlock},
+				},
+				{
+					Description:            "Kubelet API",
+					Protocol:               infrav1.SecurityGroupProtocolTCP,
+					FromPort:               10250,
+					ToPort:                 10250,
+					SourceSecurityGroupIDs: []string{"Id1", "Id2"},
+				},
+			},
+		},
+		{
+			name:       "node port cidr block provided, no default cidr block used for node port services ingress rule",
+			cidrBlocks: []string{"10.0.0.0/16"},
+			expectedIngresRules: infrav1.IngressRules{
+				{
+					Description: "Node Port Services",
+					Protocol:    infrav1.SecurityGroupProtocolTCP,
+					FromPort:    30000,
+					ToPort:      32767,
+					CidrBlocks:  []string{"10.0.0.0/16"},
+				},
+				{
+					Description:            "Kubelet API",
+					Protocol:               infrav1.SecurityGroupProtocolTCP,
+					FromPort:               10250,
+					ToPort:                 10250,
+					SourceSecurityGroupIDs: []string{"Id1", "Id2"},
+				},
+			},
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			cs, err := scope.NewClusterScope(scope.ClusterScopeParams{
+				Client: fake.NewClientBuilder().WithScheme(scheme).Build(),
+				Cluster: &clusterv1.Cluster{
+					ObjectMeta: metav1.ObjectMeta{Name: "test-cluster"},
+				},
+				AWSCluster: &infrav1.AWSCluster{
+					Spec: infrav1.AWSClusterSpec{
+						ControlPlaneLoadBalancer: &infrav1.AWSLoadBalancerSpec{},
+						NetworkSpec: infrav1.NetworkSpec{
+							VPC: infrav1.VPCSpec{
+								CidrBlock: "10.0.0.0/16",
+							},
+							NodePortIngressRuleCidrBlocks: tc.cidrBlocks,
+						},
+					},
+					Status: infrav1.AWSClusterStatus{
+						Network: infrav1.NetworkStatus{
+							SecurityGroups: map[infrav1.SecurityGroupRole]infrav1.SecurityGroup{
+								infrav1.SecurityGroupControlPlane: {ID: "Id1"},
+								infrav1.SecurityGroupNode:         {ID: "Id2"},
+							},
+						},
+					},
+				},
+			})
+			if err != nil {
+				t.Fatalf("Failed to create test context: %v", err)
+			}
+
+			s := NewService(cs, testSecurityGroupRoles)
+			rules, err := s.getSecurityGroupIngressRules(infrav1.SecurityGroupNode)
+			if err != nil {
+				t.Fatalf("Failed to lookup node security group ingress rules: %v", err)
+			}
+
+			g := NewGomegaWithT(t)
+			g.Expect(rules).To(Equal(tc.expectedIngresRules))
+		})
+	}
+}

Original file line number	Diff line number	Diff line change
`@@ -85,6 +85,7 @@ func (src *AWSCluster) ConvertTo(dstRaw conversion.Hub) error {`
`85`	`85`	`}`
`86`	`86`
`87`	`87`	`dst.Spec.NetworkSpec.AdditionalControlPlaneIngressRules = restored.Spec.NetworkSpec.AdditionalControlPlaneIngressRules`
	`88`	`+ dst.Spec.NetworkSpec.NodePortIngressRuleCidrBlocks = restored.Spec.NetworkSpec.NodePortIngressRuleCidrBlocks`
`88`	`89`
`89`	`90`	`if restored.Spec.NetworkSpec.VPC.IPAMPool != nil {`
`90`	`91`	`if dst.Spec.NetworkSpec.VPC.IPAMPool == nil {`
Original file line number	Diff line number	Diff line change
`@@ -418,3 +418,8 @@ func (s *ClusterScope) AdditionalControlPlaneIngressRules() []infrav1.IngressRul`
`418`	`418`	`func (s ClusterScope) UnstructuredControlPlane() (unstructured.Unstructured, error) {`
`419`	`419`	`return getUnstructuredControlPlane(context.TODO(), s.client, s.Cluster)`
`420`	`420`	`}`
	`421`	`+`
	`422`	`+// NodePortIngressRuleCidrBlocks returns the CIDR blocks for the node NodePort ingress rules.`
	`423`	`+func (s *ClusterScope) NodePortIngressRuleCidrBlocks() []string {`
	`424`	`+ return s.AWSCluster.Spec.NetworkSpec.DeepCopy().NodePortIngressRuleCidrBlocks`
	`425`	`+}`