Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 2 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c102390
operator v1: add reproducer for drift detection with null
birdayz Nov 20, 2024
b8a39e9
operator v1: treat "null" as null for drift detection
birdayz Nov 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
102 changes: 102 additions & 0 deletions operator/internal/controller/vectorized/cluster_controller_configuration_drift_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,115 @@
package vectorized

import (
"context"
"fmt"
"testing"

"github.com/go-logr/logr"
"github.com/go-logr/logr/testr"
"github.com/redpanda-data/common-go/rpadmin"
"github.com/stretchr/testify/require"
"github.com/testcontainers/testcontainers-go"
"github.com/testcontainers/testcontainers-go/modules/redpanda"
"sigs.k8s.io/controller-runtime/pkg/log"
)

func TestDiffIntegration(t *testing.T) {
const user = "syncer"
const password = "password"
const saslMechanism = "SCRAM-SHA-256"

ctx := context.Background()
logger := testr.New(t)
ctx = log.IntoContext(ctx, logger)

// No auth is easy, only test on a cluster with auth on admin API.
container, err := redpanda.Run(
ctx,
"docker.redpanda.com/redpandadata/redpanda:v24.2.4",
// TODO: Upgrade to testcontainers 0.33.0 so we get
// WithBootstrapConfig. For whatever reason, it seems to not get along
// with CI.
// redpanda.WithBootstrapConfig("admin_api_require_auth", true),
redpanda.WithSuperusers("syncer"),
testcontainers.WithEnv(map[string]string{
"RP_BOOTSTRAP_USER": fmt.Sprintf("%s:%s:%s", user, password, saslMechanism),
}),
)
require.NoError(t, err)
defer func() {
_ = container.Stop(ctx, nil)
}()

// Configure the same environment as we'll be executed in within the helm
// chart:
// https://github.com/redpanda-data/helm-charts/commit/081c08b6b83ba196994ec3312a7c6011e4ef0a22#diff-84c6555620e4e5f79262384a9fa3e8f4876b36bb3a64748cbd8fbdcb66e8c1b9R966
t.Setenv("RPK_USER", user)
t.Setenv("RPK_PASS", password)
t.Setenv("RPK_SASL_MECHANISM", saslMechanism)

adminAPIAddr, err := container.AdminAPIAddress(ctx)
require.NoError(t, err)

adminAPIClient, err := rpadmin.NewAdminAPI([]string{adminAPIAddr}, &rpadmin.BasicAuth{
Username: user,
Password: password,
}, nil)
require.NoError(t, err)

_, err = adminAPIClient.PatchClusterConfig(ctx, map[string]any{
"kafka_rpc_server_tcp_send_buf": 102400,
}, []string{})
require.NoError(t, err)

schema, err := adminAPIClient.ClusterConfigSchema(ctx)
require.NoError(t, err)

config, err := adminAPIClient.Config(ctx, true)
require.NoError(t, err)
require.Equal(t, config["kafka_rpc_server_tcp_send_buf"].(float64), float64(102400))

_, drift := hasDrift(logr.Discard(), map[string]any{
"kafka_rpc_server_tcp_send_buf": "null",
}, config, schema)
require.True(t, drift, `expecting to see a drift between integer in redpanda and "null" in desired configuration`)

_, err = adminAPIClient.PatchClusterConfig(ctx, map[string]any{
"kafka_rpc_server_tcp_send_buf": nil,
}, []string{})
require.NoError(t, err)

config, err = adminAPIClient.Config(ctx, true)
require.NoError(t, err)

_, drift = hasDrift(logr.Discard(), map[string]any{
"kafka_rpc_server_tcp_send_buf": "null",
}, config, schema)
require.False(t, drift, `shall have no drift if we compare null against "null"`)
}

func TestDiffWithNull(t *testing.T) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shall succeed now with the fix commit

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

@birdayz birdayz Nov 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

assert := require.New(t)

schema := map[string]rpadmin.ConfigPropertyMetadata{
"kafka_rpc_server_tcp_send_buf": {
Type: "integer",
Description: "Size of the Kafka server TCP receive buffer. If `null`, the property is disabled.",
Nullable: true,
NeedsRestart: true,
IsSecret: false,
Visibility: "user",
},
}

_, ok := hasDrift(logr.Discard(), map[string]any{
"kafka_rpc_server_tcp_send_buf": "null",
}, map[string]any{
"kafka_rpc_server_tcp_send_buf": nil,
}, schema)
assert.False(ok)
}

func TestHasDrift(t *testing.T) {
assert := require.New(t)

Expand Down
8 changes: 8 additions & 0 deletions operator/pkg/resources/configuration/patch.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"` is on the k8s side)
if metadata.Nullable {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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? 😬

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if v1 == "null" && v2 == nil {
return true
}
}
switch metadata.Type {
case "number":
if f1, f2, ok := bothFloat64(v1, v2); ok {
Expand Down