diff --git a/api/v1beta1/zz_generated.conversion.go b/api/v1beta1/zz_generated.conversion.go index 10842bb9ae..d212a1ea30 100644 --- a/api/v1beta1/zz_generated.conversion.go +++ b/api/v1beta1/zz_generated.conversion.go @@ -1970,6 +1970,7 @@ func autoConvert_v1beta2_IngressRule_To_v1beta1_IngressRule(in *v1beta2.IngressR out.IPv6CidrBlocks = *(*[]string)(unsafe.Pointer(&in.IPv6CidrBlocks)) out.SourceSecurityGroupIDs = *(*[]string)(unsafe.Pointer(&in.SourceSecurityGroupIDs)) // WARNING: in.SourceSecurityGroupRoles requires manual conversion: does not exist in peer-type + // WARNING: in.NatGatewaysIPsSource requires manual conversion: does not exist in peer-type return nil } diff --git a/api/v1beta2/awscluster_webhook.go b/api/v1beta2/awscluster_webhook.go index ae9c80f5b4..f6d701b57a 100644 --- a/api/v1beta2/awscluster_webhook.go +++ b/api/v1beta2/awscluster_webhook.go @@ -264,9 +264,7 @@ func (r *AWSCluster) validateNetwork() field.ErrorList { } 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")) - } + allErrs = append(allErrs, r.validateIngressRule(rule)...) } return allErrs @@ -307,9 +305,7 @@ func (r *AWSCluster) validateControlPlaneLBs() field.ErrorList { } for _, rule := range cp.IngressRules { - if (rule.CidrBlocks != nil || rule.IPv6CidrBlocks != nil) && (rule.SourceSecurityGroupIDs != nil || rule.SourceSecurityGroupRoles != nil) { - 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")) - } + allErrs = append(allErrs, r.validateIngressRule(rule)...) } } @@ -351,11 +347,19 @@ func (r *AWSCluster) validateControlPlaneLBs() field.ErrorList { } } - for _, rule := range r.Spec.ControlPlaneLoadBalancer.IngressRules { + return allErrs +} + +func (r *AWSCluster) validateIngressRule(rule IngressRule) field.ErrorList { + var allErrs field.ErrorList + if rule.NatGatewaysIPsSource { + 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")) + } + } else { if (rule.CidrBlocks != nil || rule.IPv6CidrBlocks != nil) && (rule.SourceSecurityGroupIDs != nil || rule.SourceSecurityGroupRoles != nil) { 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")) } } - return allErrs } diff --git a/api/v1beta2/awscluster_webhook_test.go b/api/v1beta2/awscluster_webhook_test.go index 32021c29a1..54512b2d6c 100644 --- a/api/v1beta2/awscluster_webhook_test.go +++ b/api/v1beta2/awscluster_webhook_test.go @@ -408,6 +408,59 @@ func TestAWSClusterValidateCreate(t *testing.T) { }, wantErr: true, }, + { + name: "rejects ingress rules with cidr block, source security group id, role and nat gateway IP source", + cluster: &AWSCluster{ + Spec: AWSClusterSpec{ + ControlPlaneLoadBalancer: &AWSLoadBalancerSpec{ + IngressRules: []IngressRule{ + { + Protocol: SecurityGroupProtocolTCP, + IPv6CidrBlocks: []string{"test"}, + SourceSecurityGroupIDs: []string{"test"}, + SourceSecurityGroupRoles: []SecurityGroupRole{SecurityGroupBastion}, + NatGatewaysIPsSource: true, + }, + }, + }, + }, + }, + wantErr: true, + }, + { + name: "rejects ingress rules with source security role and nat gateway IP source", + cluster: &AWSCluster{ + Spec: AWSClusterSpec{ + ControlPlaneLoadBalancer: &AWSLoadBalancerSpec{ + IngressRules: []IngressRule{ + { + Protocol: SecurityGroupProtocolTCP, + SourceSecurityGroupRoles: []SecurityGroupRole{SecurityGroupBastion}, + NatGatewaysIPsSource: true, + }, + }, + }, + }, + }, + wantErr: true, + }, + { + name: "rejects ingress rules with cidr block and nat gateway IP source", + cluster: &AWSCluster{ + Spec: AWSClusterSpec{ + ControlPlaneLoadBalancer: &AWSLoadBalancerSpec{ + IngressRules: []IngressRule{ + { + Protocol: SecurityGroupProtocolTCP, + IPv6CidrBlocks: []string{"test"}, + NatGatewaysIPsSource: true, + }, + }, + }, + }, + }, + wantErr: true, + }, { name: "accepts ingress rules with cidr block", cluster: &AWSCluster{ @@ -424,6 +477,22 @@ func TestAWSClusterValidateCreate(t *testing.T) { }, wantErr: false, }, + { + name: "accepts ingress rules with nat gateway IPs source", + cluster: &AWSCluster{ + Spec: AWSClusterSpec{ + ControlPlaneLoadBalancer: &AWSLoadBalancerSpec{ + IngressRules: []IngressRule{ + { + Protocol: SecurityGroupProtocolTCP, + NatGatewaysIPsSource: true, + }, + }, + }, + }, + }, + wantErr: false, + }, { name: "accepts ingress rules with source security group role", cluster: &AWSCluster{ diff --git a/api/v1beta2/network_types.go b/api/v1beta2/network_types.go index 83b5d3a7d1..a902687ffa 100644 --- a/api/v1beta2/network_types.go +++ b/api/v1beta2/network_types.go @@ -908,6 +908,10 @@ type IngressRule struct { // The field will be combined with source security group IDs if specified. // +optional SourceSecurityGroupRoles []SecurityGroupRole `json:"sourceSecurityGroupRoles,omitempty"` + + // NatGatewaysIPsSource use the NAT gateways IPs as the source for the ingress rule. + // +optional + NatGatewaysIPsSource bool `json:"natGatewaysIPsSource,omitempty"` } // String returns a string representation of the ingress rule. 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 c9ffb5ecd8..710bf3ea84 100644 --- a/config/crd/bases/controlplane.cluster.x-k8s.io_awsmanagedcontrolplanes.yaml +++ b/config/crd/bases/controlplane.cluster.x-k8s.io_awsmanagedcontrolplanes.yaml @@ -393,6 +393,10 @@ spec: items: type: string type: array + natGatewaysIPsSource: + description: NatGatewaysIPsSource use the NAT gateways IPs + as the source for the ingress rule. + type: boolean protocol: description: Protocol is the protocol for the ingress rule. Accepted values are "-1" (all), "4" (IP in IP),"tcp", @@ -1887,6 +1891,10 @@ spec: items: type: string type: array + natGatewaysIPsSource: + description: NatGatewaysIPsSource use the NAT gateways + IPs as the source for the ingress rule. + type: boolean protocol: description: Protocol is the protocol for the ingress rule. Accepted values are "-1" (all), "4" (IP in @@ -2343,6 +2351,10 @@ spec: items: type: string type: array + natGatewaysIPsSource: + description: NatGatewaysIPsSource use the NAT gateways IPs + as the source for the ingress rule. + type: boolean protocol: description: Protocol is the protocol for the ingress rule. Accepted values are "-1" (all), "4" (IP in IP),"tcp", @@ -3850,6 +3862,10 @@ spec: items: type: string type: array + natGatewaysIPsSource: + description: NatGatewaysIPsSource use the NAT gateways + IPs as the source for the ingress rule. + type: boolean protocol: description: Protocol is the protocol for the ingress rule. Accepted values are "-1" (all), "4" (IP in 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 f2d4b882b5..5df57564b6 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_awsclusters.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_awsclusters.yaml @@ -1164,6 +1164,10 @@ spec: items: type: string type: array + natGatewaysIPsSource: + description: NatGatewaysIPsSource use the NAT gateways IPs + as the source for the ingress rule. + type: boolean protocol: description: Protocol is the protocol for the ingress rule. Accepted values are "-1" (all), "4" (IP in IP),"tcp", @@ -1329,6 +1333,10 @@ spec: items: type: string type: array + natGatewaysIPsSource: + description: NatGatewaysIPsSource use the NAT gateways IPs + as the source for the ingress rule. + type: boolean protocol: description: Protocol is the protocol for the ingress rule. Accepted values are "-1" (all), "4" (IP in IP),"tcp", @@ -1910,6 +1918,10 @@ spec: items: type: string type: array + natGatewaysIPsSource: + description: NatGatewaysIPsSource use the NAT gateways IPs + as the source for the ingress rule. + type: boolean protocol: description: Protocol is the protocol for the ingress rule. Accepted values are "-1" (all), "4" (IP in IP),"tcp", @@ -2833,6 +2845,10 @@ spec: items: type: string type: array + natGatewaysIPsSource: + description: NatGatewaysIPsSource use the NAT gateways + IPs as the source for the ingress rule. + type: boolean protocol: description: Protocol is the protocol for the ingress rule. Accepted values are "-1" (all), "4" (IP in 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 ccc966dbb2..375a62dd0b 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_awsclustertemplates.yaml +++ b/config/crd/bases/infrastructure.cluster.x-k8s.io_awsclustertemplates.yaml @@ -756,6 +756,10 @@ spec: items: type: string type: array + natGatewaysIPsSource: + description: NatGatewaysIPsSource use the NAT gateways + IPs as the source for the ingress rule. + type: boolean protocol: description: Protocol is the protocol for the ingress rule. Accepted values are "-1" (all), "4" (IP @@ -925,6 +929,10 @@ spec: items: type: string type: array + natGatewaysIPsSource: + description: NatGatewaysIPsSource use the NAT gateways + IPs as the source for the ingress rule. + type: boolean protocol: description: Protocol is the protocol for the ingress rule. Accepted values are "-1" (all), "4" (IP @@ -1511,6 +1519,10 @@ spec: items: type: string type: array + natGatewaysIPsSource: + description: NatGatewaysIPsSource use the NAT gateways + IPs as the source for the ingress rule. + type: boolean protocol: description: Protocol is the protocol for the ingress rule. Accepted values are "-1" (all), "4" (IP diff --git a/pkg/cloud/services/securitygroup/securitygroups.go b/pkg/cloud/services/securitygroup/securitygroups.go index 4eed88e69a..0c84a40cbf 100644 --- a/pkg/cloud/services/securitygroup/securitygroups.go +++ b/pkg/cloud/services/securitygroup/securitygroups.go @@ -592,7 +592,12 @@ func (s *Service) getSecurityGroupIngressRules(role infrav1.SecurityGroupRole) ( rules = append(rules, s.defaultSSHIngressRule(s.scope.SecurityGroups()[infrav1.SecurityGroupBastion].ID)) } - rules = append(rules, s.processIngressRulesSGs(s.scope.AdditionalControlPlaneIngressRules())...) + additionalIngressRules, err := s.processIngressRulesSGs(s.scope.AdditionalControlPlaneIngressRules()) + if err != nil { + return nil, err + } + + rules = append(rules, additionalIngressRules...) return append(cniRules, rules...), nil @@ -639,7 +644,10 @@ func (s *Service) getSecurityGroupIngressRules(role infrav1.SecurityGroupRole) ( return infrav1.IngressRules{}, nil case infrav1.SecurityGroupAPIServerLB: kubeletRules := s.getIngressRulesToAllowKubeletToAccessTheControlPlaneLB() - customIngressRules := s.processIngressRulesSGs(s.getControlPlaneLBIngressRules()) + customIngressRules, err := s.processIngressRulesSGs(s.getControlPlaneLBIngressRules()) + if err != nil { + return nil, err + } rulesToApply := customIngressRules.Difference(kubeletRules) return append(kubeletRules, rulesToApply...), nil case infrav1.SecurityGroupLB: @@ -964,10 +972,25 @@ func (s *Service) getIngressRuleToAllowVPCCidrInTheAPIServer() infrav1.IngressRu } } -func (s *Service) processIngressRulesSGs(ingressRules []infrav1.IngressRule) infrav1.IngressRules { +func (s *Service) processIngressRulesSGs(ingressRules []infrav1.IngressRule) (infrav1.IngressRules, error) { output := []infrav1.IngressRule{} for _, rule := range ingressRules { + if rule.NatGatewaysIPsSource { // if the rule has NatGatewaysIPsSource set to true, use the NAT Gateway IPs as the source + natGatewaysCidrs := []string{} + natGatewaysIPs := s.scope.GetNatGatewaysIPs() + for _, ip := range natGatewaysIPs { + natGatewaysCidrs = append(natGatewaysCidrs, fmt.Sprintf("%s/32", ip)) + } + if len(natGatewaysIPs) > 0 { + rule.CidrBlocks = natGatewaysCidrs + output = append(output, rule) + continue + } + + return nil, errors.New("NAT Gateway IPs are not available yet") + } + if len(rule.CidrBlocks) != 0 || len(rule.IPv6CidrBlocks) != 0 { // don't set source security group if cidr blocks are set output = append(output, rule) continue @@ -988,5 +1011,5 @@ func (s *Service) processIngressRulesSGs(ingressRules []infrav1.IngressRule) inf output = append(output, rule) } - return output + return output, nil } diff --git a/pkg/cloud/services/securitygroup/securitygroups_test.go b/pkg/cloud/services/securitygroup/securitygroups_test.go index 23be85eb60..26e894b72e 100644 --- a/pkg/cloud/services/securitygroup/securitygroups_test.go +++ b/pkg/cloud/services/securitygroup/securitygroups_test.go @@ -905,7 +905,9 @@ func TestAdditionalControlPlaneSecurityGroup(t *testing.T) { testCases := []struct { name string networkSpec infrav1.NetworkSpec + networkStatus infrav1.NetworkStatus expectedAdditionalIngresRule infrav1.IngressRule + wantErr bool }{ { name: "default control plane security group is used", @@ -919,6 +921,16 @@ func TestAdditionalControlPlaneSecurityGroup(t *testing.T) { }, }, }, + 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, @@ -940,6 +952,16 @@ func TestAdditionalControlPlaneSecurityGroup(t *testing.T) { }, }, }, + 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, @@ -961,6 +983,16 @@ func TestAdditionalControlPlaneSecurityGroup(t *testing.T) { }, }, }, + 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, @@ -983,6 +1015,16 @@ func TestAdditionalControlPlaneSecurityGroup(t *testing.T) { }, }, }, + 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, @@ -1004,13 +1046,70 @@ func TestAdditionalControlPlaneSecurityGroup(t *testing.T) { }, }, }, + 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, + }, + }, + { + name: "set nat gateway IPs cidr as source if specified", + networkSpec: infrav1.NetworkSpec{ + AdditionalControlPlaneIngressRules: []infrav1.IngressRule{ + { + Description: "test", + Protocol: infrav1.SecurityGroupProtocolTCP, + FromPort: 9345, + ToPort: 9345, + NatGatewaysIPsSource: true, + }, + }, + }, + networkStatus: infrav1.NetworkStatus{ + SecurityGroups: map[infrav1.SecurityGroupRole]infrav1.SecurityGroup{ + infrav1.SecurityGroupControlPlane: { + ID: "cp-sg-id", + }, + infrav1.SecurityGroupNode: { + ID: "node-sg-id", + }, + }, + NatGatewaysIPs: []string{"test-ip"}, + }, expectedAdditionalIngresRule: infrav1.IngressRule{ Description: "test", Protocol: infrav1.SecurityGroupProtocolTCP, + CidrBlocks: []string{"test-ip/32"}, FromPort: 9345, ToPort: 9345, }, }, + { + name: "error if nat gateway IPs cidr as source are specified but not available", + networkSpec: infrav1.NetworkSpec{ + AdditionalControlPlaneIngressRules: []infrav1.IngressRule{ + { + Description: "test", + Protocol: infrav1.SecurityGroupProtocolTCP, + FromPort: 9345, + ToPort: 9345, + NatGatewaysIPsSource: true, + }, + }, + }, + wantErr: true, + }, } for _, tc := range testCases { @@ -1025,16 +1124,7 @@ func TestAdditionalControlPlaneSecurityGroup(t *testing.T) { NetworkSpec: tc.networkSpec, }, Status: infrav1.AWSClusterStatus{ - Network: infrav1.NetworkStatus{ - SecurityGroups: map[infrav1.SecurityGroupRole]infrav1.SecurityGroup{ - infrav1.SecurityGroupControlPlane: { - ID: "cp-sg-id", - }, - infrav1.SecurityGroupNode: { - ID: "node-sg-id", - }, - }, - }, + Network: tc.networkStatus, }, }, }) @@ -1045,7 +1135,10 @@ func TestAdditionalControlPlaneSecurityGroup(t *testing.T) { 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) + if tc.wantErr { + return + } + t.Fatalf("Failed to lookup controlplane security group ingress rules: %v, wantErr %v", err, tc.wantErr) } found := false