Skip to content

fix(operator): allow multiple keys of one Secret in spec.secrets - #6690

Merged
JAORMX merged 7 commits into
stacklok:mainfrom
lightsabit:fix/secrets-list-map-keys
Sep 30, 2026
Merged

JAORMX merged 7 commits into
stacklok:mainfrom
lightsabit:fix/secrets-list-map-keys

Conversation

@lightsabit

@lightsabit lightsabit commented Sep 18, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • spec.secrets entries are per-key references — name and key are both required, and the operator's env builder turns each entry into its own secretKeyRef env var — but the list was declared x-kubernetes-list-type: map keyed on [name] alone (MCPServer spec.secrets list-map-keys=[name] makes two keys from one Secret unrepresentable #6686). Two entries sharing a Secret name are therefore treated as duplicate list keys and rejected at admission with duplicate entries for key [name="..."].
  • Added key as a second +listMapKey on MCPServerSpec.Secrets and regenerated the CRDs. v1alpha1.MCPServerSpec is v1beta1.MCPServerSpec, so both served versions pick up the new merge keys from the single marker change.
  • Widening the key set only relaxes validation: existing objects already have unique names, so nothing changes for them.

Fixes #6686

Type of change

  • Bug fix

Test plan

  • Unit tests (task test) — via CI on this PR
  • E2E tests (task test-e2e) — via CI on this PR (E2E core + operator suites, kind 1.33–1.35)
  • Linting (task lint-fix) — via CI on this PR
  • Manual testing (describe below)

Manual:

  • The identical [name, key] schema change has been running on a live cluster as a Helm post-renderer over the released operator-crds chart since the 0.49.0 release, through 0.50.0 and now 0.51.3. Two MCPServers there — e.g. one that logs in with a username/password pair held in a single Secret — reference 5 and 2 keys of that Secret and reconcile correctly, with real tool calls round-tripping through the gateway.
  • The CRDs generated by this branch were verified directly with envtest (suite added in this PR): kube-apiserver loads the CRDs from deploy/charts/operator-crds/files/crds, and an MCPServer referencing two keys of one Secret is admitted through both create and server-side apply on v1beta1 and v1alpha1, with both entries stored. An exact (name, key) duplicate is still rejected. Run locally with envtest 1.31.0 (the version task operator-test-integration pins) and 1.35.0, via the same ginkgo invocation Operator CI uses.
  • Falsification check: with the unpatched main CRDs swapped in, the same suite fails exactly as MCPServer spec.secrets list-map-keys=[name] makes two keys from one Secret unrepresentable #6686 describes (duplicate entries for key [name="shared-secret"]), so the tests exercise the original validation failure rather than passing vacuously.

API Compatibility

  • This PR does not break the v1beta1 API, OR the api-break-allowed label is applied and the migration guidance is described above.

Touches the v1beta1 surface but is a relaxation only: adding key to the list-map keys accepts every previously valid object and newly accepts same-Secret multi-key lists that were previously rejected. The CRD Schema Compatibility check passes.

Changes

File Change
cmd/thv-operator/api/v1beta1/mcpserver_types.go +listMapKey=key on Secrets
deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_mcpservers.yaml Regenerated: key added to x-kubernetes-list-map-keys (both served versions)
deploy/charts/operator-crds/templates/toolhive.stacklok.dev_mcpservers.yaml Same regeneration, templated copy
cmd/thv-operator/test-integration/mcp-server-secrets-admission/secrets_admission_test.go envtest admission regression suite for #6686

Does this introduce a user-facing change?

Yes — MCPServer objects that reference multiple keys of the same Secret in spec.secrets now pass CRD validation instead of failing with duplicate entries for key [name="..."].

Special notes for reviewers

SecretRef entries are per-key -- name and key are both required -- and the
env builder maps each entry to its own secretKeyRef, so referencing two
keys of the same Secret is expected usage. The list was keyed on name
only, so the API server rejected the second entry as a duplicate.

Fixes stacklok#6686

Signed-off-by: lights <115397533+lightsabit@users.noreply.github.com>
@codecov

codecov Bot commented Sep 21, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.28%. Comparing base (61aaf42) to head (21a7f35).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6690      +/-   ##
==========================================
- Coverage   79.34%   79.28%   -0.07%     
==========================================
  Files         802      802              
  Lines       81211    81211              
==========================================
- Hits        64436    64385      -51     
- Misses      16770    16821      +51     
  Partials        5        5              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Sanskarzz Sanskarzz self-assigned this Sep 28, 2026
@Sanskarzz
Sanskarzz self-requested a review September 28, 2026 14:42

@Sanskarzz Sanskarzz left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lightsabit Thanks for raising the PR.
The (name, key) schema change looks right, and the generated CRDs cover both served versions. Could you add a regression test that applies an MCPServer with two keys from the same Secret through API admission? The existing Go test does not exercise the validation failure in #6686. Please also update the PR body using the repository template: include Fixes #6686, select Bug fix, list the checks you actually ran, and assess API compatibility. Your post-renderer test is useful context; please clarify whether you also tested the CRDs generated by this PR. The issue would benefit from a minimal failing manifest and the exact server-side dry-run command.

…) list-map keys

envtest suite installing the chart-shipped MCPServer CRDs and driving the
stacklok#6686 shape through API admission: two entries referencing different keys
of the same Secret are admitted (create and server-side apply, v1beta1 and
v1alpha1) and stored as two entries, while an exact (name, key) duplicate
is still rejected. Against the unpatched CRDs the two-key cases fail with
the stacklok#6686 error, so the suite guards the regression it was written for.
@lightsabit

Copy link
Copy Markdown
Contributor Author

@Sanskarzz thanks for the review — all four asks are addressed:

  • Regression test: added cmd/thv-operator/test-integration/mcp-server-secrets-admission (envtest; runs in Operator CI via task operator-test-integration). It applies an MCPServer referencing two keys of the same Secret through API admission — via both create and server-side apply, on v1beta1 and v1alpha1 — and asserts an exact (name, key) duplicate is still rejected. Verified it reproduces the MCPServer spec.secrets list-map-keys=[name] makes two keys from one Secret unrepresentable #6686 failure when run against the unpatched main CRDs.
  • PR body: rewritten with the template — Fixes #6686, Bug fix, the checks that ran, and the API-compat assessment.
  • CRDs generated by this PR: yes — the envtest suite installs the CRDs from this branch's deploy/charts/operator-crds/files/crds and exercises the multi-key shape against a real API server (details in the PR body).
  • Repro: minimal failing manifest + kubectl apply --server-side --dry-run=server command posted to MCPServer spec.secrets list-map-keys=[name] makes two keys from one Secret unrepresentable #6686.

…suite

t.Parallel in every test scope, per the paralleltest linter. The three
server-side-apply calls keep the deprecated client.Apply patch constant
under a scoped //nolint:staticcheck — the new Client.Apply API requires
generated ApplyConfiguration types, which this repo does not generate,
so the typed patch path is the only way to exercise SSA here.
TestForwarding_Logging_RealBackend timed out under CI load — the exact
flake tracked upstream in stacklok#5962 (same failure mode, previously deflaked
in stacklok#5963). Passes locally on this head; no code change, empty commit to
re-run the failed jobs.

@Sanskarzz Sanskarzz left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, @JAORMX Please take a final look.

@JAORMX JAORMX left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm. Thanks @Sanskarzz for the heads up

@JAORMX
JAORMX merged commit 47dbcab into stacklok:main Sep 30, 2026
47 of 48 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MCPServer spec.secrets list-map-keys=[name] makes two keys from one Secret unrepresentable

3 participants