8000 Fix inconsistent secret rendering in autogenerated `remote_servers` by realyota · Pull Request #1992 · Altinity/clickhouse-operator · GitHub
[go: up one dir, main page]
Skip to content

Fix inconsistent secret rendering in autogenerated remote_servers - #1992

Merged
sunsingerus merged 1 commit into
Altinity:0.27.1from
realyota:secret-fix
May 27, 2026
Merged

Fix inconsistent secret rendering in autogenerated remote_servers#1992
sunsingerus merged 1 commit into
Altinity:0.27.1from
realyota:secret-fix

Conversation

@realyota
@realyota realyota commented May 25, 2026
Copy link
Copy Markdown
Contributor

Fix for #1991

Important items to consider before making a Pull Request

Please check items PR complies to:

  • [X ] All commits in the PR are squashed. More info
  • [ X] The PR is made into dedicated next-release branch, not into master branch1. More info
  • [X ] The PR is signed. More info

--

1 If you feel your PR does not affect any Go-code or any testable functionality (for example, PR contains docs only or supplementary materials), PR can be made into master branch, but it has to be confirmed by project's maintainer.

@sunsingerus sunsingerus added the planned for review This feature is planned for review label May 26, 2026
@sunsingerus
sunsingerus commented May 27, 2026
Copy link
Copy Markdown
Collaborator

Review (4-reviewer synthesis): NEEDS-CHANGES (small) — merging with admin override; follow-up fixes landing in subsequent commit on 0.27.1

The fix is directionally correct and closes #1991 for the simple case. Helper extraction is a real DRY improvement. Merging because: zero conflicts, CI green, bug is real and the partial fix is better than the status quo. The follow-up commit will address items below.

Convergent findings (≥2 of 4 reviewers agree)

# Severity Finding File:line
C1 HIGH FindCluster(0) hardcoded — wrong under heterogeneous multi-cluster setups (different per-cluster secrets, secret.auto on cluster 0 only, selector excluding cluster 0). Sender signs with cluster-0's secret; receiver in cluster-1 has different secret → HMAC mismatch on Distributed queries via autogenerated all-* clusters. pkg/model/chi/config/generator.go:386
C2 HIGH Zero test coverage — pkg/model/chi/config/ has no *_test.go; tests/e2e/test_operator.py:4410-4419 exercises all-sharded only. The original bug existed precisely because per-site copy-paste drifted. pkg/model/chi/config/
C3 MEDIUM Cluster.GetSecret() not nil-receiver-safe; mirror GetSecurity() pattern. pkg/apis/clickhouse.altinity.com/v1/type_cluster.go:141
C4 MEDIUM XML-injection risk in <secret>%s</secret> interpolation (pre-existing class — #1578) amplified to 3 emission sites. pkg/model/chi/config/generator.go:262

Substantive (single-reviewer)

# Severity Finding
S1 MEDIUM Helper named addRemoteServersClusterSecret — file convention is get* (peers getRemoteServersReplica, getRemoteServerClusters). Rename to getRemoteServersClusterSecret.
S2 MEDIUM Param order (b, indent, secret) inverted vs peer getRemoteServersReplica(host, b). Flip to (secret, indent, b).
S3 LOW Deleted // Add secret to all-sharded from the first cluster if present carried real intent. Restore as one-liner; rename hoisted var cluster → firstCluster.
S4 MEDIUM (forward) 3-site call pattern is fragile — a future 4th autogenerated cluster will silently re-introduce the bug. Extract renderAutogenCluster(b, name, indent, renderShards func()).

Verified-clean

  1. No StatefulSet rollout triggered — IsConfigurationChangeRequiresReboot doesn't inspect remote_servers; existing CHIs upgrade with ConfigMap-only update + ClickHouse inotify reload, no pod restart.
  2. Zero blast radius for users without cluster.secret — ClusterSecret.Source() returns Unspecified; helper no-ops.
  3. No CRD / helm / generator-script regen needed (Go-only).
  4. No deploy/ YAML snapshots reference remote_servers.xml content.
  5. Existing CI checks green: Run Tests (2h46m) + Test Helm Chart (2m).

Follow-up commit on 0.27.1 will address: S1, S2, S3, C3, C4, C2 (unit test), plus optional normalizer warning for C1.

@sunsingerus
sunsingerus merged commit 6ec4e44 into Altinity:0.27.1 May 27, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

planned for review This feature is planned for review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@realyota @sunsingerus
0