[WIP] feat(bundleinstance): add call to crd validator inside of controller #199
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
| err = yaml.Unmarshal(jsonData, &unmarshalledCRD) | ||
| if err == nil && unmarshalledCRD.GroupVersionKind() == apiextensionsv1.SchemeGroupVersion.WithKind("CustomResourceDefinition") { |
There was a problem hiding this comment.
This type check is a bit awkward -- I'm not sure but since we are unmarshaling to an apiextensionsv1.CustomResourceDefinition concrete type I imagine the GVK for that type will always be the v1 apiextensions GVK regardless of whether the underlying JSON represents a v1 CRD in reality. If it doesn't, then an error occurs (probably some field is not able to be unmarshaled successfully) which doesn't seem to be handled here.
I would recommend as an intermediate step unmarshaling the jsonData into an unstructured.Unstructured object, which has a GroupVersionKind() method. From there we can do the GVK equality check and be more confident that the object we are handling is actually a CRD. It also shouldn't error since we are just converting JSON into another JSON-based type.
There was a problem hiding this comment.
Alternatively we can move this check up to L146 to do a obj.GetObjectKind() when we are handling the client.Object directly and handle the CRD from there..it makes the code a bit less clear though.
Note: There was some contention around this commit. Ultimately this should not be the end state for CRD Validation. The main sentiment right now is eventually moving toward a ValidatingWebhook that runs agonstic of Rukpak. As that runs, it will check and send back validity to the underlying Helm library Signed-off-by: Tyler Slaton <tyslaton@redhat.com>
tylerslaton
force-pushed
the
crd-validation
branch
from
99b792f
to
1854721
Compare
|
Closed in favor of #213 |
Summary
Doing a few things here that we should talk through:
crdpackage into its own separate one. Additionally remove its ability to execute a creation/upgrade.crd.Validate()function on any incoming CRD's to ensure the upgrade won't cause any destructive actions.ReasonUnsafeCRDUpgradestatus that gets created on failure toValidatethecrdWorking On
This is not complete yet, there are still a few things I'm working through but wanted to get the approach into code review while working through theses: