🐛 Don't generate schema with Any in it, its not supported

[alvaroaleman]

This reverts commit e0d7c9d.

Any is not a valid (tested with both v1.18 and v1.19):

* spec.validation.openAPIV3Schema.properties[spec].properties[pipeline_run_spec].properties[pipelineSpec].properties[params].items.properties[default].type: Unsupported value: "Any": supported values: "array", "boolean", "integer", "number", "object", "string"

See also https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/#type although that list doesn't match whats in the error

cc @skaslev

Fixes #502

/assign @vincepri

[vincepri]

/approve
/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman, vincepri

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [vincepri]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[maxsmythe]

This PR breaks the ability to generate CRDs that embed JSONSchemas. Is there some way to retain that support?

At a minimum, maybe we can use x-kubernetes-preserve-unknown-fields?

[alvaroaleman]

This PR breaks the ability to generate CRDs that embed JSONSchemas. Is there some way to retain that support?

No it does not. Any is not allowed as vs value, see the PRs title and description.

[maxsmythe]

The fact remains that previously it was possible to generate objects that had JSONSchema embedded in them via controller-gen and now it is not.

From my eyes as a user, this is a regression. Before it would be possible for me to generate the CRD and clean it up later with kustomize or similar. Now that option is no longer available.

Per my suggestion, finding some way to use x-kubernetes-preserve-unknown-fields would be a way to generate actual valid CRD output.

[alvaroaleman]

From my eyes as a user this is a regression. Before it would be possible for me to generate the CRD and clean it up later with kustomize or similar. Now that option is no longer available.

Generating an invalid object that you can then manually fix is not a feature, its a bug. Please describe the problem you are having in a new issue rather than resurrecting old issues and PRs.

[maxsmythe]

From a pragmatic standpoint, I disagree. This eliminates the possibility of users who are aware that this is an edge case that needs to be handled including controller-gen in their generation pipeline. Generation pipelines are the purpose for which controller-gen was intended.

If we want to assume that we only generate output that passes K8s validation (not necessarily wise, as it couples controller-gen to specific K8s versions), we could at least flag-gate the feature as in crd:allowDangerousTypes=true

Further, there is a pre-existing issue that this PR re-breaks:

#291

I am happy to re-open

[alvaroaleman]

From a pragmatic standpoint, I disagree

This change broke anyones crds generation who had a type in them that had a JSONUnmarshaler implementation and no kubebuilder tags.

If we want to assume that we only generate output that passes K8s validation (not necessarily wise, as it couples controller-gen to specific K8s versions)

Any is invalid on all kube versions.

I am happy to re-open

Please do that or create a new issue and please also feel free to suggest a fix that doesn't break ppl the way this one did. Conversations on old issues and PRs will go unnoticed for many.

[maxsmythe]

From a pragmatic standpoint, I disagree

This change broke anyones crds generation who had a type in them that had a JSONUnmarshaler implementation and no kubebuilder tags.

Interesting. Can you be more specific? This would happen even if the struct was not tagged with kubebuilder tags and happened to be in a package touched by controller-gen?

If we want to assume that we only generate output that passes K8s validation (not necessarily wise, as it couples controller-gen to specific K8s versions)

Any is invalid on all kube versions.

That is true for this specific instance. As a general design principal, I'm not sure it passes muster. Imagine Any becomes valid in some future kube version? What about other validation mechanisms that change over time, such as the introduction of structural schemas?

I am happy to re-open

Please do that or create a new issue and please also feel free to suggest a fix that doesn't break ppl the way this one did. Conversations on old issues and PRs will go unnoticed for many.

For sure, happy to continue the discussion. Note that the reason this thread was started is because this PR came up on a closed bug for which people were still experiencing issues.

So far I have two (hand-wavy) proposals I've mentioned that satisfy your criteria:

1. If JSONSchema is embedded, just include it and set x-kubernetes-preserve-unknown-fields instead of using the "Any" type.
2. Un-revert the PR but add a logic gate similar to crd:allowDangerousTypes=true that lets people opt into generating more edge-case output that may require further cleanup

Depending on if the bug gets re-opened, I'll add these suggestions there, though this PR is already linked to the old bug and will be linked to any new bugs.

[alvaroaleman]

Interesting. Can you be more specific? This would happen even if the struct was not tagged with kubebuilder tags and happened to be a package touched by controller-gen?

#502, as mentioned in the PR body. This would only happen if the struct was not tagged with kubebuilder tags.

That is true for this specific instance. As a general design principal, I'm not sure it passes muster. Imagine Any becomes valid in some future kube version? What about other validation mechanisms that change over time, such as the introduction of structural schemas?

Optimizing for something that one thinks could happen in the future without any evidence for that to actually happening is premature optimization. Once we would have this case, we would most likely allow it and tell ppl that it might fail on kube versions older min supported.

For sure, happy to continue the discussion. Note that the reason this thread was started is because this PR came up on a closed bug for which people were still experiencing issues.

#291 is a kitchen sink bug for all kind of issues ppl had over time, mostly with types that are part of the k8s.io api. Please open a new bug, paste the relevant part of your type and mention what you want to happen. Right now this is a thin air discussion.

If JSONSchema is embedded, just include it and set x-kubernetes-preserve-unknown-fields instead of using the "Any" type.

Its unclear to me what you mean by "if jsonschema is embedded'. The reverted change made the assumption that ppl want an invalid CRD if they have types that implement a JSONUnmarshaler and do not have kubebuilder tags. That assumption does definitely not make sense. The same would go for adding x-kubernetes-preserve-unknown-fields, such behavior must only be triggered explicitly and never implicitly. Ppl might simply have types with a JSONUnmarshaler and no kubebuilder tags because they import the type from a different project.

[maxsmythe]

#502, as mentioned in the PR body

Ack, the linked bug clarifies what you were fixing.

Optimizing for something that one thinks could happen in the future

In this case I am not suggesting optimizing for a case that we think could happen in the future.

1. I am suggesting not doing something
2. There is clearly some unmet need here as you and I are having this conversation

Unless one of us is steering the actual direction behind this project, it's probably not worth continuing this discussion here, as it would be a lot of debate with zero impact.

#291 is a kitchen sink bug for all kind of issues

#291 is most definitely not a kitchen sink bug. From its title, it is for users trying to embed JSONSchemaProps in their CRDs.

I agree that this reverted fix cast too wide a net. I'm looking at what it would take to add a new tag so people can opt-in to the schema-less behavior wrapping CRDs inside of CRDs appears to require.