Add a type to the v1 model for the updated ClusterConfig by jan-g · Pull Request #489 · redpanda-data/redpanda-operator

jan-g · 2025-03-06T12:10:16Z

This is a placeholder commit to get feedback before we go too far down that path - we'll add the CRD
definition once we get agreement on it.

jan-g · 2025-03-06T12:13:16Z

This is mostly to invite comment about the proposed attribute for cluster config (we'll do something similar for node configuration rather than splitting the AdditionalConfiguration up according to prefix).

I think this needs to be merged with the current configuration - but because we want late binding of external secrets, we'll have to teach the configuration subcommand to pre-process a bootstrap.yaml.in to locate and expand these references to generate a .bootstrap.yaml, as well as managing the expansion in the operator itself for dynamic configuration update.

alenkacz · 2025-03-06T12:59:37Z

operator/api/vectorized/v1alpha1/cluster_types.go

+	// If the value is supplied by an external source, coordinates are embedded here.
+	// For non-string target types, the string value fetched from the source will be treated as
+	// a value encoded according to YAML rules.
+	ExternalSecretRef *struct {


the original agreement was that this feature will pretty much be hidden, also I think we'll be using just secret name as a generic/platform agnostic pointer to what you want to read.

We can also emit this from the first implementation.

For reference, what I am going to get from controlplane is ${secrets.PASSWORD}

operator/api/vectorized/v1alpha1/cluster_types.go

operator/cmd/configurator/configurator.go

operator/pkg/resources/configmap.go

operator/pkg/resources/configuration/expander.go

operator/pkg/resources/statefulset.go

alenkacz

Chris made very good comments 👌 I like the direction

alenkacz · 2025-03-10T16:26:35Z

operator/pkg/resources/configuration/expander.go

+// of concrete values, references to external secrets (which are expanded at this point), or references
+// to values in environment variables. The last should be injected into the environment of the container
+// by appropriate EnvVar entries.
+func ExpandValueForTemplate(v vectorizedv1alpha1.ClusterConfigValue) (vectorizedv1alpha1.YamlRepresentation, error) {


really cool and clean 👍 nice job

jan-g · 2025-03-19T10:28:44Z

One last bit to take care of: the older GlobalConfiguration.ClusterConfiguration really isn't enough to determine whether the live admin api should be poked.

We'll use the template file for the moment, then replace all the drift controller etc. with a periodic reassertion of state.

ClusterConfiguration values can either be concrete ones, or references - either to k8s resources (such as ConfigMap or Secret) or an external secret. Concrete items are represented as strings - the attribute in a CR should have the yaml representation of the raw value. This permits us to inject it verbatim into a reconstituted `.bootstrap.yaml` file, and (with the aid of a schema from a running Redpanda cluster) turn the representation into concrete values without loss of fidelity. We use json encoding to construct these, and yaml decoding (which is a little more flexible in what it'll accept) to decode them.

jan-g · 2025-03-19T16:22:15Z

The GlobalConfiguration api is still a mess - it's used as an open structure by a lot of code, and we need (for the moment) to be backwards-compatible with the way config was initially processed.

To follow on from this (but not in this PR):

ditch drift configuration and just reassert state on a regular basis (this is a no-op if there's no change)
split the configmap and configuration building out into separate pieces; split node configuration and cluster configuration into distinct notions and stop exposing internal fields
drop the mixed-mode and classic mode support in favour of bootstrap configuration

chrisseto

Nothing blocking, nits are nice to haves and/or can be handled later

operator/api/vectorized/v1alpha1/cluster_types.go

chrisseto · 2025-03-20T14:04:54Z

operator/api/vectorized/v1alpha1/cluster_types.go

+// throughout. The octet sequence of a Representation will be inserted into a bootstrap template
+// verbatim.
+// TODO: this type should be lifted to a shared module rather than duplicated in the `redpanda` CRD definition.
+type ClusterConfigCRDValue struct {


nit: Feels funky to include CRD in the name of a value that's included as part of a CRD. Hopefully the package name and/or some kubebuilder annotations are enough to showcase that this is part of a CRD.

Suggested change

type ClusterConfigCRDValue struct {

type ClusterConfigValue struct {

OOC what is the expected behavior if all provided members are nil? If that's an invalid case could you add a TODO to add in CEL validation thereof?

operator/api/vectorized/v1alpha1/cluster_types.go

operator/cmd/configurator/bootstrap.go

This is ugly at the moment - the intention here is to add the bootstrap templating without too much change. We'll follow up with commits to remove swathes of the v1 operator behaviour - such as removing drift detection - after this has landed. There's additional behaviour in the `configure` subcommand, to template-expand environmant variables and/or external secrets. Internally, the operator uses the bootstrap template file to detemine whether reconfiguration should be applied. This is a short-term adjustment - it'll behave correctly on concrete values, but won't necessarily pick up external sources of values (such as config maps) changing. That's to come. The operator takes some care to not unnecessarily trigger a sts restart - if the sts has already been configured without a tempalted bootstrap, then the old behaviour will be preserved until such time as a restart would be required anyway.

Ensure the configuration is correctly stashed and updated. kuttl test for "ClusterConfiguration" external ref override Note: because this test causes an additional env var to be added to an initContainer, it'll cause a restart of the resulting statefulSet.

* Add a type to the v1 model for the updated ClusterConfig ClusterConfiguration values can either be concrete ones, or references - either to k8s resources (such as ConfigMap or Secret) or an external secret. Concrete items are represented as strings - the attribute in a CR should have the yaml representation of the raw value. This permits us to inject it verbatim into a reconstituted `.bootstrap.yaml` file, and (with the aid of a schema from a running Redpanda cluster) turn the representation into concrete values without loss of fidelity. We use json encoding to construct these, and yaml decoding (which is a little more flexible in what it'll accept) to decode them. * Add the construction of a bootstrap template file This is ugly at the moment - the intention here is to add the bootstrap templating without too much change. We'll follow up with commits to remove swathes of the v1 operator behaviour - such as removing drift detection - after this has landed. There's additional behaviour in the `configure` subcommand, to template-expand environmant variables and/or external secrets. Internally, the operator uses the bootstrap template file to detemine whether reconfiguration should be applied. This is a short-term adjustment - it'll behave correctly on concrete values, but won't necessarily pick up external sources of values (such as config maps) changing. That's to come. The operator takes some care to not unnecessarily trigger a sts restart - if the sts has already been configured without a tempalted bootstrap, then the old behaviour will be preserved until such time as a restart would be required anyway. * kuttl test for "ClusterConfiguration" override Ensure the configuration is correctly stashed and updated. kuttl test for "ClusterConfiguration" external ref override Note: because this test causes an additional env var to be added to an initContainer, it'll cause a restart of the resulting statefulSet. (cherry picked from commit a6c0672)

jan-g · 2025-04-02T15:42:49Z

💚 All backports created successfully

Status	Branch	Result
✅	release/v2.4.x
✅	release/v2.3.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

* Add a type to the v1 model for the updated ClusterConfig ClusterConfiguration values can either be concrete ones, or references - either to k8s resources (such as ConfigMap or Secret) or an external secret. Concrete items are represented as strings - the attribute in a CR should have the yaml representation of the raw value. This permits us to inject it verbatim into a reconstituted `.bootstrap.yaml` file, and (with the aid of a schema from a running Redpanda cluster) turn the representation into concrete values without loss of fidelity. We use json encoding to construct these, and yaml decoding (which is a little more flexible in what it'll accept) to decode them. * Add the construction of a bootstrap template file This is ugly at the moment - the intention here is to add the bootstrap templating without too much change. We'll follow up with commits to remove swathes of the v1 operator behaviour - such as removing drift detection - after this has landed. There's additional behaviour in the `configure` subcommand, to template-expand environmant variables and/or external secrets. Internally, the operator uses the bootstrap template file to detemine whether reconfiguration should be applied. This is a short-term adjustment - it'll behave correctly on concrete values, but won't necessarily pick up external sources of values (such as config maps) changing. That's to come. The operator takes some care to not unnecessarily trigger a sts restart - if the sts has already been configured without a tempalted bootstrap, then the old behaviour will be preserved until such time as a restart would be required anyway. * kuttl test for "ClusterConfiguration" override Ensure the configuration is correctly stashed and updated. kuttl test for "ClusterConfiguration" external ref override Note: because this test causes an additional env var to be added to an initContainer, it'll cause a restart of the resulting statefulSet. (cherry picked from commit a6c0672)

jan-g requested review from RafalKorepta, andrewstucki, chrisseto and gene-redpanda as code owners March 6, 2025 12:10

jan-g marked this pull request as draft March 6, 2025 12:10

jan-g requested a review from alenkacz March 6, 2025 12:10

alenkacz reviewed Mar 6, 2025

View reviewed changes

jan-g force-pushed the v1-cluster-config branch 9 times, most recently from 299bb76 to b4dfd63 Compare March 10, 2025 16:15

chrisseto reviewed Mar 10, 2025

View reviewed changes

alenkacz reviewed Mar 11, 2025

View reviewed changes

jan-g force-pushed the v1-cluster-config branch 10 times, most recently from 00174e9 to 6b056e1 Compare March 14, 2025 16:17

jan-g marked this pull request as ready for review March 19, 2025 14:55

jan-g force-pushed the v1-cluster-config branch from 310a73d to 707589f Compare March 19, 2025 15:52

chrisseto approved these changes Mar 20, 2025

View reviewed changes

jan-g force-pushed the v1-cluster-config branch from 707589f to 16af764 Compare March 21, 2025 09:17

jan-g added 2 commits March 21, 2025 09:33

jan-g force-pushed the v1-cluster-config branch from 16af764 to c38ac41 Compare March 21, 2025 09:33

jan-g merged commit a6c0672 into main Mar 21, 2025
12 checks passed

jan-g deleted the v1-cluster-config branch March 21, 2025 10:44

jan-g mentioned this pull request Apr 2, 2025

[release/v2.4.x] Add --mode to syncer; default to additive | Shift v1 to calling Syncer in declarative mode (#473) | Add a type to the v1 model for the updated ClusterConfig (#489) | pkg: add expander for cloud secrets (#519) #616

Merged

jan-g mentioned this pull request Apr 2, 2025

[release/v2.3.x] Add --mode to syncer; default to additive | Shift v1 to calling Syncer in declarative mode (#473) | Add a type to the v1 model for the updated ClusterConfig (#489) | pkg: add expander for cloud secrets (#519) #617

Merged

	type ClusterConfigCRDValue struct {
	type ClusterConfigValue struct {

Comments

Conversation

jan-g commented Mar 6, 2025

Uh oh!

jan-g commented Mar 6, 2025

Uh oh!

alenkacz Mar 6, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alenkacz left a comment

Choose a reason for hiding this comment

Uh oh!

alenkacz Mar 10, 2025

Choose a reason for hiding this comment

Uh oh!

jan-g commented Mar 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jan-g commented Mar 19, 2025

Uh oh!

chrisseto left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chrisseto Mar 20, 2025

Choose a reason for hiding this comment

Uh oh!

chrisseto Mar 20, 2025

Choose a reason for hiding this comment

Uh oh!

jan-g Mar 21, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jan-g commented Apr 2, 2025

💚 All backports created successfully

Questions ?

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jan-g commented Mar 19, 2025 •

edited

Loading