diff --git a/pkg/cloud/scope/managedcontrolplane.go b/pkg/cloud/scope/managedcontrolplane.go index 9c3b6b208d..0bccfa78e0 100644 --- a/pkg/cloud/scope/managedcontrolplane.go +++ b/pkg/cloud/scope/managedcontrolplane.go @@ -472,7 +472,7 @@ func (s *ManagedControlPlaneScope) Partition() string { // AdditionalControlPlaneIngressRules returns the additional ingress rules for the control plane security group. func (s *ManagedControlPlaneScope) AdditionalControlPlaneIngressRules() []infrav1.IngressRule { - return nil + return s.ControlPlane.Spec.NetworkSpec.DeepCopy().AdditionalControlPlaneIngressRules } // UnstructuredControlPlane returns the unstructured object for the control plane, if any. diff --git a/pkg/cloud/services/securitygroup/securitygroups.go b/pkg/cloud/services/securitygroup/securitygroups.go index f1f82193cc..01d3fcc627 100644 --- a/pkg/cloud/services/securitygroup/securitygroups.go +++ b/pkg/cloud/services/securitygroup/securitygroups.go @@ -679,12 +679,12 @@ func (s *Service) getSecurityGroupIngressRules(role infrav1.SecurityGroupRole) ( } return append(cniRules, rules...), nil case infrav1.SecurityGroupEKSNodeAdditional: + rules := infrav1.IngressRules{} if s.scope.Bastion().Enabled { - return infrav1.IngressRules{ - s.defaultSSHIngressRule(s.scope.SecurityGroups()[infrav1.SecurityGroupBastion].ID), - }, nil + rules = append(rules, s.defaultSSHIngressRule(s.scope.SecurityGroups()[infrav1.SecurityGroupBastion].ID)) } - return infrav1.IngressRules{}, nil + ingressRules := s.scope.AdditionalControlPlaneIngressRules() + return append(rules, ingressRules...), nil case infrav1.SecurityGroupAPIServerLB: kubeletRules := s.getIngressRulesToAllowKubeletToAccessTheControlPlaneLB() customIngressRules, err := s.processIngressRulesSGs(s.getControlPlaneLBIngressRules()) diff --git a/pkg/cloud/services/securitygroup/securitygroups_test.go b/pkg/cloud/services/securitygroup/securitygroups_test.go index 3bdf795ea8..a6af430087 100644 --- a/pkg/cloud/services/securitygroup/securitygroups_test.go +++ b/pkg/cloud/services/securitygroup/securitygroups_test.go @@ -18,6 +18,7 @@ package securitygroup import ( "context" + "reflect" "strings" "testing" @@ -34,6 +35,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client/fake" infrav1 "sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta2" + ekscontrolplanev1 "sigs.k8s.io/cluster-api-provider-aws/v2/controlplane/eks/api/v1beta2" "sigs.k8s.io/cluster-api-provider-aws/v2/pkg/cloud/awserrors" "sigs.k8s.io/cluster-api-provider-aws/v2/pkg/cloud/filter" "sigs.k8s.io/cluster-api-provider-aws/v2/pkg/cloud/scope" @@ -1192,11 +1194,11 @@ func TestAdditionalControlPlaneSecurityGroup(t *testing.T) { _ = infrav1.AddToScheme(scheme) testCases := []struct { - name string - networkSpec infrav1.NetworkSpec - networkStatus infrav1.NetworkStatus - expectedAdditionalIngresRule infrav1.IngressRule - wantErr bool + name string + networkSpec infrav1.NetworkSpec + networkStatus infrav1.NetworkStatus + expectedAdditionalIngressRule infrav1.IngressRule + wantErr bool }{ { name: "default control plane security group is used", @@ -1215,24 +1217,38 @@ func TestAdditionalControlPlaneSecurityGroup(t *testing.T) { infrav1.SecurityGroupControlPlane: { ID: "cp-sg-id", }, - infrav1.SecurityGroupNode: { - ID: "node-sg-id", - }, + ID: "node-sg-id", }, }, - expectedAdditionalIngresRule: infrav1.IngressRule{ - Description: "test", - Protocol: infrav1.SecurityGroupProtocolTCP, - FromPort: 9345, - ToPort: 9345, - SourceSecurityGroupIDs: []string{"cp-sg-id"}, + // }, + expectedAdditionalIngressRule: infrav1.IngressRule{ + Description: "test", + Protocol: infrav1.SecurityGroupProtocolTCP, + FromPort: 9345, }, - }, - { - name: "custom security group id is used", - networkSpec: infrav1.NetworkSpec{ - AdditionalControlPlaneIngressRules: []infrav1.IngressRule{ - { + { + name: "custom security group id is used", + networkSpec: infrav1.NetworkSpec{ + AdditionalControlPlaneIngressRules: []infrav1.IngressRule{ + { + Description: "test", + Protocol: infrav1.SecurityGroupProtocolTCP, + FromPort: 9345, + ToPort: 9345, + SourceSecurityGroupIDs: []string{"test"}, + }, + }, + }, + networkStatus: infrav1.NetworkStatus{ + SecurityGroups: map[infrav1.SecurityGroupRole]infrav1.SecurityGroup{ + infrav1.SecurityGroupControlPlane: { + ID: "cp-sg-id", + }, + infrav1.SecurityGroupNode: { + ID: "node-sg-id", + }, + }, + expectedAdditionalIngressRule: infrav1.IngressRule{ Description: "test", Protocol: infrav1.SecurityGroupProtocolTCP, FromPort: 9345, @@ -1240,28 +1256,6 @@ func TestAdditionalControlPlaneSecurityGroup(t *testing.T) { SourceSecurityGroupIDs: []string{"test"}, }, }, - }, - networkStatus: infrav1.NetworkStatus{ - SecurityGroups: map[infrav1.SecurityGroupRole]infrav1.SecurityGroup{ - infrav1.SecurityGroupControlPlane: { - ID: "cp-sg-id", - }, - infrav1.SecurityGroupNode: { - ID: "node-sg-id", - }, - }, - }, - expectedAdditionalIngresRule: infrav1.IngressRule{ - Description: "test", - Protocol: infrav1.SecurityGroupProtocolTCP, - FromPort: 9345, - ToPort: 9345, - SourceSecurityGroupIDs: []string{"test"}, - }, - }, - { - name: "another security group role is used", - networkSpec: infrav1.NetworkSpec{ AdditionalControlPlaneIngressRules: []infrav1.IngressRule{ { Description: "test", @@ -1282,7 +1276,7 @@ func TestAdditionalControlPlaneSecurityGroup(t *testing.T) { }, }, }, - expectedAdditionalIngresRule: infrav1.IngressRule{ + expectedAdditionalIngressRule: infrav1.IngressRule{ Description: "test", Protocol: infrav1.SecurityGroupProtocolTCP, FromPort: 9345, @@ -1294,16 +1288,13 @@ func TestAdditionalControlPlaneSecurityGroup(t *testing.T) { name: "another security group role and a custom security group id is used", networkSpec: infrav1.NetworkSpec{ AdditionalControlPlaneIngressRules: []infrav1.IngressRule{ - { - Description: "test", - Protocol: infrav1.SecurityGroupProtocolTCP, - FromPort: 9345, - ToPort: 9345, - SourceSecurityGroupIDs: []string{"test"}, - SourceSecurityGroupRoles: []infrav1.SecurityGroupRole{infrav1.SecurityGroupNode}, - }, + FromPort: 9345, + ToPort: 9345, + SourceSecurityGroupIDs: []string{"test"}, + SourceSecurityGroupRoles: []infrav1.SecurityGroupRole{infrav1.SecurityGroupNode}, }, }, + // }, networkStatus: infrav1.NetworkStatus{ SecurityGroups: map[infrav1.SecurityGroupRole]infrav1.SecurityGroup{ infrav1.SecurityGroupControlPlane: { @@ -1314,7 +1305,7 @@ func TestAdditionalControlPlaneSecurityGroup(t *testing.T) { }, }, }, - expectedAdditionalIngresRule: infrav1.IngressRule{ + expectedAdditionalIngressRule: infrav1.IngressRule{ Description: "test", Protocol: infrav1.SecurityGroupProtocolTCP, FromPort: 9345, @@ -1329,7 +1320,6 @@ func TestAdditionalControlPlaneSecurityGroup(t *testing.T) { { Description: "test", Protocol: infrav1.SecurityGroupProtocolTCP, - FromPort: 9345, ToPort: 9345, CidrBlocks: []string{"test-cidr-block"}, }, @@ -1340,12 +1330,10 @@ func TestAdditionalControlPlaneSecurityGroup(t *testing.T) { infrav1.SecurityGroupControlPlane: { ID: "cp-sg-id", }, - infrav1.SecurityGroupNode: { - ID: "node-sg-id", - }, + ID: "node-sg-id", }, }, - expectedAdditionalIngresRule: infrav1.IngressRule{ + expectedAdditionalIngressRule: infrav1.IngressRule{ Description: "test", Protocol: infrav1.SecurityGroupProtocolTCP, FromPort: 9345, @@ -1376,7 +1364,7 @@ func TestAdditionalControlPlaneSecurityGroup(t *testing.T) { }, NatGatewaysIPs: []string{"test-ip"}, }, - expectedAdditionalIngresRule: infrav1.IngressRule{ + expectedAdditionalIngressRule: infrav1.IngressRule{ Description: "test", Protocol: infrav1.SecurityGroupProtocolTCP, CidrBlocks: []string{"test-ip/32"}, @@ -1437,20 +1425,121 @@ func TestAdditionalControlPlaneSecurityGroup(t *testing.T) { } found = true - if r.Protocol != tc.expectedAdditionalIngresRule.Protocol { - t.Fatalf("Expected protocol %s, got %s", tc.expectedAdditionalIngresRule.Protocol, r.Protocol) + if r.Protocol != tc.expectedAdditionalIngressRule.Protocol { + t.Fatalf("Expected protocol %s, got %s", tc.expectedAdditionalIngressRule.Protocol, r.Protocol) } - if r.FromPort != tc.expectedAdditionalIngresRule.FromPort { - t.Fatalf("Expected from port %d, got %d", tc.expectedAdditionalIngresRule.FromPort, r.FromPort) + if r.FromPort != tc.expectedAdditionalIngressRule.FromPort { + t.Fatalf("Expected from port %d, got %d", tc.expectedAdditionalIngressRule.FromPort, r.FromPort) } - if r.ToPort != tc.expectedAdditionalIngresRule.ToPort { - t.Fatalf("Expected to port %d, got %d", tc.expectedAdditionalIngresRule.ToPort, r.ToPort) + if r.ToPort != tc.expectedAdditionalIngressRule.ToPort { + t.Fatalf("Expected to port %d, got %d", tc.expectedAdditionalIngressRule.ToPort, r.ToPort) } - if !sets.New(tc.expectedAdditionalIngresRule.SourceSecurityGroupIDs...).Equal(sets.New(r.SourceSecurityGroupIDs...)) { - t.Fatalf("Expected source security group IDs %v, got %v", tc.expectedAdditionalIngresRule.SourceSecurityGroupIDs, r.SourceSecurityGroupIDs) + if !sets.New[string](tc.expectedAdditionalIngressRule.SourceSecurityGroupIDs...).Equal(sets.New[string](tc.expectedAdditionalIngressRule.SourceSecurityGroupIDs...)) { + t.Fatalf("Expected source security group IDs %v, got %v", tc.expectedAdditionalIngressRule.SourceSecurityGroupIDs, r.SourceSecurityGroupIDs) + } + } + + if !found { + t.Fatal("Additional ingress rule was not found") + } + }) + } +} + +func TestAdditionalManagedControlPlaneSecurityGroup(t *testing.T) { + scheme := runtime.NewScheme() + _ = ekscontrolplanev1.AddToScheme(scheme) + + testCases := []struct { + name string + networkSpec infrav1.NetworkSpec + expectedAdditionalIngressRule infrav1.IngressRule + }{ + { + name: "default control plane security group is used", + networkSpec: infrav1.NetworkSpec{ + AdditionalControlPlaneIngressRules: []infrav1.IngressRule{ + { + Description: "test", + Protocol: infrav1.SecurityGroupProtocolTCP, + FromPort: 9345, + ToPort: 9345, + }, + }, + }, + expectedAdditionalIngressRule: infrav1.IngressRule{ + Description: "test", + }, + }, + { + name: "don't set source security groups if cidr blocks are set", + networkSpec: infrav1.NetworkSpec{ + AdditionalControlPlaneIngressRules: []infrav1.IngressRule{ + { + Description: "test", + Protocol: infrav1.SecurityGroupProtocolTCP, + FromPort: 9345, + ToPort: 9345, + CidrBlocks: []string{"test-cidr-block"}, + }, + }, + }, + expectedAdditionalIngressRule: infrav1.IngressRule{ + Description: "test", + Protocol: infrav1.SecurityGroupProtocolTCP, + FromPort: 9345, + ToPort: 9345, + CidrBlocks: []string{"test-cidr-block"}, + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + cs, err := scope.NewManagedControlPlaneScope(scope.ManagedControlPlaneScopeParams{ + Client: fake.NewClientBuilder().WithScheme(scheme).Build(), + Cluster: &clusterv1.Cluster{ + ObjectMeta: metav1.ObjectMeta{Name: "test-cluster"}, + }, + ControlPlane: &ekscontrolplanev1.AWSManagedControlPlane{ + Spec: ekscontrolplanev1.AWSManagedControlPlaneSpec{ + NetworkSpec: tc.networkSpec, + }, + Status: ekscontrolplanev1.AWSManagedControlPlaneStatus{ + Network: infrav1.NetworkStatus{ + SecurityGroups: map[infrav1.SecurityGroupRole]infrav1.SecurityGroup{ + infrav1.SecurityGroupControlPlane: { + ID: "cp-sg-id", + }, + infrav1.SecurityGroupNode: { + ID: "node-sg-id", + }, + }, + }, + }, + }, + }) + if err != nil { + t.Fatalf("Failed to create test context: %v", err) + } + + s := NewService(cs, testSecurityGroupRoles) + rules, err := s.getSecurityGroupIngressRules(infrav1.SecurityGroupControlPlane) + if err != nil { + t.Fatalf("Failed to lookup controlplane security group ingress rules: %v", err) + } + + found := false + for _, r := range rules { + if r.Description == "test" { + found = true + + if !reflect.DeepEqual(r, tc.expectedAdditionalIngressRule) { + t.Fatalf("Expected ingress rule %#v, got %#v", tc.expectedAdditionalIngressRule, r) + } } }