diff --git a/api/v1alpha2/etcdcluster_types.go b/api/v1alpha2/etcdcluster_types.go index 8bdd5253..a57d32f2 100644 --- a/api/v1alpha2/etcdcluster_types.go +++ b/api/v1alpha2/etcdcluster_types.go @@ -253,11 +253,16 @@ const ( ) // EtcdOptions carries the etcd server tuning flags the operator passes to -// each member's `etcd` command line. Deliberately a closed, typed set — the -// legacy aenix operator exposed a free-form `spec.options` map[string]string, -// which let users inject arbitrary (and operator-conflicting) flags; this -// port types exactly the keys Cozystack's etcd package actually used. New -// flags land here as new typed fields, not as an escape hatch. +// each member's `etcd` command line, typed and validated. The legacy aenix +// operator exposed a free-form `spec.options` map[string]string, which let +// users inject flags that conflict with the ones the operator itself manages +// (listen URLs, initial-cluster wiring, TLS paths); this port types exactly +// the keys Cozystack's etcd package actually used. A tuning knob worth +// validating lands here as a new typed field. +// +// Flags outside this set go through spec.extraArgs, which rejects every flag +// the operator manages — including the ones typed here, so each flag has one +// source. See EtcdClusterSpec.ExtraArgs. // // Like spec.resources, options are latched through status.observed and take // effect on newly-created members (scale-up, replacement) only; the operator @@ -550,8 +555,7 @@ type EtcdClusterSpec struct { // Options carries etcd server tuning flags (backend quota, // auto-compaction, raft snapshot count) passed to each member's - // command line. A closed typed set — see EtcdOptions for why there - // is deliberately no free-form flag map. + // command line, typed and validated. See EtcdOptions. // // Updates take effect on newly-created members (scale-up, // replacement); the operator does not roll existing Pods to apply a @@ -559,6 +563,38 @@ type EtcdClusterSpec struct { // +optional Options *EtcdOptions `json:"options,omitempty"` + // ExtraArgs passes additional flags to each member's etcd command line. + // Keys are flag names without the leading dashes; each entry renders as + // --key=value, with the value passed as is. Entries are appended after + // the operator's own flags, sorted by key so the rendered Pod is + // deterministic. + // + // The operator owns a set of flags and rejects them here: member + // identity and data placement (--name, --data-dir), the listen, + // advertise and initial-cluster wiring, the TLS material it mounts from + // spec.tls, alternative bootstrap mechanisms (--discovery*, + // --config-file), and the flags typed by spec.options. The apiserver + // rejects such a key on admission, naming the flag; the member + // controller refuses to render one as a second guard. That denylist is + // what made the original free-form map unsafe — it let a user break the + // operator's own wiring — so with it in place a free-form map is safe + // to offer. It is still needed next to spec.options: a typed set only + // grows with the operator's releases, while a cluster may run an image + // that wraps etcd and takes flags of its own, or need an etcd flag no + // field covers yet. Prefer a typed field where one exists; this map is + // not validated beyond the denylist, and a flag etcd does not know + // makes the member fail to start. + // + // Like spec.options, extraArgs are latched through status.observed and + // take effect on newly-created members (scale-up, replacement) only; + // the operator does not roll existing Pods to apply a change in place. + // +kubebuilder:validation:MaxProperties=64 + // +kubebuilder:validation:XValidation:rule="self.all(k, k.matches('^[a-z0-9]+(-[a-z0-9]+)*$'))",messageExpression="'spec.extraArgs keys are etcd flag names without the leading dashes (for example log-level); invalid key: ' + self.filter(k, !k.matches('^[a-z0-9]+(-[a-z0-9]+)*$'))[0]" + // +kubebuilder:validation:XValidation:rule="self.all(k, !(k in ['name', 'data-dir', 'wal-dir', 'listen-peer-urls', 'listen-client-urls', 'listen-client-http-urls', 'listen-metrics-urls', 'advertise-client-urls', 'initial-advertise-peer-urls', 'initial-cluster', 'initial-cluster-token', 'initial-cluster-state', 'force-new-cluster', 'discovery', 'discovery-srv', 'discovery-endpoints', 'config-file', 'auto-tls', 'cert-file', 'key-file', 'client-cert-file', 'client-key-file', 'client-cert-auth', 'trusted-ca-file', 'peer-auto-tls', 'peer-cert-file', 'peer-key-file', 'peer-client-cert-file', 'peer-client-key-file', 'peer-trusted-ca-file', 'peer-client-cert-auth']))",messageExpression="'spec.extraArgs: --' + self.filter(k, k in ['name', 'data-dir', 'wal-dir', 'listen-peer-urls', 'listen-client-urls', 'listen-client-http-urls', 'listen-metrics-urls', 'advertise-client-urls', 'initial-advertise-peer-urls', 'initial-cluster', 'initial-cluster-token', 'initial-cluster-state', 'force-new-cluster', 'discovery', 'discovery-srv', 'discovery-endpoints', 'config-file', 'auto-tls', 'cert-file', 'key-file', 'client-cert-file', 'client-key-file', 'client-cert-auth', 'trusted-ca-file', 'peer-auto-tls', 'peer-cert-file', 'peer-key-file', 'peer-client-cert-file', 'peer-client-key-file', 'peer-trusted-ca-file', 'peer-client-cert-auth'])[0] + ' is managed by the operator and cannot be set here'" + // +kubebuilder:validation:XValidation:rule="self.all(k, !(k in ['quota-backend-bytes', 'auto-compaction-mode', 'auto-compaction-retention', 'snapshot-count']))",messageExpression="'spec.extraArgs: --' + self.filter(k, k in ['quota-backend-bytes', 'auto-compaction-mode', 'auto-compaction-retention', 'snapshot-count'])[0] + ' is a typed option; set spec.options.' + {'quota-backend-bytes': 'quotaBackendBytes', 'auto-compaction-mode': 'autoCompactionMode', 'auto-compaction-retention': 'autoCompactionRetention', 'snapshot-count': 'snapshotCount'}[self.filter(k, k in ['quota-backend-bytes', 'auto-compaction-mode', 'auto-compaction-retention', 'snapshot-count'])[0]] + ' instead'" + // +optional + ExtraArgs map[string]string `json:"extraArgs,omitempty"` + // ImagePullSecrets is a list of Secret references in the cluster's // namespace used to pull the etcd (and restore initContainer) image from // a private registry — e.g. an air-gapped mirror behind credentials. @@ -640,6 +676,13 @@ type ObservedClusterSpec struct { // Pods. Latched with the rest of the target spec. // +optional ImagePullSecrets []corev1.LocalObjectReference `json:"imagePullSecrets,omitempty"` + + // ExtraArgs is the locked target set of additional etcd flags for + // member Pods. Latched with the rest of the target spec so a mid-flight + // edit only applies to members created once the current target is + // reached. + // +optional + ExtraArgs map[string]string `json:"extraArgs,omitempty"` } // EtcdClusterStatus defines the observed state of an etcd cluster. diff --git a/api/v1alpha2/etcdmember_types.go b/api/v1alpha2/etcdmember_types.go index 46599fd4..bc8b6ebf 100644 --- a/api/v1alpha2/etcdmember_types.go +++ b/api/v1alpha2/etcdmember_types.go @@ -88,12 +88,11 @@ type EtcdMemberSpec struct { // immutable per-member spec. Storage StorageSpec `json:"storage"` - // Resources mirrors EtcdCluster.spec.resources at the time this - // member was created. The cluster controller copies it onto each - // member at creation. The member controller passes the value + // Resources mirrors the latched EtcdCluster.spec.resources. The + // cluster controller copies it onto each member at creation and keeps + // it in sync afterwards. The member controller passes the value // straight to the etcd container's resources field at Pod-build - // time; existing members are not re-templated when the cluster - // spec changes. + // time, so a running Pod picks a change up only when it is recreated. // +optional Resources corev1.ResourceRequirements `json:"resources,omitempty"` @@ -115,13 +114,23 @@ type EtcdMemberSpec struct { // +optional TopologySpreadConstraints []corev1.TopologySpreadConstraint `json:"topologySpreadConstraints,omitempty"` - // Options mirrors EtcdCluster.spec.options at the time this member - // was created. The member controller renders the set fields as etcd - // command-line flags at Pod-build time; existing members are not - // re-templated when the cluster spec changes. + // Options mirrors the latched EtcdCluster.spec.options. The cluster + // controller copies it onto each member at creation and keeps it in + // sync afterwards. The member controller renders the set fields as + // etcd command-line flags at Pod-build time, so a running Pod picks a + // change up only when it is recreated. // +optional Options *EtcdOptions `json:"options,omitempty"` + // ExtraArgs mirrors the latched EtcdCluster.spec.extraArgs. The + // cluster controller copies it onto each member at creation and keeps + // it in sync afterwards. The member controller renders each entry as + // --key=value after its own flags, sorted by key, and refuses to + // create the Pod if a key collides with a flag it manages. A running + // Pod picks a change up only when it is recreated. + // +optional + ExtraArgs map[string]string `json:"extraArgs,omitempty"` + // ImagePullSecrets mirrors EtcdCluster.spec.imagePullSecrets at the time // this member was created. Passed straight to the Pod's // spec.imagePullSecrets at build time. diff --git a/api/v1alpha2/zz_generated.deepcopy.go b/api/v1alpha2/zz_generated.deepcopy.go index 7f59359e..4bf4f3b6 100644 --- a/api/v1alpha2/zz_generated.deepcopy.go +++ b/api/v1alpha2/zz_generated.deepcopy.go @@ -297,6 +297,13 @@ func (in *EtcdClusterSpec) DeepCopyInto(out *EtcdClusterSpec) { *out = new(EtcdOptions) (*in).DeepCopyInto(*out) } + if in.ExtraArgs != nil { + in, out := &in.ExtraArgs, &out.ExtraArgs + *out = make(map[string]string, len(*in)) + for key, val := range *in { + (*out)[key] = val + } + } if in.ImagePullSecrets != nil { in, out := &in.ImagePullSecrets, &out.ImagePullSecrets *out = make([]v1.LocalObjectReference, len(*in)) @@ -714,6 +721,13 @@ func (in *EtcdMemberSpec) DeepCopyInto(out *EtcdMemberSpec) { *out = new(EtcdOptions) (*in).DeepCopyInto(*out) } + if in.ExtraArgs != nil { + in, out := &in.ExtraArgs, &out.ExtraArgs + *out = make(map[string]string, len(*in)) + for key, val := range *in { + (*out)[key] = val + } + } if in.ImagePullSecrets != nil { in, out := &in.ImagePullSecrets, &out.ImagePullSecrets *out = make([]v1.LocalObjectReference, len(*in)) @@ -982,6 +996,13 @@ func (in *ObservedClusterSpec) DeepCopyInto(out *ObservedClusterSpec) { *out = make([]v1.LocalObjectReference, len(*in)) copy(*out, *in) } + if in.ExtraArgs != nil { + in, out := &in.ExtraArgs, &out.ExtraArgs + *out = make(map[string]string, len(*in)) + for key, val := range *in { + (*out)[key] = val + } + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ObservedClusterSpec. diff --git a/charts/etcd-operator/crd-bases/etcd-operator.cozystack.io_etcdclusters.yaml b/charts/etcd-operator/crd-bases/etcd-operator.cozystack.io_etcdclusters.yaml index ed4d78ad..727920c5 100644 --- a/charts/etcd-operator/crd-bases/etcd-operator.cozystack.io_etcdclusters.yaml +++ b/charts/etcd-operator/crd-bases/etcd-operator.cozystack.io_etcdclusters.yaml @@ -1166,6 +1166,72 @@ spec: rule: '!has(self.source.pvc) || (has(self.source.pvc.subPath) && size(self.source.pvc.subPath) > 0)' type: object + extraArgs: + additionalProperties: + type: string + description: |- + ExtraArgs passes additional flags to each member's etcd command line. + Keys are flag names without the leading dashes; each entry renders as + --key=value, with the value passed as is. Entries are appended after + the operator's own flags, sorted by key so the rendered Pod is + deterministic. + + The operator owns a set of flags and rejects them here: member + identity and data placement (--name, --data-dir), the listen, + advertise and initial-cluster wiring, the TLS material it mounts from + spec.tls, alternative bootstrap mechanisms (--discovery*, + --config-file), and the flags typed by spec.options. The apiserver + rejects such a key on admission, naming the flag; the member + controller refuses to render one as a second guard. That denylist is + what made the original free-form map unsafe — it let a user break the + operator's own wiring — so with it in place a free-form map is safe + to offer. It is still needed next to spec.options: a typed set only + grows with the operator's releases, while a cluster may run an image + that wraps etcd and takes flags of its own, or need an etcd flag no + field covers yet. Prefer a typed field where one exists; this map is + not validated beyond the denylist, and a flag etcd does not know + makes the member fail to start. + + Like spec.options, extraArgs are latched through status.observed and + take effect on newly-created members (scale-up, replacement) only; + the operator does not roll existing Pods to apply a change in place. + maxProperties: 64 + type: object + x-kubernetes-validations: + - messageExpression: '''spec.extraArgs keys are etcd flag names without + the leading dashes (for example log-level); invalid key: '' + + self.filter(k, !k.matches(''^[a-z0-9]+(-[a-z0-9]+)*$''))[0]' + rule: self.all(k, k.matches('^[a-z0-9]+(-[a-z0-9]+)*$')) + - messageExpression: '''spec.extraArgs: --'' + self.filter(k, k in + [''name'', ''data-dir'', ''wal-dir'', ''listen-peer-urls'', ''listen-client-urls'', + ''listen-client-http-urls'', ''listen-metrics-urls'', ''advertise-client-urls'', + ''initial-advertise-peer-urls'', ''initial-cluster'', ''initial-cluster-token'', + ''initial-cluster-state'', ''force-new-cluster'', ''discovery'', + ''discovery-srv'', ''discovery-endpoints'', ''config-file'', ''auto-tls'', + ''cert-file'', ''key-file'', ''client-cert-file'', ''client-key-file'', + ''client-cert-auth'', ''trusted-ca-file'', ''peer-auto-tls'', + ''peer-cert-file'', ''peer-key-file'', ''peer-client-cert-file'', + ''peer-client-key-file'', ''peer-trusted-ca-file'', ''peer-client-cert-auth''])[0] + + '' is managed by the operator and cannot be set here''' + rule: self.all(k, !(k in ['name', 'data-dir', 'wal-dir', 'listen-peer-urls', + 'listen-client-urls', 'listen-client-http-urls', 'listen-metrics-urls', + 'advertise-client-urls', 'initial-advertise-peer-urls', 'initial-cluster', + 'initial-cluster-token', 'initial-cluster-state', 'force-new-cluster', + 'discovery', 'discovery-srv', 'discovery-endpoints', 'config-file', + 'auto-tls', 'cert-file', 'key-file', 'client-cert-file', 'client-key-file', + 'client-cert-auth', 'trusted-ca-file', 'peer-auto-tls', 'peer-cert-file', + 'peer-key-file', 'peer-client-cert-file', 'peer-client-key-file', + 'peer-trusted-ca-file', 'peer-client-cert-auth'])) + - messageExpression: '''spec.extraArgs: --'' + self.filter(k, k in + [''quota-backend-bytes'', ''auto-compaction-mode'', ''auto-compaction-retention'', + ''snapshot-count''])[0] + '' is a typed option; set spec.options.'' + + {''quota-backend-bytes'': ''quotaBackendBytes'', ''auto-compaction-mode'': + ''autoCompactionMode'', ''auto-compaction-retention'': ''autoCompactionRetention'', + ''snapshot-count'': ''snapshotCount''}[self.filter(k, k in [''quota-backend-bytes'', + ''auto-compaction-mode'', ''auto-compaction-retention'', ''snapshot-count''])[0]] + + '' instead''' + rule: self.all(k, !(k in ['quota-backend-bytes', 'auto-compaction-mode', + 'auto-compaction-retention', 'snapshot-count'])) imagePullSecrets: description: |- ImagePullSecrets is a list of Secret references in the cluster's @@ -1197,8 +1263,7 @@ spec: description: |- Options carries etcd server tuning flags (backend quota, auto-compaction, raft snapshot count) passed to each member's - command line. A closed typed set — see EtcdOptions for why there - is deliberately no free-form flag map. + command line, typed and validated. See EtcdOptions. Updates take effect on newly-created members (scale-up, replacement); the operator does not roll existing Pods to apply a @@ -2883,6 +2948,15 @@ spec: x-kubernetes-list-type: atomic type: object type: object + extraArgs: + additionalProperties: + type: string + description: |- + ExtraArgs is the locked target set of additional etcd flags for + member Pods. Latched with the rest of the target spec so a mid-flight + edit only applies to members created once the current target is + reached. + type: object imagePullSecrets: description: |- ImagePullSecrets is the locked target pull-secret list for member diff --git a/charts/etcd-operator/crd-bases/etcd-operator.cozystack.io_etcdmembers.yaml b/charts/etcd-operator/crd-bases/etcd-operator.cozystack.io_etcdmembers.yaml index fbad5192..d1412872 100644 --- a/charts/etcd-operator/crd-bases/etcd-operator.cozystack.io_etcdmembers.yaml +++ b/charts/etcd-operator/crd-bases/etcd-operator.cozystack.io_etcdmembers.yaml @@ -1023,6 +1023,17 @@ spec: the same ClusterID and member ID. While dormant, the member does not count toward the EtcdCluster's `current` replica accounting. type: boolean + extraArgs: + additionalProperties: + type: string + description: |- + ExtraArgs mirrors the latched EtcdCluster.spec.extraArgs. The + cluster controller copies it onto each member at creation and keeps + it in sync afterwards. The member controller renders each entry as + --key=value after its own flags, sorted by key, and refuses to + create the Pod if a key collides with a flag it manages. A running + Pod picks a change up only when it is recreated. + type: object imagePullSecrets: description: |- ImagePullSecrets mirrors EtcdCluster.spec.imagePullSecrets at the time @@ -1052,10 +1063,11 @@ spec: type: string options: description: |- - Options mirrors EtcdCluster.spec.options at the time this member - was created. The member controller renders the set fields as etcd - command-line flags at Pod-build time; existing members are not - re-templated when the cluster spec changes. + Options mirrors the latched EtcdCluster.spec.options. The cluster + controller copies it onto each member at creation and keeps it in + sync afterwards. The member controller renders the set fields as + etcd command-line flags at Pod-build time, so a running Pod picks a + change up only when it is recreated. properties: autoCompactionMode: description: |- @@ -1110,12 +1122,11 @@ spec: type: integer resources: description: |- - Resources mirrors EtcdCluster.spec.resources at the time this - member was created. The cluster controller copies it onto each - member at creation. The member controller passes the value + Resources mirrors the latched EtcdCluster.spec.resources. The + cluster controller copies it onto each member at creation and keeps + it in sync afterwards. The member controller passes the value straight to the etcd container's resources field at Pod-build - time; existing members are not re-templated when the cluster - spec changes. + time, so a running Pod picks a change up only when it is recreated. properties: claims: description: |- diff --git a/controllers/etcdcluster_controller.go b/controllers/etcdcluster_controller.go index 49464ae3..96a19a1c 100644 --- a/controllers/etcdcluster_controller.go +++ b/controllers/etcdcluster_controller.go @@ -19,6 +19,7 @@ package controllers import ( "context" "fmt" + "maps" "math" "sort" "strings" @@ -271,6 +272,12 @@ func (r *EtcdClusterReconciler) Reconcile(ctx context.Context, req ctrl.Request) } } + // Before any scale step: waking a dormant member creates its Pod from + // the member's own template, so the template must already be current. + if err := r.syncMemberTemplates(ctx, cluster, active); err != nil { + return ctrl.Result{}, err + } + // If a half-baked scale-up CR is present (Spec.InitialCluster empty), // route through scaleUp regardless of replica accounting. Two reasons: // @@ -399,6 +406,35 @@ func (r *EtcdClusterReconciler) Reconcile(ctx context.Context, req ctrl.Request) return r.updateStatus(ctx, cluster, active, pending) } +// ── Member templates ───────────────────────────────────────────────────── + +// syncMemberTemplates copies the latched Pod-template fields (options, +// extraArgs, resources) onto existing members. Running Pods are left alone; +// a Pod recreated for any reason, including a deliberate delete to roll a +// change, picks the new template up. Storage, version, scheduling and +// metadata stay as they were at member creation: they are bound to the +// member's volume or are upgrades, not per-Pod settings. +func (r *EtcdClusterReconciler) syncMemberTemplates(ctx context.Context, cluster *lll.EtcdCluster, members []lll.EtcdMember) error { + o := cluster.Status.Observed + for i := range members { + m := &members[i] + if equality.Semantic.DeepEqual(m.Spec.Options, o.Options) && + equality.Semantic.DeepEqual(m.Spec.ExtraArgs, o.ExtraArgs) && + equality.Semantic.DeepEqual(m.Spec.Resources, o.Resources) { + continue + } + patch := client.MergeFrom(m.DeepCopy()) + m.Spec.Options = o.Options.DeepCopy() + m.Spec.ExtraArgs = maps.Clone(o.ExtraArgs) + m.Spec.Resources = *o.Resources.DeepCopy() + if err := r.Patch(ctx, m, patch); err != nil { + return fmt.Errorf("sync template of member %s: %w", m.Name, err) + } + log.FromContext(ctx).Info("synced member template with the cluster spec; applies when the Pod is recreated", "member", m.Name) + } + return nil +} + // ── Bootstrap ──────────────────────────────────────────────────────────── // bootstrap brings up the single seed member for a fresh cluster. Its @@ -450,6 +486,7 @@ func (r *EtcdClusterReconciler) bootstrap( Affinity: cluster.Status.Observed.Affinity, TopologySpreadConstraints: cluster.Status.Observed.TopologySpreadConstraints, Options: cluster.Status.Observed.Options, + ExtraArgs: cluster.Status.Observed.ExtraArgs, ImagePullSecrets: cluster.Status.Observed.ImagePullSecrets, Bootstrap: true, ClusterToken: cluster.Status.ClusterToken, @@ -855,6 +892,7 @@ func (r *EtcdClusterReconciler) scaleUp( Affinity: cluster.Status.Observed.Affinity, TopologySpreadConstraints: cluster.Status.Observed.TopologySpreadConstraints, Options: cluster.Status.Observed.Options, + ExtraArgs: cluster.Status.Observed.ExtraArgs, ImagePullSecrets: cluster.Status.Observed.ImagePullSecrets, Bootstrap: false, ClusterToken: cluster.Status.ClusterToken, @@ -2141,6 +2179,7 @@ func snapshotSpecIntoObserved(cluster *lll.EtcdCluster) { AdditionalMetadata: cluster.Spec.AdditionalMetadata, Options: cluster.Spec.Options, ImagePullSecrets: cluster.Spec.ImagePullSecrets, + ExtraArgs: cluster.Spec.ExtraArgs, } } @@ -2162,7 +2201,8 @@ func specEqualsObserved(cluster *lll.EtcdCluster) bool { equality.Semantic.DeepEqual(o.TopologySpreadConstraints, cluster.Spec.TopologySpreadConstraints) && equality.Semantic.DeepEqual(o.AdditionalMetadata, cluster.Spec.AdditionalMetadata) && equality.Semantic.DeepEqual(o.Options, cluster.Spec.Options) && - equality.Semantic.DeepEqual(o.ImagePullSecrets, cluster.Spec.ImagePullSecrets) + equality.Semantic.DeepEqual(o.ImagePullSecrets, cluster.Spec.ImagePullSecrets) && + equality.Semantic.DeepEqual(o.ExtraArgs, cluster.Spec.ExtraArgs) } // observedAdditionalMetadata returns the latched additionalMetadata target diff --git a/controllers/etcdcluster_controller_test.go b/controllers/etcdcluster_controller_test.go index 74ac7caa..43820d3a 100644 --- a/controllers/etcdcluster_controller_test.go +++ b/controllers/etcdcluster_controller_test.go @@ -3174,7 +3174,8 @@ func TestSpecEqualsObserved_OptionsMatter(t *testing.T) { } // TestBootstrap_PropagatesOptionsToSeed verifies the wiring of -// spec.options onto the seed EtcdMember at cluster creation. The member +// spec.options and spec.extraArgs onto the seed EtcdMember at cluster +// creation. The member // controller reads its own Spec.Options at buildPod time, so any // propagation gap shows up as the seed's etcd running with built-in // defaults instead of the user's tuning (e.g. no backend quota). @@ -3188,13 +3189,15 @@ func TestBootstrap_PropagatesOptionsToSeed(t *testing.T) { AutoCompactionRetention: "5m", SnapshotCount: &snapCount, } + extra := map[string]string{"log-level": "debug"} cluster := &lll.EtcdCluster{ ObjectMeta: metav1.ObjectMeta{Name: "test", Namespace: "ns"}, Spec: lll.EtcdClusterSpec{ - Replicas: ptrInt32(3), - Version: "3.5.17", - Storage: lll.StorageSpec{Size: quickQty(t, "1Gi")}, - Options: opts, + Replicas: ptrInt32(3), + Version: "3.5.17", + Storage: lll.StorageSpec{Size: quickQty(t, "1Gi")}, + Options: opts, + ExtraArgs: extra, }, } c, _ := newTestClient(t, cluster) @@ -3209,6 +3212,9 @@ func TestBootstrap_PropagatesOptionsToSeed(t *testing.T) { if !equality.Semantic.DeepEqual(got.Status.Observed.Options, opts) { t.Errorf("Observed.Options = %+v; want %+v", got.Status.Observed.Options, opts) } + if !equality.Semantic.DeepEqual(got.Status.Observed.ExtraArgs, extra) { + t.Errorf("Observed.ExtraArgs = %v; want %v", got.Status.Observed.ExtraArgs, extra) + } members := &lll.EtcdMemberList{} if err := c.List(ctx, members, client.InNamespace("ns")); err != nil { @@ -3220,6 +3226,9 @@ func TestBootstrap_PropagatesOptionsToSeed(t *testing.T) { if !equality.Semantic.DeepEqual(members.Items[0].Spec.Options, opts) { t.Fatalf("seed Spec.Options = %+v; want %+v", members.Items[0].Spec.Options, opts) } + if !equality.Semantic.DeepEqual(members.Items[0].Spec.ExtraArgs, extra) { + t.Fatalf("seed Spec.ExtraArgs = %v; want %v", members.Items[0].Spec.ExtraArgs, extra) + } } // TestBootstrap_NativeServicesAndNoReservedAnnotations pins the post-review @@ -3471,9 +3480,10 @@ func TestScaleUp_PropagatesResourcesToNewMember(t *testing.T) { } // TestScaleUp_PropagatesOptionsToNewMember covers the second member- -// creation site for spec.options: scaleUp's fresh-CR path. Bootstrap is -// covered by TestBootstrap_PropagatesOptionsToSeed; this one makes sure -// a member added later runs with the same tuning as the seed. +// creation site for spec.options and spec.extraArgs: scaleUp's fresh-CR +// path. Bootstrap is covered by TestBootstrap_PropagatesOptionsToSeed; +// this one makes sure a member added later runs with the same flags as +// the seed. func TestScaleUp_PropagatesOptionsToNewMember(t *testing.T) { ctx := context.Background() now := metav1.Now() @@ -3483,6 +3493,7 @@ func TestScaleUp_PropagatesOptionsToNewMember(t *testing.T) { AutoCompactionMode: lll.AutoCompactionModePeriodic, AutoCompactionRetention: "5m", } + extra := map[string]string{"log-level": "debug"} // Cluster targets 2 replicas; 1 ready voter already exists. scaleUp // must create a fresh second member carrying Observed.Options. @@ -3498,10 +3509,11 @@ func TestScaleUp_PropagatesOptionsToNewMember(t *testing.T) { ClusterToken: "ns-test-x", ClusterID: "deadbeef", Observed: &lll.ObservedClusterSpec{ - Replicas: 2, - Version: "3.5.17", - Storage: lll.StorageSpec{Size: quickQty(t, "1Gi")}, - Options: opts, + Replicas: 2, + Version: "3.5.17", + Storage: lll.StorageSpec{Size: quickQty(t, "1Gi")}, + Options: opts, + ExtraArgs: extra, }, ProgressDeadline: &metav1.Time{Time: metav1.Now().Add(time.Hour)}, }, @@ -3539,6 +3551,9 @@ func TestScaleUp_PropagatesOptionsToNewMember(t *testing.T) { if !equality.Semantic.DeepEqual(fresh.Spec.Options, opts) { t.Fatalf("scaled-up member Spec.Options = %+v; want %+v", fresh.Spec.Options, opts) } + if !equality.Semantic.DeepEqual(fresh.Spec.ExtraArgs, extra) { + t.Fatalf("scaled-up member Spec.ExtraArgs = %v; want %v", fresh.Spec.ExtraArgs, extra) + } } // TestUpdateStatus_SetsScaleSelector covers the /scale subresource diff --git a/controllers/etcdmember_controller.go b/controllers/etcdmember_controller.go index f3e87112..7ca1d698 100644 --- a/controllers/etcdmember_controller.go +++ b/controllers/etcdmember_controller.go @@ -20,6 +20,7 @@ import ( "context" "crypto/tls" "fmt" + "slices" "strconv" "time" @@ -542,6 +543,10 @@ func (r *EtcdMemberReconciler) ensurePod(ctx context.Context, member *lll.EtcdMe clusterFormed = cluster.Status.ClusterID != "" } + if err := checkExtraArgs(member.Spec.ExtraArgs); err != nil { + return fmt.Errorf("member %q: refusing to create the Pod: %w", member.Name, err) + } + pod = r.buildPod(member, clusterFormed) if err := controllerutil.SetControllerReference(member, pod, r.Scheme); err != nil { return err @@ -681,6 +686,91 @@ func optionFlags(o *lll.EtcdOptions) []string { return flags } +// deniedExtraArgs is every etcd flag spec.extraArgs may not set, with the +// reason it is reserved. It covers each flag buildPod and optionFlags render, +// plus the flags that would quietly override them (--config-file makes etcd +// ignore the command line; --discovery* replaces --initial-cluster; the client +// cert flags replace the identity mounted from spec.tls). A flag added to +// buildPod or optionFlags goes here too, and into the matching CEL rule on +// EtcdClusterSpec.ExtraArgs, which rejects the key at admission: +// TestDeniedExtraArgsCoverRenderedFlags fails until it is listed here, and +// TestExtraArgsAdmission_Envtest until the CEL rule rejects it. +var deniedExtraArgs = map[string]string{ + "name": "member identity is managed by the operator", + "data-dir": "data placement is managed by the operator", + "wal-dir": "data placement is managed by the operator", + "listen-peer-urls": "listeners are managed by the operator", + "listen-client-urls": "listeners are managed by the operator", + "listen-client-http-urls": "listeners are managed by the operator", + "listen-metrics-urls": "listeners are managed by the operator", + "advertise-client-urls": "member URLs are managed by the operator", + "initial-advertise-peer-urls": "member URLs are managed by the operator", + "initial-cluster": "cluster bootstrap is managed by the operator", + "initial-cluster-token": "cluster bootstrap is managed by the operator", + "initial-cluster-state": "cluster bootstrap is managed by the operator", + "force-new-cluster": "membership is managed by the operator", + "discovery": "cluster bootstrap is managed by the operator", + "discovery-srv": "cluster bootstrap is managed by the operator", + "discovery-endpoints": "cluster bootstrap is managed by the operator", + "config-file": "a config file makes etcd ignore the operator's flags", + "auto-tls": "client TLS is configured through spec.tls.client", + "cert-file": "client TLS is configured through spec.tls.client", + "key-file": "client TLS is configured through spec.tls.client", + "client-cert-file": "client TLS is configured through spec.tls.client", + "client-key-file": "client TLS is configured through spec.tls.client", + "client-cert-auth": "client TLS is configured through spec.tls.client", + "trusted-ca-file": "client TLS is configured through spec.tls.client", + "peer-auto-tls": "peer TLS is configured through spec.tls.peer", + "peer-cert-file": "peer TLS is configured through spec.tls.peer", + "peer-key-file": "peer TLS is configured through spec.tls.peer", + "peer-client-cert-file": "peer TLS is configured through spec.tls.peer", + "peer-client-key-file": "peer TLS is configured through spec.tls.peer", + "peer-trusted-ca-file": "peer TLS is configured through spec.tls.peer", + "peer-client-cert-auth": "peer TLS is configured through spec.tls.peer", + "quota-backend-bytes": "set it through spec.options.quotaBackendBytes", + "auto-compaction-mode": "set it through spec.options.autoCompactionMode", + "auto-compaction-retention": "set it through spec.options.autoCompactionRetention", + "snapshot-count": "set it through spec.options.snapshotCount", +} + +// checkExtraArgs reports the first extraArgs key (in sorted order) that is +// on the denylist. Admission already rejects such a key on the EtcdCluster, +// so this only fires for an EtcdMember written around the cluster +// controller; ensurePod refuses to create the Pod rather than letting a +// user flag override the operator's own. +func checkExtraArgs(args map[string]string) error { + for _, k := range sortedKeys(args) { + if reason, denied := deniedExtraArgs[k]; denied { + return fmt.Errorf("extraArgs: --%s cannot be set: %s", k, reason) + } + } + return nil +} + +// extraArgFlags renders the member's extraArgs (mirrored from +// EtcdCluster.spec.extraArgs) as --key=value flags, sorted by key so the +// rendered Pod does not depend on map iteration order. Denied keys never +// render, even if checkExtraArgs was bypassed. +func extraArgFlags(args map[string]string) []string { + var flags []string + for _, k := range sortedKeys(args) { + if _, denied := deniedExtraArgs[k]; denied { + continue + } + flags = append(flags, "--"+k+"="+args[k]) + } + return flags +} + +func sortedKeys(m map[string]string) []string { + keys := make([]string, 0, len(m)) + for k := range m { + keys = append(keys, k) + } + slices.Sort(keys) + return keys +} + // buildPod renders a member's Pod. clusterFormed reports whether the parent // cluster has latched status.clusterID; see the --initial-cluster-state // derivation below for why it is a parameter rather than read from the member. @@ -834,6 +924,9 @@ func (r *EtcdMemberReconciler) buildPod(member *lll.EtcdMember, clusterFormed bo }) } + // User flags go last, after every flag the operator renders. + cmd = append(cmd, extraArgFlags(member.Spec.ExtraArgs)...) + etcdImage := resolveEtcdImage(member, r.EtcdImageRepository) // Restore initContainers: when the seed carries a restore spec, populate diff --git a/controllers/extra_args_envtest_test.go b/controllers/extra_args_envtest_test.go new file mode 100644 index 00000000..7c2bfda5 --- /dev/null +++ b/controllers/extra_args_envtest_test.go @@ -0,0 +1,105 @@ +/* +Copyright 2023 Timofey Larkin. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 +*/ + +package controllers + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" + + "k8s.io/apimachinery/pkg/api/resource" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/envtest" + + lll "github.com/cozystack/etcd-operator/api/v1alpha2" +) + +// The admission-side copy of deniedExtraArgs is a CEL rule on +// EtcdClusterSpec.ExtraArgs, written out literally in a kubebuilder marker. +// This drives every denylisted key through a real apiserver with the +// generated CRDs installed, so the two lists cannot drift. `make test` sets +// KUBEBUILDER_ASSETS; without it the test skips. +func TestExtraArgsAdmission_Envtest(t *testing.T) { + if os.Getenv("KUBEBUILDER_ASSETS") == "" { + t.Skip("KUBEBUILDER_ASSETS not set; envtest harness not initialized") + } + env := &envtest.Environment{ + Scheme: testScheme(t), + CRDDirectoryPaths: []string{filepath.Join("..", "charts", "etcd-operator", "crd-bases")}, + ErrorIfCRDPathMissing: true, + } + cfg, err := env.Start() + if err != nil { + t.Fatalf("envtest start: %v", err) + } + t.Cleanup(func() { _ = env.Stop() }) + k8s, err := client.New(cfg, client.Options{Scheme: testScheme(t)}) + if err != nil { + t.Fatalf("client: %v", err) + } + ctx := context.Background() + + cluster := func(name string, extra map[string]string) *lll.EtcdCluster { + return &lll.EtcdCluster{ + ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: "default"}, + Spec: lll.EtcdClusterSpec{ + Replicas: ptrInt32(3), + Version: "3.6.11", + Storage: lll.StorageSpec{Size: resource.MustParse("128Mi")}, + ExtraArgs: extra, + }, + } + } + reject := func(t *testing.T, extra map[string]string, wantMsg string) { + t.Helper() + c := cluster("x", extra) + err := k8s.Create(ctx, c) + if err == nil { + _ = k8s.Delete(ctx, c) + t.Fatalf("apiserver accepted extraArgs %v; expected rejection", extra) + } + if !strings.Contains(err.Error(), wantMsg) { + t.Fatalf("rejection for %v does not mention %q: %v", extra, wantMsg, err) + } + } + + t.Run("allowed flags", func(t *testing.T) { + c := cluster("extra-ok", map[string]string{ + "log-level": "debug", + "wrapper-mode": "dry-run", + "experimental-initial-corrupt-check": "true", + "empty-value": "", + }) + if err := k8s.Create(ctx, c); err != nil { + t.Fatalf("apiserver rejected allowed extraArgs: %v", err) + } + _ = k8s.Delete(ctx, c) + }) + + for flag, reason := range deniedExtraArgs { + t.Run("denied "+flag, func(t *testing.T) { + want := "--" + flag + " is managed by the operator" + if field, typed := strings.CutPrefix(reason, "set it through "); typed { + want = "--" + flag + " is a typed option; set " + field + " instead" + } + reject(t, map[string]string{"log-level": "debug", flag: "x"}, want) + }) + } + + for _, key := range []string{"--log-level", "Log-Level", "log_level", "log-level=debug", "-log", ""} { + t.Run("malformed key "+key, func(t *testing.T) { + reject(t, map[string]string{key: "x"}, "spec.extraArgs keys are etcd flag names without the leading dashes") + }) + } +} diff --git a/controllers/extra_args_test.go b/controllers/extra_args_test.go new file mode 100644 index 00000000..5c3da42a --- /dev/null +++ b/controllers/extra_args_test.go @@ -0,0 +1,202 @@ +/* +Copyright 2023 Timofey Larkin. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 +*/ + +package controllers + +import ( + "context" + "reflect" + "strings" + "testing" + + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + + lll "github.com/cozystack/etcd-operator/api/v1alpha2" +) + +// TestDeniedExtraArgsCoverRenderedFlags renders a member Pod through every +// flag-emitting branch of buildPod (plaintext and TLS, mTLS, peer TLS and +// peer auto-TLS, all typed options) and requires each flag the operator +// renders to be on the extraArgs denylist. A flag added to buildPod without +// a denylist entry fails here. +func TestDeniedExtraArgsCoverRenderedFlags(t *testing.T) { + r := &EtcdMemberReconciler{} + quota := int64(1 << 30) + snapCount := int64(10000) + base := func() lll.EtcdMemberSpec { + return lll.EtcdMemberSpec{ + ClusterName: "test", Version: "3.6.11", + Storage: lll.StorageSpec{Size: quickQty(t, "1Gi")}, + InitialCluster: "m=http://m.test.ns.svc:2380", ClusterToken: "tok", + Options: &lll.EtcdOptions{ + QuotaBackendBytes: "a, + AutoCompactionMode: lll.AutoCompactionModePeriodic, + AutoCompactionRetention: "5m", + SnapshotCount: &snapCount, + }, + } + } + plain := base() + tlsFull := base() + tlsFull.TLS = &lll.EtcdMemberTLS{ + ClientServerSecretRef: &corev1.LocalObjectReference{Name: "srv"}, + ClientMTLS: true, + PeerSecretRef: &corev1.LocalObjectReference{Name: "peer"}, + } + autoTLS := base() + autoTLS.TLS = &lll.EtcdMemberTLS{PeerAutoTLS: true} + + rendered := map[string]bool{} + for _, spec := range []lll.EtcdMemberSpec{plain, tlsFull, autoTLS} { + pod := r.buildPod(&lll.EtcdMember{ + ObjectMeta: metav1.ObjectMeta{Name: "m", Namespace: "ns"}, + Spec: spec, + }, false) + for _, arg := range pod.Spec.Containers[0].Command[1:] { + name, _, _ := strings.Cut(strings.TrimPrefix(arg, "--"), "=") + rendered[name] = true + } + } + // Sanity: the branches above really did render the TLS and options flags. + for _, want := range []string{"peer-auto-tls", "peer-client-cert-auth", "client-cert-auth", "snapshot-count"} { + if !rendered[want] { + t.Fatalf("test fixture did not exercise --%s; rendered %v", want, rendered) + } + } + for name := range rendered { + if _, ok := deniedExtraArgs[name]; !ok { + t.Errorf("buildPod renders --%s but deniedExtraArgs does not list it; add it there and to the CEL rule on EtcdClusterSpec.ExtraArgs", name) + } + } +} + +// TestBuildPod_RendersExtraArgs pins the rendering contract: each entry is +// --key=value with the value untouched, entries come after the operator's +// own flags, sorted by key, and the result does not depend on map order. +func TestBuildPod_RendersExtraArgs(t *testing.T) { + r := &EtcdMemberReconciler{} + member := &lll.EtcdMember{ + ObjectMeta: metav1.ObjectMeta{Name: "m", Namespace: "ns"}, + Spec: lll.EtcdMemberSpec{ + ClusterName: "test", Version: "3.6.11", + Storage: lll.StorageSpec{Size: quickQty(t, "1Gi")}, + TLS: &lll.EtcdMemberTLS{PeerSecretRef: &corev1.LocalObjectReference{Name: "peer"}}, + ExtraArgs: map[string]string{ + "wrapper-mode": "dry-run", + "log-level": "debug", + "experimental-foo": "", + "log-outputs": "a=b, c", + }, + }, + } + cmd := r.buildPod(member, false).Spec.Containers[0].Command + want := []string{ + "--experimental-foo=", + "--log-level=debug", + "--log-outputs=a=b, c", + "--wrapper-mode=dry-run", + } + if len(cmd) < len(want) { + t.Fatalf("command too short: %v", cmd) + } + if got := cmd[len(cmd)-len(want):]; !reflect.DeepEqual(got, want) { + t.Fatalf("extraArgs tail = %v; want %v (full command %v)", got, want, cmd) + } + for i := 0; i < 20; i++ { + if again := r.buildPod(member, false).Spec.Containers[0].Command; !reflect.DeepEqual(again, cmd) { + t.Fatalf("command not deterministic:\n%v\n%v", cmd, again) + } + } +} + +// TestExtraArgs_DenylistRejectsEveryFlag runs every denylisted key through +// the render-time guard: checkExtraArgs must name the flag, and +// extraArgFlags must never emit it. +func TestExtraArgs_DenylistRejectsEveryFlag(t *testing.T) { + for flag := range deniedExtraArgs { + t.Run(flag, func(t *testing.T) { + args := map[string]string{"log-level": "debug", flag: "x"} + err := checkExtraArgs(args) + if err == nil || !strings.Contains(err.Error(), "--"+flag+" ") { + t.Fatalf("checkExtraArgs(%v) = %v; want an error naming --%s", args, err, flag) + } + if got := extraArgFlags(args); !reflect.DeepEqual(got, []string{"--log-level=debug"}) { + t.Fatalf("extraArgFlags(%v) = %v; want only the allowed flag", args, got) + } + }) + } + if err := checkExtraArgs(map[string]string{"log-level": "debug", "wrapper-mode": "enforce"}); err != nil { + t.Fatalf("checkExtraArgs rejected allowed flags: %v", err) + } + if err := checkExtraArgs(nil); err != nil { + t.Fatalf("checkExtraArgs(nil) = %v", err) + } +} + +// TestEnsurePod_RefusesDeniedExtraArgs covers the guard's call site: a +// member carrying a denied key (only reachable by writing the EtcdMember +// directly) gets no Pod, and the error says which flag. +func TestEnsurePod_RefusesDeniedExtraArgs(t *testing.T) { + ctx := context.Background() + member := &lll.EtcdMember{ + ObjectMeta: metav1.ObjectMeta{Name: "test-0", Namespace: "ns", UID: types.UID("uid"), Labels: memberLabels("test", "test-0")}, + Spec: lll.EtcdMemberSpec{ + ClusterName: "test", Version: "3.6.11", Storage: lll.StorageSpec{Size: quickQty(t, "1Gi")}, + InitialCluster: "x", ClusterToken: "test", + ExtraArgs: map[string]string{"data-dir": "/tmp"}, + }, + } + c, _ := newTestClient(t, member) + r := &EtcdMemberReconciler{Client: c, Scheme: testScheme(t)} + + err := r.ensurePod(ctx, member) + if err == nil || !strings.Contains(err.Error(), "--data-dir") { + t.Fatalf("ensurePod = %v; want a refusal naming --data-dir", err) + } + pods := &corev1.PodList{} + if err := c.List(ctx, pods); err != nil { + t.Fatal(err) + } + if len(pods.Items) != 0 { + t.Fatalf("ensurePod created %d Pod(s) despite a denied extraArgs key", len(pods.Items)) + } +} + +// spec.extraArgs is latched through status.observed like spec.options, so +// an edit must register as "spec ≠ observed". +func TestSpecEqualsObserved_ExtraArgsMatter(t *testing.T) { + cluster := &lll.EtcdCluster{ + Spec: lll.EtcdClusterSpec{ + Replicas: ptrInt32(3), + Version: "3.6.11", + Storage: lll.StorageSpec{Size: quickQty(t, "1Gi")}, + ExtraArgs: map[string]string{"log-level": "debug"}, + }, + Status: lll.EtcdClusterStatus{ + Observed: &lll.ObservedClusterSpec{ + Replicas: 3, + Version: "3.6.11", + Storage: lll.StorageSpec{Size: quickQty(t, "1Gi")}, + }, + }, + } + if specEqualsObserved(cluster) { + t.Fatalf("specEqualsObserved must return false when extraArgs differ") + } + snapshotSpecIntoObserved(cluster) + if !reflect.DeepEqual(cluster.Status.Observed.ExtraArgs, cluster.Spec.ExtraArgs) { + t.Fatalf("snapshotSpecIntoObserved did not latch extraArgs: %v", cluster.Status.Observed.ExtraArgs) + } + if !specEqualsObserved(cluster) { + t.Fatalf("specEqualsObserved must return true once extraArgs are latched") + } +} diff --git a/controllers/member_template_test.go b/controllers/member_template_test.go new file mode 100644 index 00000000..ba3b4b82 --- /dev/null +++ b/controllers/member_template_test.go @@ -0,0 +1,192 @@ +package controllers + +import ( + "context" + "fmt" + "slices" + "testing" + "time" + + "go.etcd.io/etcd/api/v3/etcdserverpb" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/equality" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" + + lll "github.com/cozystack/etcd-operator/api/v1alpha2" +) + +// steadyCluster returns a converged three-member cluster whose latched +// target is observed, with members still carrying an older template. +func steadyCluster(t *testing.T, observed *lll.ObservedClusterSpec) []client.Object { + t.Helper() + now := metav1.Now() + spec := lll.EtcdClusterSpec{ + Replicas: ptrInt32(observed.Replicas), + Version: observed.Version, + Storage: observed.Storage, + Resources: observed.Resources, + Options: observed.Options, + ExtraArgs: observed.ExtraArgs, + } + objs := []client.Object{&lll.EtcdCluster{ + ObjectMeta: metav1.ObjectMeta{Name: "test", Namespace: "ns"}, + Spec: spec, + Status: lll.EtcdClusterStatus{ + ClusterToken: "ns-test-x", + ClusterID: "deadbeef", + Observed: observed, + ProgressDeadline: &metav1.Time{Time: metav1.Now().Add(time.Hour)}, + }, + }} + for i := range 3 { + objs = append(objs, &lll.EtcdMember{ + ObjectMeta: metav1.ObjectMeta{Name: fmt.Sprintf("test-%d", i), Namespace: "ns", Labels: memberLabels("test", fmt.Sprintf("test-%d", i))}, + Spec: lll.EtcdMemberSpec{ + ClusterName: "test", + Version: "3.5.17", + Storage: lll.StorageSpec{Size: quickQty(t, "1Gi")}, + InitialCluster: "x", + Options: &lll.EtcdOptions{SnapshotCount: ptrInt64(10000)}, + ExtraArgs: map[string]string{"log-level": "info"}, + }, + Status: lll.EtcdMemberStatus{ + PodName: fmt.Sprintf("test-%d", i), + MemberID: "abc", + Conditions: []metav1.Condition{{Type: lll.MemberReady, Status: metav1.ConditionTrue, Reason: "PodReady", LastTransitionTime: now}}, + }, + }) + } + return objs +} + +func steadyEtcd() *fakeEtcd { + return newFakeEtcd(0xdeadbeef, + &etcdserverpb.Member{ID: 0xa01, Name: "test-0", PeerURLs: []string{peerURL("http", "test-0", "test", "ns")}}, + &etcdserverpb.Member{ID: 0xa02, Name: "test-1", PeerURLs: []string{peerURL("http", "test-1", "test", "ns")}}, + &etcdserverpb.Member{ID: 0xa03, Name: "test-2", PeerURLs: []string{peerURL("http", "test-2", "test", "ns")}}, + ) +} + +func reconcileTest(t *testing.T, c client.Client) { + t.Helper() + r := &EtcdClusterReconciler{Client: c, Scheme: testScheme(t), EtcdClientFactory: factoryReturning(steadyEtcd())} + if _, err := r.Reconcile(context.Background(), ctrl.Request{NamespacedName: types.NamespacedName{Name: "test", Namespace: "ns"}}); err != nil { + t.Fatalf("Reconcile: %v", err) + } +} + +// TestReconcile_SyncsMemberTemplatesAtSteadyState: once a spec change is +// latched, existing members carry the new Pod template, so a Pod deleted to +// roll the change comes back with it. +func TestReconcile_SyncsMemberTemplatesAtSteadyState(t *testing.T) { + observed := &lll.ObservedClusterSpec{ + Replicas: 3, Version: "3.5.17", Storage: lll.StorageSpec{Size: quickQty(t, "1Gi")}, + Resources: corev1.ResourceRequirements{Limits: corev1.ResourceList{corev1.ResourceMemory: quickQty(t, "2Gi")}}, + Options: &lll.EtcdOptions{SnapshotCount: ptrInt64(5000)}, + ExtraArgs: map[string]string{"log-level": "debug", "threek-mode": "dry-run"}, + } + c, _ := newTestClient(t, steadyCluster(t, observed)...) + reconcileTest(t, c) + + for i := range 3 { + m := mustGet(t, c, fmt.Sprintf("test-%d", i), "ns", &lll.EtcdMember{}) + if !equality.Semantic.DeepEqual(m.Spec.Options, observed.Options) { + t.Errorf("%s options = %+v, want %+v", m.Name, m.Spec.Options, observed.Options) + } + if !equality.Semantic.DeepEqual(m.Spec.ExtraArgs, observed.ExtraArgs) { + t.Errorf("%s extraArgs = %v, want %v", m.Name, m.Spec.ExtraArgs, observed.ExtraArgs) + } + if !equality.Semantic.DeepEqual(m.Spec.Resources, observed.Resources) { + t.Errorf("%s resources = %+v, want %+v", m.Name, m.Spec.Resources, observed.Resources) + } + + pod := (&EtcdMemberReconciler{}).buildPod(m, true) + cmd := pod.Spec.Containers[0].Command + for _, flag := range []string{"--snapshot-count=5000", "--log-level=debug", "--threek-mode=dry-run"} { + if !slices.Contains(cmd, flag) { + t.Errorf("%s: a recreated Pod lacks %s: %v", m.Name, flag, cmd) + } + } + } +} + +// TestReconcile_SyncLeavesOtherMemberFieldsAlone: only the Pod-template +// fields follow the cluster. Storage is bound to the member's volume and the +// version is an upgrade, so neither changes under an existing member. +func TestReconcile_SyncLeavesOtherMemberFieldsAlone(t *testing.T) { + observed := &lll.ObservedClusterSpec{ + Replicas: 3, Version: "3.6.1", Storage: lll.StorageSpec{Size: quickQty(t, "8Gi")}, + ExtraArgs: map[string]string{"threek-mode": "dry-run"}, + } + c, _ := newTestClient(t, steadyCluster(t, observed)...) + reconcileTest(t, c) + + m := mustGet(t, c, "test-0", "ns", &lll.EtcdMember{}) + if m.Spec.Version != "3.5.17" { + t.Errorf("version = %s, want the member's own 3.5.17", m.Spec.Version) + } + if got := m.Spec.Storage.Size.String(); got != "1Gi" { + t.Errorf("storage size = %s, want the member's own 1Gi", got) + } +} + +// TestReconcile_NoMemberWritesWhenTemplatesMatch: a converged cluster whose +// members already carry the latched template is not rewritten every pass. +func TestReconcile_NoMemberWritesWhenTemplatesMatch(t *testing.T) { + observed := &lll.ObservedClusterSpec{ + Replicas: 3, Version: "3.5.17", Storage: lll.StorageSpec{Size: quickQty(t, "1Gi")}, + Options: &lll.EtcdOptions{SnapshotCount: ptrInt64(10000)}, + ExtraArgs: map[string]string{"log-level": "info"}, + } + writes := 0 + countMemberWrites := func(obj client.Object) { + if _, ok := obj.(*lll.EtcdMember); ok { + writes++ + } + } + c := fake.NewClientBuilder(). + WithScheme(testScheme(t)). + WithObjects(steadyCluster(t, observed)...). + WithStatusSubresource(&lll.EtcdCluster{}, &lll.EtcdMember{}). + WithInterceptorFuncs(interceptor.Funcs{ + Patch: func(ctx context.Context, c client.WithWatch, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { + countMemberWrites(obj) + return c.Patch(ctx, obj, patch, opts...) + }, + Update: func(ctx context.Context, c client.WithWatch, obj client.Object, opts ...client.UpdateOption) error { + countMemberWrites(obj) + return c.Update(ctx, obj, opts...) + }, + }). + Build() + reconcileTest(t, c) + if writes != 0 { + t.Errorf("members written %d times without a template change", writes) + } +} + +// TestReconcile_SyncsDormantMemberTemplate: a paused cluster's dormant member +// is woken by scale-up, which creates its Pod before the cluster converges +// again, so the dormant member must already carry the latched template. +func TestReconcile_SyncsDormantMemberTemplate(t *testing.T) { + observed := &lll.ObservedClusterSpec{ + Replicas: 0, Version: "3.5.17", Storage: lll.StorageSpec{Size: quickQty(t, "1Gi")}, + ExtraArgs: map[string]string{"threek-mode": "dry-run"}, + } + objs := steadyCluster(t, observed)[:2] + dormant := objs[1].(*lll.EtcdMember) + dormant.Spec.Dormant = true + dormant.Status = lll.EtcdMemberStatus{MemberID: "abc"} + c, _ := newTestClient(t, objs...) + reconcileTest(t, c) + + m := mustGet(t, c, dormant.Name, "ns", &lll.EtcdMember{}) + if !equality.Semantic.DeepEqual(m.Spec.ExtraArgs, observed.ExtraArgs) { + t.Errorf("dormant member extraArgs = %v, want %v", m.Spec.ExtraArgs, observed.ExtraArgs) + } +} diff --git a/docs/concepts.md b/docs/concepts.md index 54eefabc..74402396 100644 --- a/docs/concepts.md +++ b/docs/concepts.md @@ -348,9 +348,40 @@ spec: Every field is optional; an unset field emits no flag, leaving etcd's built-in default in force. -The set is deliberately **closed and typed**. The legacy aenix operator exposed `spec.options` as a free-form `map[string]string`, which let users inject arbitrary flags — including ones that conflict with flags the operator itself manages (listen URLs, initial-cluster wiring, TLS paths). This operator types exactly the keys Cozystack's etcd package actually used; a new tuning knob lands as a new typed field with validation, not via an escape hatch. +The typed set covers the knobs worth validating. The legacy aenix operator exposed `spec.options` as a free-form `map[string]string`, which let users inject flags that conflict with the ones the operator itself manages (listen URLs, initial-cluster wiring, TLS paths); this operator types exactly the keys Cozystack's etcd package actually used. Any other flag goes through [`spec.extraArgs`](#extra-etcd-flags). -Like `spec.resources`, options are latched through `status.observed` and apply **to newly-created members only** (scale-up, replacement) — the operator does not roll existing Pods when they change. To apply a tuning change to an existing cluster, delete one Pod at a time and let the operator recreate it with the new flags. A transient mix of old- and new-flag members is harmless: these are per-member settings (backend quota, compaction cadence, raft snapshot interval), the same heterogeneity any manual rolling flag change passes through. +Like `spec.resources`, options are latched through `status.observed`. New members get them at creation, and the operator copies them onto existing `EtcdMember` objects without touching their running Pods — it does not roll Pods. To apply a tuning change to an existing cluster, delete one Pod at a time and let the operator recreate it with the new flags. A transient mix of old- and new-flag members is harmless: these are per-member settings (backend quota, compaction cadence, raft snapshot interval), the same heterogeneity any manual rolling flag change passes through. + +## Extra etcd flags + +`spec.extraArgs` passes flags the typed `spec.options` does not cover — an etcd flag no field exists for yet, or flags for an image that wraps etcd and reads options of its own: + +```yaml +spec: + extraArgs: + log-level: debug # --log-level=debug + experimental-initial-corrupt-check: "true" +``` + +Keys are flag names without the leading dashes (lowercase words joined by `-`); each entry renders as `--key=value` with the value passed as is. The entries come after the operator's own flags, sorted by key, so the rendered Pod does not change between reconciles. Values are strings: quote `"true"` and numbers in YAML. + +Flags the operator manages are rejected at admission, with a message naming the flag: + +| Flags | Managed through | +|---|---| +| `name`, `data-dir`, `wal-dir` | member identity and data placement | +| `listen-peer-urls`, `listen-client-urls`, `listen-client-http-urls`, `listen-metrics-urls`, `advertise-client-urls`, `initial-advertise-peer-urls` | listeners and member URLs | +| `initial-cluster`, `initial-cluster-token`, `initial-cluster-state`, `force-new-cluster`, `discovery`, `discovery-srv`, `discovery-endpoints` | cluster bootstrap and membership | +| `config-file` | would make etcd ignore the operator's command line | +| `auto-tls`, `cert-file`, `key-file`, `client-cert-file`, `client-key-file`, `client-cert-auth`, `trusted-ca-file` | `spec.tls.client` | +| `peer-auto-tls`, `peer-cert-file`, `peer-key-file`, `peer-client-cert-file`, `peer-client-key-file`, `peer-trusted-ca-file`, `peer-client-cert-auth` | `spec.tls.peer` | +| `quota-backend-bytes`, `auto-compaction-mode`, `auto-compaction-retention`, `snapshot-count` | `spec.options` | + +The member controller applies the same denylist when it builds a Pod and refuses to create one that carries a denied key. + +Anything else passes through unchecked. A flag etcd does not recognise, or a malformed value, makes the member fail to start — the operator does not know etcd's flag set. Prefer a typed field where one exists. + +`extraArgs` follow the same rule as `spec.options`: latched through `status.observed` and copied onto every member, applied to a running member only when its Pod is recreated. Delete one Pod at a time to roll a change onto existing members. ## Member DNS identity (adoption annotations) diff --git a/docs/migration.md b/docs/migration.md index d5e2bcb9..55f8b878 100644 --- a/docs/migration.md +++ b/docs/migration.md @@ -382,9 +382,9 @@ its name. The legacy operator's `spec.options` was a free-form `map[string]string` passed through as etcd flags. This operator keeps the `spec.options` path but types it -as a closed struct covering exactly the keys Cozystack's etcd package set — -arbitrary flag injection is no longer possible (see -[concepts: etcd tuning options](concepts.md#etcd-tuning-options) for why). +as a struct covering exactly the keys Cozystack's etcd package set; other +flags move to `spec.extraArgs`, which rejects the flags the operator manages +(see [concepts: etcd tuning options](concepts.md#etcd-tuning-options) for why). The key mapping, using Cozystack's actual legacy values: @@ -404,8 +404,9 @@ The key mapping, using Cozystack's actual legacy values: Note the value types: `quotaBackendBytes` and `snapshotCount` are integers, not quoted strings. `etcd-migrate` performs this mapping automatically. Any other key the legacy map accepted has no typed equivalent — the tool drops it with a -warning; if you relied on one, file an issue: the flag gets a typed field, not -a pass-through. +warning. If you relied on one, set it under `spec.extraArgs` after the +migration (see [concepts: extra etcd flags](concepts.md#extra-etcd-flags)); +flags the operator manages itself are rejected there. ## Authentication: root credentials are BYO and required diff --git a/internal/migrate/adopt.go b/internal/migrate/adopt.go index 7410ecee..538901ac 100644 --- a/internal/migrate/adopt.go +++ b/internal/migrate/adopt.go @@ -279,6 +279,7 @@ func BuildAdoption(name, namespace string, spec legacy.EtcdClusterSpec, facts Cl Affinity: cluster.Spec.Affinity, TopologySpreadConstraints: cluster.Spec.TopologySpreadConstraints, Options: cluster.Spec.Options, + ExtraArgs: cluster.Spec.ExtraArgs, ImagePullSecrets: cluster.Spec.ImagePullSecrets, Bootstrap: false, InitialCluster: initialCluster, @@ -312,6 +313,7 @@ func BuildAdoption(name, namespace string, spec legacy.EtcdClusterSpec, facts Cl TopologySpreadConstraints: cluster.Spec.TopologySpreadConstraints, AdditionalMetadata: cluster.Spec.AdditionalMetadata, Options: cluster.Spec.Options, + ExtraArgs: cluster.Spec.ExtraArgs, ImagePullSecrets: cluster.Spec.ImagePullSecrets, }, }