[Feature] Add Kubernetes manifest validation in pre-commit. #2380

LeoLiao123 · 2024-09-14T00:57:35Z

Why are these changes needed?

To resolve issue #2254, the absence of a Kubernetes manifest validation process caused the errors listed in the issue when deploying the Helm Chart. During validation, I also discovered that running helm template ./ in helm-chart/ray-cluster resulted in .template.spec.containers.ports being set to null (which should either be omitted or set to an empty list []). I have fixed this issue as well.

Related issue number

Closes #2254
#2174
#2239

Issue Reproduction and Manual Validation Results

I reproduced the error from Issue #2174. Running helm template ./ within the helm-chart/ray-cluster directory generates the following output:

ray-cluster $ helm template ./
---
# Source: ray-cluster/templates/raycluster-cluster.yaml
apiVersion: ray.io/v1
kind: RayCluster
metadata:
  labels:
    helm.sh/chart: ray-cluster-1.1.0
    app.kubernetes.io/instance: release-name
    app.kubernetes.io/managed-by: Helm
  name: release-name-kuberay

spec:
  headGroupSpec:
    serviceType: ClusterIP
    rayStartParams:
        dashboard-host: "0.0.0.0"
    template:
      spec:
        imagePullSecrets:
          []
        containers:
          -
            volumeMounts:
            - mountPath: /tmp/ray
              name: log-volume
            name: ray-head
            image: rayproject/ray:2.9.0
            imagePullPolicy: IfNotPresent
            resources:
              limits:
                cpu: "1"
                memory: 2G
              requests:
                cpu: "1"
                memory: 2G
            securityContext:
              {}
------>     env:

        volumes:
          - emptyDir: {}
            name: log-volume
        affinity:
          {}


        tolerations:
          []
        nodeSelector:
          {}
      metadata:
        annotations:
          {}
        labels:
          helm.sh/chart: ray-cluster-1.1.0
          app.kubernetes.io/instance: release-name
          app.kubernetes.io/managed-by: Helm

  workerGroupSpecs:
  - rayStartParams:
      {}
    replicas: 1
    minReplicas: 1
    maxReplicas: 3
    numOfHosts: 1
    groupName: workergroup
    template:
      spec:
        imagePullSecrets:
          []
        containers:
          -
            volumeMounts:
            - mountPath: /tmp/ray
              name: log-volume
            name: ray-worker
            image: rayproject/ray:2.9.0
            imagePullPolicy: IfNotPresent
            resources:
              limits:
                cpu: "1"
                memory: 1G
              requests:
                cpu: "1"
                memory: 1G
            securityContext:
              {}
------>     env:

        volumes:
          - emptyDir: {}
            name: log-volume
        affinity:
          {}


        tolerations:
          []
        nodeSelector:
          {}
      metadata:
        annotations:
          {}
        labels:
          helm.sh/chart: ray-cluster-1.1.0
          app.kubernetes.io/instance: release-name
          app.kubernetes.io/managed-by: Helm

After validation, the following output is generated:

stdin - RayCluster release-name-kuberay is invalid: problem validating schema. Check JSON formatting: jsonschema: '/spec/workerGroupSpecs/0/template/spec/containers/0/env' does not validate with file:///tmp/tmp.QsaaVEezYn/raycluster_v1.json#/properties/spec/properties/workerGroupSpecs/items/properties/template/properties/spec/properties/containers/items/properties/env/type: expected array, but got null
Summary: 1 resource found parsing stdin - Valid: 0, Invalid: 1, Errors: 0, Skipped: 0

If we consider the case mentioned earlier where .template.spec.containers.ports is set to null:

ray-cluster $ helm template ./
---
# Source: ray-cluster/templates/raycluster-cluster.yaml
apiVersion: ray.io/v1
kind: RayCluster
metadata:
  labels:
    helm.sh/chart: ray-cluster-1.1.0
    app.kubernetes.io/instance: release-name
    app.kubernetes.io/managed-by: Helm
  name: release-name-kuberay

