Skip to content

fix(operator): standardize secret references to custom SecretKeyRef#4580

Closed
mvanhorn wants to merge 1 commit into
stacklok:mainfrom
mvanhorn:osc/4540-standardize-secretkeyref
Closed

fix(operator): standardize secret references to custom SecretKeyRef#4580
mvanhorn wants to merge 1 commit into
stacklok:mainfrom
mvanhorn:osc/4540-standardize-secretkeyref

Conversation

@mvanhorn
Copy link
Copy Markdown
Contributor

@mvanhorn mvanhorn commented Apr 7, 2026

Summary

MCPRegistry was the only CRD using corev1.SecretKeySelector for secret references. The other 5 CRDs all use the custom SecretKeyRef type. This PR standardizes MCPRegistry to match, converting 5 fields before the API stabilizes.

Why this matters

The corev1.SecretKeySelector type carries an unused Optional *bool field that no controller code checks. The custom SecretKeyRef is simpler (just Name + Key, both required) and already used by 13+ fields across other CRDs. Standardizing now avoids a breaking change later.

Changes

  • mcpregistry_types.go: Convert 5 fields from corev1.SecretKeySelector to SecretKeyRef (preserving pointer semantics for optional fields)
  • secrets.go: Decouple GetValue from corev1 by accepting name and key as separate string parameters
  • pgpass.go, config.go, podtemplatespec.go: Update callers for the new signatures
  • Test files: Replace corev1.SecretKeySelector{LocalObjectReference: ...} constructors with simpler SecretKeyRef{Name: ..., Key: ...}

The JSON wire format is identical ({"name": "...", "key": "..."}) so existing manifests work without changes.

Testing

  • Updated all unit and integration test fixtures
  • Note: CRD manifests will need regeneration via task gen after merge (deepcopy and OpenAPI schema updates)

Fixes #4540

This contribution was developed with AI assistance (Codex).

Replace corev1.SecretKeySelector with the custom SecretKeyRef type
across all MCPRegistry CRD fields. This aligns MCPRegistry with the
5 other CRDs that already use SecretKeyRef, removing the inconsistent
corev1.SecretKeySelector usage before the API stabilizes.

Changes:
- Convert 5 fields in mcpregistry_types.go to SecretKeyRef
- Decouple secrets.GetValue from corev1 by accepting name/key strings
- Update all callers and test fixtures

The JSON wire format is identical for both types, so existing
manifests continue to work without changes.

Fixes stacklok#4540
@github-actions github-actions Bot added the size/S Small PR: 100-299 lines changed label Apr 7, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 7, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 68.92%. Comparing base (8c70343) to head (75c664b).
⚠️ Report is 243 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4580      +/-   ##
==========================================
+ Coverage   68.87%   68.92%   +0.05%     
==========================================
  Files         505      505              
  Lines       52380    52389       +9     
==========================================
+ Hits        36076    36111      +35     
+ Misses      13516    13490      -26     
  Partials     2788     2788              

☔ View full report in Codecov by Sentry.
📢 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.

@reyortiz3
Copy link
Copy Markdown
Contributor

hello @mvanhorn ! Thanks for submitting the PR to standardize the Secret references. With the recent migration to beta, most of these fields were converted to SecretKeyRef.

Out of those, only PGPassSecretRef in v1beta1/mcpregistry_types.go remains. @ChrisJBurns , does thin one need to be converted to SecretKeyRef too?

@mvanhorn
Copy link
Copy Markdown
Contributor Author

Thanks for the update @reyortiz3 - confirmed on upstream/main: only PGPassSecretRef in cmd/thv-operator/api/v1beta1/mcpregistry_types.go still uses corev1.SecretKeySelector (plus the generated zz_generated.deepcopy.go counterpart). Everything else this PR touched is already on SecretKeyRef.

Happy to rescope this PR to just that one field once @ChrisJBurns weighs in on whether it should be converted. If the answer is "leave it as SecretKeySelector for libpq compatibility reasons", I'll close this PR instead.

@ChrisJBurns
Copy link
Copy Markdown
Collaborator

hey @mvanhorn yeah I think we may want to close this PR. I raised it initially and I think sticking to the selector is probably a good shout because its standardising using the corev1 etc. sorry for the hassle on this!

@mvanhorn
Copy link
Copy Markdown
Contributor Author

Makes sense @ChrisJBurns - with most fields already converted to SecretKeyRef in beta and you preferring to stay with the selector, the remaining work here is no-op. Closing.

@mvanhorn mvanhorn closed this Apr 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/S Small PR: 100-299 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Standardize secret references to use custom SecretKeyRef across all CRDs

3 participants