generate openapi: preserve manually modified/added CRD validation

[estroz]

Description of the change: preserve manually modified/added CRD validation fields.

Motivation for the change: the generate openapi command fully overwrites CRD manifest validation blocks and any additional validation fields a user might have added. This change preserves any modifications made after scaffolding CRD manifests while adding new fields.

Note that modified fields will not be updated, even if a Go type is changed. Thoughts on this drawback?

[joelanford]

@estroz Other than the issue related to overriding the type validation (as alluded to in #1143), are there other validation fields that cannot be controlled and generated from kubebuilder annotations?

If not, I think kubebuilder annotations should be the one and only way to populate the CRD validations.

If I understand this merge correctly, it seems like users could inadvertently get themselves into interesting situations that are difficult to resolve:

• As you mentioned, old validations will persist after go types change.
• Kubebuilder annotations not being honored because a different value already exists in dstCRD.spec.validations (I think this would include trying to change a kubebuilder validation annotation).

[estroz]

@joelanford there are other validations for which no kubebuilder tags exist. There are some cases, like the one you encountered in #1143, where overriding a type is advantageous, although that case can be attributed to lack of validation directive support for overriding types. One solution I can think of
is to implement a custom merge that would override every manifest validation that kubebuilder generated from *_types.go files, which is more work but better reflects the original feature request.

@hasbro17 you originally had issue with overwriting manually modified validations. Any thoughts on @joelanford's concerns?

[joelanford]

One solution I can think of
is to implement a custom merge that would override every manifest validation that kubebuilder generated from *_types.go files

@estroz I'm pretty sure that would solve the concern with my second point in that we would prefer the kubebuilder annotation value over the existing value.

To solve the issue with changing Go types, would the merge function also be able to delete fields from dstCRD.spec.validations if they are not found in the validations produced from the kubebuilder annotations?

[hasbro17]

So I agree that we should prefer the validation generated by the kubebuilder tag if present for a particular field.

And in the case when *_types.go is modified to remove a field I think the controller-tools crd generator will not generate any validation for that field. We can use that as an indicator that all fields not found in the validation generated by the controller-tools crd generator should be removed from the existing validation block in *_crd.yaml.

So for e.g if status.nodes had custom validation added by the user and it is changed to status.pods in *_types.go then we can see that there is no validation.openAPIV3Schema.properties.status.properties.nodes in the new validation block so we don't preserve it in the merge.

@estroz I'm not sure of the actual implementation of the custom merge yet but I think it should be possible to compare and weed out validation fields for which there is no corresponding spec/status field.

[estroz]

@joelanford @hasbro17 that's doable.

[shawn-hurley]

It sounds like this might be something we would want upstream. Should we propose this to the controller-tools library?

…

On Fri, Mar 1, 2019 at 4:30 PM Eric Stroczynski ***@***.***> wrote: @joelanford <https://github.com/joelanford> @hasbro17 <https://github.com/hasbro17> that's doable. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#1155 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABza96VTKeSzh3fr77gUVIxw-DufX2Phks5vSZvtgaJpZM4bXq5e> .

[lilic]

What did we decide with this, are we going to contribute upstream, or is this ready for a review? :)

[estroz]

@lilic opened an issue upstream: kubernetes-sigs/controller-tools#156
We can hold off on reviewing this for now.

/hold

[hasbro17]

@estroz Since we're planning to add support for all validation directives upstream, we can close this PR since we don't need to merge and preserve manual additions to the CRD manifest's validation block.