Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

✨ Add support for additionalControlPlaneIngressRule on AWSManaged… #4783

Merged
merged 2 commits into from
Oct 25, 2024
Merged

✨ Add support for additionalControlPlaneIngressRule on AWSManaged… #4783

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d032bbe
feat: add support for additionalControlPlaneIngressRule on AWSManaged…
fad3t Feb 9, 2024
16b98c3
refactor: simplify ingressrules
fad3t Oct 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pkg/cloud/scope/managedcontrolplane.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
7 changes: 3 additions & 4 deletions pkg/cloud/services/securitygroup/securitygroups.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -682,12 +682,11 @@ func (s *Service) getSecurityGroupIngressRules(role infrav1.SecurityGroupRole) (
}
return append(cniRules, rules...), nil
case infrav1.SecurityGroupEKSNodeAdditional:
ingressRules := s.scope.AdditionalControlPlaneIngressRules()
if s.scope.Bastion().Enabled {
return infrav1.IngressRules{
s.defaultSSHIngressRule(s.scope.SecurityGroups()[infrav1.SecurityGroupBastion].ID),
}, nil
ingressRules = append(ingressRules, s.defaultSSHIngressRule(s.scope.SecurityGroups()[infrav1.SecurityGroupBastion].ID))
}
return infrav1.IngressRules{}, nil
return ingressRules, nil
case infrav1.SecurityGroupAPIServerLB:
kubeletRules := s.getIngressRulesToAllowKubeletToAccessTheControlPlaneLB()
customIngressRules, err := s.processIngressRulesSGs(s.getControlPlaneLBIngressRules())
Expand Down
145 changes: 126 additions & 19 deletions pkg/cloud/services/securitygroup/securitygroups_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package securitygroup

import (
"context"
"reflect"
"strings"
"testing"

Expand All @@ -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"
Expand Down Expand Up @@ -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",
Expand All @@ -1220,7 +1222,7 @@ func TestAdditionalControlPlaneSecurityGroup(t *testing.T) {
},
},
},
expectedAdditionalIngresRule: infrav1.IngressRule{
expectedAdditionalIngressRule: infrav1.IngressRule{
Description: "test",
Protocol: infrav1.SecurityGroupProtocolTCP,
FromPort: 9345,
Expand Down Expand Up @@ -1251,7 +1253,7 @@ func TestAdditionalControlPlaneSecurityGroup(t *testing.T) {
},
},
},
expectedAdditionalIngresRule: infrav1.IngressRule{
expectedAdditionalIngressRule: infrav1.IngressRule{
Description: "test",
Protocol: infrav1.SecurityGroupProtocolTCP,
FromPort: 9345,
Expand Down Expand Up @@ -1282,7 +1284,7 @@ func TestAdditionalControlPlaneSecurityGroup(t *testing.T) {
},
},
},
expectedAdditionalIngresRule: infrav1.IngressRule{
expectedAdditionalIngressRule: infrav1.IngressRule{
Description: "test",
Protocol: infrav1.SecurityGroupProtocolTCP,
FromPort: 9345,
Expand Down Expand Up @@ -1314,7 +1316,7 @@ func TestAdditionalControlPlaneSecurityGroup(t *testing.T) {
},
},
},
expectedAdditionalIngresRule: infrav1.IngressRule{
expectedAdditionalIngressRule: infrav1.IngressRule{
Description: "test",
Protocol: infrav1.SecurityGroupProtocolTCP,
FromPort: 9345,
Expand Down Expand Up @@ -1345,7 +1347,7 @@ func TestAdditionalControlPlaneSecurityGroup(t *testing.T) {
},
},
},
expectedAdditionalIngresRule: infrav1.IngressRule{
expectedAdditionalIngressRule: infrav1.IngressRule{
Description: "test",
Protocol: infrav1.SecurityGroupProtocolTCP,
FromPort: 9345,
Expand Down Expand Up @@ -1376,7 +1378,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"},
Expand Down Expand Up @@ -1437,20 +1439,125 @@ 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",
Protocol: infrav1.SecurityGroupProtocolTCP,
FromPort: 9345,
ToPort: 9345,
SourceSecurityGroupIDs: []string{"cp-sg-id"},
},
},
{
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"},
fad3t marked this conversation as resolved.
Show resolved Hide resolved
},
},
},
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)
}
}
}

Expand Down