juju-solutions / layer-caas-base

Base layer for CaaS charms
Apache License 2.0
2 stars 7 forks source link

Stricter validation in pod_set_spec #17

Open knkski opened 5 years ago

knkski commented 5 years ago

Not sure if this is the right repo vs https://github.com/juju/juju, can refile there if necessary.

It would be nice if pod_set_spec could either reject or warn about invalid properties the spec that it's passed. For example, I'm able to call this without warning:

layer.caas_base.pod_spec_set({
    'containers': {...}, # Some valid YAML
    'customResourceDefinition': [{
        ... # Some valid YAML
        'names': {...} # The `spec.names` key that appears in normal K8S CRD YAML
    }],
})

Looking at https://github.com/juju/juju/blob/d1e735e6bfe3641bc0c25c348904f59e7be91d3c/caas/containers.go#L81, I think that property is not supported, but is never complained about, which is very confusing when trying to adapt some pre-existing CRD spec to Juju.

johnsca commented 5 years ago

+1. We could do it here, but it would have to be kept in sync with core. So it might be better to have the hook tool return an error message that is then surfaced by this layer.

Edit: It looks like there is validation for required fields, but nothing to warn or reject unknown fields. Are they ignored?

knkski commented 5 years ago

@johnsca: Yeah, they're just ignored

wallyworld commented 5 years ago

The yaml required to be produced by the charm is now aligned with native k8s yaml for crds. The charm specifies a map of crd spec yaml, keyed on name, eg

customResourceDefinitions:
  pytorchjobs.kubeflow.org:
    group: kubeflow.org
    version: v1beta1
    names:
      kind: PyTorchJob
      plural: pytorchjobs
      singular: pytorchjob
    scope: Namespaced
    validation:
      openAPIV3Schema:
        properties:
          spec:
            properties:
              pytorchReplicaSpecs:
                properties:
                  Master:
                    properties:
                      replicas:
                        maximum: 1
                        minimum: 1
                        type: integer
                  Worker:
                    properties:
                      replicas:
                        minimum: 1
                        type: integer