operator v1: add reproducer for drift detection with null #320

birdayz · 2024-11-20T11:14:14Z

this is a reproducer that shows "null" is treated as a diff from null.

birdayz · 2024-11-20T11:15:06Z

operator/internal/controller/vectorized/cluster_controller_configuration_drift_test.go

@@ -17,6 +17,28 @@ import (
 	"github.com/stretchr/testify/require"
 )

+func TestDiffWithNull(t *testing.T) {


test is expected to fail, currently it's a diff,keeping operator spinning forever.

shall succeed now with the fix commit

RafalKorepta · 2024-11-20T13:29:15Z

operator/pkg/resources/configuration/patch.go

@@ -145,6 +145,14 @@ func PropertiesEqual(
 	l logr.Logger, v1, v2 interface{}, metadata rpadmin.ConfigPropertyMetadata,
 ) bool {
 	log := l.WithName("PropertiesEqual")
+
+	// "null" (string) is the only way to define null as the desired value.
+	// Therefore, treat `"null"` as equal to `null` (only if `null`


Could you rephrase? The end of the sense is miss leading

apparently i did not save the file. updated the comment

andrewstucki · 2024-11-20T16:05:58Z

So, QQ about this -- it seems to me like this is potentially working around the fact that we're populating our map[string]any from a map[string]string defined here:

redpanda-operator/operator/api/vectorized/v1alpha1/cluster_types.go

Line 168 in f10f989

    
           AdditionalConfiguration map[string]string `json:"additionalConfiguration,omitempty"`

I'm wondering if it'd make more sense to just relax the type some and use some sort of schemaless field that we can unmarshal directly to a map[string]any? You can tell me "no" if it's too much work, but it'd be nice to see us not use "magic" strings to mean something other than a string.

chrisseto · 2024-11-20T16:06:08Z

So there's another case were this will fail. I'm not entirely certain of how it happens but I've seen a cloud cluster have a numeric value specified in scientific notation (1.23e+5)

Is there a reason we can't call json/yaml.Unmarshal on each value and load them into a map[string]any? If we wanted to get really tricky, we could iterate over the admin schema and and unmarshal into the appropriate variable.

birdayz · 2024-11-20T16:22:35Z

string does not work well with json unmarshal.

example: i set:

key: value

In yaml.

json unmarshal does not work, because value is not valid JSON. "value" would be valid JSON, strings must always be quoted. it's not directly compatible

ofc, we could work around it, but i fear breaking stuff

@chrisseto i have seen scientific notation as well somewhere. i do not recall for sure how it happens, but we don't want it (anymore).. i would be glad if we rejected that honestly.

chrisseto · 2024-11-20T16:59:45Z

I'm wondering if it'd make more sense to just relax the type some and use some sort of schemaless field that we can unmarshal directly to a map[string]any? You can tell me "no" if it's too much work, but it'd be nice to see us not use "magic" strings to mean something other than a string.

The problem with schemaless fields is that it makes the CRD "non-structural" which causes terraform's kubernetes_manifest resource to fallback to delete and recreate on any updates. The V2 CRD suffers from this problem but we've apparently side stepped it in V1.

Long term, @RafalKorepta and I have discussed having separate CRDs for different use cases. We'd keep a structural schema to play nice with terraform and then an unstructured/schemaless one for GitOps setups. One would of the two would be a compatibility layer that just creates the other.

json unmarshal does not work, because value is not valid JSON. "value" would be valid JSON, strings must always be quoted. it's not directly compatible

Seems like yaml.Unmarshal would work in that case? Bare strings would be handled, scientific notation would be handled, nulls would be handled.

ofc, we could work around it, but i fear breaking stuff

If we don't have tests in this repo that catch these issues we'll always be afraid of breaking stuff. Only one way to find out ;)

Refactoring all of this will be a good candidate to tackle with the merger work. We can punt until then.

chrisseto · 2024-11-20T17:01:44Z

operator/internal/controller/vectorized/cluster_controller_configuration_drift_test.go

@@ -17,6 +17,28 @@ import (
 	"github.com/stretchr/testify/require"
 )

+func TestDiffWithNull(t *testing.T) {


Would you mind copying the setup of a similar test that we have in operator/cmd/syncclusterconfig? We stand up a redpanda test container to make sure that the config is actually accepted by the redpanda instance. Way faster than a full k8s setup with much more validation (and no need to write out the ConfigPropertyMetadata manually)

Yep will add this test, i am interested in a few corner cases

chrisseto · 2024-11-20T17:03:11Z

operator/pkg/resources/configuration/patch.go

+
+	// "null" (string) is the only way to define null as the desired value.
+	// Therefore, treat `"null"` as equal to `null` (only if `"null"` is on the k8s side)
+	if metadata.Nullable {


So this fixes the diffing but are we going to have bugs with actually setting null values? If not does that mean we have duplicate code attempting to massage string values into the correct format? 😬

we cannot set null because our type in the CRD is string, so it can never be an actual nil. well, at least have null on the CRD side of things. but i suspect there's some magic in redpanda to turn "null" into null - this stuff actually worked, "null" turned into null in redpanda.

definitely needs more test for sure, i will add them hopefully tomorrow

this stops operator from detecting non-existing drifts over and over again, if "null" is used to set `null`. Since spec.additionalConfiguration is map[string]string, string "null" is the only way to set `null.`

RafalKorepta · 2024-11-21T20:26:22Z

Fixed in #324

operator v1: add reproducer for drift detection with null

c102390

birdayz commented Nov 20, 2024

View reviewed changes

birdayz marked this pull request as ready for review November 20, 2024 13:21

birdayz requested review from RafalKorepta, chrisseto and andrewstucki as code owners November 20, 2024 13:21

RafalKorepta reviewed Nov 20, 2024

View reviewed changes

birdayz force-pushed the jb/cluster-config-drift-null branch from 09ecb1c to c2188e6 Compare November 20, 2024 13:39

chrisseto approved these changes Nov 20, 2024

View reviewed changes

birdayz mentioned this pull request Nov 21, 2024

operator v1: alternative solution for drift detection with "null" #324

Merged

operator v1: treat "null" as null for drift detection

b8a39e9

this stops operator from detecting non-existing drifts over and over again, if "null" is used to set `null`. Since spec.additionalConfiguration is map[string]string, string "null" is the only way to set `null.`

birdayz force-pushed the jb/cluster-config-drift-null branch from c2188e6 to b8a39e9 Compare November 21, 2024 17:38

RafalKorepta closed this Nov 21, 2024

RafalKorepta deleted the jb/cluster-config-drift-null branch December 2, 2024 13:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

operator v1: add reproducer for drift detection with null #320

operator v1: add reproducer for drift detection with null #320

birdayz commented Nov 20, 2024

birdayz Nov 20, 2024

birdayz Nov 20, 2024

RafalKorepta Nov 20, 2024

birdayz Nov 20, 2024

andrewstucki commented Nov 20, 2024

chrisseto commented Nov 20, 2024

birdayz commented Nov 20, 2024 •

edited

Loading

chrisseto commented Nov 20, 2024

chrisseto Nov 20, 2024

birdayz Nov 20, 2024 •

edited

Loading

chrisseto Nov 20, 2024

birdayz Nov 20, 2024

RafalKorepta commented Nov 21, 2024

operator v1: add reproducer for drift detection with null #320

operator v1: add reproducer for drift detection with null #320

Conversation

birdayz commented Nov 20, 2024

birdayz Nov 20, 2024

Choose a reason for hiding this comment

birdayz Nov 20, 2024

Choose a reason for hiding this comment

RafalKorepta Nov 20, 2024

Choose a reason for hiding this comment

birdayz Nov 20, 2024

Choose a reason for hiding this comment

andrewstucki commented Nov 20, 2024

chrisseto commented Nov 20, 2024

birdayz commented Nov 20, 2024 • edited Loading

chrisseto commented Nov 20, 2024

chrisseto Nov 20, 2024

Choose a reason for hiding this comment

birdayz Nov 20, 2024 • edited Loading

Choose a reason for hiding this comment

chrisseto Nov 20, 2024

Choose a reason for hiding this comment

birdayz Nov 20, 2024

Choose a reason for hiding this comment

RafalKorepta commented Nov 21, 2024

birdayz commented Nov 20, 2024 •

edited

Loading

birdayz Nov 20, 2024 •

edited

Loading