ci: add lint ci for helm chart.

liangyuanpeng · liangyuanpeng · commit ae4f61323f8b · 2024-08-08T14:12:37.000Z
Signed-off-by: Lan Liang &lt;gcslyp@gmail.com&gt;
diff --git a/.github/workflows/lint-chart.yaml b/.github/workflows/lint-chart.yaml
@@ -0,0 +1,81 @@
+# validate any chart changes under charts directory
+name: Chart Lint
+
+env:
+  HELM_VERSION: v3.11.2
+  KIND_VERSION: v0.23.0
+  KIND_NODE_IMAGE: kindest/node:v1.30.0
+  K8S_VERSION: v1.30.0
+
+on:
+  push:
+    # Exclude branches created by Dependabot to avoid triggering current workflow
+    # for PRs initiated by Dependabot.
+    branches-ignore:
+      - 'dependabot/**'
+    paths:
+      - "kwok/charts/**"
+  pull_request:
+    paths:
+      - "charts/**"
+
+permissions:
+  contents: read
+
+jobs:
+  chart-lint-test:
+    runs-on: ubuntu-22.04
+    steps:
+      - name: Checkout
+        uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332
+        with:
+          fetch-depth: 0
+
+      - name: Set up Helm
+        uses: azure/setup-helm@v4
+        with:
+          version: ${{ env.HELM_VERSION }}
+
+      - name: Run chart-testing (template)
+        run: |
+          helm template --dependency-update ./kwok/charts --debug > /dev/null   
+
+      # Python is required because `ct lint` runs Yamale (https://github.com/23andMe/Yamale) and
+      # yamllint (https://github.com/adrienverge/yamllint) which require Python
+      - name: Set up Python
+        uses: actions/setup-python@39cd14951b08e74b54015e9e001cdefcf80e669f # v5.1
+        with:
+          python-version: 3.10
+          check-latest: true
+
+      - name: Set up chart-testing
+        uses: helm/chart-testing-action@e6669bcd63d7cb57cb4380c33043eebe5d111992
+
+      - name: Add dependency chart repos
+        run: |
+          helm repo add bitnami https://charts.bitnami.com/bitnami
+
+      - name: Run chart-testing (list-changed)
+        id: list-changed
+        run: |
+          changed=$( ct list-changed )
+          if [[ -n "$changed" ]]; then
+            echo "changed=true" >> $GITHUB_OUTPUT
+          fi
+
+      - name: Run chart-testing (lint)
+        if: steps.list-changed.outputs.changed == 'true'
+        run: ct lint --debug --check-version-increment=false
+
+      - name: Create kind cluster
+        uses: helm/kind-action@0025e74a8c7512023d06dc019c617aa3cf561fde # v1.10.0
+        if: steps.list-changed.outputs.changed == 'true'
+        with:
+          wait: 120s
+          version: ${{ env.KIND_VERSION }}
+          node_image: ${{ env.KIND_NODE_IMAGE }}
+          kubectl_version: ${{ env.K8S_VERSION }}
+
+      - name: Run chart-testing (install)
+        if: steps.list-changed.outputs.changed == 'true'
+        run: ct install --debug --helm-extra-args "--timeout 800s"
diff --git a/kwok/charts/crds/karpenter.sh_nodeclaims.yaml b/kwok/charts/crds/karpenter.sh_nodeclaims.yaml
@@ -86,6 +86,7 @@ spec:
                   properties:
                     group:
                       description: API version of the referent
+                      pattern: ^[^/]*$
                       type: string
                     kind:
                       description: 'Kind of the referent; More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds"'
diff --git a/kwok/charts/crds/karpenter.sh_nodepools.yaml b/kwok/charts/crds/karpenter.sh_nodepools.yaml
@@ -225,6 +225,7 @@ spec:
                           properties:
                             group:
                               description: API version of the referent
+                              pattern: ^[^/]*$
                               type: string
                             kind:
                               description: 'Kind of the referent; More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds"'
diff --git a/kwok/charts/templates/role.yaml b/kwok/charts/templates/role.yaml
@@ -75,4 +75,4 @@ rules:
   # Write
   - apiGroups: ["coordination.k8s.io"]
     resources: ["leases"]
-    verbs: ["delete"]
+    verbs: ["delete"]
diff --git a/kwok/charts/templates/secret-webhook-cert.yaml b/kwok/charts/templates/secret-webhook-cert.yaml
@@ -11,4 +11,4 @@ metadata:
     {{- toYaml . | nindent 4 }}
   {{- end }}
 # data: {} # Injected by karpenter-webhook
-{{- end }}
+{{- end }}
diff --git a/pkg/apis/crds/karpenter.sh_nodeclaims.yaml b/pkg/apis/crds/karpenter.sh_nodeclaims.yaml
@@ -86,6 +86,7 @@ spec:
                   properties:
                     group:
                       description: API version of the referent
+                      pattern: ^[^/]*$
                       type: string
                     kind:
                       description: 'Kind of the referent; More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds"'
