diff --git a/pkg/apis/v1/ec2nodeclass_status.go b/pkg/apis/v1/ec2nodeclass_status.go index d122880cc612..4c210ef81789 100644 --- a/pkg/apis/v1/ec2nodeclass_status.go +++ b/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) } diff --git a/pkg/cloudprovider/cloudprovider.go b/pkg/cloudprovider/cloudprovider.go index 26f54b69eef5..c63c258c7974 100644 --- a/pkg/cloudprovider/cloudprovider.go +++ b/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 { diff --git a/pkg/cloudprovider/suite_test.go b/pkg/cloudprovider/suite_test.go index d59f95be9b33..7fdcf9d37c87 100644 --- a/pkg/cloudprovider/suite_test.go +++ b/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{ diff --git a/pkg/controllers/nodeclass/status/controller.go b/pkg/controllers/nodeclass/status/controller.go index d49306f90044..9845bf5a064b 100644 --- a/pkg/controllers/nodeclass/status/controller.go +++ b/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) diff --git a/pkg/controllers/nodeclass/status/readiness_test.go b/pkg/controllers/nodeclass/status/readiness_test.go index 419732dbb6ba..ed6bd9d5ea91 100644 --- a/pkg/controllers/nodeclass/status/readiness_test.go +++ b/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() { diff --git a/pkg/controllers/nodeclass/status/suite_test.go b/pkg/controllers/nodeclass/status/suite_test.go index afb51f1f069a..97ee37a51e07 100644 --- a/pkg/controllers/nodeclass/status/suite_test.go +++ b/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) diff --git a/pkg/controllers/nodeclass/status/validation.go b/pkg/controllers/nodeclass/status/validation.go new file mode 100644 index 000000000000..2e0b1eccedc4 --- /dev/null +++ b/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 +} diff --git a/pkg/controllers/nodeclass/status/validation_test.go b/pkg/controllers/nodeclass/status/validation_test.go new file mode 100644 index 000000000000..9f590db52c6b --- /dev/null +++ b/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()) + }) +}) diff --git a/pkg/test/utils.go b/pkg/test/utils.go new file mode 100644 index 000000000000..4e4adebd5752 --- /dev/null +++ b/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 +}