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

fix: move crds to templates #235

Merged
merged 1 commit into from
Apr 2, 2021
Merged

fix: move crds to templates #235

merged 1 commit into from
Apr 2, 2021

Conversation

m1pl
Copy link
Contributor

@m1pl m1pl commented Mar 31, 2021

Is there any reason why CRDs are not being directly installed, but through a job? The problem with the current setup is that it breaks cluster automation (e.g. Flux). If someone adds e.g. oathkeeper with maester and some Rules to the Flux repository, the deployment fails as the Rules are being applied before the Jobs are being run and the CRDs are available. If the CRDs are directly available in the Helm chart's templates, the correct order can be ensured.

@m1pl
Copy link
Contributor Author

m1pl commented Mar 31, 2021

I updated the commit and moved the crds to their own directory, as stated in the docs.

@aeneasr aeneasr requested a review from Demonsthere April 1, 2021 08:13
@aeneasr aeneasr self-assigned this Apr 1, 2021
@Demonsthere
Copy link
Collaborator

Hey @m1pl thanks for the contribution!
The job was used as in the previous version of helm, crd installation could be wacky at best. We didn't want the CRD to be a part of the chart, so that deleting the chart wouldn't delete the CRDs.

With helm3 this may in fact be no longer needed :)

@Demonsthere
Copy link
Collaborator

I cannot add this as a suggestion, since you only move the CRD files around, but could you update the apiVersion in the files as well?
apiVersion: apiextensions.k8s.io/v1beta1 -> apiVersion: apiextensions.k8s.io/v1

@m1pl
Copy link
Contributor Author

m1pl commented Apr 1, 2021

@Demonsthere done

Copy link
Collaborator

@Demonsthere Demonsthere left a comment

Choose a reason for hiding this comment

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

LGTM @aeneasr

@m1pl m1pl marked this pull request as draft April 1, 2021 15:01
@m1pl
Copy link
Contributor Author

m1pl commented Apr 1, 2021

Do not merge yet, as the CRDs are broken. Just changing the version as suggested doesn't work. I converted it to a draft.

@Demonsthere
Copy link
Collaborator

Could you elaborate? I was able to install and operate on them without issues.

@m1pl
Copy link
Contributor Author

m1pl commented Apr 1, 2021

@Demonsthere I get a bunch of errors like these:

error: error validating "hydra-maester/crds/crd-oauth2clients.yaml": error validating data: ValidationError(CustomResourceDefinition.spec): unknown field "validation" in io.k8s.apiextensions-apiserver.pkg.apis.apiextensions.v1.CustomResourceDefinitionSpec; if you choose to ignore these errors, turn validation off with --validate=false

The CustomResourceDefinition "oauth2clients.hydra.ory.sh" is invalid: spec.versions[0].schema.openAPIV3Schema: Required value: schemas are required

error: error validating "hydra-maester/crds/crd-oauth2clients.yaml": error validating data: [ValidationError(CustomResourceDefinition.spec.versions[0]): unknown field "versions" in io.k8s.apiextensions-apiserver.pkg.apis.apiextensions.v1.CustomResourceDefinitionVersion, ValidationError(CustomResourceDefinition.spec.versions[0]): missing required field "served" in io.k8s.apiextensions-apiserver.pkg.apis.apiextensions.v1.CustomResourceDefinitionVersion, ValidationError(CustomResourceDefinition.spec.versions[0]): missing required field "storage" in io.k8s.apiextensions-apiserver.pkg.apis.apiextensions.v1.CustomResourceDefinitionVersion]; if you choose to ignore these errors, turn validation off with --validate=false

The CustomResourceDefinition "oauth2clients.hydra.ory.sh" is invalid: spec.validation.openAPIV3Schema.properties[metadata]: Forbidden: must not specify anything other than name and generateName, but metadata is implicitly specified

@m1pl m1pl requested a review from Demonsthere April 1, 2021 15:17
@m1pl m1pl marked this pull request as ready for review April 1, 2021 15:18
@Demonsthere
Copy link
Collaborator

Demonsthere commented Apr 1, 2021

I see, got it too after making a 1.19 or newer k8s cluster. Some api fields in the kind: CRD were updated and this is the result.

Changing the versions to the following should be enough:

versions:
  - name: v1alpha1
    served: true
    storage: true
    schema:
      openAPIV3Schema:
        type: object
        properties:
          spec:
            x-kubernetes-preserve-unknown-fields: true
          status:
            x-kubernetes-preserve-unknown-fields: true

@m1pl
Copy link
Contributor Author

m1pl commented Apr 1, 2021

The schema has changed from v1beta1 to v1, I updated the CRDs to match it. I can install both in my cluster now. Delete the old ones first and try it out. I didn't change the schemas, just the structure to match the new version.

@Demonsthere
Copy link
Collaborator

I think we confused versions here. We want to update apiextensions.k8s.io from v1beta1 to v1, the value in versions.[0] refers to the OAuth2Client.hydra.ory.sh which should stay in v1alpha1 as we didn't promote the version in the controller.

I see that the version update may not be as easy as I assumed, maybe it will be better to cut the scope here and stick to moving crds? I will create a new issue regarding updating the version, so we can focus on that there

@m1pl
Copy link
Contributor Author

m1pl commented Apr 1, 2021

@Demonsthere done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants