cluster-controller drops unknown SGCluster fields on status compare-and-swap after an operator upgrade

Problem to solve

After a new StackGres version is deployed, the cluster-controller silently removes fields of existing SGCluster resources that its own build does not know about.

The compare-and-swap itself is correct — the controller re-reads the resource and locks on the fetched resourceVersion. The loss happens in the body of the write: the SGCluster CRD POJOs are annotated @JsonIgnoreProperties(ignoreUnknown = true) and have no additionalProperties catch-all, so any field present on the server but absent from the running build's model is dropped on the deserialize/serialize round trip and disappears from the object that is PUT back.

Current behaviour

Note on paths. This was reported against main-1.18, and the paths below are that branch's. On main the helper and its per-kind subclasses have been renamed from *Scheduler to *Writer (AbstractCustomResourceWriter.java:58-80, ClusterWriter) and the call sites moved to ClusterControllerReconciliator.java:168 and :173, ManagedSqlReconciliator.java:300, PatroniBackupFailoverRestartReconciliator.java:112. The code is otherwise unchanged and the defect is the same on both branches.

All four SGCluster writes in the cluster-controller go through the same compare-and-swap helper:

  • cluster-controller/.../ClusterControllerReconciliator.java:161 — updateClusterPodStatus (status.arch, status.os, status.podStatuses)
  • cluster-controller/.../ClusterControllerReconciliator.java:166 — extension build sync (status.extensions[].build)
  • cluster-controller/.../ManagedSqlReconciliator.java:300 — status.managedSql
  • cluster-controller/.../PatroniBackupFailoverRestartReconciliator.java:112 — status.replicationInitializationFailedSgBackup

common/.../resource/AbstractCustomResourceScheduler.java:58-80:

@Override
public T update(T resource, Consumer<T> setter) {
  return KubernetesClientUtil.retryOnConflict(
      () -> {
        T resourceToUpdate = getCustomResourceEndpoints()
            .inNamespace(...).withName(...).get();          // <-- GET, deserialize: unknown fields lost here
        ...
        setter.accept(resourceToUpdate);
        return getCustomResourceEndpoints()
            .inNamespace(...).resource(resourceToUpdate)
            .lockResourceVersion(resourceToUpdate.getMetadata().getResourceVersion())
            .update();                                       // <-- full PUT replace of the whole object
      });
}

Two aggravating details:

  1. .update() is a full PUT replace of the entire SGCluster, not a status-only write. The SGCluster CRD declares only the scale subresource (common/src/main/resources/crds/SGCluster.yaml:52-57), there is no status subresource, so .updateStatus() is not usable and spec is inside the blast radius as well — a controller that only means to write status.podStatuses rewrites spec too.
  2. StackGresClusterSpec and StackGresClusterStatus are annotated @JsonInclude(JsonInclude.Include.NON_DEFAULT), so anything the model maps but leaves at its default value is also omitted from the serialized body.

The model:

// common/.../crd/sgcluster/StackGresCluster.java:28-45
@RegisterForReflection
@JsonInclude(JsonInclude.Include.NON_NULL)
@JsonIgnoreProperties(ignoreUnknown = true)      // <-- nothing captures unknown fields
...
public final class StackGresCluster
    extends CustomResource<StackGresClusterSpec, StackGresClusterStatus>
    implements Namespaced {

@JsonIgnoreProperties is present on 64 of the 79 classes in io.stackgres.common.crd.sgcluster. None of them has an additionalProperties map or a @JsonAnySetter, and the fabric8 base class io.fabric8.kubernetes.client.CustomResource does not provide one either (unlike the generated built-in models such as ObjectMeta, which do have @JsonAnySetter + additionalProperties and therefore round-trip unknown content safely).

When it triggers

The cluster-controller runs as a sidecar inside the cluster Pods, with its image pinned at StatefulSet-generation time to the operator's own build (operator/.../sidecars/controller/SingleReconciliationCycle.java:74, StackGresModules.CLUSTER_CONTROLLER.getImageName()). Upgrading the operator does not restart the existing Pods — they keep the previous sidecar image until an explicit restart (e.g. an SGDbOps restart/securityUpgrade).

That leaves a window in which:

  • the new operator has installed the new SGCluster CRD, so the API server accepts and persists the newly added fields (CRD structural-schema pruning keeps them precisely because the new CRD declares them), and the new operator writes them;
  • every still-running Pod hosts an older cluster-controller whose POJOs do not model those fields;
  • on the next reconciliation tick that old controller does GET → mutate status → PUT, and the new fields are gone.

The window can be long (it lasts until every Pod of every cluster has been restarted) and the controller reconciles continuously, so the loss is effectively immediate and repeated. Nothing reports it: the write succeeds, the CAS succeeds, no conflict, no event, no log line.

Proposal

Make the SGCluster model preserve fields it does not know about, so that a round trip through an older build is lossless:

  1. Add a @JsonAnySetter / @JsonAnyGetter backed additionalProperties map to the SGCluster CRD POJOs, mirroring what the fabric8 built-in models already do.

  2. Leave @JsonIgnoreProperties alone. An earlier revision of this issue claimed the ignoreUnknown = true had to be removed for the catch-all to be reachable. That is wrong: BeanDeserializerBase.handleUnknownVanilla consults the any-setter before _ignoreAllUnknown, so once a class has the catch-all the annotation is inert. Keeping it halves the change, leaves CrdIgnoreUnknownPropertiesTest and the shared ModelTestUtil helper untouched, and preserves the safety net for the classes that cannot take the catch-all (below). Properties named in a @JsonIgnoreProperties(value = {...}) list are still dropped before the any-setter, so the deliberate exclusions on ServiceSpec, SecretKeySelector and friends keep working.

  3. The REST API needs the same treatment. AbstractResourceService.update re-reads the custom resource, but the transformers then do transformation.setSpec(mapper.convertValue(dto.getSpec(), <CRD spec>.class)) — the whole spec subtree is replaced from the DTO, and anything the DTO model does not declare was already dropped on the way out. So the web console strips unknown fields on every save, through a parallel model (io.stackgres.apiweb.dto.*) that does not inherit from the CRD POJOs. The DTOs need the same catch-all, and the transformers that rebuild the spec field by field instead of converting it (PostgresConfigTransformer, PoolingConfigTransformer) have to carry it over explicitly.

Implementation notes

  • The whole model tree needs it, not just the root. Unknown fields only survive at the level where a catch-all exists, so covering only StackGresCluster would still drop a new spec.pods.* field. On main that is 279 classes under io.stackgres.common.crd.** (including .external) and 257 under io.stackgres.apiweb.dto.**. A single abstract base class that the classes without an extends clause inherit from keeps this to one line plus one import per file; classes extending another project class inherit it transitively, and those extending a generated fabric8 model already have the map.
  • @Buildable / sundrio: the generated XBuilder.build() calls setAdditionalProperties(Map) unconditionally, so that setter is mandatory and, because the models are built with lazyMapInitEnabled = false, it has to be null safe or every builder-constructed object ends up with a null map.
  • Jackson: the any-getter needs @JsonIgnore on it. Since jackson-databind#4775 (2.19) an any-getter also registers as a regular getter, so without it the map is additionally serialized under a literal additionalProperties key.
  • equals/hashCode: the models implement them explicitly and do not call super, so with a base class the map stays out of them. That is the wanted default — it keeps the operator's drift detection (DeployedResourcesCache) and ConfigInstaller behaving exactly as before.
  • Known gap: the 22 custom resource roots extend io.fabric8.kubernetes.client.CustomResource, which has no catch-all and cannot be changed, so unknown root-level keys are still dropped. A CR root only holds apiVersion, kind, metadata, spec and status, all of them covered by the models they point at, so this is inert in practice.
  • Side effect on the published schema: models that declare their own @JsonIgnoreProperties(value = {...}) list end up showing additionalProperties as a property of the OpenAPI schema the REST API publishes, which reaches sg-swagger.yaml and the admin UI tooltips. Measured against the base commit this goes from 8 schemas to 15. It is cosmetic — only the published schema is affected, not what is serialized — and the models extending a fabric8 model already behaved this way.
  • Secondary hardening (optional, larger change): enabling a status subresource on SGCluster would let the controller use .updateStatus() and confine status writes to status, removing spec from the blast radius entirely. It changes the API surface and needs a migration path, so it is worth evaluating separately.

Acceptance criteria

  • An SGCluster carrying a field that is present in the installed CRD but unknown to the running cluster-controller build survives a full cluster-controller reconciliation cycle unchanged.
  • Regression test covering the deserialize → mutate status → serialize round trip, asserting the unknown field is still present in the serialized body.
  • Upgrade test: deploy version N, create a cluster, upgrade the operator to N+1 with a CRD field added, and verify the field is not stripped from existing SGCluster resources before the Pods are restarted.

Related

#2939 (409s on resource updates), #3222 (unretried 409 on Pod patch), #2911 (reconciliation churn with ArgoCD), #520 (fabric8 and unrecognized CRD fields).

Edited by Matteo Melli