From 08fe30e19c7d4dfa0ba7e263bde12b77a36daa71 Mon Sep 17 00:00:00 2001 From: Timofei Larkin Date: Sat, 17 Oct 2026 21:38:04 +0200 Subject: [PATCH 1/3] revert: feat(api): set trafficDistribution on the client Service (#376) This reverts commit eccc4fb2255e0b02b06837e9ca14d374bedb5e56, removing spec.clientService.trafficDistribution from the EtcdCluster API. The capability returns in the next commit as an annotation on the EtcdCluster, so the API does not change for now. Assisted-By: LLM Signed-off-by: Timofei Larkin --- README.md | 1 - api/v1alpha2/client_service_envtest_test.go | 83 ---------- api/v1alpha2/etcdcluster_types.go | 35 ---- api/v1alpha2/zz_generated.deepcopy.go | 25 --- ...cd-operator.cozystack.io_etcdclusters.yaml | 34 ---- controllers/client_service_test.go | 155 ------------------ controllers/etcdcluster_controller.go | 24 +-- docs/concepts.md | 21 --- 8 files changed, 5 insertions(+), 373 deletions(-) delete mode 100644 api/v1alpha2/client_service_envtest_test.go delete mode 100644 controllers/client_service_test.go diff --git a/README.md b/README.md index 9fa61424..70184d73 100644 --- a/README.md +++ b/README.md @@ -25,7 +25,6 @@ The full design rationale is in [docs/concepts.md](docs/concepts.md). - **TLS (BYO Secrets or cert-manager)**: `spec.tls.client` / `spec.tls.peer` enable TLS on each surface independently. Material comes from either user-provided Secrets (`serverSecretRef` / `operatorClientSecretRef` / `secretRef`) or operator-emitted `cert-manager.io/v1` Certificates (`certManager.{serverIssuerRef,operatorClientIssuerRef,issuerRef}`) — mutually exclusive per subtree, enforced by CEL. mTLS is the implicit mode when an operator-client source is supplied; server-TLS-only when it isn't. The whole `tls` subtree is CEL-locked immutable post-create. cert-manager-emitted certs auto-renew via cert-manager; Pod-side rotation is a manual one-at-a-time `kubectl delete pod` either way. See [docs/concepts.md](docs/concepts.md#tls). - **Resource sizing**: `spec.resources` (a `corev1.ResourceRequirements`) sets the etcd container's CPU/memory requests and limits. Unset uses a conservative 100m/128Mi-request default. Updates take effect on newly-created members; pair with a `VerticalPodAutoscaler` targeting the cluster for live recommendation/rollout. - **Scheduling & extra metadata**: `spec.affinity` and `spec.topologySpreadConstraints` pass through to every member Pod (anti-affinity is not defaulted — set it for production); `spec.additionalMetadata` merges user labels/annotations onto every object the operator creates (member Pods, data PVCs, Services, PDB, `EtcdMember` CRs), with operator-owned keys winning on collision. All three apply on object creation and are latched like the rest of the spec. See [docs/concepts.md](docs/concepts.md#pod-scheduling-and-additional-metadata). -- **Zone-local clients**: `spec.clientService.trafficDistribution` (`PreferClose` / `PreferSameZone` / `PreferSameNode`) sets `trafficDistribution` on the `-client` Service, so in a cluster stretched across zones clients prefer a ready member in their own zone. Unset leaves the field to whoever manages the Service. See [docs/concepts.md](docs/concepts.md#client-service). - **Monitoring / autoscaling hooks**: every member Pod always exposes a plaintext `metrics` container port at `2381` (etcd's `/health` + Prometheus `/metrics`) for `VMPodScrape` / `PodMonitor`. The `EtcdCluster` CRD exposes the `/scale` subresource with a populated `status.selector`, making it a valid target for `kubectl scale` and `VerticalPodAutoscaler.targetRef`. - **Locking pattern**: `status.observed` snapshots the in-flight target so mid-flight spec edits don't corrupt consensus; `progressDeadline` bounds how long the operator will spend trying to reach a target. - **Cluster deletion**: cascading owner refs clean up everything; finalizers detect "the whole cluster is going away" and skip etcd-side removal to avoid deadlock. diff --git a/api/v1alpha2/client_service_envtest_test.go b/api/v1alpha2/client_service_envtest_test.go deleted file mode 100644 index 4e322294..00000000 --- a/api/v1alpha2/client_service_envtest_test.go +++ /dev/null @@ -1,83 +0,0 @@ -/* -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 v1alpha2_test - -import ( - "context" - "strings" - "testing" - - corev1 "k8s.io/api/core/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - ctrlclient "sigs.k8s.io/controller-runtime/pkg/client" - - lll "github.com/cozystack/etcd-operator/api/v1alpha2" -) - -// spec.clientService.trafficDistribution admits exactly the values the core -// Service API defines; anything else is rejected at admission rather than -// surfacing later as a failed Service patch inside the reconcile loop. -func TestClientService_TrafficDistributionEnum(t *testing.T) { - skipIfNoEnvtest(t) - ctx := context.Background() - - for _, v := range []string{"PreferClose", "PreferSameZone", "PreferSameNode"} { - c := validCluster("td-" + strings.ToLower(v)) - c.Spec.ClientService = &lll.ClientServiceSpec{TrafficDistribution: &v} - if err := k8s.Create(ctx, c); err != nil { - t.Fatalf("apiserver rejected trafficDistribution=%q: %v", v, err) - } - t.Cleanup(func() { _ = k8s.Delete(ctx, c) }) - } - - bogus := "Anywhere" - c := validCluster("td-bogus") - c.Spec.ClientService = &lll.ClientServiceSpec{TrafficDistribution: &bogus} - err := k8s.Create(ctx, c) - if err == nil { - _ = k8s.Delete(ctx, c) - t.Fatalf("apiserver accepted trafficDistribution=%q; expected rejection", bogus) - } - if !strings.Contains(err.Error(), "spec.clientService.trafficDistribution") { - t.Fatalf("error did not name the field: %v", err) - } -} - -// The envtest apiserver (pinned to the k8s.io/api minor in go.mod) must -// persist trafficDistribution on a Service. An apiserver with the field -// disabled drops it silently, which would leave the operator re-patching -// the client Service on every reconcile; this guards the version floor the -// field's docs promise. -func TestClientService_ApiserverPersistsTrafficDistribution(t *testing.T) { - skipIfNoEnvtest(t) - ctx := context.Background() - - td := corev1.ServiceTrafficDistributionPreferClose - svc := &corev1.Service{ - ObjectMeta: metav1.ObjectMeta{Name: "td-probe", Namespace: "default"}, - Spec: corev1.ServiceSpec{ - Ports: []corev1.ServicePort{{Name: "client", Port: 2379}}, - TrafficDistribution: &td, - }, - } - if err := k8s.Create(ctx, svc); err != nil { - t.Fatalf("Create Service: %v", err) - } - t.Cleanup(func() { _ = k8s.Delete(ctx, svc) }) - - got := &corev1.Service{} - if err := k8s.Get(ctx, ctrlclient.ObjectKeyFromObject(svc), got); err != nil { - t.Fatalf("Get Service: %v", err) - } - if got.Spec.TrafficDistribution == nil || *got.Spec.TrafficDistribution != td { - t.Fatalf("apiserver did not persist trafficDistribution: %v", got.Spec.TrafficDistribution) - } -} diff --git a/api/v1alpha2/etcdcluster_types.go b/api/v1alpha2/etcdcluster_types.go index 2e88106f..8bdd5253 100644 --- a/api/v1alpha2/etcdcluster_types.go +++ b/api/v1alpha2/etcdcluster_types.go @@ -569,41 +569,6 @@ type EtcdClusterSpec struct { // status.observed. // +optional ImagePullSecrets []corev1.LocalObjectReference `json:"imagePullSecrets,omitempty"` - - // ClientService configures the "-client" Service the operator - // creates for applications. Absent means the operator manages only the - // fields it always owns (selector and the client port) and leaves the - // rest of the Service alone. Does not affect the headless Service. - // - // Unlike the member-level fields above, this is not latched through - // status.observed: the Service carries no consensus state, so a change - // is applied on the next reconcile. - // +optional - ClientService *ClientServiceSpec `json:"clientService,omitempty"` -} - -// ClientServiceSpec configures the "-client" Service. Grouped in its -// own struct so further Service-level knobs land next to TrafficDistribution -// rather than as loose fields on EtcdClusterSpec. -type ClientServiceSpec struct { - // TrafficDistribution sets the client Service's spec.trafficDistribution, - // a hint to the Service proxy (kube-proxy, Cilium, ...) on how to route - // traffic across the ready etcd members. The common use is a cluster - // stretched across zones: "PreferClose" (or "PreferSameZone") keeps - // clients talking to a member in their own zone while one is ready. - // The values mirror corev1.ServiceSpec.TrafficDistribution. - // - // When set, the operator writes it on create and reconciles drift back - // to it. When absent, the operator does not touch the field, so a value - // set on the Service by hand is preserved. - // - // Requires an apiserver that serves the field: Kubernetes 1.31+ for - // "PreferClose" (beta, on by default; GA in 1.33), and 1.34+ for - // "PreferSameZone" and "PreferSameNode" (PreferSameTrafficDistribution - // feature gate, beta and on by default from 1.34). - // +kubebuilder:validation:Enum=PreferClose;PreferSameZone;PreferSameNode - // +optional - TrafficDistribution *string `json:"trafficDistribution,omitempty"` } // AdditionalMetadata is a set of labels and annotations the operator merges diff --git a/api/v1alpha2/zz_generated.deepcopy.go b/api/v1alpha2/zz_generated.deepcopy.go index b4a7f23f..7f59359e 100644 --- a/api/v1alpha2/zz_generated.deepcopy.go +++ b/api/v1alpha2/zz_generated.deepcopy.go @@ -116,26 +116,6 @@ func (in *ClientCertManagerTLS) DeepCopy() *ClientCertManagerTLS { return out } -// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. -func (in *ClientServiceSpec) DeepCopyInto(out *ClientServiceSpec) { - *out = *in - if in.TrafficDistribution != nil { - in, out := &in.TrafficDistribution, &out.TrafficDistribution - *out = new(string) - **out = **in - } -} - -// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ClientServiceSpec. -func (in *ClientServiceSpec) DeepCopy() *ClientServiceSpec { - if in == nil { - return nil - } - out := new(ClientServiceSpec) - in.DeepCopyInto(out) - return out -} - // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *ClientTLS) DeepCopyInto(out *ClientTLS) { *out = *in @@ -322,11 +302,6 @@ func (in *EtcdClusterSpec) DeepCopyInto(out *EtcdClusterSpec) { *out = make([]v1.LocalObjectReference, len(*in)) copy(*out, *in) } - if in.ClientService != nil { - in, out := &in.ClientService, &out.ClientService - *out = new(ClientServiceSpec) - (*in).DeepCopyInto(*out) - } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new EtcdClusterSpec. 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 8663fa1a..ed4d78ad 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,40 +1166,6 @@ spec: rule: '!has(self.source.pvc) || (has(self.source.pvc.subPath) && size(self.source.pvc.subPath) > 0)' type: object - clientService: - description: |- - ClientService configures the "-client" Service the operator - creates for applications. Absent means the operator manages only the - fields it always owns (selector and the client port) and leaves the - rest of the Service alone. Does not affect the headless Service. - - Unlike the member-level fields above, this is not latched through - status.observed: the Service carries no consensus state, so a change - is applied on the next reconcile. - properties: - trafficDistribution: - description: |- - TrafficDistribution sets the client Service's spec.trafficDistribution, - a hint to the Service proxy (kube-proxy, Cilium, ...) on how to route - traffic across the ready etcd members. The common use is a cluster - stretched across zones: "PreferClose" (or "PreferSameZone") keeps - clients talking to a member in their own zone while one is ready. - The values mirror corev1.ServiceSpec.TrafficDistribution. - - When set, the operator writes it on create and reconciles drift back - to it. When absent, the operator does not touch the field, so a value - set on the Service by hand is preserved. - - Requires an apiserver that serves the field: Kubernetes 1.31+ for - "PreferClose" (beta, on by default; GA in 1.33), and 1.34+ for - "PreferSameZone" and "PreferSameNode" (PreferSameTrafficDistribution - feature gate, beta and on by default from 1.34). - enum: - - PreferClose - - PreferSameZone - - PreferSameNode - type: string - type: object imagePullSecrets: description: |- ImagePullSecrets is a list of Secret references in the cluster's diff --git a/controllers/client_service_test.go b/controllers/client_service_test.go deleted file mode 100644 index 1e0f0b07..00000000 --- a/controllers/client_service_test.go +++ /dev/null @@ -1,155 +0,0 @@ -/* -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" - "testing" - - corev1 "k8s.io/api/core/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/types" - "sigs.k8s.io/controller-runtime/pkg/client" - - lll "github.com/cozystack/etcd-operator/api/v1alpha2" -) - -// clusterWithTrafficDistribution returns a cluster whose client Service asks -// for td; a nil td leaves spec.clientService unset. -func clusterWithTrafficDistribution(td *string) *lll.EtcdCluster { - cluster := &lll.EtcdCluster{ - ObjectMeta: metav1.ObjectMeta{Name: "test", Namespace: "ns"}, - } - if td != nil { - cluster.Spec.ClientService = &lll.ClientServiceSpec{TrafficDistribution: td} - } - return cluster -} - -// existingService returns a Service as it would sit on the apiserver before -// the operator's reconcile, carrying td on spec.trafficDistribution. -func existingService(name string, td *string) *corev1.Service { - svc := &corev1.Service{ - ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: "ns"}, - Spec: corev1.ServiceSpec{ - Selector: map[string]string{LabelCluster: "test"}, - Ports: []corev1.ServicePort{{Name: "client", Port: 2379}}, - TrafficDistribution: td, - }, - } - if name == "test" { - svc.Spec.ClusterIP = corev1.ClusterIPNone - svc.Spec.PublishNotReadyAddresses = true - svc.Spec.Ports = append(svc.Spec.Ports, corev1.ServicePort{Name: "peer", Port: 2380}) - } - return svc -} - -func strPtr(s string) *string { return &s } - -func getTrafficDistribution(t *testing.T, c client.Client, name string) *string { - t.Helper() - svc := &corev1.Service{} - if err := c.Get(context.Background(), types.NamespacedName{Namespace: "ns", Name: name}, svc); err != nil { - t.Fatalf("Get Service %s: %v", name, err) - } - return svc.Spec.TrafficDistribution -} - -func assertTrafficDistribution(t *testing.T, c client.Client, name string, want *string) { - t.Helper() - got := getTrafficDistribution(t, c, name) - switch { - case want == nil && got != nil: - t.Fatalf("Service %s trafficDistribution = %q, want unset", name, *got) - case want != nil && got == nil: - t.Fatalf("Service %s trafficDistribution unset, want %q", name, *want) - case want != nil && *got != *want: - t.Fatalf("Service %s trafficDistribution = %q, want %q", name, *got, *want) - } -} - -// On create the client Service carries the requested value; the headless -// Service never does. -func TestEnsureServices_TrafficDistributionOnCreate(t *testing.T) { - cluster := clusterWithTrafficDistribution(strPtr(corev1.ServiceTrafficDistributionPreferClose)) - c, _ := newTestClient(t, cluster) - r := &EtcdClusterReconciler{Client: c, Scheme: testScheme(t)} - - if err := r.ensureServices(context.Background(), cluster); err != nil { - t.Fatalf("ensureServices: %v", err) - } - assertTrafficDistribution(t, c, "test-client", strPtr(corev1.ServiceTrafficDistributionPreferClose)) - assertTrafficDistribution(t, c, "test", nil) -} - -// A client Service that drifted (value removed or changed by hand) is -// brought back to the spec. -func TestEnsureServices_TrafficDistributionDriftReconciled(t *testing.T) { - for name, onService := range map[string]*string{ - "unset": nil, - "different": strPtr(corev1.ServiceTrafficDistributionPreferSameNode), - } { - t.Run(name, func(t *testing.T) { - cluster := clusterWithTrafficDistribution(strPtr(corev1.ServiceTrafficDistributionPreferClose)) - c, _ := newTestClient(t, cluster, existingService("test", nil), existingService("test-client", onService)) - r := &EtcdClusterReconciler{Client: c, Scheme: testScheme(t)} - - if err := r.ensureServices(context.Background(), cluster); err != nil { - t.Fatalf("ensureServices: %v", err) - } - assertTrafficDistribution(t, c, "test-client", strPtr(corev1.ServiceTrafficDistributionPreferClose)) - assertTrafficDistribution(t, c, "test", nil) - }) - } -} - -// With the field unset — no clientService block, or a block without -// trafficDistribution — the operator does not own it: a value patched onto -// the Service by hand survives reconcile, and no value is added. -func TestEnsureServices_TrafficDistributionUnsetLeavesServiceAlone(t *testing.T) { - for name, cluster := range map[string]*lll.EtcdCluster{ - "no clientService": clusterWithTrafficDistribution(nil), - "empty clientService": func() *lll.EtcdCluster { - cl := clusterWithTrafficDistribution(nil) - cl.Spec.ClientService = &lll.ClientServiceSpec{} - return cl - }(), - } { - t.Run(name, func(t *testing.T) { - manual := strPtr(corev1.ServiceTrafficDistributionPreferClose) - c, _ := newTestClient(t, cluster, existingService("test", nil), existingService("test-client", manual)) - r := &EtcdClusterReconciler{Client: c, Scheme: testScheme(t)} - - if err := r.ensureServices(context.Background(), cluster); err != nil { - t.Fatalf("ensureServices: %v", err) - } - assertTrafficDistribution(t, c, "test-client", manual) - assertTrafficDistribution(t, c, "test", nil) - }) - } -} - -// The headless Service is not the operator's to set traffic distribution on: -// a value someone put there is neither cleared nor overwritten with the -// client Service's value. -func TestEnsureServices_HeadlessTrafficDistributionUntouched(t *testing.T) { - cluster := clusterWithTrafficDistribution(strPtr(corev1.ServiceTrafficDistributionPreferClose)) - manual := strPtr(corev1.ServiceTrafficDistributionPreferSameNode) - c, _ := newTestClient(t, cluster, existingService("test", manual), existingService("test-client", nil)) - r := &EtcdClusterReconciler{Client: c, Scheme: testScheme(t)} - - if err := r.ensureServices(context.Background(), cluster); err != nil { - t.Fatalf("ensureServices: %v", err) - } - assertTrafficDistribution(t, c, "test", manual) - assertTrafficDistribution(t, c, "test-client", strPtr(corev1.ServiceTrafficDistributionPreferClose)) -} diff --git a/controllers/etcdcluster_controller.go b/controllers/etcdcluster_controller.go index 49d8230b..4d973f93 100644 --- a/controllers/etcdcluster_controller.go +++ b/controllers/etcdcluster_controller.go @@ -1917,25 +1917,19 @@ func (r *EtcdClusterReconciler) ensureServices(ctx context.Context, cluster *lll } // Client service — stable endpoint for applications. - clientSpec := corev1.ServiceSpec{ + return r.ensureService(ctx, cluster, cluster.Name+"-client", corev1.ServiceSpec{ Selector: map[string]string{LabelCluster: cluster.Name}, Ports: []corev1.ServicePort{ {Name: "client", Port: 2379}, }, - } - if cs := cluster.Spec.ClientService; cs != nil { - clientSpec.TrafficDistribution = cs.TrafficDistribution - } - return r.ensureService(ctx, cluster, cluster.Name+"-client", clientSpec) + }) } // ensureService creates the Service if absent and reconciles drift on the // fields the operator actually owns: Selector, PublishNotReadyAddresses, -// the named Ports it requires, and TrafficDistribution when the desired -// spec sets it. Everything else (labels, annotations, user-added ports, -// type, a hand-set TrafficDistribution the spec doesn't ask for) is -// preserved — admission webhooks routinely inject these and we don't want -// to fight them. +// and the named Ports it requires. Everything else (labels, annotations, +// user-added ports, type) is preserved — admission webhooks routinely +// inject these and we don't want to fight them. func (r *EtcdClusterReconciler) ensureService( ctx context.Context, cluster *lll.EtcdCluster, @@ -1976,14 +1970,6 @@ func (r *EtcdClusterReconciler) ensureService( svc.Spec.PublishNotReadyAddresses = desired.PublishNotReadyAddresses changed = true } - // TrafficDistribution is owned only when asked for: nil leaves whatever - // is on the Service, including a value set by hand. - if desired.TrafficDistribution != nil && - (svc.Spec.TrafficDistribution == nil || *svc.Spec.TrafficDistribution != *desired.TrafficDistribution) { - td := *desired.TrafficDistribution - svc.Spec.TrafficDistribution = &td - changed = true - } // Required ports must be present with correct values. Extra ports the // user has added stay. for _, want := range desired.Ports { diff --git a/docs/concepts.md b/docs/concepts.md index b0e0103a..e7ea03b4 100644 --- a/docs/concepts.md +++ b/docs/concepts.md @@ -281,27 +281,6 @@ Merge semantics: - **Apply-on-create.** Objects are stamped when created; editing the field re-stamps nothing retroactively. Newly-created objects (scale-up members and their PVCs, replacements) pick up the latest latched value. - **Latched.** A mid-flight edit only takes effect once the current `status.observed` target is reached, like every other latched field. -## Client Service - -The operator creates two Services per cluster: the headless `` (per-Pod DNS for peers, publishes not-ready addresses) and `-client` (a ClusterIP over ready members, the stable endpoint for applications). On both it reconciles drift only on the fields it owns — the selector, `publishNotReadyAddresses`, and its named ports — and leaves everything else (labels, annotations, extra ports, type) to whoever set it. - -`spec.clientService` configures `-client` further. Today it carries one field: - -```yaml -spec: - clientService: - trafficDistribution: PreferClose # PreferClose | PreferSameZone | PreferSameNode -``` - -`trafficDistribution` is copied to the Service's `spec.trafficDistribution`. The main use is a cluster stretched across zones (for example two sites plus a witness): with `PreferClose`, the EndpointSlice controller writes zone hints on the client Service's endpoints and a proxy that honours them (kube-proxy, Cilium) sends each client to a ready member in its own zone, falling back to the others when its zone has none. Clients stop paying cross-zone latency on every request, and a site keeps serving reads locally. - -Semantics: - -- **Owned only when set.** With the field set, the operator writes it on create and reconciles any drift back to it. With it unset (no `clientService`, or no `trafficDistribution` in it), the operator does not touch the field — a value patched onto the Service by hand survives. -- **Client Service only.** The headless Service is never given a traffic distribution. -- **Not latched.** The Service carries no consensus state, so unlike the member-level fields a change is applied on the next reconcile rather than waiting for the current `status.observed` target. -- **Apiserver support.** `PreferClose` needs Kubernetes 1.31+ (beta, on by default; GA in 1.33). `PreferSameZone` and `PreferSameNode` need 1.34+, where the `PreferSameTrafficDistribution` feature gate is beta and on by default. An apiserver that does not know the field drops it silently (the setting has no effect); one that knows the field but not the value rejects the Service write, which surfaces as a reconcile error for that cluster. - ## etcd tuning options `spec.options` carries the etcd server tuning flags the operator renders onto each member's command line: From 88c31746136ec48e28d991bde3046f8ce41ae7a7 Mon Sep 17 00:00:00 2001 From: Timofei Larkin Date: Fri, 16 Oct 2026 09:56:10 +0200 Subject: [PATCH 2/3] feat(controllers): set client Service trafficDistribution from an annotation The etcd-operator.cozystack.io/client-service-traffic-distribution annotation on an EtcdCluster sets spec.trafficDistribution on the -client Service (PreferClose, PreferSameZone or PreferSameNode), so clients of a cluster stretched across zones prefer a nearby member. It stands in for a typed spec field without changing the API for now. The field is written with server-side apply under its own field manager, so removing the annotation clears it unless another manager also set it, and a value someone else set without the annotation is left alone. A value outside that set, one the apiserver rejects (PreferSameZone and PreferSameNode before 1.34) or one it drops (no support before 1.31) is reported as a Warning Event and leaves the field unset; the rest of the reconcile carries on. The headless Service is not touched. Builds on the controller tests and docs of #376 by Andrei Kvapil. Assisted-By: LLM Signed-off-by: Timofei Larkin --- README.md | 1 + controllers/client_service.go | 165 ++++++++++++++ controllers/client_service_envtest_test.go | 145 ++++++++++++ controllers/client_service_test.go | 245 +++++++++++++++++++++ controllers/etcdcluster_controller.go | 18 +- controllers/helpers.go | 6 + docs/concepts.md | 52 +++++ main.go | 1 + 8 files changed, 631 insertions(+), 2 deletions(-) create mode 100644 controllers/client_service.go create mode 100644 controllers/client_service_envtest_test.go create mode 100644 controllers/client_service_test.go diff --git a/README.md b/README.md index 70184d73..2bf78daf 100644 --- a/README.md +++ b/README.md @@ -25,6 +25,7 @@ The full design rationale is in [docs/concepts.md](docs/concepts.md). - **TLS (BYO Secrets or cert-manager)**: `spec.tls.client` / `spec.tls.peer` enable TLS on each surface independently. Material comes from either user-provided Secrets (`serverSecretRef` / `operatorClientSecretRef` / `secretRef`) or operator-emitted `cert-manager.io/v1` Certificates (`certManager.{serverIssuerRef,operatorClientIssuerRef,issuerRef}`) — mutually exclusive per subtree, enforced by CEL. mTLS is the implicit mode when an operator-client source is supplied; server-TLS-only when it isn't. The whole `tls` subtree is CEL-locked immutable post-create. cert-manager-emitted certs auto-renew via cert-manager; Pod-side rotation is a manual one-at-a-time `kubectl delete pod` either way. See [docs/concepts.md](docs/concepts.md#tls). - **Resource sizing**: `spec.resources` (a `corev1.ResourceRequirements`) sets the etcd container's CPU/memory requests and limits. Unset uses a conservative 100m/128Mi-request default. Updates take effect on newly-created members; pair with a `VerticalPodAutoscaler` targeting the cluster for live recommendation/rollout. - **Scheduling & extra metadata**: `spec.affinity` and `spec.topologySpreadConstraints` pass through to every member Pod (anti-affinity is not defaulted — set it for production); `spec.additionalMetadata` merges user labels/annotations onto every object the operator creates (member Pods, data PVCs, Services, PDB, `EtcdMember` CRs), with operator-owned keys winning on collision. All three apply on object creation and are latched like the rest of the spec. See [docs/concepts.md](docs/concepts.md#pod-scheduling-and-additional-metadata). +- **Zone-local clients**: the `etcd-operator.cozystack.io/client-service-traffic-distribution` annotation on an `EtcdCluster` (`PreferClose` / `PreferSameZone` / `PreferSameNode`) sets `trafficDistribution` on the `-client` Service, so in a cluster stretched across zones clients prefer a ready member in their own zone. Removing the annotation clears the field; an invalid value is reported as a Warning Event and ignored. See [docs/concepts.md](docs/concepts.md#client-service-traffic-distribution). - **Monitoring / autoscaling hooks**: every member Pod always exposes a plaintext `metrics` container port at `2381` (etcd's `/health` + Prometheus `/metrics`) for `VMPodScrape` / `PodMonitor`. The `EtcdCluster` CRD exposes the `/scale` subresource with a populated `status.selector`, making it a valid target for `kubectl scale` and `VerticalPodAutoscaler.targetRef`. - **Locking pattern**: `status.observed` snapshots the in-flight target so mid-flight spec edits don't corrupt consensus; `progressDeadline` bounds how long the operator will spend trying to reach a target. - **Cluster deletion**: cascading owner refs clean up everything; finalizers detect "the whole cluster is going away" and skip etcd-side removal to avoid deadlock. diff --git a/controllers/client_service.go b/controllers/client_service.go new file mode 100644 index 00000000..32c40e19 --- /dev/null +++ b/controllers/client_service.go @@ -0,0 +1,165 @@ +package controllers + +import ( + "context" + "fmt" + + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + corev1ac "k8s.io/client-go/applyconfigurations/core/v1" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/log" + + lll "github.com/cozystack/etcd-operator/api/v1alpha2" +) + +// trafficDistributionFieldManager is the server-side-apply field manager +// that owns spec.trafficDistribution on the client Service. Kept apart +// from the operator's merge patches so the apiserver tracks exactly which +// value the annotation put there. +const trafficDistributionFieldManager = "etcd-operator-traffic-distribution" + +// Event reasons for AnnClientServiceTrafficDistribution problems. Each one +// leaves the cluster's reconcile running and the Service field unset. +const ( + ReasonTrafficDistributionInvalid = "InvalidTrafficDistribution" + ReasonTrafficDistributionRejected = "TrafficDistributionRejected" + ReasonTrafficDistributionNotPersisted = "TrafficDistributionNotPersisted" +) + +// knownTrafficDistributions are the values corev1.ServiceSpec.TrafficDistribution +// defines. Whether the apiserver accepts one depends on its version, which +// reconcileClientTrafficDistribution handles at write time. +var knownTrafficDistributions = []string{ + corev1.ServiceTrafficDistributionPreferClose, + corev1.ServiceTrafficDistributionPreferSameZone, + corev1.ServiceTrafficDistributionPreferSameNode, +} + +func clientServiceName(cluster *lll.EtcdCluster) string { + return cluster.Name + "-client" +} + +// desiredTrafficDistribution reads AnnClientServiceTrafficDistribution. +// It returns "" when the annotation is absent, and "" with valid=false +// when its value is not one of knownTrafficDistributions. +func desiredTrafficDistribution(cluster *lll.EtcdCluster) (value string, valid bool) { + v, ok := cluster.Annotations[AnnClientServiceTrafficDistribution] + if !ok { + return "", true + } + for _, known := range knownTrafficDistributions { + if v == known { + return v, true + } + } + return "", false +} + +// reconcileClientTrafficDistribution drives spec.trafficDistribution on the +// client Service from AnnClientServiceTrafficDistribution, via server-side +// apply under trafficDistributionFieldManager: +// +// - annotation set: the value is applied (forcing ownership) whenever the +// Service carries a different one; +// - annotation absent or invalid: the field manager's ownership is +// released, which clears the field unless another manager also owns +// it. A value set by someone else is never touched. +// +// A value the apiserver rejects (e.g. PreferSameZone before Kubernetes +// 1.34) or silently drops (no trafficDistribution support at all) is +// reported as a Warning Event and does not fail the reconcile. +func (r *EtcdClusterReconciler) reconcileClientTrafficDistribution(ctx context.Context, cluster *lll.EtcdCluster) error { + svc := &corev1.Service{} + if err := r.Get(ctx, types.NamespacedName{Namespace: cluster.Namespace, Name: clientServiceName(cluster)}, svc); err != nil { + // Just created and not in the cache yet: its watch event brings + // the cluster back here. + return client.IgnoreNotFound(err) + } + + want, valid := desiredTrafficDistribution(cluster) + if !valid { + r.event(cluster, corev1.EventTypeWarning, ReasonTrafficDistributionInvalid, fmt.Sprintf( + "annotation %s=%q is not one of %v; leaving trafficDistribution unset on Service %s", + AnnClientServiceTrafficDistribution, cluster.Annotations[AnnClientServiceTrafficDistribution], + knownTrafficDistributions, svc.Name)) + } + + if want == "" { + owned, err := ownsTrafficDistribution(svc) + if err != nil || !owned { + return err + } + _, err = r.applyTrafficDistribution(ctx, svc, "") + return err + } + + if svc.Spec.TrafficDistribution != nil && *svc.Spec.TrafficDistribution == want { + return nil + } + got, err := r.applyTrafficDistribution(ctx, svc, want) + switch { + case errors.IsInvalid(err) || errors.IsBadRequest(err): + r.event(cluster, corev1.EventTypeWarning, ReasonTrafficDistributionRejected, fmt.Sprintf( + "apiserver rejected trafficDistribution=%q on Service %s; leaving it unset: %v", want, svc.Name, err)) + owned, err := ownsTrafficDistribution(svc) + if err != nil || !owned { + return err + } + _, err = r.applyTrafficDistribution(ctx, svc, "") + return err + case err != nil: + return err + case got != want: + r.event(cluster, corev1.EventTypeWarning, ReasonTrafficDistributionNotPersisted, fmt.Sprintf( + "apiserver dropped trafficDistribution=%q on Service %s; it does not serve the field (Kubernetes 1.31+ required)", want, svc.Name)) + default: + log.FromContext(ctx).Info("set client Service trafficDistribution", "service", svc.Name, "trafficDistribution", want) + } + return nil +} + +// applyTrafficDistribution server-side-applies spec.trafficDistribution=value +// (or, for "", an empty configuration that releases the field) and returns +// the value the apiserver persisted. +func (r *EtcdClusterReconciler) applyTrafficDistribution(ctx context.Context, svc *corev1.Service, value string) (string, error) { + ac := corev1ac.Service(svc.Name, svc.Namespace) + if value != "" { + ac.WithSpec(corev1ac.ServiceSpec().WithTrafficDistribution(value)) + } + raw, err := runtime.DefaultUnstructuredConverter.ToUnstructured(ac) + if err != nil { + return "", err + } + obj := &unstructured.Unstructured{Object: raw} + if err := r.Patch(ctx, obj, client.Apply, client.FieldOwner(trafficDistributionFieldManager), client.ForceOwnership); err != nil { + return "", err + } + got, _, err := unstructured.NestedString(obj.Object, "spec", "trafficDistribution") + return got, err +} + +// ownsTrafficDistribution reports whether trafficDistributionFieldManager +// holds spec.trafficDistribution on svc. The managedFields scan keeps the +// common case (the annotation was never used) from parsing anything. +func ownsTrafficDistribution(svc *corev1.Service) (bool, error) { + applied := false + for _, mf := range svc.ManagedFields { + if mf.Manager == trafficDistributionFieldManager && mf.Operation == metav1.ManagedFieldsOperationApply { + applied = true + break + } + } + if !applied { + return false, nil + } + ac, err := corev1ac.ExtractService(svc, trafficDistributionFieldManager) + if err != nil { + return false, err + } + return ac.Spec != nil && ac.Spec.TrafficDistribution != nil, nil +} diff --git a/controllers/client_service_envtest_test.go b/controllers/client_service_envtest_test.go new file mode 100644 index 00000000..f79cdcb7 --- /dev/null +++ b/controllers/client_service_envtest_test.go @@ -0,0 +1,145 @@ +/* +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" + "testing" + + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/tools/record" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/envtest" + + lll "github.com/cozystack/etcd-operator/api/v1alpha2" +) + +// The ownership semantics of AnnClientServiceTrafficDistribution live in +// the apiserver's server-side apply, which the fake client doesn't +// implement, so these run against envtest. `make test` sets +// KUBEBUILDER_ASSETS; without it the test skips. +func TestClientServiceTrafficDistribution_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)} + 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() + + // Each subtest gets its own namespace and an in-memory cluster: the + // Services only need its name, UID and annotations. + setup := func(t *testing.T, ns string) (*lll.EtcdCluster, *EtcdClusterReconciler, *record.FakeRecorder) { + t.Helper() + if err := k8s.Create(ctx, &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: ns}}); err != nil { + t.Fatalf("create namespace: %v", err) + } + cluster := &lll.EtcdCluster{ObjectMeta: metav1.ObjectMeta{Name: "test", Namespace: ns, UID: types.UID(ns + "-uid")}} + rec := record.NewFakeRecorder(20) + return cluster, &EtcdClusterReconciler{Client: k8s, Scheme: testScheme(t), Recorder: rec}, rec + } + reconcile := func(t *testing.T, r *EtcdClusterReconciler, cluster *lll.EtcdCluster, ann *string) { + t.Helper() + if ann == nil { + delete(cluster.Annotations, AnnClientServiceTrafficDistribution) + } else { + cluster.Annotations = map[string]string{AnnClientServiceTrafficDistribution: *ann} + } + if err := r.ensureServices(ctx, cluster); err != nil { + t.Fatalf("ensureServices: %v", err) + } + } + // handSet writes trafficDistribution the way `kubectl edit` would: an + // update-style patch under a different field manager. + handSet := func(t *testing.T, ns string, v *string) { + t.Helper() + svc := &corev1.Service{} + if err := k8s.Get(ctx, types.NamespacedName{Namespace: ns, Name: "test-client"}, svc); err != nil { + t.Fatal(err) + } + orig := svc.DeepCopy() + svc.Spec.TrafficDistribution = v + if err := k8s.Patch(ctx, svc, client.MergeFrom(orig), client.FieldOwner("kubectl-edit")); err != nil { + t.Fatalf("hand-set trafficDistribution: %v", err) + } + } + preferClose := strPtr(corev1.ServiceTrafficDistributionPreferClose) + + t.Run("set, then remove clears", func(t *testing.T) { + cluster, r, _ := setup(t, "td-set-remove") + reconcile(t, r, cluster, preferClose) + assertTrafficDistribution(t, k8s, cluster.Namespace, "test-client", preferClose) + assertTrafficDistribution(t, k8s, cluster.Namespace, "test", nil) + + reconcile(t, r, cluster, nil) + assertTrafficDistribution(t, k8s, cluster.Namespace, "test-client", nil) + }) + + t.Run("drift is reconciled back", func(t *testing.T) { + cluster, r, _ := setup(t, "td-drift") + reconcile(t, r, cluster, preferClose) + handSet(t, cluster.Namespace, nil) + reconcile(t, r, cluster, preferClose) + assertTrafficDistribution(t, k8s, cluster.Namespace, "test-client", preferClose) + + // The hand edit took ownership away; the re-apply took it back, + // so removing the annotation still clears the field. + reconcile(t, r, cluster, nil) + assertTrafficDistribution(t, k8s, cluster.Namespace, "test-client", nil) + }) + + t.Run("hand-set value without annotation is left alone", func(t *testing.T) { + cluster, r, _ := setup(t, "td-hand-set") + reconcile(t, r, cluster, nil) + handSet(t, cluster.Namespace, preferClose) + reconcile(t, r, cluster, nil) + assertTrafficDistribution(t, k8s, cluster.Namespace, "test-client", preferClose) + }) + + t.Run("invalid value clears and warns", func(t *testing.T) { + cluster, r, rec := setup(t, "td-invalid") + reconcile(t, r, cluster, preferClose) + reconcile(t, r, cluster, strPtr("Anywhere")) + assertTrafficDistribution(t, k8s, cluster.Namespace, "test-client", nil) + requireEvent(t, drainEvents(rec), ReasonTrafficDistributionInvalid) + }) + + // PreferSameZone is served from Kubernetes 1.34 (beta, on by default). + // The envtest apiserver follows the k8s.io/api minor in go.mod, so this + // checks whichever outcome that version produces: updated in place, or + // rejected with an Event and the field cleared. + t.Run("update to PreferSameZone", func(t *testing.T) { + cluster, r, rec := setup(t, "td-update") + reconcile(t, r, cluster, preferClose) + reconcile(t, r, cluster, strPtr(corev1.ServiceTrafficDistributionPreferSameZone)) + + svc := &corev1.Service{} + if err := k8s.Get(ctx, types.NamespacedName{Namespace: cluster.Namespace, Name: "test-client"}, svc); err != nil { + t.Fatal(err) + } + if svc.Spec.TrafficDistribution == nil { + requireEvent(t, drainEvents(rec), ReasonTrafficDistributionRejected) + t.Logf("apiserver rejected PreferSameZone; field cleared as documented") + return + } + assertTrafficDistribution(t, k8s, cluster.Namespace, "test-client", strPtr(corev1.ServiceTrafficDistributionPreferSameZone)) + }) +} diff --git a/controllers/client_service_test.go b/controllers/client_service_test.go new file mode 100644 index 00000000..fafed8b0 --- /dev/null +++ b/controllers/client_service_test.go @@ -0,0 +1,245 @@ +/* +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" + "strings" + "testing" + + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/validation/field" + "k8s.io/client-go/tools/record" + "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" +) + +// clusterWithTrafficDistributionAnnotation returns a cluster carrying the +// annotation with value v; a nil v leaves it off. +func clusterWithTrafficDistributionAnnotation(v *string) *lll.EtcdCluster { + cluster := &lll.EtcdCluster{ + ObjectMeta: metav1.ObjectMeta{Name: "test", Namespace: "ns", UID: "test-uid"}, + } + if v != nil { + cluster.Annotations = map[string]string{AnnClientServiceTrafficDistribution: *v} + } + return cluster +} + +func strPtr(s string) *string { return &s } + +func assertTrafficDistribution(t *testing.T, c client.Client, ns, name string, want *string) { + t.Helper() + svc := &corev1.Service{} + if err := c.Get(context.Background(), types.NamespacedName{Namespace: ns, Name: name}, svc); err != nil { + t.Fatalf("Get Service %s: %v", name, err) + } + got := svc.Spec.TrafficDistribution + switch { + case want == nil && got != nil: + t.Fatalf("Service %s trafficDistribution = %q, want unset", name, *got) + case want != nil && got == nil: + t.Fatalf("Service %s trafficDistribution unset, want %q", name, *want) + case want != nil && *got != *want: + t.Fatalf("Service %s trafficDistribution = %q, want %q", name, *got, *want) + } +} + +// drainEvents returns every event the FakeRecorder has buffered. +func drainEvents(rec *record.FakeRecorder) []string { + var out []string + for { + select { + case e := <-rec.Events: + out = append(out, e) + default: + return out + } + } +} + +func requireEvent(t *testing.T, events []string, reason string) { + t.Helper() + for _, e := range events { + if strings.HasPrefix(e, corev1.EventTypeWarning+" "+reason+" ") { + return + } + } + t.Fatalf("no Warning %s event among %q", reason, events) +} + +// applyInterceptingClient is a fake client whose server-side-apply patches +// (unsupported by the fake) go to onApply instead; applies counts them. +func applyInterceptingClient(t *testing.T, applies *int, onApply func(obj client.Object) error, objs ...client.Object) client.Client { + t.Helper() + return fake.NewClientBuilder(). + WithScheme(testScheme(t)). + WithObjects(objs...). + WithInterceptorFuncs(interceptor.Funcs{ + Patch: func(ctx context.Context, c client.WithWatch, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { + if patch.Type() == types.ApplyPatchType { + *applies++ + return onApply(obj) + } + return c.Patch(ctx, obj, patch, opts...) + }, + }). + Build() +} + +func TestDesiredTrafficDistribution(t *testing.T) { + for _, tc := range []struct { + name string + ann *string + wantValue string + wantValid bool + }{ + {"absent", nil, "", true}, + {"PreferClose", strPtr("PreferClose"), "PreferClose", true}, + {"PreferSameZone", strPtr("PreferSameZone"), "PreferSameZone", true}, + {"PreferSameNode", strPtr("PreferSameNode"), "PreferSameNode", true}, + {"empty", strPtr(""), "", false}, + {"wrong case", strPtr("preferclose"), "", false}, + {"unknown", strPtr("Anywhere"), "", false}, + } { + t.Run(tc.name, func(t *testing.T) { + v, ok := desiredTrafficDistribution(clusterWithTrafficDistributionAnnotation(tc.ann)) + if v != tc.wantValue || ok != tc.wantValid { + t.Fatalf("got (%q, %v), want (%q, %v)", v, ok, tc.wantValue, tc.wantValid) + } + }) + } +} + +// Without the annotation and without prior ownership, the operator never +// issues an apply: clusters that don't use the annotation see no extra +// writes, and a value someone set by hand stays. +func TestEnsureServices_NoAnnotationNoApply(t *testing.T) { + cluster := clusterWithTrafficDistributionAnnotation(nil) + applies := 0 + c := applyInterceptingClient(t, &applies, func(client.Object) error { + t.Fatal("unexpected apply") + return nil + }) + r := &EtcdClusterReconciler{Client: c, Scheme: testScheme(t)} + + if err := r.ensureServices(context.Background(), cluster); err != nil { + t.Fatalf("ensureServices (create): %v", err) + } + // Hand-set value on the existing client Service. + svc := &corev1.Service{} + if err := c.Get(context.Background(), types.NamespacedName{Namespace: "ns", Name: "test-client"}, svc); err != nil { + t.Fatal(err) + } + svc.Spec.TrafficDistribution = strPtr(corev1.ServiceTrafficDistributionPreferClose) + if err := c.Update(context.Background(), svc); err != nil { + t.Fatal(err) + } + if err := r.ensureServices(context.Background(), cluster); err != nil { + t.Fatalf("ensureServices (existing): %v", err) + } + if applies != 0 { + t.Fatalf("applies = %d, want 0", applies) + } + assertTrafficDistribution(t, c, "ns", "test-client", strPtr(corev1.ServiceTrafficDistributionPreferClose)) +} + +// An annotation value Kubernetes doesn't define is reported and otherwise +// ignored: the reconcile carries on and nothing is applied. +func TestEnsureServices_InvalidAnnotation(t *testing.T) { + cluster := clusterWithTrafficDistributionAnnotation(strPtr("Anywhere")) + applies := 0 + c := applyInterceptingClient(t, &applies, func(client.Object) error { return nil }) + rec := record.NewFakeRecorder(10) + r := &EtcdClusterReconciler{Client: c, Scheme: testScheme(t), Recorder: rec} + + for range 2 { // create, then reconcile the existing Services + if err := r.ensureServices(context.Background(), cluster); err != nil { + t.Fatalf("ensureServices: %v", err) + } + } + if applies != 0 { + t.Fatalf("applies = %d, want 0", applies) + } + requireEvent(t, drainEvents(rec), ReasonTrafficDistributionInvalid) + assertTrafficDistribution(t, c, "ns", "test-client", nil) + assertTrafficDistribution(t, c, "ns", "test", nil) +} + +// A value the apiserver refuses (PreferSameZone before Kubernetes 1.34) is +// reported as an Event; ensureServices still succeeds so bootstrap, scaling +// and status updates for the cluster are not blocked. +func TestEnsureServices_ApiserverRejectsValue(t *testing.T) { + cluster := clusterWithTrafficDistributionAnnotation(strPtr(corev1.ServiceTrafficDistributionPreferSameZone)) + applies := 0 + c := applyInterceptingClient(t, &applies, func(obj client.Object) error { + return apierrors.NewInvalid(schema.GroupKind{Kind: "Service"}, obj.GetName(), field.ErrorList{ + field.NotSupported(field.NewPath("spec", "trafficDistribution"), "PreferSameZone", []string{"PreferClose"}), + }) + }) + rec := record.NewFakeRecorder(10) + r := &EtcdClusterReconciler{Client: c, Scheme: testScheme(t), Recorder: rec} + + for range 2 { + if err := r.ensureServices(context.Background(), cluster); err != nil { + t.Fatalf("ensureServices: %v", err) + } + } + if applies == 0 { + t.Fatal("expected an apply attempt") + } + requireEvent(t, drainEvents(rec), ReasonTrafficDistributionRejected) + assertTrafficDistribution(t, c, "ns", "test-client", nil) +} + +// An apiserver that doesn't serve the field accepts the write and drops the +// value; the operator reports that instead of claiming success. +func TestEnsureServices_ApiserverDropsValue(t *testing.T) { + cluster := clusterWithTrafficDistributionAnnotation(strPtr(corev1.ServiceTrafficDistributionPreferClose)) + applies := 0 + // The "response" is the applied object minus the field. + c := applyInterceptingClient(t, &applies, func(obj client.Object) error { + unstructured.RemoveNestedField(obj.(*unstructured.Unstructured).Object, "spec", "trafficDistribution") + return nil + }) + rec := record.NewFakeRecorder(10) + r := &EtcdClusterReconciler{Client: c, Scheme: testScheme(t), Recorder: rec} + + for range 2 { + if err := r.ensureServices(context.Background(), cluster); err != nil { + t.Fatalf("ensureServices: %v", err) + } + } + requireEvent(t, drainEvents(rec), ReasonTrafficDistributionNotPersisted) +} + +// Errors that aren't about the value (apiserver unavailable, throttling) +// fail the reconcile so it retries, like every other Service write. +func TestEnsureServices_TransientApplyErrorRetries(t *testing.T) { + cluster := clusterWithTrafficDistributionAnnotation(strPtr(corev1.ServiceTrafficDistributionPreferClose)) + applies := 0 + c := applyInterceptingClient(t, &applies, func(client.Object) error { + return apierrors.NewServiceUnavailable("try later") + }) + r := &EtcdClusterReconciler{Client: c, Scheme: testScheme(t)} + + if err := r.ensureServices(context.Background(), cluster); !apierrors.IsServiceUnavailable(err) { + t.Fatalf("ensureServices err = %v, want ServiceUnavailable", err) + } +} diff --git a/controllers/etcdcluster_controller.go b/controllers/etcdcluster_controller.go index 4d973f93..49464ae3 100644 --- a/controllers/etcdcluster_controller.go +++ b/controllers/etcdcluster_controller.go @@ -34,6 +34,7 @@ import ( "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/intstr" + "k8s.io/client-go/tools/record" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" @@ -56,6 +57,9 @@ type EtcdClusterReconciler struct { // production wiring uses DefaultEtcdClientFactory. EtcdClientFactory EtcdClientFactory + // Recorder emits events on the EtcdCluster. Tests may leave it nil. + Recorder record.EventRecorder + // CertManagerAvailable reports whether the cert-manager.io/v1 API // is registered on this cluster. Detected once at operator startup // via the discovery API. When false, clusters using @@ -84,6 +88,7 @@ type EtcdClusterReconciler struct { //+kubebuilder:rbac:groups="",resources=services,verbs=get;list;watch;create;update;patch //+kubebuilder:rbac:groups=policy,resources=poddisruptionbudgets,verbs=get;list;watch;create;update;patch;delete //+kubebuilder:rbac:groups="",resources=secrets,verbs=get;list;watch +//+kubebuilder:rbac:groups="",resources=events,verbs=create;patch // `delete` is intentionally omitted from the certificates verb list: // each Certificate is owned by the EtcdCluster via SetControllerReference, // and spec.tls is CEL-immutable post-create, so the only Certificate @@ -1917,12 +1922,15 @@ func (r *EtcdClusterReconciler) ensureServices(ctx context.Context, cluster *lll } // Client service — stable endpoint for applications. - return r.ensureService(ctx, cluster, cluster.Name+"-client", corev1.ServiceSpec{ + if err := r.ensureService(ctx, cluster, clientServiceName(cluster), corev1.ServiceSpec{ Selector: map[string]string{LabelCluster: cluster.Name}, Ports: []corev1.ServicePort{ {Name: "client", Port: 2379}, }, - }) + }); err != nil { + return err + } + return r.reconcileClientTrafficDistribution(ctx, cluster) } // ensureService creates the Service if absent and reconciles drift on the @@ -2013,6 +2021,12 @@ func upsertPortByName(ports *[]corev1.ServicePort, want corev1.ServicePort) bool return true } +func (r *EtcdClusterReconciler) event(obj client.Object, eventType, reason, msg string) { + if r.Recorder != nil { + r.Recorder.Event(obj, eventType, reason, msg) + } +} + // ── Manager wiring ─────────────────────────────────────────────────────── func (r *EtcdClusterReconciler) SetupWithManager(mgr ctrl.Manager) error { diff --git a/controllers/helpers.go b/controllers/helpers.go index 1202f78f..2792ad17 100644 --- a/controllers/helpers.go +++ b/controllers/helpers.go @@ -82,6 +82,12 @@ const ( // footgun. Superseded by an explicit spec.tls.peer.secretRef/certManager // (real mTLS wins; precedence lives in clusterPeerAutoTLS). AnnPeerAutoTLS = ReservedAnnotationPrefix + "peer-auto-tls" + + // AnnClientServiceTrafficDistribution, set on an EtcdCluster, is copied + // to spec.trafficDistribution of the "-client" Service. A + // stopgap until the setting gets a typed spec field; see + // reconcileClientTrafficDistribution. + AnnClientServiceTrafficDistribution = ReservedAnnotationPrefix + "client-service-traffic-distribution" ) // etcdDataDirRoot is the mount path of every member's data volume; --data-dir diff --git a/docs/concepts.md b/docs/concepts.md index e7ea03b4..54eefabc 100644 --- a/docs/concepts.md +++ b/docs/concepts.md @@ -281,6 +281,58 @@ Merge semantics: - **Apply-on-create.** Objects are stamped when created; editing the field re-stamps nothing retroactively. Newly-created objects (scale-up members and their PVCs, replacements) pick up the latest latched value. - **Latched.** A mid-flight edit only takes effect once the current `status.observed` target is reached, like every other latched field. +## Client Service traffic distribution + +The operator creates two Services per cluster: the headless `` (per-Pod DNS for peers, publishes not-ready addresses) and `-client` (a ClusterIP over ready members, the stable endpoint for applications). In a cluster stretched across zones, clients of `-client` can be kept on a member in their own zone by setting the Service's [`spec.trafficDistribution`](https://kubernetes.io/docs/concepts/services-networking/service/#traffic-distribution). The operator manages that field from an annotation on the `EtcdCluster`: + +```yaml +apiVersion: etcd-operator.cozystack.io/v1alpha2 +kind: EtcdCluster +metadata: + name: my-etcd + annotations: + etcd-operator.cozystack.io/client-service-traffic-distribution: PreferClose +spec: + replicas: 3 + topologySpreadConstraints: # one member per zone + - maxSkew: 1 + topologyKey: topology.kubernetes.io/zone + whenUnsatisfiable: DoNotSchedule + labelSelector: + matchLabels: + etcd-operator.cozystack.io/cluster: my-etcd +``` + +or, on an existing cluster, `kubectl annotate etcdcluster my-etcd etcd-operator.cozystack.io/client-service-traffic-distribution=PreferClose`. + +The annotation is a stopgap until the setting gets a typed field in the `EtcdCluster` spec. When that field lands it will take precedence, and the annotation will be deprecated. + +Accepted values, and the Kubernetes version whose apiserver serves each: + +| Value | Routes to | Kubernetes | +|---|---|---| +| `PreferClose` | a ready member in the client's zone | 1.31+ (beta, on by default; GA in 1.33). From 1.34 an alias of `PreferSameZone`. | +| `PreferSameZone` | a ready member in the client's zone | 1.34+ (beta, on by default) | +| `PreferSameNode` | a ready member on the client's node | 1.34+ (beta, on by default) | + +In every case the proxy falls back to all ready members when none matches. + +Semantics: + +- **Set or changed:** applied to `-client` on the next reconcile. It is not latched through `status.observed`: the Service carries no consensus state. +- **Removed:** the field is cleared from the Service. +- **Owned via server-side apply.** The operator writes only this one field, with field manager `etcd-operator-traffic-distribution`, and reconciles drift back while the annotation is set. Removing the annotation releases the field: it is cleared unless another field manager also set it. A value set on the Service by someone else while there is no annotation is left alone. +- **Invalid values never block the cluster.** A value outside the table (typos, wrong case, empty) leaves the field unset and emits a `Warning` Event `InvalidTrafficDistribution` on the `EtcdCluster`. A value the apiserver rejects (`PreferSameZone` / `PreferSameNode` before 1.34) emits `TrafficDistributionRejected` and also leaves the field unset. An apiserver that doesn't serve the field at all (before 1.31) drops it; the operator notices and emits `TrafficDistributionNotPersisted`. In all three cases the rest of the reconcile carries on. Check with `kubectl describe etcdcluster my-etcd`. +- **Client Service only.** The headless Service is never given a traffic distribution: it has no ClusterIP, so no proxy routes through it. + +Prerequisites for zone-local routing (`PreferClose` / `PreferSameZone`): + +- Every node that runs etcd members **or their clients** carries the [`topology.kubernetes.io/zone`](https://kubernetes.io/docs/reference/labels-annotations-taints/#topologykubernetesiozone) label. Cloud providers set it; on bare metal, label nodes yourself (`kubectl label node topology.kubernetes.io/zone=`). The EndpointSlice controller derives each endpoint's zone hint from its node's label, and kube-proxy ignores hints altogether (routing cluster-wide) when an endpoint has none or the client's node has no zone. +- Members are spread across the zones, e.g. with the `topologySpreadConstraints` above. A zone without a ready member sends its clients cluster-wide. +- The Service proxy honours EndpointSlice hints: kube-proxy does; for a kube-proxy replacement such as Cilium, check that its service-topology support is enabled. + +`PreferSameNode` needs no labels; it only helps when clients run on the same nodes as members. + ## etcd tuning options `spec.options` carries the etcd server tuning flags the operator renders onto each member's command line: diff --git a/main.go b/main.go index 6f82c523..0051bdfe 100644 --- a/main.go +++ b/main.go @@ -229,6 +229,7 @@ func main() { Scheme: mgr.GetScheme(), CertManagerAvailable: certManagerAvailable, ClusterDomain: clusterDomain, + Recorder: mgr.GetEventRecorderFor("etcd-operator"), }).SetupWithManager(mgr); err != nil { setupLog.Error(err, "unable to create controller", "controller", "EtcdCluster") os.Exit(1) From 6d847e72b5e8b80a49e711fdd03d49221d7d7fb9 Mon Sep 17 00:00:00 2001 From: Timofei Larkin Date: Sat, 17 Oct 2026 22:00:02 +0200 Subject: [PATCH 3/3] chore(github): make @lllamnyp the code owner of the API Changes to the API types and the generated CRDs now route review to @lllamnyp alone. Assisted-By: LLM Signed-off-by: Timofei Larkin --- .github/CODEOWNERS | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index 64ab8d85..da4b2a7d 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -7,3 +7,7 @@ # because the area is theirs, others because the role carries the repository. * @lllamnyp @androndo @sircthulhu + +# API surface: the Go types and the CRDs generated from them. +/api/ @lllamnyp +/charts/etcd-operator/crd-bases/ @lllamnyp