Skip to content

fix(config): extraConfig shallow merge silently wipes the generated storage block #1

Description

@tdakkota

renderConfig merges spec.extraConfig over the generated config with a shallow top-level copy:

// internal/controller/config.go:120
maps.Copy(cfg, extra)

maps.Copy replaces whole top-level values. Since the operator renders everything that matters under a single storage key — backend, dir, cluster.etcd, cluster.rf, cluster.shards_per_tenant, s3, and the engine tuning — any extraConfig that mentions storage at all discards all of it.

Reproduction

spec:
  replicas: 3
  etcd:
    endpoints: ["http://etcd:2379"]
  storage:
    backend: file
    dir: /var/lib/oteldb
    size: 100Gi
  extraConfig:
    storage:
      policy:
        recompress: {after: 3d, level: 19}

Rendered ConfigMap:

storage:
  policy:
    recompress: {after: 3d, level: 19}

backend, dir and the entire cluster block are gone.

Impact

This fails silently, and it fails in the worst possible direction. In oteldb, StorageConfig.setDefaults defaults an empty backend to memory, and an absent cluster block means single-node:

  • the durable file backend becomes the ephemeral in-memory backend — the PVC is mounted and unused, and all data is lost on every restart
  • the node leaves the cluster — no etcd, no ring, no replication, so each pod becomes an isolated island and queries return a fraction of the data

Nothing in the CR status or the pod logs flags this as a misconfiguration; both are legal configs on their own. extraConfig is documented as the escape hatch for anything the CRD doesn't model, so this is exactly the path a user takes when they need, say, a merge policy (see #3) — and it quietly destroys durability and clustering instead.

Proposal

Two parts, both worth doing:

  1. Deep-merge extraConfig — recursively merge maps, with the user's leaves winning over generated leaves. Then the example above adds storage.policy while preserving storage.backend/dir/cluster.
  2. Guard the keys the operator owns. Even with a deep merge, letting a user overwrite storage.cluster.etcd or storage.backend from extraConfig re-opens a smaller version of the same hole. Either reject an extraConfig that targets operator-owned paths with a clear validation error on the CR, or document them as reserved and strip them.

A regression test asserting that extraConfig: {storage: {policy: ...}} keeps backend, dir and cluster would pin it — internal/controller/builders_test.go already has the harness.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions