Use Security Groups status for node launch

aws · Apr 25, 2024 · c04fa86 · c04fa86
1 parent b7745bf
commit c04fa86
Show file tree

Hide file tree

Showing 6 changed files with 220 additions and 34 deletions.
diff --git a/pkg/cloudprovider/drift.go b/pkg/cloudprovider/drift.go
@@ -54,7 +54,7 @@ func (c *CloudProvider) isNodeClassDrifted(ctx context.Context, nodeClaim *corev
 	if err != nil {
 		return "", fmt.Errorf("calculating ami drift, %w", err)
 	}
-	securitygroupDrifted, err := c.areSecurityGroupsDrifted(ctx, instance, nodeClass)
+	securitygroupDrifted, err := c.areSecurityGroupsDrifted(instance, nodeClass)
 	if err != nil {
 		return "", fmt.Errorf("calculating securitygroup drift, %w", err)
 	}
@@ -118,14 +118,10 @@ func (c *CloudProvider) isSubnetDrifted(ctx context.Context, instance *instance.
 
 // Checks if the security groups are drifted, by comparing the security groups returned from the SecurityGroupProvider
 // to the ec2 instance security groups
-func (c *CloudProvider) areSecurityGroupsDrifted(ctx context.Context, ec2Instance *instance.Instance, nodeClass *v1beta1.EC2NodeClass) (cloudprovider.DriftReason, error) {
-	securitygroup, err := c.securityGroupProvider.List(ctx, nodeClass)
-	if err != nil {
-		return "", err
-	}
-	securityGroupIds := sets.New(lo.Map(securitygroup, func(sg *ec2.SecurityGroup, _ int) string { return aws.StringValue(sg.GroupId) })...)
+func (c *CloudProvider) areSecurityGroupsDrifted(ec2Instance *instance.Instance, nodeClass *v1beta1.EC2NodeClass) (cloudprovider.DriftReason, error) {
+	securityGroupIds := sets.New(lo.Map(nodeClass.Status.SecurityGroups, func(sg v1beta1.SecurityGroup, _ int) string { return sg.ID })...)
 	if len(securityGroupIds) == 0 {
-		return "", fmt.Errorf("no security groups are discovered")
+		return "", fmt.Errorf("no security groups are present in the status")
 	}
 
 	if !securityGroupIds.Equal(sets.New(ec2Instance.SecurityGroupIDs...)) {

diff --git a/pkg/cloudprovider/suite_test.go b/pkg/cloudprovider/suite_test.go
@@ -138,6 +138,20 @@ var _ = Describe("CloudProvider", func() {
 				},
 			},
 		})
+		nodeClass.Status.SecurityGroups = []v1beta1.SecurityGroup{
+			{
+				ID:   "sg-test1",
+				Name: "securityGroup-test1",
+			},
+			{
+				ID:   "sg-test2",
+				Name: "securityGroup-test2",
+			},
+			{
+				ID:   "sg-test3",
+				Name: "securityGroup-test3",
+			},
+		}
 		Expect(awsEnv.InstanceTypesProvider.UpdateInstanceTypes(ctx)).To(Succeed())
 		Expect(awsEnv.InstanceTypesProvider.UpdateInstanceTypeOfferings(ctx)).To(Succeed())
 	})
@@ -586,6 +600,11 @@ var _ = Describe("CloudProvider", func() {
 					},
 				},
 			})
+			nodeClass.Status.SecurityGroups = []v1beta1.SecurityGroup{
+				{
+					ID: validSecurityGroup,
+				},
+			}
 			ExpectApplied(ctx, env.Client, nodePool, nodeClass)
 			instanceTypes, err := cloudProvider.GetInstanceTypes(ctx, nodePool)
 			Expect(err).ToNot(HaveOccurred())
