Skip to content

Add explicit imagePullSecrets field to MCPRegistry#5106

Merged
yrobla merged 6 commits intomainfrom
feat/5101-mcpregistry-image-pull-secrets
Apr 29, 2026
Merged

Add explicit imagePullSecrets field to MCPRegistry#5106
yrobla merged 6 commits intomainfrom
feat/5101-mcpregistry-image-pull-secrets

Conversation

@JAORMX
Copy link
Copy Markdown
Collaborator

@JAORMX JAORMX commented Apr 29, 2026

Summary

MCPRegistry workloads previously had no first-class image pull secrets field — users had to use the raw-JSON spec.podTemplateSpec escape hatch. That works (the existing integration test at cmd/thv-operator/test-integration/mcp-registry/deployment_update_test.go:63 proves it) but it has two drawbacks:

  1. Discoverability — a user reading the CRD schema sees no field for image pull secrets and won't know the workaround exists.
  2. Inconsistency — MCPServer has a first-class field at spec.resourceOverrides.proxyDeployment.imagePullSecrets. Anyone managing both kinds of resources has to remember the API differs.

This PR adds spec.imagePullSecrets to MCPRegistry. The field is []corev1.LocalObjectReference and propagates to:

  • the registry-api Deployment's PodSpec.ImagePullSecrets
  • the operator-managed ServiceAccount the registry API runs as

Placement choice. I chose spec.imagePullSecrets (flat, top-level on the spec) rather than introducing a spec.resourceOverrides.deployment.imagePullSecrets struct. MCPRegistry has no ResourceOverrides today, and adding one with a single field would be speculative scaffolding. If/when other override fields land, imagePullSecrets can be moved or kept as a top-level alias — but adding a struct now without a second field to justify it is unnecessary.

Precedence with podTemplateSpec. When both spec.imagePullSecrets and spec.podTemplateSpec.spec.imagePullSecrets are set:

  • spec.imagePullSecrets is applied first as a controller-generated default.
  • spec.podTemplateSpec.spec.imagePullSecrets is the user override and wins on overlap (the list is treated atomically — user's list replaces the default entirely on the Deployment).
  • The ServiceAccount is always populated from spec.imagePullSecrets; podTemplateSpec has no notion of a ServiceAccount.

This matches the existing podTemplateSpec merge semantics in MergePodTemplateSpecs and is documented in the field's Go doc comment. The legacy podTemplateSpec workaround keeps working unchanged.

Part of #5101

Type of change

  • New feature

Test plan

  • Unit tests (task test) — operator unit tests pass; the new TestBuildRegistryAPIDeployment_ImagePullSecrets covers explicit field, empty, podTemplateSpec override, podTemplateSpec without imagePullSecrets, and legacy podTemplateSpec-only behaviour. TestEnsureRBACResources_ImagePullSecrets verifies the ServiceAccount picks up the explicit field.
  • Linting (task lint-fix)
  • Manual testing — generated CRD YAML inspected (deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_mcpregistries.yaml) shows the new imagePullSecrets array property and the legacy podTemplateSpec field is unchanged.

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.

This adds a new optional field. Existing CRs continue to work; no migration required.

Does this introduce a user-facing change?

Yes. Users can now set spec.imagePullSecrets directly on MCPRegistry:

apiVersion: toolhive.stacklok.dev/v1beta1
kind: MCPRegistry
metadata:
  name: my-registry
spec:
  configYAML: |
    sources:
      - name: default
        format: toolhive
  imagePullSecrets:
    - name: regcred

The previous spec.podTemplateSpec workaround keeps working.

Special notes for reviewers

VirtualMCPServer has the same gap (tracked by #5101) but is being landed as a separate follow-up PR to keep each diff small and reviewable. The two CRDs do not share the deployment-build code path so the changes don't combine cleanly.

Generated with Claude Code

MCPRegistry workloads previously had no first-class image pull secret
field — users had to pass them through the raw-JSON podTemplateSpec
escape hatch. That works but is undiscoverable in the CRD schema and
inconsistent with MCPServer, which has spec.resourceOverrides.proxyDeployment.imagePullSecrets.

This adds spec.imagePullSecrets directly to MCPRegistrySpec rather
than introducing a nested ResourceOverrides struct, since this is the
only override field today and adding scaffolding for future fields
that may not arrive is speculative. The field is a
[]corev1.LocalObjectReference and propagates to:

  - the registry-api Deployment's PodSpec.ImagePullSecrets
  - the operator-managed ServiceAccount the registry API runs as

When both spec.imagePullSecrets and spec.podTemplateSpec.spec.imagePullSecrets
are set, podTemplateSpec wins on the Deployment (atomic replace); the
ServiceAccount always uses spec.imagePullSecrets, since podTemplateSpec
has no notion of a ServiceAccount. This matches the existing
podTemplateSpec merge semantics. The legacy podTemplateSpec workaround
keeps working unchanged.

Part of #5101

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added the size/M Medium PR: 300-599 lines changed label Apr 29, 2026
@JAORMX JAORMX self-assigned this Apr 29, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 29, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 67.03%. Comparing base (5c54e54) to head (a377203).
⚠️ Report is 5 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5106      +/-   ##
==========================================
+ Coverage   66.92%   67.03%   +0.11%     
==========================================
  Files         595      595              
  Lines       60086    60009      -77     
==========================================
+ Hits        40213    40230      +17     
+ Misses      16804    16725      -79     
+ Partials     3069     3054      -15     

☔ 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.

@github-actions github-actions Bot added size/M Medium PR: 300-599 lines changed and removed size/M Medium PR: 300-599 lines changed labels Apr 29, 2026
Copy link
Copy Markdown
Contributor

@yrobla yrobla left a comment

Choose a reason for hiding this comment

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

Review: Add spec.imagePullSecrets to MCPRegistrySpec

Clean, well-structured feature — the CRD field, RBAC wiring, and deployment propagation are all correct. Main concerns are a dead dedup path in WithImagePullSecrets, a subtle SA/Deployment behavioral split that users may not expect, and missing integration test coverage for the new field.


Summary of findings:

  • [warning] WithImagePullSecrets deduplication logic is dead code — defaultSpec always starts empty, so the existing map never has anything to deduplicate
  • [warning] No integration test exercises the new spec.imagePullSecrets field — existing integration tests only cover the podTemplateSpec path
  • [warning] SA/Deployment split-brain: using only spec.podTemplateSpec.spec.imagePullSecrets (the old way) populates the Deployment but leaves the SA without credentials; the docs mention this but the risk is subtle
  • [nit] MergePodTemplateSpecs docstring says "All other PodSpec fields: User values preserved as-is" — now stale since imagePullSecrets has explicit merge logic
  • [nit] WithImagePullSecrets function comment is misleading about what it does

Comment thread cmd/thv-operator/pkg/registryapi/podtemplatespec.go Outdated
Comment thread cmd/thv-operator/pkg/registryapi/podtemplatespec.go Outdated
Comment thread cmd/thv-operator/pkg/registryapi/podtemplatespec.go
Comment thread cmd/thv-operator/api/v1beta1/mcpregistry_types.go
Comment thread cmd/thv-operator/pkg/registryapi/deployment_test.go
JAORMX added 2 commits April 29, 2026 11:32
The map-based dedup logic in WithImagePullSecrets was a no-op:
the option only ever runs against an empty defaultSpec inside the
PodTemplateSpecBuilder, so the "existing" map was always empty.
Replace it with a direct assignment now that the merge step in
MergePodTemplateSpecs handles user-vs-default precedence atomically.

Also rewrite the misleading function comment to match what the code
actually does, and document the ImagePullSecrets atomic-replace rule
on MergePodTemplateSpecs's godoc bullet list.
Strengthen the godoc on MCPRegistrySpec.ImagePullSecrets to call out
that this field is the only path that reaches the operator-managed
ServiceAccount. The legacy spec.podTemplateSpec.spec.imagePullSecrets
populates the Deployment pod spec only and silently bypasses
ServiceAccount-level credential injection used by GKE Workload Identity,
OpenShift, EKS IRSA, and similar managed platforms.

Regenerate CRD manifests and CRD reference docs so the warning lands
in the user-facing documentation.
Comment thread cmd/thv-operator/pkg/registryapi/podtemplatespec.go
Existing integration coverage only exercised the legacy
spec.podTemplateSpec.spec.imagePullSecrets path, leaving the new
spec.imagePullSecrets field and the ServiceAccount path untested
end-to-end. Add four scenarios that go through the controller and a
real (envtest) apiserver:

  - spec.imagePullSecrets only -> Deployment pod spec carries the value
  - spec.imagePullSecrets only -> ServiceAccount carries the value
  - spec.imagePullSecrets updated -> both Deployment and SA rotate
  - spec.imagePullSecrets + podTemplateSpec.imagePullSecrets together ->
    Deployment uses the PodTemplateSpec override (atomic replacement),
    SA still tracks spec.imagePullSecrets

This locks in the documented merge rule and proves the SA propagation
that managed Kubernetes platforms (GKE Workload Identity, OpenShift,
EKS IRSA) depend on.
@github-actions github-actions Bot added size/M Medium PR: 300-599 lines changed and removed size/M Medium PR: 300-599 lines changed labels Apr 29, 2026
When spec.imagePullSecrets is set to an explicit empty slice (kustomize
or helm overlays often produce this unintentionally), the RBAC client
was wiping the ServiceAccount's existing ImagePullSecrets. On
OpenShift this destroys the auto-managed dockercfg entries and breaks
private-image pulls in a way that's silent and undiagnosable from the
field's docs.

Normalize the contract in pkg/kubernetes/rbac so an empty Secrets or
ImagePullSecrets slice is treated identically to nil — both leave the
existing SA fields untouched. Document the equivalence on
MCPRegistrySpec.ImagePullSecrets so users know recreating the resource
is the supported way to clear SA-level pull secrets.
@github-actions github-actions Bot added size/L Large PR: 600-999 lines changed and removed size/M Medium PR: 300-599 lines changed labels Apr 29, 2026
@yrobla yrobla merged commit e396150 into main Apr 29, 2026
102 of 104 checks passed
@yrobla yrobla deleted the feat/5101-mcpregistry-image-pull-secrets branch April 29, 2026 09:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/L Large PR: 600-999 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants