diff --git a/README.md b/README.md index a1d19d30..4efdc071 100644 --- a/README.md +++ b/README.md @@ -18,11 +18,7 @@ ENI Trunking is a private feature even though the APIs are publicly accessible u Please follow the [guide](https://docs.aws.amazon.com/eks/latest/userguide/security-groups-for-pods.html) for enabling Security Group for Pods on your EKS Cluster. -Note: The SecurityGroupPolicy CRD only supports up to 5 security groups per custom resource. If you need more than 5 security groups for a pod, please consider to use more than one custom resources. For example, you can have two custom resources to associate up to 10 security groups to a pod. Please be aware when you are doing so: - -1, you need to request increasing the limit since the default limit is 5 security groups per interface and there is a hard limit of 16 currently. - -2, currently Fargate only allows up to 5 security groups. If you are using Fargate, you can only use up to 5 security groups per pod. +Note: The SecurityGroupPolicy CRD only supports up to 16 security groups per custom resource. currently Fargate only allows up to 5 security groups. If you are using Fargate, you can only use up to 5 security groups per pod. ## Windows IPv4 Address Management diff --git a/apis/vpcresources/v1beta1/securitygrouppolicy_types.go b/apis/vpcresources/v1beta1/securitygrouppolicy_types.go index b929dcf5..1abfba86 100644 --- a/apis/vpcresources/v1beta1/securitygrouppolicy_types.go +++ b/apis/vpcresources/v1beta1/securitygrouppolicy_types.go @@ -30,7 +30,7 @@ type SecurityGroupPolicySpec struct { type GroupIds struct { // Groups is the list of EC2 Security Groups Ids that need to be applied to the ENI of a Pod. // +kubebuilder:validation:MinItems=1 - // +kubebuilder:validation:MaxItems=5 + // +kubebuilder:validation:MaxItems=16 Groups []string `json:"groupIds,omitempty"` } diff --git a/config/crd/bases/vpcresources.k8s.aws_securitygrouppolicies.yaml b/config/crd/bases/vpcresources.k8s.aws_securitygrouppolicies.yaml index 64d4aac0..15d6b905 100644 --- a/config/crd/bases/vpcresources.k8s.aws_securitygrouppolicies.yaml +++ b/config/crd/bases/vpcresources.k8s.aws_securitygrouppolicies.yaml @@ -99,7 +99,7 @@ spec: need to be applied to the ENI of a Pod. items: type: string - maxItems: 5 + maxItems: 16 minItems: 1 type: array type: object diff --git a/mocks/amazon-vcp-resource-controller-k8s/pkg/aws/ec2/api/mock_ec2_apihelper.go b/mocks/amazon-vcp-resource-controller-k8s/pkg/aws/ec2/api/mock_ec2_apihelper.go index 6bd94eb3..338fb74d 100644 --- a/mocks/amazon-vcp-resource-controller-k8s/pkg/aws/ec2/api/mock_ec2_apihelper.go +++ b/mocks/amazon-vcp-resource-controller-k8s/pkg/aws/ec2/api/mock_ec2_apihelper.go @@ -310,3 +310,18 @@ func (mr *MockEC2APIHelperMockRecorder) WaitForNetworkInterfaceStatusChange(arg0 mr.mock.ctrl.T.Helper() return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "WaitForNetworkInterfaceStatusChange", reflect.TypeOf((*MockEC2APIHelper)(nil).WaitForNetworkInterfaceStatusChange), arg0, arg1) } + +// GetSecurityGroupQuota mocks base method. +func (m *MockEC2APIHelper) GetSecurityGroupQuota(quotaCode, serviceCode *string) (int, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetSecurityGroupQuota", quotaCode, serviceCode) + ret0, _ := ret[0].(int) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// GetSecurityGroupQuota indicates an expected call of GetSecurityGroupQuota. +func (mr *MockEC2APIHelperMockRecorder) GetSecurityGroupQuota(quotaCode, serviceCode interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetSecurityGroupQuota", reflect.TypeOf((*MockEC2APIHelper)(nil).GetSecurityGroupQuota), quotaCode, serviceCode) +} \ No newline at end of file diff --git a/mocks/amazon-vcp-resource-controller-k8s/pkg/aws/ec2/api/mock_ec2_wrapper.go b/mocks/amazon-vcp-resource-controller-k8s/pkg/aws/ec2/api/mock_ec2_wrapper.go index 53515c5d..87c5ea7e 100644 --- a/mocks/amazon-vcp-resource-controller-k8s/pkg/aws/ec2/api/mock_ec2_wrapper.go +++ b/mocks/amazon-vcp-resource-controller-k8s/pkg/aws/ec2/api/mock_ec2_wrapper.go @@ -20,6 +20,7 @@ package mock_api import ( reflect "reflect" + servicequotas "github.com/aws/aws-sdk-go/service/servicequotas" ec2 "github.com/aws/aws-sdk-go/service/ec2" gomock "github.com/golang/mock/gomock" ) @@ -285,3 +286,18 @@ func (mr *MockEC2WrapperMockRecorder) UnassignPrivateIPAddresses(arg0 interface{ mr.mock.ctrl.T.Helper() return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UnassignPrivateIPAddresses", reflect.TypeOf((*MockEC2Wrapper)(nil).UnassignPrivateIPAddresses), arg0) } + +// GetServiceQuota mocks base method. +func (m *MockEC2Wrapper) GetServiceQuota(input *servicequotas.GetServiceQuotaInput) (int, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetServiceQuota", input) + ret0, _ := ret[0].(int) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// GetServiceQuota indicates an expected call of GetServiceQuota. +func (mr *MockEC2WrapperMockRecorder) GetServiceQuota(input interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetServiceQuota", reflect.TypeOf((*MockEC2Wrapper)(nil).GetServiceQuota), input) +} diff --git a/pkg/aws/ec2/api/helper.go b/pkg/aws/ec2/api/helper.go index 17ff32eb..eabff4de 100644 --- a/pkg/aws/ec2/api/helper.go +++ b/pkg/aws/ec2/api/helper.go @@ -19,6 +19,7 @@ import ( "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/ec2" + "github.com/aws/aws-sdk-go/service/servicequotas" "k8s.io/apimachinery/pkg/util/wait" "k8s.io/client-go/util/retry" @@ -94,6 +95,7 @@ type EC2APIHelper interface { AssignIPv4ResourcesAndWaitTillReady(eniID string, resourceType config.ResourceType, count int) ([]string, error) UnassignIPv4Resources(eniID string, resourceType config.ResourceType, resources []string) error DisassociateTrunkInterface(associationID *string) error + GetSecurityGroupQuota(quotaCode, serviceCode *string) (int, error) } // CreateNetworkInterface creates a new network interface @@ -630,3 +632,16 @@ func (h *ec2APIHelper) DisassociateTrunkInterface(associationID *string) error { } return h.ec2Wrapper.DisassociateTrunkInterface(input) } + +func (h *ec2APIHelper) GetSecurityGroupQuota(quotaCode, serviceCode *string) (int, error) { + input := &servicequotas.GetServiceQuotaInput{ + QuotaCode: quotaCode, + ServiceCode: serviceCode, + } + serviceQuotaOutput, err := h.ec2Wrapper.GetServiceQuota(input) + if err != nil { + // Default value of security groups per network interface + return 5, fmt.Errorf("failed to retrieve security group quota: %v", err) + } + return int(serviceQuotaOutput), nil +} diff --git a/pkg/aws/ec2/api/helper_test.go b/pkg/aws/ec2/api/helper_test.go index b2dd7644..92011859 100644 --- a/pkg/aws/ec2/api/helper_test.go +++ b/pkg/aws/ec2/api/helper_test.go @@ -23,6 +23,7 @@ import ( "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/ec2" + "github.com/aws/aws-sdk-go/service/servicequotas" "github.com/golang/mock/gomock" "github.com/stretchr/testify/assert" "k8s.io/apimachinery/pkg/util/wait" @@ -1185,3 +1186,46 @@ func TestEc2APIHelper_GetBranchNetworkInterface(t *testing.T) { assert.NoError(t, err) assert.ElementsMatch(t, []*ec2.NetworkInterface{&networkInterface1, &networkInterface2}, branchInterfaces) } + +// TestEc2APIHelper_GetSecurityGroupQuota tests the GetSecurityGroupQuota function +func TestEc2APIHelper_GetSecurityGroupQuota(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + ec2ApiHelper, mockWrapper := getMockWrapper(ctrl) + + quotaCode := "L-1234ABCD" + serviceCode := "ec2" + + getServiceQuotaInput := &servicequotas.GetServiceQuotaInput{ + QuotaCode: "aCode, + ServiceCode: &serviceCode, + } + + mockWrapper.EXPECT().GetServiceQuota(getServiceQuotaInput).Return(5, nil) + + quota, err := ec2ApiHelper.GetSecurityGroupQuota("aCode, &serviceCode) + assert.NoError(t, err) + assert.Equal(t, 5, quota) +} + +// TestEc2APIHelper_GetSecurityGroupQuota_Error tests that error is returned if GetServiceQuota call fails +func TestEc2APIHelper_GetSecurityGroupQuota_Error(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + ec2ApiHelper, mockWrapper := getMockWrapper(ctrl) + + quotaCode := "L-1234ABCD" + serviceCode := "ec2" + + getServiceQuotaInput := &servicequotas.GetServiceQuotaInput{ + QuotaCode: "aCode, + ServiceCode: &serviceCode, + } + + mockWrapper.EXPECT().GetServiceQuota(getServiceQuotaInput).Return(5, mockError) + + _, err := ec2ApiHelper.GetSecurityGroupQuota("aCode, &serviceCode) + assert.Error(t, err) +} diff --git a/pkg/aws/ec2/api/wrapper.go b/pkg/aws/ec2/api/wrapper.go index 97a20f3b..f3018223 100644 --- a/pkg/aws/ec2/api/wrapper.go +++ b/pkg/aws/ec2/api/wrapper.go @@ -31,6 +31,7 @@ import ( "github.com/aws/aws-sdk-go/aws/request" "github.com/aws/aws-sdk-go/aws/session" "github.com/aws/aws-sdk-go/service/ec2" + "github.com/aws/aws-sdk-go/service/servicequotas" "github.com/aws/aws-sdk-go/service/sts" "github.com/go-logr/logr" "github.com/prometheus/client_golang/prometheus" @@ -61,6 +62,7 @@ type EC2Wrapper interface { ModifyNetworkInterfaceAttribute(input *ec2.ModifyNetworkInterfaceAttributeInput) (*ec2.ModifyNetworkInterfaceAttributeOutput, error) CreateNetworkInterfacePermission(input *ec2.CreateNetworkInterfacePermissionInput) (*ec2.CreateNetworkInterfacePermissionOutput, error) DisassociateTrunkInterface(input *ec2.DisassociateTrunkInterfaceInput) error + GetServiceQuota(input *servicequotas.GetServiceQuotaInput) (int, error) } var ( @@ -358,6 +360,19 @@ var ( }, ) + ec2GetServiceQuotaAPICallCnt = prometheus.NewCounter( + prometheus.CounterOpts{ + Name: "ec2_describe_network_interfaces_pages_api_err_count", + Help: "The number of calls made to get service quotas (paginated)", + }, + ) + ec2GetServiceQuotaAPIErrCnt = prometheus.NewCounter( + prometheus.CounterOpts{ + Name: "ec2_get_service_quota_api_err_count", + Help: "The number of errors encountered while making call to get service quotas (paginated)", + }, + ) + prometheusRegistered = false ) @@ -403,6 +418,8 @@ func prometheusRegister() { LeakedENIClusterCleanupCnt, ec2DescribeNetworkInterfacesPagesAPICallCnt, ec2DescribeNetworkInterfacesPagesAPIErrCnt, + ec2GetServiceQuotaAPICallCnt, + ec2GetServiceQuotaAPIErrCnt, ) prometheusRegistered = true @@ -413,6 +430,7 @@ type ec2Wrapper struct { log logr.Logger instanceServiceClient *ec2.EC2 userServiceClient *ec2.EC2 + serviceQuotasClient *servicequotas.ServiceQuotas accountID string } @@ -429,6 +447,10 @@ func NewEC2Wrapper(roleARN, clusterName, region string, log logr.Logger) (EC2Wra return nil, err } + // Initialize the service quotas client + serviceQuotasClient := servicequotas.New(instanceSession) + ec2Wrapper.serviceQuotasClient = serviceQuotasClient + // Role ARN is passed, assume the role ARN to make EC2 API Calls if roleARN != "" { // Create the instance service client with low QPS, it will be only used for associate branch to trunk calls @@ -892,3 +914,20 @@ func (e *ec2Wrapper) DisassociateTrunkInterface(input *ec2.DisassociateTrunkInte } return err } + +func (e *ec2Wrapper) GetServiceQuota(input *servicequotas.GetServiceQuotaInput) (int, error) { + start := time.Now() + // Using the service quotas client + output, err := e.serviceQuotasClient.GetServiceQuota(input) + ec2APICallLatencies.WithLabelValues("get_service_quota").Observe(time.Since(start).Seconds() * 1e3) // milliseconds + + // Metric Update + ec2APICallCnt.Inc() + ec2GetServiceQuotaAPICallCnt.Inc() + + if err != nil { + ec2APIErrCnt.Inc() + ec2GetServiceQuotaAPIErrCnt.Inc() + } + return int(*output.Quota.Value), nil +} diff --git a/webhooks/core/pod_webhook.go b/webhooks/core/pod_webhook.go index b6fa0019..a1767998 100644 --- a/webhooks/core/pod_webhook.go +++ b/webhooks/core/pod_webhook.go @@ -25,8 +25,10 @@ import ( "sigs.k8s.io/controller-runtime/pkg/healthz" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/aws/ec2/api" "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/condition" "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/config" + rcHealthz "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/healthz" "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/utils" ) @@ -41,10 +43,11 @@ const ( // PodResourceInjector injects resources into Pods type PodMutationWebHook struct { - decoder *admission.Decoder - SGPAPI utils.SecurityGroupForPodsAPI - Log logr.Logger - Condition condition.Conditions + decoder *admission.Decoder + SGPAPI utils.SecurityGroupForPodsAPI + Log logr.Logger + Condition condition.Conditions + ec2ApiHelper api.EC2APIHelper } func NewPodMutationWebHook( @@ -53,12 +56,14 @@ func NewPodMutationWebHook( condition condition.Conditions, d *admission.Decoder, healthzHandler *rcHealthz.HealthzHandler, + ec2ApiHelper api.EC2APIHelper, ) *PodMutationWebHook { podWebhook := &PodMutationWebHook{ - SGPAPI: sgpAPI, - Log: log, - Condition: condition, - decoder: d, + SGPAPI: sgpAPI, + Log: log, + Condition: condition, + decoder: d, + ec2ApiHelper: ec2ApiHelper, } // add health check on subpath for pod mutation webhook healthzHandler.AddControllersHealthCheckers( @@ -149,6 +154,13 @@ func (i *PodMutationWebHook) HandleFargatePod(req admission.Request, pod *corev1 response = admission.Allowed("Fargate pod not matching any SGP") } default: + // If more than 5 SGs match for the Pod, deny the request + // Fargate only allows up to 5 security groups. If you are using Fargate, you can only use up to 5 security groups per pod + if len(sgList) > 5 { + log.Info("Denying request due to too many matching security groups", + "Security Groups", sgList) + return admission.Denied("Too many matching security groups, rejecting event") + } // If more than 1 SG match for the Pod then add all matching SG to the Annotation pod.Annotations[FargatePodSGAnnotationKey] = strings.Join(sgList, ",") log.Info("annotating Fargate pod with matching security groups", @@ -163,7 +175,6 @@ func (i *PodMutationWebHook) HandleFargatePod(req admission.Request, pod *corev1 // Limit to the Pod when the Windows IPAM feature is enabled via ConfigMap func (i *PodMutationWebHook) HandleWindowsPod(req admission.Request, pod *corev1.Pod, log logr.Logger) (response admission.Response) { - if !i.Condition.IsWindowsIPAMEnabled() { return admission.Allowed("") } @@ -182,17 +193,34 @@ func (i *PodMutationWebHook) HandleWindowsPod(req admission.Request, pod *corev1 // matches any SGP func (i *PodMutationWebHook) HandleLinuxPod(req admission.Request, pod *corev1.Pod, log logr.Logger) (response admission.Response) { - sgList, err := i.SGPAPI.GetMatchingSecurityGroupForPods(pod) if err != nil { i.Log.Error(err, "failed to get matching SGP for Pods", "namespace", pod.Namespace, "name", pod.Name) return admission.Denied("Failed to get Matching SGP for Pods, rejecting event") } + if len(sgList) == 0 { return admission.Allowed("Pod didn't match any SGP") } + quotaCode := "L-2AFB9258" // Security groups per network interface QuotaCode + serviceCode := "vpc" + + securityGroupQuota, err := i.ec2ApiHelper.GetSecurityGroupQuota("aCode, &serviceCode) + if err != nil { + i.Log.Error(err, "failed to get security group quota", + "namespace", pod.Namespace, "name", pod.Name) + return admission.Denied("Failed to get security group quota, rejecting event") + } + + // Verify that the number of securitygroups matches sevicequtoa + if len(sgList) > securityGroupQuota { + log.Info("Denied the security groups to match quota", + "Original Security Groups", sgList, "Quota", securityGroupQuota) + return admission.Denied("Exceed security group quota, rejecting event") + } + log.Info("injecting resource to the first container of the pod", "resource name", config.ResourceNamePodENI, "resource count", DefaultResourceLimit) diff --git a/webhooks/core/pod_webhook_test.go b/webhooks/core/pod_webhook_test.go index cd934b87..0f709862 100644 --- a/webhooks/core/pod_webhook_test.go +++ b/webhooks/core/pod_webhook_test.go @@ -20,6 +20,7 @@ import ( "strings" "testing" + mock_api "github.com/aws/amazon-vpc-resource-controller-k8s/mocks/amazon-vcp-resource-controller-k8s/pkg/aws/ec2/api" mock_condition "github.com/aws/amazon-vpc-resource-controller-k8s/mocks/amazon-vcp-resource-controller-k8s/pkg/condition" mock_utils "github.com/aws/amazon-vpc-resource-controller-k8s/mocks/amazon-vcp-resource-controller-k8s/pkg/utils" "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/config" @@ -48,6 +49,7 @@ var ( type Mock struct { SGPMock *mock_utils.MockSecurityGroupForPodsAPI ConditionMock *mock_condition.MockConditions + EC2Mock *mock_api.MockEC2APIHelper } func TestPodMutationWebHook_Handle(t *testing.T) { @@ -159,6 +161,7 @@ func TestPodMutationWebHook_Handle(t *testing.T) { }, mockInvocation: func(mock Mock) { mock.SGPMock.EXPECT().GetMatchingSecurityGroupForPods(gomock.AssignableToTypeOf(sgpPod)).Return(sgList, nil) + mock.EC2Mock.EXPECT().GetSecurityGroupQuota(gomock.Any(), gomock.Any()).Return(5, nil) }, want: admission.Response{ @@ -192,6 +195,7 @@ func TestPodMutationWebHook_Handle(t *testing.T) { }, mockInvocation: func(mock Mock) { mock.SGPMock.EXPECT().GetMatchingSecurityGroupForPods(gomock.AssignableToTypeOf(sgpPod)).Return(sgList, nil) + mock.EC2Mock.EXPECT().GetSecurityGroupQuota(gomock.Any(), gomock.Any()).Return(5, nil) }, want: admission.Response{ @@ -225,6 +229,7 @@ func TestPodMutationWebHook_Handle(t *testing.T) { }, mockInvocation: func(mock Mock) { mock.SGPMock.EXPECT().GetMatchingSecurityGroupForPods(gomock.AssignableToTypeOf(sgpPod)).Return([]string{}, nil) + mock.EC2Mock.EXPECT().GetSecurityGroupQuota(gomock.Any(), gomock.Any()).Return(5, nil) }, want: admission.Response{ @@ -245,6 +250,7 @@ func TestPodMutationWebHook_Handle(t *testing.T) { }, mockInvocation: func(mock Mock) { mock.SGPMock.EXPECT().GetMatchingSecurityGroupForPods(gomock.AssignableToTypeOf(sgpPod)).Return(nil, mockErr) + mock.EC2Mock.EXPECT().GetSecurityGroupQuota(gomock.Any(), gomock.Any()).Return(5, nil) }, want: admission.Response{ @@ -517,13 +523,15 @@ func TestPodMutationWebHook_Handle(t *testing.T) { ctx := context.TODO() mock := Mock{ SGPMock: mock_utils.NewMockSecurityGroupForPodsAPI(ctrl), + EC2Mock: mock_api.NewMockEC2APIHelper(ctrl), ConditionMock: mock_condition.NewMockConditions(ctrl), } h := &PodMutationWebHook{ - decoder: decoder, - Log: zap.New(), - SGPAPI: mock.SGPMock, - Condition: mock.ConditionMock, + decoder: decoder, + Log: zap.New(), + SGPAPI: mock.SGPMock, + ec2ApiHelper: mock.EC2Mock, + Condition: mock.ConditionMock, } if tt.mockInvocation != nil {