spec:
  headGroupSpec:
    serviceType: ClusterIP
    rayStartParams:
        dashboard-host: "0.0.0.0"
    template:
      spec:
        imagePullSecrets:
          []
        containers:
          -
            volumeMounts:
            - mountPath: /tmp/ray
              name: log-volume
            name: ray-head
            image: rayproject/ray:2.9.0
            imagePullPolicy: IfNotPresent
            resources:
              limits:
                cpu: "1"
                memory: 2G
              requests:
                cpu: "1"
                memory: 2G
            securityContext:
              {}

        volumes:
          - emptyDir: {}
            name: log-volume
        affinity:
          {}


        tolerations:
          []
        nodeSelector:
          {}
      metadata:
        annotations:
          {}
        labels:
          helm.sh/chart: ray-cluster-1.1.0
          app.kubernetes.io/instance: release-name
          app.kubernetes.io/managed-by: Helm

  workerGroupSpecs:
  - rayStartParams:
      {}
    replicas: 1
    minReplicas: 1
    maxReplicas: 3
    numOfHosts: 1
    groupName: workergroup
    template:
      spec:
        imagePullSecrets:
          []
        containers:
          -
            volumeMounts:
            - mountPath: /tmp/ray
              name: log-volume
            name: ray-worker
            image: rayproject/ray:2.9.0
            imagePullPolicy: IfNotPresent
            resources:
              limits:
                cpu: "1"
                memory: 1G
              requests:
                cpu: "1"
                memory: 1G
            securityContext:
              {}
------>     ports:
              null

        volumes:
          - emptyDir: {}
            name: log-volume
        affinity:
          {}


        tolerations:
          []
        nodeSelector:
          {}
      metadata:
        annotations:
          {}
        labels:
          helm.sh/chart: ray-cluster-1.1.0
          app.kubernetes.io/instance: release-name
          app.kubernetes.io/managed-by: Helm

After validation, the following output is generated:

stdin - RayCluster release-name-kuberay is invalid: problem validating schema. Check JSON formatting: jsonschema: '/spec/workerGroupSpecs/0/template/spec/containers/0/ports' does not validate with file:///tmp/tmp.CnlCCHOuhb/raycluster_v1.json#/properties/spec/properties/workerGroupSpecs/items/properties/template/properties/spec/properties/containers/items/properties/ports/type: expected array, but got null
Summary: 1 resource found parsing stdin - Valid: 0, Invalid: 1, Errors: 0, Skipped: 0

Checks

I've made sure the tests are passing.
Testing Strategy
- Unit tests
- Manual tests
- This PR is not tested :(

Signed-off-by: leoliao <leoyeepaa@gmail.com>

Signed-off-by: Leo Liao <93932709+LeoLiao123@users.noreply.github.com>

LeoLiao123 · 2024-09-14T18:28:10Z

cc: @MortalHappiness, @kevin85421

scripts/validate-helm.sh

MortalHappiness · 2024-09-15T04:00:08Z

scripts/validate-helm.sh

+curl -o "${KUBECONFORM_INSTALL}/crd2schema.py" https://raw.githubusercontent.com/yannh/kubeconform/master/scripts/openapi2jsonschema.py
+SCRIPT_PATH="${KUBECONFORM_INSTALL}/crd2schema.py"


We can consider store this script in our repo so that we don't need to download it everytime.

MortalHappiness · 2024-09-15T04:00:52Z

scripts/validate-helm.sh

+curl -L https://github.com/yannh/kubeconform/releases/latest/download/kubeconform-linux-amd64.tar.gz | tar xz -C "${KUBECONFORM_INSTALL}"
+curl -o "${KUBECONFORM_INSTALL}/crd2schema.py" https://raw.githubusercontent.com/yannh/kubeconform/master/scripts/openapi2jsonschema.py
+SCRIPT_PATH="${KUBECONFORM_INSTALL}/crd2schema.py"
+RAYCLUSTER_CRD_PATH="$KUBERAY_HOME/ray-operator/config/crd/bases/ray.io_rayclusters.yaml"


Why only RayCluster? How about also validate RayJob and RayService?

I think this can be done in either this PR or follow-up PRs.

scripts/validate-helm.sh

.pre-commit-config.yaml

he install info at 3. Store the at 4. Call , 5. check the version of in Signed-off-by: leoliao <leoyeepaa@gmail.com>

scripts/openapi2jsonschema.py

Signed-off-by: leoliao <leoyeepaa@gmail.com>

Signed-off-by: Leo Liao <93932709+LeoLiao123@users.noreply.github.com>

MortalHappiness · 2024-09-17T04:15:06Z

.pre-commit-config.yaml

+
+  - repo: local
+    hooks:
+      - id: validate-helm-charts
+        name: validate helm charts with kubeconform
+        entry: bash -c 'version="0.6.7"; [ "$(kubeconform --v | grep -oP "(?<=v)[\d\.]+")" = "$version" ] || echo "kubeconform version is not $version"; bash scripts/validate-helm.sh'
+        language: system
+        pass_filenames: false


This is wrong. You must separate them to 2 hooks. Otherwise it won't catch the error.

See the following image, I set the version to "invalid" but it still passed.

scripts/validate-helm.sh

Signed-off-by: leoliao <leoyeepaa@gmail.com>

MortalHappiness · 2024-09-17T09:19:03Z

.pre-commit-config.yaml

+    hooks:
+      - id: check-kubeconform-version
+        name: kubeconform version check
+        entry: bash -c 'version="0.6.7"; [ "$(kubeconform -v | grep -oP "(?<=v)[\d\.]+")" = "$version" ] || { echo "kubeconform version is not $version"; }'


Can you replace this line with bash -c 'version="0.6.7"; [ "$(kubeconform -v | grep -oP "(?<=v)[\d\.]+")" = "$version" ] || { echo "kubeconform version is not $version"; exit 1; }'

Also I find that I made a mistake in line 34, can you replace it with bash -c 'version="1.60.3"; [ "$(golangci-lint --version | grep -oP "(?<=version )[\d\.]+")" = "$version" ] || { echo "golangci-lint version is not $version"; exit 1; }'? Thanks.

Otherwise LGTM.

…alidation if the version is not validated. Signed-off-by: leoliao <leoyeepaa@gmail.com>

Signed-off-by: leoliao <leoyeepaa@gmail.com>

MortalHappiness

LGTM

Follow-up PR: Also validate RayJob and RayService

cc @kevin85421

kevin85421 · 2024-09-17T11:52:05Z

Follow-up PR: Also validate RayJob and RayService

@LeoLiao123 ~~can you open an issue to track the progress?~~

@MortalHappiness we only have RayCluster Helm chart.

MortalHappiness · 2024-09-17T11:57:46Z

@kevin85421 Oops I didn't notice that. Thanks.

LeoLiao123 and others added 2 commits September 14, 2024 01:01

Add Kubernetes manifest validation in pre-commit.

ad268aa

Signed-off-by: leoliao <leoyeepaa@gmail.com>

Merge branch 'ray-project:master' into add-strict-parser

6c4d70a

LeoLiao123 changed the title ~~Add strict parser~~ [Feature]Add Kubernetes manifest validation in pre-commit. Sep 14, 2024

LeoLiao123 changed the title ~~[Feature]Add Kubernetes manifest validation in pre-commit.~~ [Feature] Add Kubernetes manifest validation in pre-commit. Sep 14, 2024

LeoLiao123 added 3 commits September 14, 2024 09:15

modified by trailing-whitespace hook

3d9b4e4

Signed-off-by: Leo Liao <93932709+LeoLiao123@users.noreply.github.com>

Add trap to clean up temporary directory after script execution

76ea385

Signed-off-by: Leo Liao <93932709+LeoLiao123@users.noreply.github.com>

Change names to lowercase for consistency

61d4c58

Signed-off-by: Leo Liao <93932709+LeoLiao123@users.noreply.github.com>

MortalHappiness requested changes Sep 15, 2024

View reviewed changes

1. Use instead. 2. don't install for user, and add t

7c48e85

he install info at 3. Store the at 4. Call , 5. check the version of in Signed-off-by: leoliao <leoyeepaa@gmail.com>

MortalHappiness requested changes Sep 17, 2024

View reviewed changes

scripts/openapi2jsonschema.py Outdated Show resolved Hide resolved

LeoLiao123 and others added 2 commits September 17, 2024 10:32

add openapi2jsonschema.py source

6b48e59

Signed-off-by: leoliao <leoyeepaa@gmail.com>

Fix version of kubeconform

b453a73

Signed-off-by: Leo Liao <93932709+LeoLiao123@users.noreply.github.com>

MortalHappiness requested changes Sep 17, 2024

View reviewed changes

LeoLiao123 added 3 commits September 17, 2024 14:07

fix error catch & version validation

aaf3eb6

Signed-off-by: leoliao <leoyeepaa@gmail.com>

split version validation into another hook

78a2a06

Signed-off-by: leoliao <leoyeepaa@gmail.com>

install kubeconform before pre-commit action

72b1b8a

Signed-off-by: leoliao <leoyeepaa@gmail.com>

MortalHappiness reviewed Sep 17, 2024

View reviewed changes

LeoLiao123 added 2 commits September 17, 2024 17:50

add exit 1 in the version check of the pre-commit hook to terminate v…

c6bff87

…alidation if the version is not validated. Signed-off-by: leoliao <leoyeepaa@gmail.com>

fix version extract

f55e8f2

Signed-off-by: leoliao <leoyeepaa@gmail.com>

MortalHappiness approved these changes Sep 17, 2024

View reviewed changes

kevin85421 approved these changes Sep 17, 2024

View reviewed changes

kevin85421 merged commit 39d42fb into ray-project:master Sep 17, 2024
27 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feature] Add Kubernetes manifest validation in pre-commit. #2380