@@ -671,7 +690,8 @@ var _ = Describe("CloudProvider", func() {
 			Expect(isDrifted).To(BeEmpty())
 		})
 		It("should return an error if the security groups are empty", func() {
-			awsEnv.EC2API.DescribeSecurityGroupsOutput.Set(&ec2.DescribeSecurityGroupsOutput{SecurityGroups: []*ec2.SecurityGroup{}})
+			nodeClass.Status.SecurityGroups = []v1beta1.SecurityGroup{}
+			ExpectApplied(ctx, env.Client, nodeClass)
 			// Instance is a reference to what we return in the GetInstances call
 			instance.SecurityGroups = []*ec2.GroupIdentifier{{GroupId: aws.String(fake.SecurityGroupID())}}
 			_, err := cloudProvider.IsDrifted(ctx, nodeClaim)
@@ -692,18 +712,17 @@ var _ = Describe("CloudProvider", func() {
 			Expect(isDrifted).To(Equal(cloudprovider.SecurityGroupDrift))
 		})
 		It("should return drifted if more security groups are present than instance security groups then discovered from nodeclass", func() {
-			awsEnv.EC2API.DescribeSecurityGroupsOutput.Set(&ec2.DescribeSecurityGroupsOutput{
-				SecurityGroups: []*ec2.SecurityGroup{
-					{
-						GroupId:   aws.String(validSecurityGroup),
-						GroupName: aws.String("test-securitygroup"),
-					},
-					{
-						GroupId:   aws.String(fake.SecurityGroupID()),
-						GroupName: aws.String("test-securitygroup"),
-					},
+			nodeClass.Status.SecurityGroups = []v1beta1.SecurityGroup{
+				{
+					ID:   validSecurityGroup,
+					Name: "test-securitygroup",
 				},
-			})
+				{
+					ID:   fake.SecurityGroupID(),
+					Name: "test-securitygroup",
+				},
+			}
+			ExpectApplied(ctx, env.Client, nodeClass)
 			isDrifted, err := cloudProvider.IsDrifted(ctx, nodeClaim)
 			Expect(err).ToNot(HaveOccurred())
 			Expect(isDrifted).To(Equal(cloudprovider.SecurityGroupDrift))
@@ -777,6 +796,13 @@ var _ = Describe("CloudProvider", func() {
 							},
 						},
 					},
+					Status: v1beta1.EC2NodeClassStatus{
+						SecurityGroups: []v1beta1.SecurityGroup{
+							{
+								ID: validSecurityGroup,
+							},
+						},
+					},
 				}
 				nodeClass.Annotations = lo.Assign(nodeClass.Annotations, map[string]string{v1beta1.AnnotationEC2NodeClassHash: nodeClass.Hash()})
 				nodeClaim.Annotations = lo.Assign(nodeClaim.Annotations, map[string]string{v1beta1.AnnotationEC2NodeClassHash: nodeClass.Hash()})
@@ -1013,6 +1039,13 @@ var _ = Describe("CloudProvider", func() {
 						},
 					},
 				},
+				Status: v1beta1.EC2NodeClassStatus{
+					SecurityGroups: []v1beta1.SecurityGroup{
+						{
+							ID: "sg-test1",
+						},
+					},
+				},
 			})
 			nodePool2 := coretest.NodePool(corev1beta1.NodePool{
 				Spec: corev1beta1.NodePoolSpec{

diff --git a/pkg/providers/instance/suite_test.go b/pkg/providers/instance/suite_test.go
@@ -26,16 +26,19 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/client-go/tools/record"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 	corev1beta1 "sigs.k8s.io/karpenter/pkg/apis/v1beta1"
 	corecloudprovider "sigs.k8s.io/karpenter/pkg/cloudprovider"
 	"sigs.k8s.io/karpenter/pkg/events"
+	"sigs.k8s.io/karpenter/pkg/operator/controller"
 	coreoptions "sigs.k8s.io/karpenter/pkg/operator/options"
 	"sigs.k8s.io/karpenter/pkg/operator/scheme"
 	coretest "sigs.k8s.io/karpenter/pkg/test"
 
 	"github.com/aws/karpenter-provider-aws/pkg/apis"
 	"github.com/aws/karpenter-provider-aws/pkg/apis/v1beta1"
 	"github.com/aws/karpenter-provider-aws/pkg/cloudprovider"
+	"github.com/aws/karpenter-provider-aws/pkg/controllers/nodeclass/status"
 	"github.com/aws/karpenter-provider-aws/pkg/fake"
 	"github.com/aws/karpenter-provider-aws/pkg/operator/options"
 	"github.com/aws/karpenter-provider-aws/pkg/providers/instance"
@@ -51,6 +54,7 @@ var ctx context.Context
 var env *coretest.Environment
 var awsEnv *test.Environment
 var cloudProvider *cloudprovider.CloudProvider
+var statusController controller.Controller
 
 func TestAWS(t *testing.T) {
 	ctx = TestContextWithLogger(t)
@@ -65,6 +69,14 @@ var _ = BeforeSuite(func() {
 	awsEnv = test.NewEnvironment(ctx, env)
 	cloudProvider = cloudprovider.New(awsEnv.InstanceTypesProvider, awsEnv.InstanceProvider, events.NewRecorder(&record.FakeRecorder{}),
 		env.Client, awsEnv.AMIProvider, awsEnv.SecurityGroupProvider, awsEnv.SubnetProvider)
+	statusController = status.NewController(
+		env.Client,
+		awsEnv.SubnetProvider,
+		awsEnv.SecurityGroupProvider,
+		awsEnv.AMIProvider,
+		awsEnv.InstanceProfileProvider,
+		awsEnv.LaunchTemplateProvider,
+	)
 })
 
 var _ = AfterSuite(func() {
@@ -111,6 +123,8 @@ var _ = Describe("InstanceProvider", func() {
 	})
 	It("should return an ICE error when all attempted instance types return an ICE error", func() {
 		ExpectApplied(ctx, env.Client, nodeClaim, nodePool, nodeClass)
+		ExpectReconcileSucceeded(ctx, statusController, client.ObjectKeyFromObject(nodeClass))
+		nodeClass = ExpectExists(ctx, env.Client, nodeClass)
 		awsEnv.EC2API.InsufficientCapacityPools.Set([]fake.CapacityPool{
 			{CapacityType: corev1beta1.CapacityTypeOnDemand, InstanceType: "m5.xlarge", Zone: "test-zone-1a"},
 			{CapacityType: corev1beta1.CapacityTypeOnDemand, InstanceType: "m5.xlarge", Zone: "test-zone-1b"},