Deserialize WatchEvents using the specific object type #3786
Conversation
|
SonarCloud Quality Gate failed. |
|
|
||
| private WatchEvent contextAwareWatchEventDeserializer(String messageSource) { | ||
| try { | ||
| return Serialization.unmarshal(messageSource, WatchEvent.class); |
There was a problem hiding this comment.
If the current deserialization mechanism fails we fallback to try to deserialize the more specific class
| updateResourceVersion(obj.getMetadata().getResourceVersion()); | ||
| Action action = Action.valueOf(event.getType()); | ||
| eventReceived(action, obj); | ||
| if (object instanceof Status) { |
There was a problem hiding this comment.
This is just a little bit of reordering:
- deserializing the specific type is "more likely" to end up with something that
extends HasMetadata Statuswill be checked first
| setupMockServerExpectations(Animal.class, "ns1", this::getList, r -> new WatchEvent(getAnimal("red-panda", "Carnivora", r), "ADDED"), null, null); | ||
|
|
||
| // When | ||
| KubernetesDeserializer.registerCustomKind("jungle.example.com/v1","Animal", CronTab.class); |
There was a problem hiding this comment.
This is a little "extreme", instead of registering an incompatible version I'm registering a completely wrong deserializer that will take precedence; but it shows the behaviour described in #3785
|
@shawkins this is ready for review now, managed to reproduce the issue in a test case. |
My concern is that this is an invalid situation to start off with, so it seems like this handling is not generally needed. |
|
I can argue that watching If you want I can include a more idiomatic test showing this situation. |
This would not occur if the resources you were getting back conformed to their expected schema. The general question - is that an expected case? If the consensus is that we need to guard against it - then we need to think more broadly. Does that only apply to the generic inform case? Because from what I can see once you create the v2 resource, any usage of the v1 typed api (or subsequent) generic will be broken. For example in your original test case, once the value exists try doing a list operation using the typed v1 api - it will fail. |
This is not completely correct, when you change the This is the use-case that I was trying to support: and this is the example project that I'm targeting with those modifications: My expectation is not having two working controllers, the old controller can e.g. simply log the fact that a CR has been modified (and the situation will be eventually handled by WebHooks). |
Only if you choose not to have a conversion strategy.
You're referring to the conversion webhooks right? https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definition-versioning/#overview - if they were in place wouldn't this change be moot? Wouldn't it be a best practice to have that in place anytime there is a breaking schema modfication? |
It's best practice, but, you cannot guarantee the webhooks to be in place. This comes extremely useful for handling controlled upgrades procedures, such as Canary releases etc. |
There was a problem hiding this comment.
I do believe that this library should be agnostic and support this valid use-case/situation gracefully.
Sorry I wasn't seeing that all other generic usage is fine because it's rooted by parsing from a class that doesn't reference the KubernetesDeserializer. The only places that will have problems are from classes, like the WatchEvent, which do reference the KubernetesDeserializer and no way to control the type of contained resources. WatchEvent is probably the only affected place. I don't think within fabric8 we'll try to independently parse things like ControllerRevision or other resources that contain references to HasMetadata or KubernetesResource under the generic api.
So the understanding here is that any continued usage of the typed api will break (which is not a fabric8 specific problem), but you'd like the generic watches/inform to continue to work even if the values are invalid for the reported version.
I think your solution is fine for that. There's probably a way to override the derserializer with a mixin or something, but nothing that jumps out quickly with a couple of attempts.
* Deserialize WatchEvents using the specific object type * fallback * tested * sonar (cherry picked from commit 0bc814a)
Description
This is a proposal to fix the specific issue reproduced in #3785
Instead of relying on the
KubernetesDeserializerfallback mechanism here we are trying to deserialize theWatchEvents using the specific deserializer for the type.Type of change
test, version modification, documentation, etc.)
Checklist
cc. @shawkins