Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 13 commits into from
Sep 17, 2024
Merged

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

merged 13 commits into from
Sep 17, 2024

Conversation

LeoLiao123
Copy link
Contributor

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 :(

@LeoLiao123 LeoLiao123 changed the title Add strict parser [Feature]Add Kubernetes manifest validation in pre-commit. Sep 14, 2024
@LeoLiao123 LeoLiao123 changed the title [Feature]Add Kubernetes manifest validation in pre-commit. [Feature] Add Kubernetes manifest validation in pre-commit. Sep 14, 2024
@LeoLiao123
modified by trailing-whitespace hook
3d9b4e4 
Signed-off-by: Leo Liao  <93932709+LeoLiao123@users.noreply.github.com>
@LeoLiao123
Add trap to clean up temporary directory after script execution
76ea385 
Signed-off-by: Leo Liao  <93932709+LeoLiao123@users.noreply.github.com>
@LeoLiao123
Change names to lowercase for consistency
61d4c58 
Signed-off-by: Leo Liao  <93932709+LeoLiao123@users.noreply.github.com>
@LeoLiao123
Copy link
Contributor Author

cc: @MortalHappiness, @kevin85421

MortalHappiness
MortalHappiness requested changes Sep 15, 2024
View reviewed changes
@LeoLiao123
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
MortalHappiness requested changes Sep 17, 2024
View reviewed changes
LeoLiao123 and others added 2 commits September 17, 2024 10:32
@LeoLiao123
add openapi2jsonschema.py source
6b48e59 
Signed-off-by: leoliao <leoyeepaa@gmail.com>
@LeoLiao123
Fix version of kubeconform
b453a73 
Signed-off-by: Leo Liao  <93932709+LeoLiao123@users.noreply.github.com>
MortalHappiness
MortalHappiness requested changes Sep 17, 2024
View reviewed changes
@LeoLiao123
fix error catch & version validation
aaf3eb6 
Signed-off-by: leoliao <leoyeepaa@gmail.com>
@LeoLiao123
split version validation into another hook
78a2a06 
Signed-off-by: leoliao <leoyeepaa@gmail.com>
@LeoLiao123
install kubeconform before pre-commit action
72b1b8a 
Signed-off-by: leoliao <leoyeepaa@gmail.com>
MortalHappiness
MortalHappiness reviewed Sep 17, 2024
View reviewed changes
@LeoLiao123
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>
@LeoLiao123
fix version extract
f55e8f2 
Signed-off-by: leoliao <leoyeepaa@gmail.com>
MortalHappiness
MortalHappiness approved these changes Sep 17, 2024
View reviewed changes
Copy link
Member

@MortalHappiness MortalHappiness left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Follow-up PR: Also validate RayJob and RayService

cc @kevin85421

@kevin85421
Copy link
Member

kevin85421 commented Sep 17, 2024 •
edited
Loading

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.

@kevin85421 kevin85421 merged commit 39d42fb into ray-project:master Sep 17, 2024
27 checks passed
@MortalHappiness
Copy link
Member

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

@LeoLiao123 LeoLiao123 deleted the add-strict-parser branch December 11, 2024 10:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kevin85421 kevin85421 kevin85421 approved these changes

@MortalHappiness MortalHappiness MortalHappiness approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feature] Add a strict parser for YAMLs in CI
3 participants
@LeoLiao123 @kevin85421 @MortalHappiness