argoproj / argo-workflows

Workflow Engine for Kubernetes
https://argo-workflows.readthedocs.io/
Apache License 2.0
15.09k stars 3.2k forks source link

Workflow OpenAPI validation #859

Closed vicaire closed 4 years ago

vicaire commented 6 years ago

Is this a BUG REPORT or FEATURE REQUEST?: FEATURE REQUEST

Do you have plans to support validation of the Argo workflow spec?

It should be added here:

https://github.com/argoproj/argo/blob/06c0d324bf93a037010186fe54e40590ea39d92c/install/manifests/01_workflow-crd.yaml

Here is the documentation:

https://kubernetes.io/docs/tasks/access-kubernetes-api/extend-api-custom-resource-definitions/

Here is a sample:

apiVersion: apiextensions.k8s.io/v1beta1
kind: CustomResourceDefinition
metadata:
  name: crontabs.stable.example.com
spec:
  group: stable.example.com
  version: v1
  scope: Namespaced
  names:
    plural: crontabs
    singular: crontab
    kind: CronTab
    shortNames:
    - ct
  validation:
   # openAPIV3Schema is the schema for validating custom objects.
    openAPIV3Schema:
      properties:
        spec:
          properties:
            cronSpec:
              type: string
              pattern: '^(\d+|\*)(/\d+)?(\s+(\d+|\*)(/\d+)?){4}$'
            replicas:
              type: integer
              minimum: 1
              maximum: 10
jessesuen commented 6 years ago

Yes, I agree with this. I even went so far as to generate an open-api schema for exactly this purpose: https://github.com/argoproj/argo/blob/master/api/openapi-spec/swagger.json

The main motivation for having validating schema is to avoid problems like this: https://github.com/argoproj/argo/issues/632

That said, adding the OpenAPI validation schema to the CRD needs to be optional because once you define the schema, you may have trouble running multiple controllers at different argo versions -- which will cause problems when bundling the argo workflow controller as part of a higher level application.

Also, the internal struct representation of Item object, will be changing in the near future to support protobuf encoding. So we will want to wait for this to come in: https://github.com/argoproj/argo/issues/853

vicaire commented 6 years ago

Sounds good. It makes sense to have this optional.

stormmore commented 5 years ago

Do we have any ETA for this enhancement? I filed https://github.com/argoproj/argo-events/issues/281 for the same reasons and I suspect that it impacts all the CRDs used by Argo - Worflows, Events, CD.

I did see that Rollouts has the validation in there

danxmoran commented 4 years ago

Re: Making it optional, it looks like that won't be possible if/when the CRDs update from apiextensions.k8s.io/v1beta1 to apiextensions.k8s.io/v1. I'm not sure if it's feasible to keep using the beta API version forever (some version of k8s will drop support for it eventually? not sure)

Edit: forgot to link: https://kubernetes.io/docs/tasks/access-kubernetes-api/custom-resources/custom-resource-definitions/#specifying-a-structural-schema

logicfox commented 4 years ago

@jessesuen Now that Argo 2.6 has an OpenAPI spec (https://github.com/argoproj/argo/blob/v2.6.1/api/openapi-spec/swagger.json#L2526-L2680), can we not use that to solve this issue and migrate to apiextensions.k8s.io/v1?

crenshaw-dev commented 4 years ago

My use case is just Intellij linting. Validation wouldn't even have to be built into the CRD that gets installed. It would just have to exist in another file that can be imported to Intellij's Kubernetes plugin.

alexec commented 4 years ago

@jessesuen @mukulikak I'm gathering some notes. The lack of YAML validation is one of the key reason that people want to use a Python DSL. I think we should revisit this popular issue.