From b1383c84217e093bb33c55d1eb0f04e4161403a8 Mon Sep 17 00:00:00 2001 From: Jose Armesto Date: Wed, 29 Jan 2025 12:40:08 +0100 Subject: [PATCH] Only manage security groups for ENIs tagged by CAPA --- .../awsmachine_controller_unit_test.go | 270 ++++++++++++++++++ pkg/cloud/services/ec2/instances.go | 8 + 2 files changed, 278 insertions(+) diff --git a/controllers/awsmachine_controller_unit_test.go b/controllers/awsmachine_controller_unit_test.go index 9395db0794..ad7e5be1a4 100644 --- a/controllers/awsmachine_controller_unit_test.go +++ b/controllers/awsmachine_controller_unit_test.go @@ -2768,6 +2768,276 @@ func TestAWSMachineReconcilerReconcileDefaultsToLoadBalancerTypeClassic(t *testi g.Expect(err).To(BeNil()) } +func TestAWSMachineReconcilerReconcileDoesntAddSecurityGroupsToNonManagedNetworkInterfaces(t *testing.T) { + g := NewWithT(t) + + ns := "testns" + + cp := &kubeadmv1beta1.KubeadmControlPlane{} + cp.SetName("capi-cp-test-1") + cp.SetNamespace(ns) + + ownerCluster := &clusterv1.Cluster{ + ObjectMeta: metav1.ObjectMeta{Name: "capi-test-1", Namespace: ns}, + Spec: clusterv1.ClusterSpec{ + InfrastructureRef: &corev1.ObjectReference{ + Kind: "AWSCluster", + Name: "capi-test-1", // assuming same name + Namespace: ns, + APIVersion: infrav1.GroupVersion.String(), + }, + ControlPlaneRef: &corev1.ObjectReference{ + Kind: "KubeadmControlPlane", + Namespace: cp.Namespace, + Name: cp.Name, + APIVersion: kubeadmv1beta1.GroupVersion.String(), + }, + }, + Status: clusterv1.ClusterStatus{ + InfrastructureReady: true, + }, + } + + awsCluster := &infrav1.AWSCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "capi-test-1", + Namespace: ns, + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: clusterv1.GroupVersion.String(), + Kind: "Cluster", + Name: ownerCluster.Name, + UID: "1", + }, + }, + }, + Spec: infrav1.AWSClusterSpec{ + ControlPlaneLoadBalancer: &infrav1.AWSLoadBalancerSpec{ + Scheme: &infrav1.ELBSchemeInternetFacing, + // `LoadBalancerType` not set (i.e. empty string; must default to attaching instance to classic LB) + }, + NetworkSpec: infrav1.NetworkSpec{ + Subnets: infrav1.Subnets{ + infrav1.SubnetSpec{ + ID: "subnet-1", + IsPublic: false, + }, + infrav1.SubnetSpec{ + IsPublic: false, + }, + }, + }, + }, + Status: infrav1.AWSClusterStatus{ + Ready: true, + Network: infrav1.NetworkStatus{ + SecurityGroups: map[infrav1.SecurityGroupRole]infrav1.SecurityGroup{ + infrav1.SecurityGroupControlPlane: { + ID: "1", + }, + infrav1.SecurityGroupNode: { + ID: "2", + }, + infrav1.SecurityGroupLB: { + ID: "3", + }, + }, + }, + }, + } + + ownerMachine := &clusterv1.Machine{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + clusterv1.ClusterNameLabel: "capi-test-1", + clusterv1.MachineControlPlaneLabel: "", // control plane node so that controller tries to register it with LB + }, + Name: "capi-test-machine", + Namespace: ns, + }, + Spec: clusterv1.MachineSpec{ + ClusterName: "capi-test", + Bootstrap: clusterv1.Bootstrap{ + DataSecretName: aws.String("bootstrap-data"), + }, + }, + } + + awsMachine := &infrav1.AWSMachine{ + ObjectMeta: metav1.ObjectMeta{ + Name: "aws-test-7", + Namespace: ns, + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: clusterv1.GroupVersion.String(), + Kind: "Machine", + Name: "capi-test-machine", + UID: "1", + }, + }, + }, + Spec: infrav1.AWSMachineSpec{ + InstanceType: "test", + ProviderID: aws.String("aws://the-zone/two"), + CloudInit: infrav1.CloudInit{ + SecureSecretsBackend: infrav1.SecretBackendSecretsManager, + SecretPrefix: "prefix", + SecretCount: 1000, + }, + }, + } + + controllerIdentity := &infrav1.AWSClusterControllerIdentity{ + TypeMeta: metav1.TypeMeta{ + Kind: string(infrav1.ControllerIdentityKind), + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "default", + }, + Spec: infrav1.AWSClusterControllerIdentitySpec{ + AWSClusterIdentitySpec: infrav1.AWSClusterIdentitySpec{ + AllowedNamespaces: &infrav1.AllowedNamespaces{}, + }, + }, + } + + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "bootstrap-data", + Namespace: ns, + }, + Data: map[string][]byte{ + "value": []byte("shell-script"), + }, + } + + fakeClient := fake.NewClientBuilder().WithObjects(ownerCluster, awsCluster, ownerMachine, awsMachine, controllerIdentity, secret, cp).WithStatusSubresource(awsCluster, awsMachine).Build() + + recorder := record.NewFakeRecorder(10) + reconciler := &AWSMachineReconciler{ + Client: fakeClient, + Recorder: recorder, + } + + mockCtrl := gomock.NewController(t) + ec2Mock := mocks.NewMockEC2API(mockCtrl) + elbMock := mocks.NewMockELBAPI(mockCtrl) + secretMock := mock_services.NewMockSecretInterface(mockCtrl) + + cs, err := getClusterScope(*awsCluster) + g.Expect(err).To(BeNil()) + + ec2Svc := ec2Service.NewService(cs) + ec2Svc.EC2Client = ec2Mock + reconciler.ec2ServiceFactory = func(scope scope.EC2Scope) services.EC2Interface { + return ec2Svc + } + + elbSvc := elbService.NewService(cs) + elbSvc.EC2Client = ec2Mock + elbSvc.ELBClient = elbMock + reconciler.elbServiceFactory = func(scope scope.ELBScope) services.ELBInterface { + return elbSvc + } + + reconciler.secretsManagerServiceFactory = func(clusterScope cloud.ClusterScoper) services.SecretInterface { + return secretMock + } + + ec2Mock.EXPECT().DescribeInstancesWithContext(context.TODO(), gomock.Eq(&ec2.DescribeInstancesInput{ + InstanceIds: aws.StringSlice([]string{"two"}), + })).Return(&ec2.DescribeInstancesOutput{ + Reservations: []*ec2.Reservation{ + { + Instances: []*ec2.Instance{ + { + InstanceId: aws.String("two"), + InstanceType: aws.String("m5.large"), + SubnetId: aws.String("subnet-1"), + ImageId: aws.String("ami-1"), + State: &ec2.InstanceState{ + Name: aws.String(ec2.InstanceStateNameRunning), + }, + Placement: &ec2.Placement{ + AvailabilityZone: aws.String("thezone"), + }, + MetadataOptions: &ec2.InstanceMetadataOptionsResponse{ + HttpEndpoint: aws.String(string(infrav1.InstanceMetadataEndpointStateEnabled)), + HttpPutResponseHopLimit: aws.Int64(1), + HttpTokens: aws.String(string(infrav1.HTTPTokensStateOptional)), + InstanceMetadataTags: aws.String(string(infrav1.InstanceMetadataEndpointStateDisabled)), + }, + }, + }, + }, + }, + }, nil) + + // Must attach to a classic LB, not another type. Only these mock calls are therefore expected. + mockedCreateLBCalls(t, elbMock.EXPECT()) + + ec2Mock.EXPECT().DescribeNetworkInterfacesWithContext(context.TODO(), gomock.Eq(&ec2.DescribeNetworkInterfacesInput{Filters: []*ec2.Filter{ + { + Name: aws.String("attachment.instance-id"), + Values: aws.StringSlice([]string{"two"}), + }, + }})).Return(&ec2.DescribeNetworkInterfacesOutput{ + NetworkInterfaces: []*ec2.NetworkInterface{ + { + NetworkInterfaceId: aws.String("eni-1"), + Groups: []*ec2.GroupIdentifier{ + { + GroupId: aws.String("2"), + }, + { + GroupId: aws.String("3"), + }, + { + GroupId: aws.String("1"), + }, + }, + TagSet: []*ec2.Tag{ + { + Key: aws.String("sigs.k8s.io/cluster-api-provider-aws/cluster/test-cluster"), + Value: aws.String("owned"), + }, + }, + }, + { + NetworkInterfaceId: aws.String("eni-cilium"), + Groups: []*ec2.GroupIdentifier{ + { + GroupId: aws.String("3"), + }, + }, + TagSet: []*ec2.Tag{ + { + Key: aws.String("cilium-managed"), + Value: aws.String("true"), + }, + }, + }, + }}, nil).MaxTimes(3) + ec2Mock.EXPECT().DescribeNetworkInterfaceAttributeWithContext(context.TODO(), gomock.Eq(&ec2.DescribeNetworkInterfaceAttributeInput{ + NetworkInterfaceId: aws.String("eni-1"), + Attribute: aws.String("groupSet"), + })).Return(&ec2.DescribeNetworkInterfaceAttributeOutput{Groups: []*ec2.GroupIdentifier{{GroupId: aws.String("3")}}}, nil).MaxTimes(1) + ec2Mock.EXPECT().DescribeNetworkInterfaceAttributeWithContext(context.TODO(), gomock.Eq(&ec2.DescribeNetworkInterfaceAttributeInput{ + NetworkInterfaceId: aws.String("eni-cilium"), + Attribute: aws.String("groupSet"), + })).Return(&ec2.DescribeNetworkInterfaceAttributeOutput{Groups: []*ec2.GroupIdentifier{{GroupId: aws.String("3")}}}, nil).MaxTimes(1) + ec2Mock.EXPECT().ModifyNetworkInterfaceAttributeWithContext(context.TODO(), gomock.Any()).Times(1) + + _, err = reconciler.Reconcile(ctx, ctrl.Request{ + NamespacedName: client.ObjectKey{ + Namespace: awsMachine.Namespace, + Name: awsMachine.Name, + }, + }) + + g.Expect(err).To(BeNil()) +} + func createObject(g *WithT, obj client.Object, namespace string) { if obj.DeepCopyObject() != nil { obj.SetNamespace(namespace) diff --git a/pkg/cloud/services/ec2/instances.go b/pkg/cloud/services/ec2/instances.go index 0742b0c589..d18fe2ba96 100644 --- a/pkg/cloud/services/ec2/instances.go +++ b/pkg/cloud/services/ec2/instances.go @@ -20,6 +20,7 @@ import ( "context" "encoding/base64" "fmt" + "slices" "sort" "strings" @@ -737,6 +738,13 @@ func (s *Service) UpdateInstanceSecurityGroups(instanceID string, ids []string) s.scope.Debug("Found ENIs on instance", "number-of-enis", len(enis), "instance-id", instanceID) for _, eni := range enis { + // Other components like cilium may add ENIs, and we don't want CAPA changing the security groups on those. + if !slices.ContainsFunc(eni.TagSet, func(t *ec2.Tag) bool { + return aws.StringValue(t.Key) == infrav1.ClusterTagKey(s.scope.Name()) + }) { + s.scope.Debug("Skipping ENI without cluster tag", "eni-id", *eni.NetworkInterfaceId) + continue + } if err := s.attachSecurityGroupsToNetworkInterface(ids, aws.StringValue(eni.NetworkInterfaceId)); err != nil { return errors.Wrapf(err, "failed to modify network interfaces on instance %q", instanceID) }