From c7cbffb0ffd5682e60b3205bbd064f2a876b1633 Mon Sep 17 00:00:00 2001 From: Michael Tweten Date: Thu, 18 Sep 2025 16:35:57 -0500 Subject: [PATCH] feat: Introduce annotation for setting explicit egress CIDR ranges on Service LB frontend security group --- docs/guide/service/annotations.md | 19 +- pkg/annotations/constants.go | 1 + pkg/aws/services/ec2.go | 18 ++ pkg/aws/services/ec2_mocks.go | 30 ++ pkg/deploy/ec2/security_group_manager.go | 28 ++ pkg/model/ec2/security_group.go | 3 + pkg/networking/networking_manager_test.go | 25 +- pkg/networking/security_group_info.go | 10 + pkg/networking/security_group_manager.go | 46 +++ .../security_group_manager_mocks.go | 28 ++ pkg/networking/security_group_reconciler.go | 66 ++++ pkg/service/model_build_managed_sg.go | 75 +++++ pkg/service/model_build_managed_sg_test.go | 283 +++++++++++++++++- 13 files changed, 625 insertions(+), 7 deletions(-) diff --git a/docs/guide/service/annotations.md b/docs/guide/service/annotations.md index 11dd6289cd..067cb59241 100644 --- a/docs/guide/service/annotations.md +++ b/docs/guide/service/annotations.md @@ -65,7 +65,8 @@ | [service.beta.kubernetes.io/aws-load-balancer-minimum-load-balancer-capacity](#load-balancer-capacity-reservation) | stringMap | | | [service.beta.kubernetes.io/aws-load-balancer-enable-icmp-for-path-mtu-discovery](#icmp-path-mtu-discovery) | string | | If specified, a security group rule is added to the managed security group to allow explicit ICMP traffic for [Path MTU discovery](https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/network_mtu.html#path_mtu_discovery) for IPv4 and dual-stack VPCs. Creates a rule for each source range if `service.beta.kubernetes.io/load-balancer-source-ranges` is present. | | [service.beta.kubernetes.io/aws-load-balancer-enable-tcp-udp-listener](#tcp-udp-listener) | boolean | false | If specified, the controller will attempt to try TCP_UDP Listeners when the service defines a TCP and UDP port on the same port number. | -| [service.beta.kubernetes.io/aws-load-balancer-disable-nlb-sg](#nlb-sg-disable) | boolean | false | If specified, the controller will not create or manage Security Groups for the service. | +| [service.beta.kubernetes.io/aws-load-balancer-disable-nlb-sg](#nlb-sg-disable) | boolean | false | If specified, the controller will not create or manage Security Groups for the service. | +| [service.beta.kubernetes.io/aws-load-balancer-outbound-cidrs](#outbound-cidrs) | stringList | | If specified, the controller will add the CIDR ranges as egress rules to the managed frontend security group, instead of relying on the default AWS `0.0.0.0/0` egress rule. If not set, aws-load-balancer-controller will maintain previous behavior and not manage egress rules at all. | ## Traffic Routing Traffic Routing can be controlled with following annotations: @@ -354,7 +355,6 @@ for proxy protocol v2 configuration. service.beta.kubernetes.io/aws-load-balancer-disable-nlb-sg: "true" ``` - - the following annotations are deprecated in v2.3.0 release in favor of [service.beta.kubernetes.io/aws-load-balancer-attributes](#load-balancer-attributes) !!!note "" @@ -621,6 +621,21 @@ Load balancer access can be controlled via following annotations: service.beta.kubernetes.io/aws-load-balancer-inbound-sg-rules-on-private-link-traffic: "off" ``` +- `service.beta.kubernetes.io/aws-load-balancer-outbound-cidrs` allows specifying a comma-delimited list of CIDR ranges to be added as egress rules to the frontend security group. + + !!!note "" + - Historically, `aws-load-balancer-controller` hasn't explicitly added any egress rules to managed frontend security groups - instead, it relies on the fact that AWS will add a default `0.0.0.0/0` outbound egress rule for all SGs created without an explicit egress rule list. This is required for the load balancer to be able to talk to the target group and potentially other services (e.g. CloudWatch). + + - However, some organizations may have issues with the default `0.0.0.0/0` egress rule (e.g. security scanners may flag them) and would rather be able to further limit the rule to a specific set of CIDR range(s). This annotation allows that. + + !!!warning "Note" + - If this annotation is not present, `aws-load-balancer-controller` will effectively not manage egress rules at all, maintaining the behavior before the annotation was added. This means that if the annotation is added to a service to set the egress security group rules and then subsequently removed, the egress security group rule will not be removed automatically. + + !!!example + ``` + service.beta.kubernetes.io/aws-load-balancer-outbound-cidrs: "172.18.0.0/16" + ``` + ## Capacity Unit Reservation Load balancer capacity unit reservation can be configured via following annotations: diff --git a/pkg/annotations/constants.go b/pkg/annotations/constants.go index b28a403e29..5c845da548 100644 --- a/pkg/annotations/constants.go +++ b/pkg/annotations/constants.go @@ -124,4 +124,5 @@ const ( SvcLBSuffixEnableIcmpForPathMtuDiscovery = "aws-load-balancer-enable-icmp-for-path-mtu-discovery" SvcLBSuffixEnableTCPUDPListener = "aws-load-balancer-enable-tcp-udp-listener" SvcLBSuffixDisableNLBSG = "aws-load-balancer-disable-nlb-sg" + SvcLBSuffixOutboundCIDRs = "aws-load-balancer-outbound-cidrs" ) diff --git a/pkg/aws/services/ec2.go b/pkg/aws/services/ec2.go index 7084bc1459..ea410b1a12 100644 --- a/pkg/aws/services/ec2.go +++ b/pkg/aws/services/ec2.go @@ -33,7 +33,9 @@ type EC2 interface { CreateSecurityGroupWithContext(ctx context.Context, input *ec2.CreateSecurityGroupInput) (*ec2.CreateSecurityGroupOutput, error) DeleteSecurityGroupWithContext(ctx context.Context, input *ec2.DeleteSecurityGroupInput) (*ec2.DeleteSecurityGroupOutput, error) AuthorizeSecurityGroupIngressWithContext(ctx context.Context, input *ec2.AuthorizeSecurityGroupIngressInput) (*ec2.AuthorizeSecurityGroupIngressOutput, error) + AuthorizeSecurityGroupEgressWithContext(ctx context.Context, input *ec2.AuthorizeSecurityGroupEgressInput) (*ec2.AuthorizeSecurityGroupEgressOutput, error) RevokeSecurityGroupIngressWithContext(ctx context.Context, input *ec2.RevokeSecurityGroupIngressInput) (*ec2.RevokeSecurityGroupIngressOutput, error) + RevokeSecurityGroupEgressWithContext(ctx context.Context, input *ec2.RevokeSecurityGroupEgressInput) (*ec2.RevokeSecurityGroupEgressOutput, error) DescribeAvailabilityZonesWithContext(ctx context.Context, input *ec2.DescribeAvailabilityZonesInput) (*ec2.DescribeAvailabilityZonesOutput, error) DescribeVpcsWithContext(ctx context.Context, input *ec2.DescribeVpcsInput) (*ec2.DescribeVpcsOutput, error) DescribeInstancesWithContext(ctx context.Context, input *ec2.DescribeInstancesInput) (*ec2.DescribeInstancesOutput, error) @@ -202,6 +204,14 @@ func (c *ec2Client) AuthorizeSecurityGroupIngressWithContext(ctx context.Context return client.AuthorizeSecurityGroupIngress(ctx, input) } +func (c *ec2Client) AuthorizeSecurityGroupEgressWithContext(ctx context.Context, input *ec2.AuthorizeSecurityGroupEgressInput) (*ec2.AuthorizeSecurityGroupEgressOutput, error) { + client, err := c.awsClientsProvider.GetEC2Client(ctx, "AuthorizeSecurityGroupIngress") + if err != nil { + return nil, err + } + return client.AuthorizeSecurityGroupEgress(ctx, input) +} + func (c *ec2Client) RevokeSecurityGroupIngressWithContext(ctx context.Context, input *ec2.RevokeSecurityGroupIngressInput) (*ec2.RevokeSecurityGroupIngressOutput, error) { client, err := c.awsClientsProvider.GetEC2Client(ctx, "RevokeSecurityGroupIngress") if err != nil { @@ -210,6 +220,14 @@ func (c *ec2Client) RevokeSecurityGroupIngressWithContext(ctx context.Context, i return client.RevokeSecurityGroupIngress(ctx, input) } +func (c *ec2Client) RevokeSecurityGroupEgressWithContext(ctx context.Context, input *ec2.RevokeSecurityGroupEgressInput) (*ec2.RevokeSecurityGroupEgressOutput, error) { + client, err := c.awsClientsProvider.GetEC2Client(ctx, "RevokeSecurityGroupEgress") + if err != nil { + return nil, err + } + return client.RevokeSecurityGroupEgress(ctx, input) +} + func (c *ec2Client) DescribeAvailabilityZonesWithContext(ctx context.Context, input *ec2.DescribeAvailabilityZonesInput) (*ec2.DescribeAvailabilityZonesOutput, error) { client, err := c.awsClientsProvider.GetEC2Client(ctx, "DescribeAvailabilityZones") if err != nil { diff --git a/pkg/aws/services/ec2_mocks.go b/pkg/aws/services/ec2_mocks.go index 7de2d04272..382fca5f56 100644 --- a/pkg/aws/services/ec2_mocks.go +++ b/pkg/aws/services/ec2_mocks.go @@ -36,6 +36,21 @@ func (m *MockEC2) EXPECT() *MockEC2MockRecorder { return m.recorder } +// AuthorizeSecurityGroupEgressWithContext mocks base method. +func (m *MockEC2) AuthorizeSecurityGroupEgressWithContext(arg0 context.Context, arg1 *ec2.AuthorizeSecurityGroupEgressInput) (*ec2.AuthorizeSecurityGroupEgressOutput, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "AuthorizeSecurityGroupEgressWithContext", arg0, arg1) + ret0, _ := ret[0].(*ec2.AuthorizeSecurityGroupEgressOutput) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// AuthorizeSecurityGroupEgressWithContext indicates an expected call of AuthorizeSecurityGroupEgressWithContext. +func (mr *MockEC2MockRecorder) AuthorizeSecurityGroupEgressWithContext(arg0, arg1 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "AuthorizeSecurityGroupEgressWithContext", reflect.TypeOf((*MockEC2)(nil).AuthorizeSecurityGroupEgressWithContext), arg0, arg1) +} + // AuthorizeSecurityGroupIngressWithContext mocks base method. func (m *MockEC2) AuthorizeSecurityGroupIngressWithContext(arg0 context.Context, arg1 *ec2.AuthorizeSecurityGroupIngressInput) (*ec2.AuthorizeSecurityGroupIngressOutput, error) { m.ctrl.T.Helper() @@ -246,6 +261,21 @@ func (mr *MockEC2MockRecorder) DescribeVpcsWithContext(arg0, arg1 interface{}) * return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DescribeVpcsWithContext", reflect.TypeOf((*MockEC2)(nil).DescribeVpcsWithContext), arg0, arg1) } +// RevokeSecurityGroupEgressWithContext mocks base method. +func (m *MockEC2) RevokeSecurityGroupEgressWithContext(arg0 context.Context, arg1 *ec2.RevokeSecurityGroupEgressInput) (*ec2.RevokeSecurityGroupEgressOutput, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "RevokeSecurityGroupEgressWithContext", arg0, arg1) + ret0, _ := ret[0].(*ec2.RevokeSecurityGroupEgressOutput) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// RevokeSecurityGroupEgressWithContext indicates an expected call of RevokeSecurityGroupEgressWithContext. +func (mr *MockEC2MockRecorder) RevokeSecurityGroupEgressWithContext(arg0, arg1 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "RevokeSecurityGroupEgressWithContext", reflect.TypeOf((*MockEC2)(nil).RevokeSecurityGroupEgressWithContext), arg0, arg1) +} + // RevokeSecurityGroupIngressWithContext mocks base method. func (m *MockEC2) RevokeSecurityGroupIngressWithContext(arg0 context.Context, arg1 *ec2.RevokeSecurityGroupIngressInput) (*ec2.RevokeSecurityGroupIngressOutput, error) { m.ctrl.T.Helper() diff --git a/pkg/deploy/ec2/security_group_manager.go b/pkg/deploy/ec2/security_group_manager.go index 0084229576..2a7caedc02 100644 --- a/pkg/deploy/ec2/security_group_manager.go +++ b/pkg/deploy/ec2/security_group_manager.go @@ -98,6 +98,18 @@ func (m *defaultSecurityGroupManager) Create(ctx context.Context, resSG *ec2mode return ec2model.SecurityGroupStatus{}, err } + // Only reconcile egress rules when explicitly set by the service.beta.kubernetes.io/aws-load-balancer-outbound-cidrs annotation. Otherwise, preserve the previous behavior of not touching egress rules. + if resSG.Spec.Egress != nil { + permissionInfosEgress, err := buildIPPermissionInfos(resSG.Spec.Egress) + if err != nil { + return ec2model.SecurityGroupStatus{}, err + } + + if err := m.networkingSGReconciler.ReconcileEgress(ctx, sgID, permissionInfosEgress); err != nil { + return ec2model.SecurityGroupStatus{}, err + } + } + return ec2model.SecurityGroupStatus{ GroupID: sgID, }, nil @@ -105,15 +117,31 @@ func (m *defaultSecurityGroupManager) Create(ctx context.Context, resSG *ec2mode func (m *defaultSecurityGroupManager) Update(ctx context.Context, resSG *ec2model.SecurityGroup, sdkSG networking.SecurityGroupInfo) (ec2model.SecurityGroupStatus, error) { permissionInfos, err := buildIPPermissionInfos(resSG.Spec.Ingress) + if err != nil { return ec2model.SecurityGroupStatus{}, err } + if err := m.updateSDKSecurityGroupGroupWithTags(ctx, resSG, sdkSG); err != nil { return ec2model.SecurityGroupStatus{}, err } if err := m.networkingSGReconciler.ReconcileIngress(ctx, sdkSG.SecurityGroupID, permissionInfos); err != nil { return ec2model.SecurityGroupStatus{}, err } + + // Only reconcile egress rules when explicitly set by the service.beta.kubernetes.io/aws-load-balancer-outbound-cidrs annotation. Otherwise, preserve the previous behavior of not touching egress rules. + if resSG.Spec.Egress != nil { + permissionInfosEgress, err := buildIPPermissionInfos(resSG.Spec.Egress) + + if err != nil { + return ec2model.SecurityGroupStatus{}, err + } + + if err := m.networkingSGReconciler.ReconcileEgress(ctx, sdkSG.SecurityGroupID, permissionInfosEgress); err != nil { + return ec2model.SecurityGroupStatus{}, err + } + } + return ec2model.SecurityGroupStatus{ GroupID: sdkSG.SecurityGroupID, }, nil diff --git a/pkg/model/ec2/security_group.go b/pkg/model/ec2/security_group.go index 3d40369f25..8692ff22b4 100644 --- a/pkg/model/ec2/security_group.go +++ b/pkg/model/ec2/security_group.go @@ -61,6 +61,9 @@ type SecurityGroupSpec struct { // +optional Ingress []IPPermission `json:"ingress,omitempty"` + + // +optional + Egress []IPPermission `json:"egress,omitempty"` } // SecurityGroupStatus defines the observed state of SecurityGroup diff --git a/pkg/networking/networking_manager_test.go b/pkg/networking/networking_manager_test.go index 6a12ca7fb3..37db15993e 100644 --- a/pkg/networking/networking_manager_test.go +++ b/pkg/networking/networking_manager_test.go @@ -2069,8 +2069,8 @@ func Test_AttemptGarbageCollection(t *testing.T) { assert.Error(t, err) } else { assert.NoError(t, err) - assert.Equal(t, len(tt.expectedSgReconciles), len(mockReconciler.calls)) - for _, call := range mockReconciler.calls { + assert.Equal(t, len(tt.expectedSgReconciles), len(mockReconciler.ingressCalls)) + for _, call := range mockReconciler.ingressCalls { assert.True(t, tt.expectedSgReconciles.Has(call.sgID), fmt.Sprintf("expected sgID: %s to be in calls", call.sgID)) } } @@ -2083,12 +2083,29 @@ type reconcileIngressCall struct { desiredPermissions []IPPermissionInfo opts []SecurityGroupReconcileOption } + +type reconcileEgressCall struct { + sgID string + desiredPermissions []IPPermissionInfo + opts []SecurityGroupReconcileOption +} + type mockSGReconciler struct { - calls []reconcileIngressCall + ingressCalls []reconcileIngressCall + egressCalls []reconcileEgressCall } func (m *mockSGReconciler) ReconcileIngress(ctx context.Context, sgID string, desiredPermissions []IPPermissionInfo, opts ...SecurityGroupReconcileOption) error { - m.calls = append(m.calls, reconcileIngressCall{ + m.ingressCalls = append(m.ingressCalls, reconcileIngressCall{ + sgID: sgID, + desiredPermissions: desiredPermissions, + opts: opts, + }) + return nil +} + +func (m *mockSGReconciler) ReconcileEgress(ctx context.Context, sgID string, desiredPermissions []IPPermissionInfo, opts ...SecurityGroupReconcileOption) error { + m.egressCalls = append(m.egressCalls, reconcileEgressCall{ sgID: sgID, desiredPermissions: desiredPermissions, opts: opts, diff --git a/pkg/networking/security_group_info.go b/pkg/networking/security_group_info.go index 69db125138..5ce0f1292d 100644 --- a/pkg/networking/security_group_info.go +++ b/pkg/networking/security_group_info.go @@ -23,6 +23,9 @@ type SecurityGroupInfo struct { // Ingress permission for securityGroup. Ingress []IPPermissionInfo + // Egress permission for securityGroup + Egress []IPPermissionInfo + // Tags for securityGroup. Tags map[string]string } @@ -74,10 +77,17 @@ func NewRawSecurityGroupInfo(sdkSG ec2types.SecurityGroup) SecurityGroupInfo { ingress = append(ingress, NewRawIPPermission(expandedPermission)) } } + var egress []IPPermissionInfo + for _, sdkPermission := range sdkSG.IpPermissionsEgress { + for _, expandedPermission := range expandSDKIPPermission(sdkPermission) { + egress = append(egress, NewRawIPPermission(expandedPermission)) + } + } tags := buildSecurityGroupTags(sdkSG) return SecurityGroupInfo{ SecurityGroupID: sgID, Ingress: ingress, + Egress: egress, Tags: tags, } } diff --git a/pkg/networking/security_group_manager.go b/pkg/networking/security_group_manager.go index 18b4bc194d..b308feb81b 100644 --- a/pkg/networking/security_group_manager.go +++ b/pkg/networking/security_group_manager.go @@ -51,8 +51,14 @@ type SecurityGroupManager interface { // AuthorizeSGIngress will authorize Ingress permissions to SecurityGroup. AuthorizeSGIngress(ctx context.Context, sgID string, permissions []IPPermissionInfo) error + // AuthorizeSGEgress will authorize Ingress permissions to SecurityGroup. + AuthorizeSGEgress(ctx context.Context, sgID string, permissions []IPPermissionInfo) error + // RevokeSGIngress will revoke Ingress permissions from SecurityGroup. RevokeSGIngress(ctx context.Context, sgID string, permissions []IPPermissionInfo) error + + // RevokeSGEgress will revoke Ingress permissions from SecurityGroup. + RevokeSGEgress(ctx context.Context, sgID string, permissions []IPPermissionInfo) error } // NewDefaultSecurityGroupManager constructs new defaultSecurityGroupManager. @@ -146,6 +152,26 @@ func (m *defaultSecurityGroupManager) AuthorizeSGIngress(ctx context.Context, sg return nil } +func (m *defaultSecurityGroupManager) AuthorizeSGEgress(ctx context.Context, sgID string, permissions []IPPermissionInfo) error { + sdkIPPermissions := buildSDKIPPermissions(permissions) + req := &ec2sdk.AuthorizeSecurityGroupEgressInput{ + GroupId: awssdk.String(sgID), + IpPermissions: sdkIPPermissions, + } + m.logger.Info("authorizing securityGroup egress", + "securityGroupID", sgID, + "permission", sdkIPPermissions) + if _, err := m.ec2Client.AuthorizeSecurityGroupEgressWithContext(ctx, req); err != nil { + return err + } + m.logger.Info("authorized securityGroup egress", + "securityGroupID", sgID) + + // TODO: ideally we can remember the permissions we granted to save DescribeSecurityGroup API calls. + m.clearSGInfosFromCache(sgID) + return nil +} + func (m *defaultSecurityGroupManager) RevokeSGIngress(ctx context.Context, sgID string, permissions []IPPermissionInfo) error { sdkIPPermissions := buildSDKIPPermissions(permissions) req := &ec2sdk.RevokeSecurityGroupIngressInput{ @@ -166,6 +192,26 @@ func (m *defaultSecurityGroupManager) RevokeSGIngress(ctx context.Context, sgID return nil } +func (m *defaultSecurityGroupManager) RevokeSGEgress(ctx context.Context, sgID string, permissions []IPPermissionInfo) error { + sdkIPPermissions := buildSDKIPPermissions(permissions) + req := &ec2sdk.RevokeSecurityGroupEgressInput{ + GroupId: awssdk.String(sgID), + IpPermissions: sdkIPPermissions, + } + m.logger.Info("revoking securityGroup egress", + "securityGroupID", sgID, + "permission", sdkIPPermissions) + if _, err := m.ec2Client.RevokeSecurityGroupEgressWithContext(ctx, req); err != nil { + return err + } + m.logger.Info("revoked securityGroup egress", + "securityGroupID", sgID) + + // TODO: ideally we can remember the permissions we revoked to save DescribeSecurityGroup API calls. + m.clearSGInfosFromCache(sgID) + return nil +} + func (m *defaultSecurityGroupManager) fetchSGInfosFromCache(sgIDs []string) map[string]SecurityGroupInfo { m.sgInfoCacheMutex.RLock() defer m.sgInfoCacheMutex.RUnlock() diff --git a/pkg/networking/security_group_manager_mocks.go b/pkg/networking/security_group_manager_mocks.go index dfb496eae9..0927862624 100644 --- a/pkg/networking/security_group_manager_mocks.go +++ b/pkg/networking/security_group_manager_mocks.go @@ -35,6 +35,20 @@ func (m *MockSecurityGroupManager) EXPECT() *MockSecurityGroupManagerMockRecorde return m.recorder } +// AuthorizeSGEgress mocks base method. +func (m *MockSecurityGroupManager) AuthorizeSGEgress(arg0 context.Context, arg1 string, arg2 []IPPermissionInfo) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "AuthorizeSGEgress", arg0, arg1, arg2) + ret0, _ := ret[0].(error) + return ret0 +} + +// AuthorizeSGEgress indicates an expected call of AuthorizeSGEgress. +func (mr *MockSecurityGroupManagerMockRecorder) AuthorizeSGEgress(arg0, arg1, arg2 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "AuthorizeSGEgress", reflect.TypeOf((*MockSecurityGroupManager)(nil).AuthorizeSGEgress), arg0, arg1, arg2) +} + // AuthorizeSGIngress mocks base method. func (m *MockSecurityGroupManager) AuthorizeSGIngress(arg0 context.Context, arg1 string, arg2 []IPPermissionInfo) error { m.ctrl.T.Helper() @@ -84,6 +98,20 @@ func (mr *MockSecurityGroupManagerMockRecorder) FetchSGInfosByRequest(arg0, arg1 return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "FetchSGInfosByRequest", reflect.TypeOf((*MockSecurityGroupManager)(nil).FetchSGInfosByRequest), arg0, arg1) } +// RevokeSGEgress mocks base method. +func (m *MockSecurityGroupManager) RevokeSGEgress(arg0 context.Context, arg1 string, arg2 []IPPermissionInfo) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "RevokeSGEgress", arg0, arg1, arg2) + ret0, _ := ret[0].(error) + return ret0 +} + +// RevokeSGEgress indicates an expected call of RevokeSGEgress. +func (mr *MockSecurityGroupManagerMockRecorder) RevokeSGEgress(arg0, arg1, arg2 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "RevokeSGEgress", reflect.TypeOf((*MockSecurityGroupManager)(nil).RevokeSGEgress), arg0, arg1, arg2) +} + // RevokeSGIngress mocks base method. func (m *MockSecurityGroupManager) RevokeSGIngress(arg0 context.Context, arg1 string, arg2 []IPPermissionInfo) error { m.ctrl.T.Helper() diff --git a/pkg/networking/security_group_reconciler.go b/pkg/networking/security_group_reconciler.go index 1a54a86559..babe378f0d 100644 --- a/pkg/networking/security_group_reconciler.go +++ b/pkg/networking/security_group_reconciler.go @@ -48,6 +48,9 @@ func WithAuthorizeOnly(authorizeOnly bool) SecurityGroupReconcileOption { type SecurityGroupReconciler interface { // ReconcileIngress will reconcile Ingress permission on SecurityGroup to be desiredPermission. ReconcileIngress(ctx context.Context, sgID string, desiredPermissions []IPPermissionInfo, opts ...SecurityGroupReconcileOption) error + + // ReconcileEgress will reconcile Egress permission on SecurityGroup to be desiredPermission. + ReconcileEgress(ctx context.Context, sgID string, desiredPermissions []IPPermissionInfo, opts ...SecurityGroupReconcileOption) error } // NewDefaultSecurityGroupReconciler constructs new defaultSecurityGroupReconciler. @@ -94,6 +97,34 @@ func (r *defaultSecurityGroupReconciler) ReconcileIngress(ctx context.Context, s return nil } +func (r *defaultSecurityGroupReconciler) ReconcileEgress(ctx context.Context, sgID string, desiredPermissions []IPPermissionInfo, opts ...SecurityGroupReconcileOption) error { + reconcileOpts := SecurityGroupReconcileOptions{ + PermissionSelector: labels.Everything(), + } + reconcileOpts.ApplyOptions(opts...) + + sgInfoByID, err := r.sgManager.FetchSGInfosByID(ctx, []string{sgID}) + if err != nil { + return err + } + sgInfo := sgInfoByID[sgID] + + if err := r.reconcileEgressWithSGInfo(ctx, sgInfo, desiredPermissions, false, reconcileOpts); err != nil { + if !r.shouldRetryWithoutCache(err) { + return err + } + revokeFirst := r.shouldRemoveSGRulesFirst(err) + r.logger.Info("Retrying ReconcileEgress without using cache", "revokeFirst", revokeFirst) + sgInfoByID, err := r.sgManager.FetchSGInfosByID(ctx, []string{sgID}, WithReloadIgnoringCache()) + if err != nil { + return err + } + sgInfo := sgInfoByID[sgID] + return r.reconcileEgressWithSGInfo(ctx, sgInfo, desiredPermissions, revokeFirst, reconcileOpts) + } + return nil +} + func (r *defaultSecurityGroupReconciler) reconcileIngressWithSGInfo(ctx context.Context, sgInfo SecurityGroupInfo, desiredPermissions []IPPermissionInfo, revokeFirst bool, reconcileOpts SecurityGroupReconcileOptions) error { extraPermissions := diffIPPermissionInfos(sgInfo.Ingress, desiredPermissions) permissionsToRevoke := make([]IPPermissionInfo, 0, len(extraPermissions)) @@ -129,6 +160,41 @@ func (r *defaultSecurityGroupReconciler) reconcileIngressWithSGInfo(ctx context. return nil } +func (r *defaultSecurityGroupReconciler) reconcileEgressWithSGInfo(ctx context.Context, sgInfo SecurityGroupInfo, desiredPermissions []IPPermissionInfo, revokeFirst bool, reconcileOpts SecurityGroupReconcileOptions) error { + extraPermissions := diffIPPermissionInfos(sgInfo.Egress, desiredPermissions) + permissionsToRevoke := make([]IPPermissionInfo, 0, len(extraPermissions)) + for _, permission := range extraPermissions { + if reconcileOpts.PermissionSelector.Matches(labels.Set(permission.Labels)) { + permissionsToRevoke = append(permissionsToRevoke, permission) + } + } + permissionsToGrant := diffIPPermissionInfos(desiredPermissions, sgInfo.Egress) + + if revokeFirst { + if len(permissionsToRevoke) > 0 && !reconcileOpts.AuthorizeOnly { + if err := r.sgManager.RevokeSGEgress(ctx, sgInfo.SecurityGroupID, permissionsToRevoke); err != nil { + return err + } + } + } + + if len(permissionsToGrant) > 0 { + if err := r.sgManager.AuthorizeSGEgress(ctx, sgInfo.SecurityGroupID, permissionsToGrant); err != nil { + return err + } + } + + if !revokeFirst { + if len(permissionsToRevoke) > 0 && !reconcileOpts.AuthorizeOnly { + if err := r.sgManager.RevokeSGEgress(ctx, sgInfo.SecurityGroupID, permissionsToRevoke); err != nil { + return err + } + } + } + + return nil +} + // shouldRetryWithoutCache tests whether we should retry SecurityGroup rules reconcile without cache. func (r *defaultSecurityGroupReconciler) shouldRetryWithoutCache(err error) bool { var apiErr smithy.APIError diff --git a/pkg/service/model_build_managed_sg.go b/pkg/service/model_build_managed_sg.go index 0416468cfe..c69c003f6c 100644 --- a/pkg/service/model_build_managed_sg.go +++ b/pkg/service/model_build_managed_sg.go @@ -5,7 +5,9 @@ import ( "crypto/sha256" "encoding/hex" "fmt" + "net" "regexp" + "sigs.k8s.io/aws-load-balancer-controller/pkg/k8s" "sigs.k8s.io/aws-load-balancer-controller/pkg/shared_constants" "strings" @@ -40,11 +42,18 @@ func (t *defaultModelBuildTask) buildManagedSecurityGroupSpec(ctx context.Contex if err != nil { return ec2model.SecurityGroupSpec{}, err } + + egressPermissions, err := t.buildManagedSecurityGroupEgressPermissions(ctx, ipAddressType) + if err != nil { + return ec2model.SecurityGroupSpec{}, err + } + return ec2model.SecurityGroupSpec{ GroupName: name, Description: "[k8s] Managed SecurityGroup for LoadBalancer", Tags: tags, Ingress: ingressPermissions, + Egress: egressPermissions, }, nil } @@ -145,6 +154,72 @@ func (t *defaultModelBuildTask) buildManagedSecurityGroupIngressPermissions(ctx return permissions, nil } +func (t *defaultModelBuildTask) buildManagedSecurityGroupEgressPermissions(ctx context.Context, ipAddressType elbv2model.IPAddressType) ([]ec2model.IPPermission, error) { + permissions := []ec2model.IPPermission{} + + cidrs, err := t.buildExplicitOutboundCIDRs(ctx, ipAddressType) + if err != nil { + return nil, err + } + + if cidrs == nil { + return nil, nil + } + + for _, port := range t.service.Spec.Ports { + listenPort := int32(port.Port) + for _, cidr := range cidrs { + if !strings.Contains(cidr, ":") { + permissions = append(permissions, ec2model.IPPermission{ + IPProtocol: strings.ToLower(string(port.Protocol)), + FromPort: awssdk.Int32(listenPort), + ToPort: awssdk.Int32(listenPort), + IPRanges: []ec2model.IPRange{ + { + CIDRIP: cidr, + }, + }, + }) + } else { + permissions = append(permissions, ec2model.IPPermission{ + IPProtocol: strings.ToLower(string(port.Protocol)), + FromPort: awssdk.Int32(listenPort), + ToPort: awssdk.Int32(listenPort), + IPv6Range: []ec2model.IPv6Range{ + { + CIDRIPv6: cidr, + }, + }, + }) + } + } + } + + return permissions, nil +} + +func (t *defaultModelBuildTask) buildExplicitOutboundCIDRs(_ context.Context, ipAddressType elbv2model.IPAddressType) ([]string, error) { + var rawOutboundCIDRs []string + + exists := t.annotationParser.ParseStringSliceAnnotation(annotations.SvcLBSuffixOutboundCIDRs, &rawOutboundCIDRs, t.service.Annotations) + if !exists { + return nil, nil + } + + outboundCIDRs := []string{} + for _, cidr := range rawOutboundCIDRs { + _, _, err := net.ParseCIDR(cidr) + if err != nil { + return nil, fmt.Errorf("invalid %v settings on Service: %v: %w", annotations.SvcLBSuffixOutboundCIDRs, k8s.NamespacedName(t.service), err) + } + if strings.Contains(cidr, ":") && ipAddressType != elbv2model.IPAddressTypeDualStack { + return nil, errors.Errorf("unsupported v6 cidr %v when lb is not dualstack", cidr) + } + outboundCIDRs = append(outboundCIDRs, cidr) + } + return outboundCIDRs, nil +} + func (t *defaultModelBuildTask) buildCIDRsFromSourceRanges(_ context.Context, ipAddressType elbv2model.IPAddressType, prefixListsConfigured bool) ([]string, error) { var cidrs []string for _, cidr := range t.service.Spec.LoadBalancerSourceRanges { diff --git a/pkg/service/model_build_managed_sg_test.go b/pkg/service/model_build_managed_sg_test.go index 26188ec5f7..9cb57e6c9c 100644 --- a/pkg/service/model_build_managed_sg_test.go +++ b/pkg/service/model_build_managed_sg_test.go @@ -146,7 +146,7 @@ func Test_buildCIDRsFromSourceRanges_buildManagedSecurityGroupIngressPermissions svc: &corev1.Service{ ObjectMeta: metav1.ObjectMeta{ Annotations: map[string]string{ - "service.beta.kubernetes.io/aws-load-balancer-ip-address-type": "daulstack", + "service.beta.kubernetes.io/aws-load-balancer-ip-address-type": "dualstack", }, }, Spec: corev1.ServiceSpec{ @@ -288,3 +288,284 @@ func Test_buildCIDRsFromSourceRanges_buildManagedSecurityGroupIngressPermissions }) } } + +func Test_buildExplicitOutboundCIDRs_buildExplicitOutboundCIDRs(t *testing.T) { + type fields struct { + svc *corev1.Service + ipAddressType elbv2model.IPAddressType + prefixListsConfigured bool + } + tests := []struct { + name string + fields fields + want []string + wantErr bool + }{ + { + name: "no annotation configured, nil slice returned", + fields: fields{ + svc: &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{}, + }, + }, + ipAddressType: elbv2model.IPAddressTypeIPV4, + prefixListsConfigured: false, + }, + wantErr: false, + want: nil, + }, + { + name: "empty annotation configured, empty slice returned", + fields: fields{ + svc: &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + "service.beta.kubernetes.io/aws-load-balancer-outbound-cidrs": "", + }, + }, + }, + ipAddressType: elbv2model.IPAddressTypeIPV4, + prefixListsConfigured: false, + }, + wantErr: false, + want: []string{}, + }, + { + name: "configured with single valid ipv4 value", + fields: fields{ + svc: &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + "service.beta.kubernetes.io/aws-load-balancer-outbound-cidrs": "0.0.0.0/0", + }, + }, + }, + ipAddressType: elbv2model.IPAddressTypeIPV4, + prefixListsConfigured: true, + }, + wantErr: false, + want: []string{ + "0.0.0.0/0", + }, + }, + { + name: "configured with invalid cidr", + fields: fields{ + svc: &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + "service.beta.kubernetes.io/aws-load-balancer-outbound-cidrs": "10.0.0.0", + }, + }, + }, + ipAddressType: elbv2model.IPAddressTypeIPV4, + prefixListsConfigured: true, + }, + wantErr: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t1 *testing.T) { + annotationParser := annotations.NewSuffixAnnotationParser("service.beta.kubernetes.io") + task := &defaultModelBuildTask{ + annotationParser: annotationParser, + service: tt.fields.svc, + } + got, err := task.buildExplicitOutboundCIDRs(context.Background(), tt.fields.ipAddressType) + if tt.wantErr { + assert.Error(t, err) + } else { + assert.NoError(t, err) + assert.Equal(t, got, tt.want) + } + }) + } +} + +func Test_buildManagedSecurityGroupEgressPermissions(t *testing.T) { + type fields struct { + svc *corev1.Service + ipAddressType elbv2model.IPAddressType + } + tests := []struct { + name string + fields fields + want []ec2model.IPPermission + wantErr bool + }{ + { + name: "no annotation configured, nil slice returned", + fields: fields{ + svc: &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{}, + }, + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeNodePort, + Ports: []corev1.ServicePort{ + { + Name: "http", + Port: 80, + NodePort: 18080, + }, + }, + }, + }, + ipAddressType: elbv2model.IPAddressTypeIPV4, + }, + wantErr: false, + want: nil, + }, + { + name: "empty annotation configured, empty slice returned", + fields: fields{ + svc: &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + "service.beta.kubernetes.io/aws-load-balancer-outbound-cidrs": "", + }, + }, + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeNodePort, + Ports: []corev1.ServicePort{ + { + Name: "http", + Port: 80, + NodePort: 18080, + }, + }, + }, + }, + ipAddressType: elbv2model.IPAddressTypeIPV4, + }, + wantErr: false, + want: []ec2model.IPPermission{}, + }, + { + name: "single valid ipv4", + fields: fields{ + svc: &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + "service.beta.kubernetes.io/aws-load-balancer-outbound-cidrs": "0.0.0.0/0", + }, + }, + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeNodePort, + Ports: []corev1.ServicePort{ + { + Name: "http", + Port: 80, + NodePort: 18080, + }, + }, + LoadBalancerSourceRanges: []string{}, + }, + }, + ipAddressType: elbv2model.IPAddressTypeIPV4, + }, + wantErr: false, + want: []ec2model.IPPermission{ + { + IPProtocol: "", + FromPort: aws.Int32(80), + ToPort: aws.Int32(80), + IPRanges: []ec2model.IPRange{ + { + CIDRIP: "0.0.0.0/0", + }, + }, + }, + }, + }, + { + name: "multiple valid ipv4 and ipv6 with dualstack", + fields: fields{ + svc: &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + "service.beta.kubernetes.io/aws-load-balancer-outbound-cidrs": "0.0.0.0/0,::/0", + }, + }, + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeNodePort, + Ports: []corev1.ServicePort{ + { + Name: "http", + Port: 80, + NodePort: 18080, + }, + }, + LoadBalancerSourceRanges: []string{}, + }, + }, + ipAddressType: elbv2model.IPAddressTypeDualStack, + }, + wantErr: false, + want: []ec2model.IPPermission{ + { + IPProtocol: "", + FromPort: aws.Int32(80), + ToPort: aws.Int32(80), + IPRanges: []ec2model.IPRange{ + { + CIDRIP: "0.0.0.0/0", + }, + }, + }, + { + IPProtocol: "", + FromPort: aws.Int32(80), + ToPort: aws.Int32(80), + IPv6Range: []ec2model.IPv6Range{ + { + CIDRIPv6: "::/0", + }, + }, + }, + }, + }, + { + name: "multiple valid ipv4 and ipv6 no dualstack", + fields: fields{ + svc: &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + "service.beta.kubernetes.io/aws-load-balancer-outbound-cidrs": "0.0.0.0/0,::/0", + }, + }, + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeNodePort, + Ports: []corev1.ServicePort{ + { + Name: "http", + Port: 80, + NodePort: 18080, + }, + }, + LoadBalancerSourceRanges: []string{}, + }, + }, + ipAddressType: elbv2model.IPAddressTypeIPV4, + }, + wantErr: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t1 *testing.T) { + annotationParser := annotations.NewSuffixAnnotationParser("service.beta.kubernetes.io") + task := &defaultModelBuildTask{ + annotationParser: annotationParser, + service: tt.fields.svc, + } + got, err := task.buildManagedSecurityGroupEgressPermissions(context.Background(), tt.fields.ipAddressType) + if tt.wantErr { + assert.Error(t, err) + } else { + assert.NoError(t, err) + assert.Equal(t, got, tt.want) + } + }) + } +}