Use Security Groups status for node launch

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...)) {

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{

pkg/providers/instance/suite_test.go

@@ -106,11 +106,23 @@ var _ = Describe("InstanceProvider", func() {
 				},
 			},
 		})
+		nodeClass.Status.SecurityGroups = []v1beta1.SecurityGroup{
+			{
+				ID: "sg-test1",
+			},
+			{
+				ID: "sg-test2",
+			},
+			{
+				ID: "sg-test3",
+			},
+		}
 		Expect(awsEnv.InstanceTypesProvider.UpdateInstanceTypes(ctx)).To(Succeed())
 		Expect(awsEnv.InstanceTypesProvider.UpdateInstanceTypeOfferings(ctx)).To(Succeed())
 	})
 	It("should return an ICE error when all attempted instance types return an ICE error", func() {
 		ExpectApplied(ctx, env.Client, nodeClaim, nodePool, 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"},

pkg/providers/instancetype/suite_test.go

@@ -155,6 +155,28 @@ var _ = Describe("InstanceTypeProvider", func() {
 				},
 			},
 		})
+		nodeClass.Status.SecurityGroups = []v1beta1.SecurityGroup{
+			{
+				ID: "sg-test1",
+			},
+			{
+				ID: "sg-test2",
+			},
+			{
+				ID: "sg-test3",
+			},
+		}
+		windowsNodeClass.Status.SecurityGroups = []v1beta1.SecurityGroup{
+			{
+				ID: "sg-test1",
+			},
+			{
+				ID: "sg-test2",
+			},
+			{
+				ID: "sg-test3",
+			},
+		}
 		Expect(awsEnv.InstanceTypesProvider.UpdateInstanceTypes(ctx)).To(Succeed())
 		Expect(awsEnv.InstanceTypesProvider.UpdateInstanceTypeOfferings(ctx)).To(Succeed())
 	})

pkg/providers/launchtemplate/launchtemplate.go

@@ -163,28 +163,28 @@ func (p *DefaultProvider) createAMIOptions(ctx context.Context, nodeClass *v1bet
 	if err != nil {
 		return nil, err
 	}
+	// Relying on the status rather than an API call means that Karpenter is subject to a race
+	// condition where EC2NodeClass spec changes haven't propagated to the status once a node 
+	// has launched. 
+	// If a user changes their EC2NodeClass and shortly after Karpenter launches a node,
+	// in the worst case, the node could be drifted and re-created.
+	// TODO @aengeda: add status generation fields to gate node creation until the status is updated from a spec change
 	// Get constrained security groups
-	securityGroups, err := p.securityGroupProvider.List(ctx, nodeClass)
-	if err != nil {
-		return nil, err
-	}
-	if len(securityGroups) == 0 {
-		return nil, fmt.Errorf("no security groups exist given constraints")
+	if len(nodeClass.Status.SecurityGroups) == 0 {
+		return nil, fmt.Errorf("no security groups are present in the status")
 	}
 	options := &amifamily.Options{
 		ClusterName:         options.FromContext(ctx).ClusterName,
 		ClusterEndpoint:     p.ClusterEndpoint,
 		ClusterCIDR:         p.ClusterCIDR.Load(),
 		InstanceProfile:     instanceProfile,
 		InstanceStorePolicy: nodeClass.Spec.InstanceStorePolicy,
-		SecurityGroups: lo.Map(securityGroups, func(s *ec2.SecurityGroup, _ int) v1beta1.SecurityGroup {
-			return v1beta1.SecurityGroup{ID: aws.StringValue(s.GroupId), Name: aws.StringValue(s.GroupName)}
-		}),
-		Tags:          tags,
-		Labels:        labels,
-		CABundle:      p.CABundle,
-		KubeDNSIP:     p.KubeDNSIP,
-		NodeClassName: nodeClass.Name,
+		SecurityGroups:      nodeClass.Status.SecurityGroups,
+		Tags:                tags,
+		Labels:              labels,
+		CABundle:            p.CABundle,
+		KubeDNSIP:           p.KubeDNSIP,
+		NodeClassName:       nodeClass.Name,
 	}
 	if nodeClass.Spec.AssociatePublicIPAddress != nil {
 		options.AssociatePublicIPAddress = nodeClass.Spec.AssociatePublicIPAddress

pkg/providers/launchtemplate/suite_test.go

@@ -146,6 +146,17 @@ var _ = Describe("LaunchTemplate Provider", func() {
 				},
 			},
 		})
+		nodeClass.Status.SecurityGroups = []v1beta1.SecurityGroup{
+			{
+				ID: "sg-test1",
+			},
+			{
+				ID: "sg-test2",
+			},
+			{
+				ID: "sg-test3",
+			},
+		}
 		Expect(awsEnv.InstanceTypesProvider.UpdateInstanceTypes(ctx)).To(Succeed())
 		Expect(awsEnv.InstanceTypesProvider.UpdateInstanceTypeOfferings(ctx)).To(Succeed())
 	})
@@ -171,6 +182,17 @@ var _ = Describe("LaunchTemplate Provider", func() {
 				},
 			},
 		})
+		nodeClass2.Status.SecurityGroups = []v1beta1.SecurityGroup{
+			{
+				ID: "sg-test1",
+			},
+			{
+				ID: "sg-test2",
+			},
+			{
+				ID: "sg-test3",
+			},
+		}
 		pods := []*v1.Pod{
 			coretest.UnschedulablePod(coretest.PodOptions{NodeRequirements: []v1.NodeSelectorRequirement{
 				{