diff --git a/pkg/apis/crds/karpenter.sh_nodepools.yaml b/pkg/apis/crds/karpenter.sh_nodepools.yaml
@@ -225,6 +225,7 @@ spec:
                           properties:
                             group:
                               description: API version of the referent
+                              pattern: ^[^/]*$
                               type: string
                             kind:
                               description: 'Kind of the referent; More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds"'
diff --git a/pkg/apis/v1/nodeclaim.go b/pkg/apis/v1/nodeclaim.go
@@ -103,6 +103,7 @@ type NodeClassReference struct {
 	// +required
 	Name string `json:"name"`
 	// API version of the referent
+	// +kubebuilder:validation:Pattern=`^[^/]*$`
 	// +required
 	Group string `json:"group"`
 }
diff --git a/pkg/apis/v1/nodeclaim_validation_cel_test.go b/pkg/apis/v1/nodeclaim_validation_cel_test.go
@@ -21,6 +21,10 @@ import (
 	"strings"
 	"time"
 
+	"github.com/awslabs/operatorpkg/object"
+
+	"sigs.k8s.io/karpenter/pkg/test/v1alpha1"
+
 	"sigs.k8s.io/karpenter/pkg/test"
 
 	"github.com/Pallinder/go-randomdata"
@@ -95,6 +99,24 @@ var _ = Describe("Validation", func() {
 			Expect(env.Client.Create(ctx, nodeClaim)).To(Succeed())
 		})
 	})
