From 1d206bf80d1d5e47173bffa4eeff89730e0276df Mon Sep 17 00:00:00 2001 From: Alexandr Demicev Date: Fri, 23 Jun 2023 17:14:10 +0200 Subject: [PATCH] Add additional ingress rules for CP to API Fix setting additional ingress rules for CP Update documentation with CP ingress rules --- api/v1beta1/awscluster_conversion.go | 2 + api/v1beta1/conversion.go | 4 + api/v1beta1/zz_generated.conversion.go | 16 +- api/v1beta2/awscluster_webhook.go | 12 +- api/v1beta2/awscluster_webhook_test.go | 85 +++++++++++ api/v1beta2/network_types.go | 4 + api/v1beta2/zz_generated.deepcopy.go | 7 + ...ster.x-k8s.io_awsmanagedcontrolplanes.yaml | 144 ++++++++++++++++++ ...tructure.cluster.x-k8s.io_awsclusters.yaml | 72 +++++++++ ....cluster.x-k8s.io_awsclustertemplates.yaml | 74 +++++++++ .../api/v1beta1/zz_generated.conversion.go | 5 - .../bring-your-own-aws-infrastructure.md | 15 +- pkg/cloud/scope/cluster.go | 5 + pkg/cloud/scope/managedcontrolplane.go | 5 + pkg/cloud/scope/sg.go | 3 + .../services/securitygroup/securitygroups.go | 29 ++-- .../securitygroup/securitygroups_test.go | 43 ++++-- 17 files changed, 483 insertions(+), 42 deletions(-) diff --git a/api/v1beta1/awscluster_conversion.go b/api/v1beta1/awscluster_conversion.go index 3a5279936a..a95efaa113 100644 --- a/api/v1beta1/awscluster_conversion.go +++ b/api/v1beta1/awscluster_conversion.go @@ -55,6 +55,8 @@ func (src *AWSCluster) ConvertTo(dstRaw conversion.Hub) error { } dst.Status.Network.NatGatewaysIPs = restored.Status.Network.NatGatewaysIPs + dst.Spec.NetworkSpec.AdditionalControlPlaneIngressRules = restored.Spec.NetworkSpec.AdditionalControlPlaneIngressRules + return nil } diff --git a/api/v1beta1/conversion.go b/api/v1beta1/conversion.go index 2f7c76b006..aedb43b0ee 100644 --- a/api/v1beta1/conversion.go +++ b/api/v1beta1/conversion.go @@ -82,3 +82,7 @@ func Convert_v1beta2_LoadBalancer_To_v1beta1_ClassicELB(in *v1beta2.LoadBalancer func Convert_v1beta2_IngressRule_To_v1beta1_IngressRule(in *v1beta2.IngressRule, out *IngressRule, s conversion.Scope) error { return autoConvert_v1beta2_IngressRule_To_v1beta1_IngressRule(in, out, s) } + +func Convert_v1beta2_NetworkSpec_To_v1beta1_NetworkSpec(in *v1beta2.NetworkSpec, out *NetworkSpec, s conversion.Scope) error { + return autoConvert_v1beta2_NetworkSpec_To_v1beta1_NetworkSpec(in, out, s) +} diff --git a/api/v1beta1/zz_generated.conversion.go b/api/v1beta1/zz_generated.conversion.go index 6267c3c7d7..66b4adbf3f 100644 --- a/api/v1beta1/zz_generated.conversion.go +++ b/api/v1beta1/zz_generated.conversion.go @@ -470,11 +470,6 @@ func RegisterConversions(s *runtime.Scheme) error { }); err != nil { return err } - if err := s.AddGeneratedConversionFunc((*v1beta2.NetworkSpec)(nil), (*NetworkSpec)(nil), func(a, b interface{}, scope conversion.Scope) error { - return Convert_v1beta2_NetworkSpec_To_v1beta1_NetworkSpec(a.(*v1beta2.NetworkSpec), b.(*NetworkSpec), scope) - }); err != nil { - return err - } if err := s.AddGeneratedConversionFunc((*NetworkStatus)(nil), (*v1beta2.NetworkStatus)(nil), func(a, b interface{}, scope conversion.Scope) error { return Convert_v1beta1_NetworkStatus_To_v1beta2_NetworkStatus(a.(*NetworkStatus), b.(*v1beta2.NetworkStatus), scope) }); err != nil { @@ -595,6 +590,11 @@ func RegisterConversions(s *runtime.Scheme) error { }); err != nil { return err } + if err := s.AddConversionFunc((*v1beta2.NetworkSpec)(nil), (*NetworkSpec)(nil), func(a, b interface{}, scope conversion.Scope) error { + return Convert_v1beta2_NetworkSpec_To_v1beta1_NetworkSpec(a.(*v1beta2.NetworkSpec), b.(*NetworkSpec), scope) + }); err != nil { + return err + } if err := s.AddConversionFunc((*v1beta2.NetworkStatus)(nil), (*NetworkStatus)(nil), func(a, b interface{}, scope conversion.Scope) error { return Convert_v1beta2_NetworkStatus_To_v1beta1_NetworkStatus(a.(*v1beta2.NetworkStatus), b.(*NetworkStatus), scope) }); err != nil { @@ -2022,14 +2022,10 @@ func autoConvert_v1beta2_NetworkSpec_To_v1beta1_NetworkSpec(in *v1beta2.NetworkS out.Subnets = *(*Subnets)(unsafe.Pointer(&in.Subnets)) out.CNI = (*CNISpec)(unsafe.Pointer(in.CNI)) out.SecurityGroupOverrides = *(*map[SecurityGroupRole]string)(unsafe.Pointer(&in.SecurityGroupOverrides)) + // WARNING: in.AdditionalControlPlaneIngressRules requires manual conversion: does not exist in peer-type return nil } -// Convert_v1beta2_NetworkSpec_To_v1beta1_NetworkSpec is an autogenerated conversion function. -func Convert_v1beta2_NetworkSpec_To_v1beta1_NetworkSpec(in *v1beta2.NetworkSpec, out *NetworkSpec, s conversion.Scope) error { - return autoConvert_v1beta2_NetworkSpec_To_v1beta1_NetworkSpec(in, out, s) -} - func autoConvert_v1beta1_NetworkStatus_To_v1beta2_NetworkStatus(in *NetworkStatus, out *v1beta2.NetworkStatus, s conversion.Scope) error { if in.SecurityGroups != nil { in, out := &in.SecurityGroups, &out.SecurityGroups diff --git a/api/v1beta2/awscluster_webhook.go b/api/v1beta2/awscluster_webhook.go index 51d35f18ba..3c24fdca8e 100644 --- a/api/v1beta2/awscluster_webhook.go +++ b/api/v1beta2/awscluster_webhook.go @@ -56,7 +56,7 @@ func (r *AWSCluster) ValidateCreate() error { allErrs = append(allErrs, r.Spec.AdditionalTags.Validate()...) allErrs = append(allErrs, r.Spec.S3Bucket.Validate()...) allErrs = append(allErrs, r.validateNetwork()...) - allErrs = append(allErrs, r.validateAdditionalIngressRules()...) + allErrs = append(allErrs, r.validateControlPlaneLBIngressRules()...) return aggregateObjErrors(r.GroupVersionKind().GroupKind(), r.Name, allErrs) } @@ -189,10 +189,16 @@ func (r *AWSCluster) validateNetwork() field.ErrorList { allErrs = append(allErrs, field.Invalid(field.NewPath("subnets"), r.Spec.NetworkSpec.Subnets, "IPv6 cannot be used with unmanaged clusters at this time.")) } } + + for _, rule := range r.Spec.NetworkSpec.AdditionalControlPlaneIngressRules { + if (rule.CidrBlocks != nil || rule.IPv6CidrBlocks != nil) && (rule.SourceSecurityGroupIDs != nil || rule.SourceSecurityGroupRoles != nil) { + allErrs = append(allErrs, field.Invalid(field.NewPath("additionalControlPlaneIngressRules"), r.Spec.NetworkSpec.AdditionalControlPlaneIngressRules, "CIDR blocks and security group IDs or security group roles cannot be used together")) + } + } return allErrs } -func (r *AWSCluster) validateAdditionalIngressRules() field.ErrorList { +func (r *AWSCluster) validateControlPlaneLBIngressRules() field.ErrorList { var allErrs field.ErrorList if r.Spec.ControlPlaneLoadBalancer == nil { @@ -201,7 +207,7 @@ func (r *AWSCluster) validateAdditionalIngressRules() field.ErrorList { for _, rule := range r.Spec.ControlPlaneLoadBalancer.IngressRules { if (rule.CidrBlocks != nil || rule.IPv6CidrBlocks != nil) && (rule.SourceSecurityGroupIDs != nil || rule.SourceSecurityGroupRoles != nil) { - allErrs = append(allErrs, field.Invalid(field.NewPath("additionalIngressRules"), r.Spec.ControlPlaneLoadBalancer.IngressRules, "CIDR blocks and security group IDs or security group roles cannot be used together")) + allErrs = append(allErrs, field.Invalid(field.NewPath("spec", "controlPlaneLoadBalancer", "ingressRules"), r.Spec.ControlPlaneLoadBalancer.IngressRules, "CIDR blocks and security group IDs or security group roles cannot be used together")) } } diff --git a/api/v1beta2/awscluster_webhook_test.go b/api/v1beta2/awscluster_webhook_test.go index dfce2b2c7c..8054c5ea4a 100644 --- a/api/v1beta2/awscluster_webhook_test.go +++ b/api/v1beta2/awscluster_webhook_test.go @@ -335,6 +335,91 @@ func TestAWSClusterValidateCreate(t *testing.T) { }, wantErr: false, }, + { + name: "accepts CP ingress rules with source security group id and role", + cluster: &AWSCluster{ + Spec: AWSClusterSpec{ + NetworkSpec: NetworkSpec{ + AdditionalControlPlaneIngressRules: []IngressRule{ + { + Protocol: SecurityGroupProtocolTCP, + SourceSecurityGroupIDs: []string{"test"}, + SourceSecurityGroupRoles: []SecurityGroupRole{SecurityGroupBastion}, + }, + }, + }, + }, + }, + wantErr: false, + }, + { + name: "rejects CP ingress rules with cidr block and source security group id", + cluster: &AWSCluster{ + Spec: AWSClusterSpec{ + NetworkSpec: NetworkSpec{ + AdditionalControlPlaneIngressRules: []IngressRule{ + { + Protocol: SecurityGroupProtocolTCP, + CidrBlocks: []string{"test"}, + SourceSecurityGroupIDs: []string{"test"}, + }, + }, + }, + }, + }, + wantErr: true, + }, + { + name: "rejects CP ingress rules with cidr block and source security group id and role", + cluster: &AWSCluster{ + Spec: AWSClusterSpec{ + NetworkSpec: NetworkSpec{ + AdditionalControlPlaneIngressRules: []IngressRule{ + { + Protocol: SecurityGroupProtocolTCP, + IPv6CidrBlocks: []string{"test"}, + SourceSecurityGroupIDs: []string{"test"}, + SourceSecurityGroupRoles: []SecurityGroupRole{SecurityGroupBastion}, + }, + }, + }, + }, + }, + wantErr: true, + }, + { + name: "accepts CP ingress rules with cidr block", + cluster: &AWSCluster{ + Spec: AWSClusterSpec{ + NetworkSpec: NetworkSpec{ + AdditionalControlPlaneIngressRules: []IngressRule{ + { + Protocol: SecurityGroupProtocolTCP, + CidrBlocks: []string{"test"}, + }, + }, + }, + }, + }, + wantErr: false, + }, + { + name: "accepts CP ingress rules with source security group id and role", + cluster: &AWSCluster{ + Spec: AWSClusterSpec{ + NetworkSpec: NetworkSpec{ + AdditionalControlPlaneIngressRules: []IngressRule{ + { + Protocol: SecurityGroupProtocolTCP, + SourceSecurityGroupIDs: []string{"test"}, + SourceSecurityGroupRoles: []SecurityGroupRole{SecurityGroupBastion}, + }, + }, + }, + }, + }, + wantErr: false, + }, } 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 index 89dccf1d5f..8503319bf0 100644 --- a/api/v1beta2/network_types.go +++ b/api/v1beta2/network_types.go @@ -240,6 +240,10 @@ type NetworkSpec struct { // This is optional - if not provided new security groups will be created for the cluster // +optional SecurityGroupOverrides map[SecurityGroupRole]string `json:"securityGroupOverrides,omitempty"` + + // AdditionalControlPlaneIngressRules is an optional set of ingress rules to add to the control plane + // +optional + AdditionalControlPlaneIngressRules []IngressRule `json:"additionalControlPlaneIngressRules,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 index 17c3823791..7041557d04 100644 --- a/api/v1beta2/zz_generated.deepcopy.go +++ b/api/v1beta2/zz_generated.deepcopy.go @@ -1561,6 +1561,13 @@ func (in *NetworkSpec) DeepCopyInto(out *NetworkSpec) { (*out)[key] = val } } + if in.AdditionalControlPlaneIngressRules != nil { + in, out := &in.AdditionalControlPlaneIngressRules, &out.AdditionalControlPlaneIngressRules + *out = make([]IngressRule, len(*in)) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new NetworkSpec. diff --git a/config/crd/bases/controlplane.cluster.x-k8s.io_awsmanagedcontrolplanes.yaml b/config/crd/bases/controlplane.cluster.x-k8s.io_awsmanagedcontrolplanes.yaml index ae493fda4c..ede1acb7a2 100644 --- a/config/crd/bases/controlplane.cluster.x-k8s.io_awsmanagedcontrolplanes.yaml +++ b/config/crd/bases/controlplane.cluster.x-k8s.io_awsmanagedcontrolplanes.yaml @@ -357,6 +357,78 @@ spec: network: description: NetworkSpec encapsulates all things related to AWS network. properties: + additionalControlPlaneIngressRules: + description: AdditionalControlPlaneIngressRules is an optional + set of ingress rules to add to the control plane + items: + description: IngressRule defines an AWS ingress rule for security + groups. + properties: + cidrBlocks: + description: List of CIDR blocks to allow access from. Cannot + be specified with SourceSecurityGroupID. + items: + type: string + type: array + description: + description: Description provides extended information about + the ingress rule. + type: string + fromPort: + description: FromPort is the start of port range. + format: int64 + type: integer + ipv6CidrBlocks: + description: List of IPv6 CIDR blocks to allow access from. + Cannot be specified with SourceSecurityGroupID. + items: + type: string + type: array + protocol: + description: Protocol is the protocol for the ingress rule. + Accepted values are "-1" (all), "4" (IP in IP),"tcp", + "udp", "icmp", and "58" (ICMPv6). + enum: + - "-1" + - "4" + - tcp + - udp + - icmp + - "58" + type: string + sourceSecurityGroupIds: + description: The security group id to allow access from. + Cannot be specified with CidrBlocks. + items: + type: string + type: array + sourceSecurityGroupRoles: + description: The security group role to allow access from. + Cannot be specified with CidrBlocks. The field will be + combined with source security group IDs if specified. + items: + description: SecurityGroupRole defines the unique role + of a security group. + enum: + - bastion + - node + - controlplane + - apiserver-lb + - lb + - node-eks-additional + type: string + type: array + toPort: + description: ToPort is the end of port range. + format: int64 + type: integer + required: + - description + - fromPort + - protocol + - toPort + type: object + type: array cni: description: CNI configuration properties: @@ -1811,6 +1883,78 @@ spec: network: description: NetworkSpec encapsulates all things related to AWS network. properties: + additionalControlPlaneIngressRules: + description: AdditionalControlPlaneIngressRules is an optional + set of ingress rules to add to the control plane + items: + description: IngressRule defines an AWS ingress rule for security + groups. + properties: + cidrBlocks: + description: List of CIDR blocks to allow access from. Cannot + be specified with SourceSecurityGroupID. + items: + type: string + type: array + description: + description: Description provides extended information about + the ingress rule. + type: string + fromPort: + description: FromPort is the start of port range. + format: int64 + type: integer + ipv6CidrBlocks: + description: List of IPv6 CIDR blocks to allow access from. + Cannot be specified with SourceSecurityGroupID. + items: + type: string + type: array + protocol: + description: Protocol is the protocol for the ingress rule. + Accepted values are "-1" (all), "4" (IP in IP),"tcp", + "udp", "icmp", and "58" (ICMPv6). + enum: + - "-1" + - "4" + - tcp + - udp + - icmp + - "58" + type: string + sourceSecurityGroupIds: + description: The security group id to allow access from. + Cannot be specified with CidrBlocks. + items: + type: string + type: array + sourceSecurityGroupRoles: + description: The security group role to allow access from. + Cannot be specified with CidrBlocks. The field will be + combined with source security group IDs if specified. + items: + description: SecurityGroupRole defines the unique role + of a security group. + enum: + - bastion + - node + - controlplane + - apiserver-lb + - lb + - node-eks-additional + type: string + type: array + toPort: + description: ToPort is the end of port range. + format: int64 + type: integer + required: + - description + - fromPort + - protocol + - toPort + type: object + type: array cni: description: CNI configuration properties: diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_awsclusters.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_awsclusters.yaml index 8c88ca2c7b..a1bea16275 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_awsclusters.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_awsclusters.yaml @@ -1160,6 +1160,78 @@ spec: network: description: NetworkSpec encapsulates all things related to AWS network. properties: + additionalControlPlaneIngressRules: + description: AdditionalControlPlaneIngressRules is an optional + set of ingress rules to add to the control plane + items: + description: IngressRule defines an AWS ingress rule for security + groups. + properties: + cidrBlocks: + description: List of CIDR blocks to allow access from. Cannot + be specified with SourceSecurityGroupID. + items: + type: string + type: array + description: + description: Description provides extended information about + the ingress rule. + type: string + fromPort: + description: FromPort is the start of port range. + format: int64 + type: integer + ipv6CidrBlocks: + description: List of IPv6 CIDR blocks to allow access from. + Cannot be specified with SourceSecurityGroupID. + items: + type: string + type: array + protocol: + description: Protocol is the protocol for the ingress rule. + Accepted values are "-1" (all), "4" (IP in IP),"tcp", + "udp", "icmp", and "58" (ICMPv6). + enum: + - "-1" + - "4" + - tcp + - udp + - icmp + - "58" + type: string + sourceSecurityGroupIds: + description: The security group id to allow access from. + Cannot be specified with CidrBlocks. + items: + type: string + type: array + sourceSecurityGroupRoles: + description: The security group role to allow access from. + Cannot be specified with CidrBlocks. The field will be + combined with source security group IDs if specified. + items: + description: SecurityGroupRole defines the unique role + of a security group. + enum: + - bastion + - node + - controlplane + - apiserver-lb + - lb + - node-eks-additional + type: string + type: array + toPort: + description: ToPort is the end of port range. + format: int64 + type: integer + required: + - description + - fromPort + - protocol + - toPort + type: object + type: array cni: description: CNI configuration properties: diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_awsclustertemplates.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_awsclustertemplates.yaml index 66f3927be0..ba1606a8ba 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_awsclustertemplates.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_awsclustertemplates.yaml @@ -761,6 +761,80 @@ spec: description: NetworkSpec encapsulates all things related to AWS network. properties: + additionalControlPlaneIngressRules: + description: AdditionalControlPlaneIngressRules is an + optional set of ingress rules to add to the control + plane + items: + description: IngressRule defines an AWS ingress rule + for security groups. + properties: + cidrBlocks: + description: List of CIDR blocks to allow access + from. Cannot be specified with SourceSecurityGroupID. + items: + type: string + type: array + description: + description: Description provides extended information + about the ingress rule. + type: string + fromPort: + description: FromPort is the start of port range. + format: int64 + type: integer + ipv6CidrBlocks: + description: List of IPv6 CIDR blocks to allow access + from. Cannot be specified with SourceSecurityGroupID. + items: + type: string + type: array + protocol: + description: Protocol is the protocol for the ingress + rule. Accepted values are "-1" (all), "4" (IP + in IP),"tcp", "udp", "icmp", and "58" (ICMPv6). + enum: + - "-1" + - "4" + - tcp + - udp + - icmp + - "58" + type: string + sourceSecurityGroupIds: + description: The security group id to allow access + from. Cannot be specified with CidrBlocks. + items: + type: string + type: array + sourceSecurityGroupRoles: + description: The security group role to allow access + from. Cannot be specified with CidrBlocks. The + field will be combined with source security group + IDs if specified. + items: + description: SecurityGroupRole defines the unique + role of a security group. + enum: + - bastion + - node + - controlplane + - apiserver-lb + - lb + - node-eks-additional + type: string + type: array + toPort: + description: ToPort is the end of port range. + format: int64 + type: integer + required: + - description + - fromPort + - protocol + - toPort + type: object + type: array cni: description: CNI configuration properties: diff --git a/controlplane/eks/api/v1beta1/zz_generated.conversion.go b/controlplane/eks/api/v1beta1/zz_generated.conversion.go index 4b9f0df378..ecc37543d6 100644 --- a/controlplane/eks/api/v1beta1/zz_generated.conversion.go +++ b/controlplane/eks/api/v1beta1/zz_generated.conversion.go @@ -245,11 +245,6 @@ func RegisterConversions(s *runtime.Scheme) error { }); err != nil { return err } - if err := s.AddConversionFunc((*apiv1beta2.NetworkSpec)(nil), (*apiv1beta1.NetworkSpec)(nil), func(a, b interface{}, scope conversion.Scope) error { - return Convert_v1beta2_NetworkSpec_To_v1beta1_NetworkSpec(a.(*apiv1beta2.NetworkSpec), b.(*apiv1beta1.NetworkSpec), scope) - }); err != nil { - return err - } if err := s.AddConversionFunc((*v1beta2.VpcCni)(nil), (*VpcCni)(nil), func(a, b interface{}, scope conversion.Scope) error { return Convert_v1beta2_VpcCni_To_v1beta1_VpcCni(a.(*v1beta2.VpcCni), b.(*VpcCni), scope) }); err != nil { diff --git a/docs/book/src/topics/bring-your-own-aws-infrastructure.md b/docs/book/src/topics/bring-your-own-aws-infrastructure.md index 51b4e52372..4841425158 100644 --- a/docs/book/src/topics/bring-your-own-aws-infrastructure.md +++ b/docs/book/src/topics/bring-your-own-aws-infrastructure.md @@ -142,7 +142,7 @@ To specify additional security groups for the control plane load balancer for a ```yaml spec: controlPlaneLoadBalancer: - AdditionalsecurityGroups: + additionalSecurityGroups: - sg-0200a3507a5ad2c5c8c3 - ... ``` @@ -175,6 +175,19 @@ spec: > >An incorrectly configured Classic ELB can easily lead to a non-functional cluster. We strongly recommend you let Cluster API create the Classic ELB. +### Control Plane ingress rules + +It's possible to specify custom ingress rules for the control plane itself. To do so, add this to the AWSCluster specification: + +```yaml +spec: + network: + additionalControlPlaneIngressRules: + - description: "example ingress rule" + protocol: "-1" # all + fromPort: 7777 + toPort: 7777 +``` ### Caveats/Notes * When both public and private subnets are available in an AZ, CAPI will choose the private subnet in the AZ over the public subnet for placing EC2 instances. diff --git a/pkg/cloud/scope/cluster.go b/pkg/cloud/scope/cluster.go index 7c5e53ec54..1f939fdd0f 100644 --- a/pkg/cloud/scope/cluster.go +++ b/pkg/cloud/scope/cluster.go @@ -380,3 +380,8 @@ func (s *ClusterScope) Partition() string { } return s.AWSCluster.Spec.Partition } + +// AdditionalControlPlaneIngressRules returns the additional ingress rules for control plane security group. +func (s *ClusterScope) AdditionalControlPlaneIngressRules() []infrav1.IngressRule { + return s.AWSCluster.Spec.NetworkSpec.DeepCopy().AdditionalControlPlaneIngressRules +} diff --git a/pkg/cloud/scope/managedcontrolplane.go b/pkg/cloud/scope/managedcontrolplane.go index 7f369ac5fb..63b5f895d6 100644 --- a/pkg/cloud/scope/managedcontrolplane.go +++ b/pkg/cloud/scope/managedcontrolplane.go @@ -428,3 +428,8 @@ func (s *ManagedControlPlaneScope) Partition() string { } return s.ControlPlane.Spec.Partition } + +// AdditionalControlPlaneIngressRules returns the additional ingress rules for the control plane security group. +func (s *ManagedControlPlaneScope) AdditionalControlPlaneIngressRules() []infrav1.IngressRule { + return nil +} diff --git a/pkg/cloud/scope/sg.go b/pkg/cloud/scope/sg.go index 2db4d64519..793ff80669 100644 --- a/pkg/cloud/scope/sg.go +++ b/pkg/cloud/scope/sg.go @@ -51,4 +51,7 @@ type SGScope interface { // GetNatGatewaysIPs gets the Nat Gateways Public IPs. GetNatGatewaysIPs() []string + + // AdditionalControlPlaneIngressRules returns the additional ingress rules for the control plane security group. + AdditionalControlPlaneIngressRules() []infrav1.IngressRule } diff --git a/pkg/cloud/services/securitygroup/securitygroups.go b/pkg/cloud/services/securitygroup/securitygroups.go index 19e1ac2b00..ddbf67aee7 100644 --- a/pkg/cloud/services/securitygroup/securitygroups.go +++ b/pkg/cloud/services/securitygroup/securitygroups.go @@ -24,6 +24,7 @@ import ( "github.com/aws/aws-sdk-go/service/ec2" "github.com/pkg/errors" kerrors "k8s.io/apimachinery/pkg/util/errors" + "k8s.io/apimachinery/pkg/util/sets" infrav1 "sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta2" "sigs.k8s.io/cluster-api-provider-aws/v2/pkg/cloud/awserrors" @@ -519,20 +520,26 @@ func (s *Service) getSecurityGroupIngressRules(role infrav1.SecurityGroupRole) ( if s.scope.Bastion().Enabled { rules = append(rules, s.defaultSSHIngressRule(s.scope.SecurityGroups()[infrav1.SecurityGroupBastion].ID)) } - if s.scope.ControlPlaneLoadBalancer() != nil { - ingressRules := s.scope.ControlPlaneLoadBalancer().IngressRules - for i := range ingressRules { - if ingressRules[i].SourceSecurityGroupIDs == nil && ingressRules[i].SourceSecurityGroupRoles == nil { // if the rule doesn't have a source security group, use the control plane security group - ingressRules[i].SourceSecurityGroupIDs = []string{s.scope.SecurityGroups()[infrav1.SecurityGroupControlPlane].ID} - continue - } - for _, sourceSGRole := range ingressRules[i].SourceSecurityGroupRoles { - ingressRules[i].SourceSecurityGroupIDs = append(ingressRules[i].SourceSecurityGroupIDs, s.scope.SecurityGroups()[sourceSGRole].ID) - } + ingressRules := s.scope.AdditionalControlPlaneIngressRules() + for i := range ingressRules { + if len(ingressRules[i].CidrBlocks) != 0 || len(ingressRules[i].IPv6CidrBlocks) != 0 { // don't set source security group if cidr blocks are set + continue } - rules = append(rules, s.scope.ControlPlaneLoadBalancer().IngressRules...) + + if len(ingressRules[i].SourceSecurityGroupIDs) == 0 && len(ingressRules[i].SourceSecurityGroupRoles) == 0 { // if the rule doesn't have a source security group, use the control plane security group + ingressRules[i].SourceSecurityGroupIDs = []string{s.scope.SecurityGroups()[infrav1.SecurityGroupControlPlane].ID} + continue + } + + securityGroupIDs := sets.New[string](ingressRules[i].SourceSecurityGroupIDs...) + for _, sourceSGRole := range ingressRules[i].SourceSecurityGroupRoles { + securityGroupIDs.Insert(s.scope.SecurityGroups()[sourceSGRole].ID) + } + ingressRules[i].SourceSecurityGroupIDs = sets.List[string](securityGroupIDs) } + rules = append(rules, ingressRules...) + return append(cniRules, rules...), nil case infrav1.SecurityGroupNode: diff --git a/pkg/cloud/services/securitygroup/securitygroups_test.go b/pkg/cloud/services/securitygroup/securitygroups_test.go index efb34b3deb..2e194a9a36 100644 --- a/pkg/cloud/services/securitygroup/securitygroups_test.go +++ b/pkg/cloud/services/securitygroup/securitygroups_test.go @@ -17,7 +17,6 @@ limitations under the License. package securitygroup import ( - "reflect" "strings" "testing" @@ -619,13 +618,13 @@ func TestAdditionalControlPlaneSecurityGroup(t *testing.T) { testCases := []struct { name string - controlPlaneLBSpec *infrav1.AWSLoadBalancerSpec + networkSpec infrav1.NetworkSpec expectedAdditionalIngresRule infrav1.IngressRule }{ { name: "default control plane security group is used", - controlPlaneLBSpec: &infrav1.AWSLoadBalancerSpec{ - IngressRules: []infrav1.IngressRule{ + networkSpec: infrav1.NetworkSpec{ + AdditionalControlPlaneIngressRules: []infrav1.IngressRule{ { Description: "test", Protocol: infrav1.SecurityGroupProtocolTCP, @@ -644,8 +643,8 @@ func TestAdditionalControlPlaneSecurityGroup(t *testing.T) { }, { name: "custom security group id is used", - controlPlaneLBSpec: &infrav1.AWSLoadBalancerSpec{ - IngressRules: []infrav1.IngressRule{ + networkSpec: infrav1.NetworkSpec{ + AdditionalControlPlaneIngressRules: []infrav1.IngressRule{ { Description: "test", Protocol: infrav1.SecurityGroupProtocolTCP, @@ -665,8 +664,8 @@ func TestAdditionalControlPlaneSecurityGroup(t *testing.T) { }, { name: "another security group role is used", - controlPlaneLBSpec: &infrav1.AWSLoadBalancerSpec{ - IngressRules: []infrav1.IngressRule{ + networkSpec: infrav1.NetworkSpec{ + AdditionalControlPlaneIngressRules: []infrav1.IngressRule{ { Description: "test", Protocol: infrav1.SecurityGroupProtocolTCP, @@ -686,8 +685,8 @@ func TestAdditionalControlPlaneSecurityGroup(t *testing.T) { }, { name: "another security group role and a custom security group id is used", - controlPlaneLBSpec: &infrav1.AWSLoadBalancerSpec{ - IngressRules: []infrav1.IngressRule{ + networkSpec: infrav1.NetworkSpec{ + AdditionalControlPlaneIngressRules: []infrav1.IngressRule{ { Description: "test", Protocol: infrav1.SecurityGroupProtocolTCP, @@ -706,6 +705,26 @@ func TestAdditionalControlPlaneSecurityGroup(t *testing.T) { SourceSecurityGroupIDs: []string{"test", "node-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"}, + }, + }, + }, + expectedAdditionalIngresRule: infrav1.IngressRule{ + Description: "test", + Protocol: infrav1.SecurityGroupProtocolTCP, + FromPort: 9345, + ToPort: 9345, + }, + }, } for _, tc := range testCases { @@ -717,7 +736,7 @@ func TestAdditionalControlPlaneSecurityGroup(t *testing.T) { }, AWSCluster: &infrav1.AWSCluster{ Spec: infrav1.AWSClusterSpec{ - ControlPlaneLoadBalancer: tc.controlPlaneLBSpec, + NetworkSpec: tc.networkSpec, }, Status: infrav1.AWSClusterStatus{ Network: infrav1.NetworkStatus{ @@ -760,7 +779,7 @@ func TestAdditionalControlPlaneSecurityGroup(t *testing.T) { t.Fatalf("Expected to port %d, got %d", tc.expectedAdditionalIngresRule.ToPort, r.ToPort) } - if !reflect.DeepEqual(r.SourceSecurityGroupIDs, tc.expectedAdditionalIngresRule.SourceSecurityGroupIDs) { + if !sets.New[string](tc.expectedAdditionalIngresRule.SourceSecurityGroupIDs...).Equal(sets.New[string](tc.expectedAdditionalIngresRule.SourceSecurityGroupIDs...)) { t.Fatalf("Expected source security group IDs %v, got %v", tc.expectedAdditionalIngresRule.SourceSecurityGroupIDs, r.SourceSecurityGroupIDs) } }