[Feature] Add Kubernetes manifest validation in pre-commit. #2380

LeoLiao123 commented Sep 14, 2024

LeoLiao123 commented Sep 14, 2024

MortalHappiness Sep 15, 2024

MortalHappiness Sep 15, 2024 •

edited

Loading

MortalHappiness Sep 17, 2024 •

edited

Loading

MortalHappiness Sep 17, 2024

MortalHappiness left a comment •

edited

Loading

kevin85421 commented Sep 17, 2024 •

edited

Loading

MortalHappiness commented Sep 17, 2024

		curl -o "${KUBECONFORM_INSTALL}/crd2schema.py" https://raw.githubusercontent.com/yannh/kubeconform/master/scripts/openapi2jsonschema.py
		SCRIPT_PATH="${KUBECONFORM_INSTALL}/crd2schema.py"

[Feature] Add Kubernetes manifest validation in pre-commit. #2380

[Feature] Add Kubernetes manifest validation in pre-commit. #2380

Conversation

LeoLiao123 commented Sep 14, 2024

Why are these changes needed?

Related issue number

Issue Reproduction and Manual Validation Results

Checks

LeoLiao123 commented Sep 14, 2024

MortalHappiness Sep 15, 2024

Choose a reason for hiding this comment

MortalHappiness Sep 15, 2024 • edited Loading

Choose a reason for hiding this comment

MortalHappiness Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

MortalHappiness Sep 17, 2024

Choose a reason for hiding this comment

MortalHappiness left a comment • edited Loading

Choose a reason for hiding this comment

kevin85421 commented Sep 17, 2024 • edited Loading

MortalHappiness commented Sep 17, 2024

MortalHappiness Sep 15, 2024 •

edited

Loading

MortalHappiness Sep 17, 2024 •

edited

Loading

MortalHappiness left a comment •

edited

Loading

kevin85421 commented Sep 17, 2024 •

edited

Loading