Skip to content
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.

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

Mark the Version field required and adapt CRD generation for trivialVersions #2480

Merged
merged 3 commits into from
Jan 30, 2020

Conversation

sebgl
Copy link
Contributor

@sebgl sebgl commented Jan 29, 2020

This PR should have been split into 2:

  1. Mark the Version field as mandatory
  2. Fix CRD generation so multiple versions can be generated even though their schema differ

However they both rely on each other:

  • CRD generation fails if we make the version field mandatory
  • fixing CRD generation requires some JsonPath that do not exist if there is no change in the version schema
    CI would fail building manifests in both individual cases. Hence a single PR with 2 commits.

1. Mark the Version field as required in all CRD

Currently if the Version field is not set, we reject the resource in the
validation webhook.
If the webhook is disabled, the operator returns an error early in the
reconciliation.

This could be considered a breaking API change, but the Version field
was already required implicitly, just not caught by the OpenAPI
validation.

Fixes #2395.

2. Customize CRDs to work around trivialVersions gen bug

A bug in controller-tools prevent validation to appear in the generated
CRDs if trivialVersions is set and multiple versions do not share the
exact same schema.
See kubernetes-sigs/controller-tools#349.

This commit removes the usage of crd:trivialVersions=true, to
"manually" (through Kustomize) apply the CRD modifications so we end up
with the trivialVersion scheme. That is: a single validation field
matching the OpenAPISchema of the currently stored version (v1).

Fixes #2479.

sebgl added 2 commits January 29, 2020 15:40
Currently if the Version field is not set, we reject the resource in the
validation webhook.
If the webhook is disabled, the operator returns an error early in the
reconciliation.

This could be considered a breaking API change, but the Version field
was already required implicitly, just not caught by the OpenAPI
validation.
A bug in controller-tools prevent validation to appear in the generated
CRDs if trivialVersions is set and multiple versions do not share the
exact same schema.
See kubernetes-sigs/controller-tools#349.

This commit removes the usage of `crd:trivialVersions=true`, to
"manually" (through Kustomize) apply the CRD modifications so we end up
with the trivialVersion scheme. That is: a single `validation` field
matching the OpenAPISchema of the currently stored version (v1).
@sebgl sebgl added >bug Something isn't working v1.1.0 labels Jan 29, 2020
config/crds/all-crds.yaml Outdated Show resolved Hide resolved
@anyasabo
Copy link
Contributor

One thing I noticed while testing is that we will want to update the controller-gen check in the makefile:
https://github.com/elastic/cloud-on-k8s/blob/master/Makefile#L36
since it downloads 0.2.1 if it does not exist but we use 0.2.4 in go.mod. controller-gen doesn't have a command to expose the version, so I wonder if we just always want to go get the specific version. It takes ~5s on my machine though so maybe not.

I think any changes should probably be a separate PR though, because 0.2.4 also adds some of the markers required for structural schema (x-int-or-string).

Copy link
Contributor

@barkbay barkbay left a comment

Choose a reason for hiding this comment

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

I did a quick test on Openshift 3.11, looks good 👍
Thanks !

@sebgl sebgl merged commit 716ee2e into elastic:master Jan 30, 2020
@anyasabo anyasabo changed the title Mark the Version field required & adapt CRD generation for trivialVersions Mark the Version field required and adapt CRD generation for trivialVersions Apr 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug Something isn't working v1.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Multi-version CRDs with trivialVersions don't have validation Mark version field as required
4 participants