+	Context("NodeClassRef", func() {
+		It("should succeed for valid group", func() {
+			nodeClaim.Spec.NodeClassRef = &NodeClassReference{
+				Kind:  object.GVK(&v1alpha1.TestNodeClass{}).Kind,
+				Name:  "nodeclass-test",
+				Group: object.GVK(&v1alpha1.TestNodeClass{}).Group,
+			}
+			Expect(env.Client.Create(ctx, nodeClaim)).To(Succeed())
+		})
+		It("should fail for invalid group", func() {
+			nodeClaim.Spec.NodeClassRef = &NodeClassReference{
+				Kind:  object.GVK(&v1alpha1.TestNodeClass{}).Kind,
+				Name:  "nodeclass-test",
+				Group: "karpenter.test.sh/v1",
+			}
+			Expect(env.Client.Create(ctx, nodeClaim)).ToNot(Succeed())
+		})
+	})
 	Context("Requirements", func() {
 		It("should allow supported ops", func() {
 			nodeClaim.Spec.Requirements = []NodeSelectorRequirementWithMinValues{
diff --git a/pkg/controllers/nodeclaim/disruption/drift_test.go b/pkg/controllers/nodeclaim/disruption/drift_test.go
@@ -368,7 +368,7 @@ var _ = Describe("Drift", func() {
 							NodeClassRef: &v1.NodeClassReference{
 								Kind:  "fakeKind",
 								Name:  "fakeName",
-								Group: "fakeGroup/fakeVerion",
+								Group: "fakeGroup",
 							},
 							Taints: []corev1.Taint{
 								{
diff --git a/pkg/controllers/nodeclaim/lifecycle/registration.go b/pkg/controllers/nodeclaim/lifecycle/registration.go
@@ -64,6 +64,7 @@ func (r *Registration) Reconcile(ctx context.Context, nodeClaim *v1.NodeClaim) (
 	// check if sync succeeded but setting the registered status condition failed
 	// if sync succeeded, then the label will be present and the taint will be gone
 	if _, ok := node.Labels[v1.NodeRegisteredLabelKey]; !ok && !hasStartupTaint {
+		nodeClaim.StatusConditions().SetFalse(v1.ConditionTypeRegistered, "UnregisteredTaintNotFound", fmt.Sprintf("Invariant violated, %s taint must be present on Karpenter-managed nodes", v1.UnregisteredTaintKey))
 		return reconcile.Result{}, fmt.Errorf("missing required startup taint, %s", v1.UnregisteredTaintKey)
 	}
 	ctx = log.IntoContext(ctx, log.FromContext(ctx).WithValues("Node", klog.KRef("", node.Name)))
diff --git a/pkg/controllers/nodeclaim/lifecycle/registration_test.go b/pkg/controllers/nodeclaim/lifecycle/registration_test.go
@@ -107,6 +107,8 @@ var _ = Describe("Registration", func() {
 		node := test.Node(test.NodeOptions{ProviderID: nodeClaim.Status.ProviderID})
 		ExpectApplied(ctx, env.Client, node)
 		_ = ExpectObjectReconcileFailed(ctx, env.Client, nodeClaimController, nodeClaim)
+		nodeClaim = ExpectExists(ctx, env.Client, nodeClaim)
+		Expect(ExpectStatusConditionExists(nodeClaim, v1.ConditionTypeRegistered).Status).To(Equal(metav1.ConditionFalse))
 	})
 	It("should sync the labels to the Node when the Node comes online", func() {
 		nodeClaim := test.NodeClaim(v1.NodeClaim{
diff --git a/pkg/controllers/provisioning/suite_test.go b/pkg/controllers/provisioning/suite_test.go
@@ -1260,7 +1260,7 @@ var _ = Describe("Provisioning", func() {
 					Template: v1.NodeClaimTemplate{
 						Spec: v1.NodeClaimTemplateSpec{
 							NodeClassRef: &v1.NodeClassReference{
-								Group: "cloudprovider.karpenter.sh/v1",
+								Group: "cloudprovider.karpenter.sh",
 								Kind:  "CloudProvider",
 								Name:  "default",
 							},
@@ -1275,7 +1275,7 @@ var _ = Describe("Provisioning", func() {
 			Expect(cloudProvider.CreateCalls).To(HaveLen(1))
 			Expect(cloudProvider.CreateCalls[0].Spec.NodeClassRef).To(Equal(
 				&v1.NodeClassReference{
-					Group: "cloudprovider.karpenter.sh/v1",
+					Group: "cloudprovider.karpenter.sh",
 					Kind:  "CloudProvider",
 					Name:  "default",
 				},
diff --git a/pkg/scheduling/taints.go b/pkg/scheduling/taints.go
@@ -21,24 +21,27 @@ import (
 
 	"github.com/samber/lo"
 	"go.uber.org/multierr"
-	v1 "k8s.io/api/core/v1"
+	corev1 "k8s.io/api/core/v1"
 	cloudproviderapi "k8s.io/cloud-provider/api"
+
+	v1 "sigs.k8s.io/karpenter/pkg/apis/v1"
 )
 
 // KnownEphemeralTaints are taints that are expected to be added to a node while it's initializing
 // If the node is a Karpenter-managed node, we don't consider these taints while the node is uninitialized
 // since we expect these taints to eventually be removed
-var KnownEphemeralTaints = []v1.Taint{
-	{Key: v1.TaintNodeNotReady, Effect: v1.TaintEffectNoSchedule},
-	{Key: v1.TaintNodeUnreachable, Effect: v1.TaintEffectNoSchedule},
-	{Key: cloudproviderapi.TaintExternalCloudProvider, Effect: v1.TaintEffectNoSchedule, Value: "true"},
+var KnownEphemeralTaints = []corev1.Taint{
+	{Key: corev1.TaintNodeNotReady, Effect: corev1.TaintEffectNoSchedule},
+	{Key: corev1.TaintNodeUnreachable, Effect: corev1.TaintEffectNoSchedule},
+	{Key: cloudproviderapi.TaintExternalCloudProvider, Effect: corev1.TaintEffectNoSchedule, Value: "true"},
+	v1.UnregisteredNoExecuteTaint,
 }
 
-// Taints is a decorated alias type for []v1.Taint
-type Taints []v1.Taint
+// Taints is a decorated alias type for []corev1.Taint
+type Taints []corev1.Taint
 
 // Tolerates returns true if the pod tolerates all taints.
-func (ts Taints) Tolerates(pod *v1.Pod) (errs error) {
+func (ts Taints) Tolerates(pod *corev1.Pod) (errs error) {
 	for i := range ts {
 		taint := ts[i]
 		tolerates := false
@@ -54,11 +57,11 @@ func (ts Taints) Tolerates(pod *v1.Pod) (errs error) {
 
 // Merge merges in taints with the passed in taints.
 func (ts Taints) Merge(with Taints) Taints {
-	res := lo.Map(ts, func(t v1.Taint, _ int) v1.Taint {
+	res := lo.Map(ts, func(t corev1.Taint, _ int) corev1.Taint {
 		return t
 	})
 	for _, taint := range with {
-		if _, ok := lo.Find(res, func(t v1.Taint) bool {
+		if _, ok := lo.Find(res, func(t corev1.Taint) bool {
 			return taint.MatchTaint(&t)
 		}); !ok {
 			res = append(res, taint)
diff --git a/pkg/utils/node/suite_test.go b/pkg/utils/node/suite_test.go
@@ -69,7 +69,7 @@ var _ = Describe("NodeUtils", func() {
 			Spec: v1.NodeClaimSpec{
 				NodeClassRef: &v1.NodeClassReference{
 					Kind:  "NodeClassRef",
-					Group: "test.cloudprovider/v1",
+					Group: "test.cloudprovider",
 					Name:  "default",
 				},
 			},

Original file line number	Diff line number	Diff line change
`@@ -103,6 +103,7 @@ type NodeClassReference struct {`
`103`	`103`	`// +required`
`104`	`104`	Name string `json:"name"`
`105`	`105`	`// API version of the referent`
	`106`	+ // +kubebuilder:validation:Pattern=`^[^/]*$`
`106`	`107`	`// +required`
`107`	`108`	Group string `json:"group"`
`108`	`109`	`}`
Original file line number	Diff line number	Diff line change
`@@ -368,7 +368,7 @@ var _ = Describe("Drift", func() {`
`368`	`368`	`NodeClassRef: &v1.NodeClassReference{`
`369`	`369`	`Kind: "fakeKind",`
`370`	`370`	`Name: "fakeName",`
`371`		`- Group: "fakeGroup/fakeVerion",`
	`371`	`+ Group: "fakeGroup",`
`372`	`372`	`},`
`373`	`373`	`Taints: []corev1.Taint{`
`374`	`374`	`{`