fix: mark ec2nodeclass as NotReady when validation is bypassed (#7404)

pkg/apis/v1/ec2nodeclass_status.go

@@ -24,6 +24,7 @@ const (
 	ConditionTypeSecurityGroupsReady  = "SecurityGroupsReady"
 	ConditionTypeAMIsReady            = "AMIsReady"
 	ConditionTypeInstanceProfileReady = "InstanceProfileReady"
+	ConditionTypeValidationSucceeded  = "ValidationSucceeded"
 )
 
 // Subnet contains resolved Subnet selector values utilized for node launch
@@ -93,6 +94,7 @@ func (in *EC2NodeClass) StatusConditions() status.ConditionSet {
 		ConditionTypeSubnetsReady,
 		ConditionTypeSecurityGroupsReady,
 		ConditionTypeInstanceProfileReady,
+		ConditionTypeValidationSucceeded,
 	).For(in)
 }
 

pkg/cloudprovider/cloudprovider.go

@@ -18,7 +18,6 @@ import (
 	"context"
 	stderrors "errors"
 	"fmt"
-	"strings"
 	"time"
 
 	ec2types "github.com/aws/aws-sdk-go-v2/service/ec2/types"
@@ -104,7 +103,11 @@ func (c *CloudProvider) Create(ctx context.Context, nodeClaim *karpv1.NodeClaim)
 	if len(instanceTypes) == 0 {
 		return nil, cloudprovider.NewInsufficientCapacityError(fmt.Errorf("all requested instance types were unavailable during launch"))
 	}
-	instance, err := c.instanceProvider.Create(ctx, nodeClass, nodeClaim, getTags(ctx, nodeClass, nodeClaim), instanceTypes)
+	tags, err := getTags(ctx, nodeClass, nodeClaim)
+	if err != nil {
+		return nil, cloudprovider.NewNodeClassNotReadyError(err)
+	}
+	instance, err := c.instanceProvider.Create(ctx, nodeClass, nodeClaim, tags, instanceTypes)
 	if err != nil {
 		conditionMessage := "Error creating instance"
 		var createError *cloudprovider.CreateError
@@ -234,16 +237,24 @@ func (c *CloudProvider) GetSupportedNodeClasses() []status.Object {
 	return []status.Object{&v1.EC2NodeClass{}}
 }
 
-func getTags(ctx context.Context, nodeClass *v1.EC2NodeClass, nodeClaim *karpv1.NodeClaim) map[string]string {
+func getTags(ctx context.Context, nodeClass *v1.EC2NodeClass, nodeClaim *karpv1.NodeClaim) (map[string]string, error) {
+	if offendingTag, found := lo.FindKeyBy(nodeClass.Spec.Tags, func(k string, v string) bool {
+		for _, exp := range v1.RestrictedTagPatterns {
+			if exp.MatchString(k) {
+				return true
+			}
+		}
+		return false
+	}); found {
+		return nil, fmt.Errorf("%q tag does not pass tag validation requirements", offendingTag)
+	}
 	staticTags := map[string]string{
 		fmt.Sprintf("kubernetes.io/cluster/%s", options.FromContext(ctx).ClusterName): "owned",
 		karpv1.NodePoolLabelKey: nodeClaim.Labels[karpv1.NodePoolLabelKey],
 		v1.EKSClusterNameTagKey: options.FromContext(ctx).ClusterName,
 		v1.LabelNodeClass:       nodeClass.Name,
 	}
-	return lo.Assign(lo.OmitBy(nodeClass.Spec.Tags, func(key string, _ string) bool {
-		return strings.HasPrefix(key, "kubernetes.io/cluster/")
-	}), staticTags)
+	return lo.Assign(nodeClass.Spec.Tags, staticTags), nil
 }
 
 func (c *CloudProvider) RepairPolicies() []cloudprovider.RepairPolicy {

pkg/cloudprovider/suite_test.go

@@ -80,7 +80,7 @@ func TestAWS(t *testing.T) {
 }
 
 var _ = BeforeSuite(func() {
-	env = coretest.NewEnvironment(coretest.WithCRDs(apis.CRDs...), coretest.WithCRDs(v1alpha1.CRDs...))
+	env = coretest.NewEnvironment(coretest.WithCRDs(test.RemoveNodeClassTagValidation(apis.CRDs)...), coretest.WithCRDs(v1alpha1.CRDs...))
 	ctx = coreoptions.ToContext(ctx, coretest.Options())
 	ctx = options.ToContext(ctx, test.Options())
 	ctx, stop = context.WithCancel(ctx)
@@ -213,6 +213,14 @@ var _ = Describe("CloudProvider", func() {
 		Expect(err).To(HaveOccurred())
 		Expect(corecloudprovider.IsNodeClassNotReadyError(err)).To(BeTrue())
 	})
+	It("should return NodeClassNotReady error on creation if NodeClass tag validation fails", func() {
+		ExpectApplied(ctx, env.Client, nodePool, nodeClass, nodeClaim)
+		nodeClass.Spec.Tags = map[string]string{"kubernetes.io/cluster/thewrongcluster": "owned"}
+		ExpectApplied(ctx, env.Client, nodeClass)
+		_, err := cloudProvider.Create(ctx, nodeClaim)
+		Expect(err).To(HaveOccurred())
+		Expect(corecloudprovider.IsNodeClassNotReadyError(err)).To(BeTrue())
+	})
 	It("should return an ICE error when there are no instance types to launch", func() {
 		// Specify no instance types and expect to receive a capacity error
 		nodeClaim.Spec.Requirements = []karpv1.NodeSelectorRequirementWithMinValues{

pkg/controllers/nodeclass/status/controller.go

@@ -51,6 +51,7 @@ type Controller struct {
 	instanceprofile *InstanceProfile
 	subnet          *Subnet
 	securitygroup   *SecurityGroup
+	validation      *Validation
 	readiness       *Readiness //TODO : Remove this when we have sub status conditions
 }
 
@@ -63,6 +64,7 @@ func NewController(kubeClient client.Client, subnetProvider subnet.Provider, sec
 		subnet:          &Subnet{subnetProvider: subnetProvider},
 		securitygroup:   &SecurityGroup{securityGroupProvider: securityGroupProvider},
 		instanceprofile: &InstanceProfile{instanceProfileProvider: instanceProfileProvider},
+		validation:      &Validation{},
 		readiness:       &Readiness{launchTemplateProvider: launchTemplateProvider},
 	}
 }
@@ -93,6 +95,7 @@ func (c *Controller) Reconcile(ctx context.Context, nodeClass *v1.EC2NodeClass)
 		c.subnet,
 		c.securitygroup,
 		c.instanceprofile,
+		c.validation,
 		c.readiness,
 	} {
 		res, err := reconciler.Reconcile(ctx, nodeClass)

pkg/controllers/nodeclass/status/readiness_test.go

@@ -53,7 +53,7 @@ var _ = Describe("NodeClass Status Condition Controller", func() {
 		ExpectApplied(ctx, env.Client, nodeClass)
 		ExpectObjectReconciled(ctx, env.Client, statusController, nodeClass)
 		nodeClass = ExpectExists(ctx, env.Client, nodeClass)
-		Expect(nodeClass.Status.Conditions).To(HaveLen(5))
+		Expect(nodeClass.Status.Conditions).To(HaveLen(6))
 		Expect(nodeClass.StatusConditions().Get(status.ConditionReady).IsTrue()).To(BeTrue())
 	})
 	It("should update status condition as Not Ready", func() {

pkg/controllers/nodeclass/status/suite_test.go

@@ -48,7 +48,7 @@ func TestAPIs(t *testing.T) {
 }
 
 var _ = BeforeSuite(func() {
-	env = coretest.NewEnvironment(coretest.WithCRDs(apis.CRDs...), coretest.WithCRDs(v1alpha1.CRDs...), coretest.WithFieldIndexers(coretest.NodeClaimNodeClassRefFieldIndexer(ctx)))
+	env = coretest.NewEnvironment(coretest.WithCRDs(test.RemoveNodeClassTagValidation(apis.CRDs)...), coretest.WithCRDs(v1alpha1.CRDs...), coretest.WithFieldIndexers(coretest.NodeClaimNodeClassRefFieldIndexer(ctx)))
 	ctx = coreoptions.ToContext(ctx, coretest.Options())
 	ctx = options.ToContext(ctx, test.Options())
 	awsEnv = test.NewEnvironment(ctx, env)

pkg/controllers/nodeclass/status/validation.go

@@ -0,0 +1,45 @@
+/*
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package status
+
+import (
+	"context"
+	"fmt"
+
+	"github.com/samber/lo"
+
+	"sigs.k8s.io/controller-runtime/pkg/reconcile"
+
+	v1 "github.com/aws/karpenter-provider-aws/pkg/apis/v1"
+)
+
+type Validation struct{}
+
+func (n Validation) Reconcile(ctx context.Context, nodeClass *v1.EC2NodeClass) (reconcile.Result, error) {
+	if offendingTag, found := lo.FindKeyBy(nodeClass.Spec.Tags, func(k string, v string) bool {
+		for _, exp := range v1.RestrictedTagPatterns {
+			if exp.MatchString(k) {
+				return true
+			}
+		}
+		return false
+	}); found {
+		nodeClass.StatusConditions().SetFalse(v1.ConditionTypeValidationSucceeded, "TagValidationFailed",
+			fmt.Sprintf("%q tag does not pass tag validation requirements", offendingTag))
+		return reconcile.Result{}, reconcile.TerminalError(fmt.Errorf("%q tag does not pass tag validation requirements", offendingTag))
+	}
+	nodeClass.StatusConditions().SetTrue(v1.ConditionTypeValidationSucceeded)
+	return reconcile.Result{}, nil
+}

pkg/controllers/nodeclass/status/validation_test.go

@@ -0,0 +1,81 @@
+/*
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package status_test
+
+import (
+	status "github.com/awslabs/operatorpkg/status"
+	"github.com/samber/lo"
+
+	v1 "github.com/aws/karpenter-provider-aws/pkg/apis/v1"
+	"github.com/aws/karpenter-provider-aws/pkg/test"
+
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+	. "sigs.k8s.io/karpenter/pkg/test/expectations"
+)
+
+var _ = Describe("NodeClass Validation Status Controller", func() {
+	BeforeEach(func() {
+		nodeClass = test.EC2NodeClass(v1.EC2NodeClass{
+			Spec: v1.EC2NodeClassSpec{
+				SubnetSelectorTerms: []v1.SubnetSelectorTerm{
+					{
+						Tags: map[string]string{"*": "*"},
+					},
+				},
+				SecurityGroupSelectorTerms: []v1.SecurityGroupSelectorTerm{
+					{
+						Tags: map[string]string{"*": "*"},
+					},
+				},
+				AMIFamily: lo.ToPtr(v1.AMIFamilyCustom),
+				AMISelectorTerms: []v1.AMISelectorTerm{
+					{
+						Tags: map[string]string{"*": "*"},
+					},
+				},
+				Tags: map[string]string{
+					"kubernetes.io/cluster/anothercluster": "owned",
+				},
+			},
+		})
+	})
+	DescribeTable("should update status condition on nodeClass as NotReady when tag validation fails", func(illegalTag map[string]string) {
+		nodeClass.Spec.Tags = illegalTag
+		ExpectApplied(ctx, env.Client, nodeClass)
+		err := ExpectObjectReconcileFailed(ctx, env.Client, statusController, nodeClass)
+		Expect(err).To(HaveOccurred())
+		nodeClass = ExpectExists(ctx, env.Client, nodeClass)
+		Expect(nodeClass.Status.Conditions).To(HaveLen(6))
+		Expect(nodeClass.StatusConditions().Get(v1.ConditionTypeValidationSucceeded).IsFalse()).To(BeTrue())
+		Expect(nodeClass.StatusConditions().Get(status.ConditionReady).IsFalse()).To(BeTrue())
+		Expect(nodeClass.StatusConditions().Get(status.ConditionReady).Message).To(Equal("ValidationSucceeded=False"))
+	},
+		Entry("kubernetes.io/cluster*", map[string]string{"kubernetes.io/cluster/acluster": "owned"}),
+		Entry(v1.NodePoolTagKey, map[string]string{v1.NodePoolTagKey: "testnodepool"}),
+		Entry(v1.EKSClusterNameTagKey, map[string]string{v1.EKSClusterNameTagKey: "acluster"}),
+		Entry(v1.NodeClassTagKey, map[string]string{v1.NodeClassTagKey: "testnodeclass"}),
+		Entry(v1.NodeClaimTagKey, map[string]string{v1.NodeClaimTagKey: "testnodeclaim"}),
+	)
+	It("should update status condition as Ready when tags are valid", func() {
+		nodeClass.Spec.Tags = map[string]string{}
+		ExpectApplied(ctx, env.Client, nodeClass)
+		ExpectObjectReconciled(ctx, env.Client, statusController, nodeClass)
+		nodeClass = ExpectExists(ctx, env.Client, nodeClass)
+
+		Expect(nodeClass.StatusConditions().Get(v1.ConditionTypeValidationSucceeded).IsTrue()).To(BeTrue())
+		Expect(nodeClass.StatusConditions().Get(status.ConditionReady).IsTrue()).To(BeTrue())
+	})
+})

pkg/test/utils.go

@@ -0,0 +1,33 @@
+/*
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package test
+
+import (
+	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
+
+	"github.com/aws/karpenter-provider-aws/pkg/apis"
+)
+
+func RemoveNodeClassTagValidation(crds []*apiextensionsv1.CustomResourceDefinition) []*apiextensionsv1.CustomResourceDefinition {
+	for _, crd := range apis.CRDs {
+		if crd.Name != "ec2nodeclasses.karpenter.k8s.aws" {
+			continue
+		}
+		overrideProperties := crd.Spec.Versions[0].Schema.OpenAPIV3Schema.Properties["spec"].Properties["tags"]
+		overrideProperties.XValidations = nil
+		crd.Spec.Versions[0].Schema.OpenAPIV3Schema.Properties["spec"].Properties["tags"] = overrideProperties
+	}
+	return crds
+}