Multiple version in CRD Support #879
Conversation
| metadata: | ||
| name: mvcv1 | ||
| labels: | ||
| version: v1 |
There was a problem hiding this comment.
Can you add an example to show what happens if one of the label is missing and the Spec is incompatible?
There was a problem hiding this comment.
To cover that we would need to add a conversion web-hook, build it and etc, what would be a relatively significant effort. Without conversion hook that would fail, but that is expected; it would not be a bug; We can create a separate example to cover that, here just plan to add an integration test.
But the goal should not be to "test Kubernetes", maybe show best practices also. But that again not sure if it should go to core JOSDK. The current goal is to showcase features of sdk, and show how operators are implement / tested, not best practices - that is already covered in kubernetes.
On the other hand I recognize that this might be not trivial case. So I would propose to create a separate issues, first discuss where to put such sample - with conversion hook - and implement it after.
There was a problem hiding this comment.
I think that the SDK should not simply fails if there are no labels and no conversion webhooks, but should gracefully handle the situation.
Consider the following scenario:
- the webhook is not in place
- a user creates a CR without labels
- the operator will start crashing
I do believe that this is not the desired nor expected behaviour.
There was a problem hiding this comment.
@andreaTP I updated the sample, pls check last commit. Now every CR that has no label is processed only by v1 reconciler. Maybe this is a better example this way to shed a light how this is meant. (The integration test is not there yet)
Basically we can create a separate issues and discuss the graceful handling of error case. But this is not how it is meant, if there is missing conversion hook and the custom resources are not compatible without it, it is a bug in the system. These are typed APIs for a reason.
What I can imagin is if a CR received what we are not able to par, we log an error, but no means should reach the reconciler.
There was a problem hiding this comment.
a user creates a CR without labels
This can be easily prevented by a validation hook. (But again changed the example so its more aware of this situtation)
There was a problem hiding this comment.
To me this sounds like this should not be part of the SDK at all if it's possible for a user to crash the operator just by using a specially crafted CR, webhooks or no webhooks because that basically means that you're trying to bypass the type system and I don't think that this should be encouraged.
There was a problem hiding this comment.
This is just an integration test that test if multiple versions of CR and Controllers can be created. (btw this changed a little bit since). Nothing more.
I think our goal should be in general to support all the possible use cases that Kubernetes supports. Unfortunately K8S is quite open how to use this feature (the multiple versions of CR in CRD). So in my opinion this approach is how it makes sense, thus to allow registering controller for different versions. From that point only the users responsibility to figure out the rest, get conversion hooks in place etc.
I think we should have a separate sample eventually in combination with the other framework (now called admission-controller-framework) - that will support conversion hooks - a complete setup.
I think it's totall valid to use labels for canary testing and add labels to the CRs only if user want it to be used with an other version of controller. But again this is just a test, don't want to set a patter. We could simplify it and just remove the labels, create as simple IT as possible.
There was a problem hiding this comment.
But also open to suggestions to alternatives how to support and/or test this.
|
Kudos, SonarCloud Quality Gate passed! |
No description provided.