Skip to content

Extract shared proxy env builder to eliminate deploymentForMCPServer/deploymentNeedsUpdate drift #5645

Description

@jerm-dro

Problem

MCPServerReconciler builds the proxy container's environment variables in two places that must be kept byte-for-byte identical:

  • deploymentForMCPServer — constructs the env for the live Deployment.
  • deploymentNeedsUpdate — re-derives the expected env slice and equality.Semantic.DeepEquals it against the live container to detect drift.

Both assemble the same nine sections in the same order:

# Section
1 OpenTelemetry (TelemetryConfigRef)
2 Token exchange (ExternalAuthConfigRef)
3 OBO secret (ExternalAuthConfigRef)
4 Webhook (WebhookConfigRef)
5 OIDC client secret (OIDCConfigRef inline)
6 ResourceOverrides.ProxyDeployment.Env
7 Redis password (SessionStorage)
8 THV_MCPSERVER_GENERATION (downward API)
9 Embedded auth server

Because DeepEqual on the env slice is order- and content-sensitive, any env var added to the builder but not mirrored into the drift check (or added at a mismatched position) causes the controller to detect false drift on every reconcile — bumping resourceVersion endlessly and destabilizing rolling updates.

This is a recurring bug class, not a one-off:

The code comments already acknowledge the fragility, with "Must mirror deploymentNeedsUpdate exactly" / "Must mirror deploymentForMCPServer exactly" annotations at multiple sections. When correctness depends on a human keeping two code paths identical by hand, the duplication is the root cause.

Proposed fix

Extract a single source of truth for the env sequence:

// proxyEnvVars assembles the proxy container env in canonical order.
// Single source of truth for both the builder (deploymentForMCPServer)
// and the drift check (deploymentNeedsUpdate), so the two can never diverge.
func (r *MCPServerReconciler) proxyEnvVars(ctx context.Context, m *mcpv1beta1.MCPServer) ([]corev1.EnvVar, error)

Both callers consume proxyEnvVars, making it structurally impossible to add an env var to one path without the other.

Caveat — preserve divergent error handling

The two paths intentionally differ on error handling (only the happy path is identical):

  • Builder: logs-and-continues on telemetry/token-exchange/OBO/OIDC fetch errors, producing a Deployment with that section omitted.
  • Drift check: returns true (assume needs-update) on those same errors.

The extracted helper should return ([]corev1.EnvVar, error) and let each caller keep its own error policy. The happy-path sequence — the only path where the drift bug actually fires, since valid config should produce matching env — is 100% identical and extracts cleanly.

Acceptance criteria

  • Single helper assembles the proxy env; both deploymentForMCPServer and deploymentNeedsUpdate call it.
  • Existing per-caller error semantics preserved (builder log-and-continue; drift check assume-update).
  • Idempotency test: build to steady state, reconcile again, assert RequeueAfter == 0 and unchanged resourceVersion, covering each env-producing config combination (telemetry, token exchange, OBO, webhook, OIDC, resource-override env, Redis, embedded auth).
  • The "Must mirror …" comments are removed (no longer applicable).

Follow-up to #5639.

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Fields

    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions