flink-kubernetes-operator icon indicating copy to clipboard operation
flink-kubernetes-operator copied to clipboard

[FLINK-29261] Use FAIL_ON_UNKNOWN_PROPERTIES=false in ReconciliationUtils

Open morhidi opened this issue 2 years ago • 5 comments

What is the purpose of the change

The operator cannot be downgraded with new CRD, once the CR containing a new field is written to the last reconciled/stable spec.

Brief change log

The ObjectMapper is modified in ReconciliationUtils

private static final ObjectMapper objectMapper =
            new ObjectMapper().configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false);

Verifying this change

Added new ReconciliationUtilsTest.testSpecDeserializationWithUnknownField()

Does this pull request potentially affect one of the following parts:

  • Dependencies (does it add or upgrade a dependency): no
  • The public API, i.e., is any changes to the CustomResourceDescriptors: no
  • Core observer or reconciler logic that is regularly executed: yes

Documentation

  • Does this pull request introduce a new feature? no

morhidi avatar Sep 19 '22 16:09 morhidi

There are other components that use ObjectMapper the FlinkValidator for instance should probably also need this config otherwise the webhook might not be compatible

gyfora avatar Sep 19 '22 16:09 gyfora

There are other components that use ObjectMapper the FlinkValidator for instance should probably also need this config otherwise the webhook might not be compatible

I've been thinking about this and I guess this is the only place we want to deserialize an object with unknown fields, otherwise there's no other way for rolling back to an older operator version. I don't see a reason yet we should allow this in webhook. I'm convincible though :)

morhidi avatar Sep 19 '22 19:09 morhidi

There are other components that use ObjectMapper the FlinkValidator for instance should probably also need this config otherwise the webhook might not be compatible

I've been thinking about this and I guess this is the only place we want to deserialize an object with unknown fields, otherwise there's no other way for rolling back to an older operator version. I don't see a reason yet we should allow this in webhook. I'm convincible though :)

  1. You deploy a new CRD and operator version that has a new flinkSpec field.
  2. You set that field in a CR
  3. You roll back the operator
  4. Any change to the CR will call the webhook with the CR containing the now unknown field you set in 2.

gyfora avatar Sep 19 '22 19:09 gyfora

@morhidi @gyfora when using FAIL_ON_UNKNOWN_PROPERTIES=false, is it necessary to tell the users to make sure that the correct json properties have already verified and guaranteed by Kubernetes itself based on the installed CRD on the document?

SteNicholas avatar Sep 20 '22 01:09 SteNicholas

@morhidi @gyfora when using FAIL_ON_UNKNOWN_PROPERTIES=false, is it necessary to tell the users to make sure that the correct json properties have already verified and guaranteed by Kubernetes itself based on the installed CRD on the document?

Not sure if I understand the question correctly but Kubernetes always validates the json against the CRD and even if the user could submit extra fields they would be ignored and dropped without the user noticing. So I don't think it has any user impact this way.

gyfora avatar Sep 20 '22 06:09 gyfora