Skip to content

Commit

Permalink
Only manage security groups for ENIs tagged by CAPA
Browse files Browse the repository at this point in the history
  • Loading branch information
@fiunchinho
fiunchinho committed Jan 29, 2025
1 parent bf50223 commit b1383c8
Show file tree
Hide file tree
Showing 2 changed files with 278 additions and 0 deletions.
270 changes: 270 additions & 0 deletions controllers/awsmachine_controller_unit_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
8 changes: 8 additions & 0 deletions pkg/cloud/services/ec2/instances.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"context"
"encoding/base64"
"fmt"
"slices"
"sort"
"strings"

Expand Down Expand Up @@ -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)
}
Expand Down

0 comments on commit b1383c8

Please sign in to comment.