Repository navigation
Conversation
Surface missing CA ConfigMaps and keys on VirtualMCPServer resources through an advisory CABundleRefValidated condition. Keep the existing deployment and TLS behavior while making configuration failures visible. Refresh the condition when its ConfigMap changes or its reference is removed, and preserve prior observations during transient API failures. Fixes stacklok#5632 Signed-off-by: Sanskarzz <sanskar.gur@gmail.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6987 +/- ##
==========================================
+ Coverage 80.29% 80.34% +0.05%
==========================================
Files 847 850 +3
Lines 86142 86695 +553
==========================================
+ Hits 69165 69657 +492
- Misses 16972 17033 +61
Partials 5 5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| } else { | ||
| // Preserve the last observation on API errors and retry rather than | ||
| // misreporting a transient failure as a missing ConfigMap. | ||
| return fmt.Errorf("failed to get OIDC CA bundle ConfigMap %s/%s: %w", vmcp.Namespace, cmName, err) |
There was a problem hiding this comment.
Returning the error here means a transient ConfigMap read failure stops the whole reconcile: runValidations returns (false, err) and Reconcile exits before handleConfigRefs/reconcileResources, and without applying the status collected so far. So an advisory check can hold up a pending Deployment change. I think returning is defensible (operator.md says transient Get failures should be returned and not painted as NotFound, which is what MCPServer and MCPRemoteProxy do today), but the doc comment on this function says "without blocking reconciliation, matching MCPServer and MCPRemoteProxy", and the arch doc says it doesn't change deployment behavior, and neither is true on this path. Could you either keep the return and fix the comment/arch doc and call out the deviation from the siblings in the PR body, or log and continue with a comment explaining why (operator.md allows that for advisory checks)? Either is fine with me, I'd just like the code and docs to agree.
There was a problem hiding this comment.
Addressed in 73345c7. I kept the transient-error return so controller-runtime retries with backoff. The function comment and architecture/API docs now explain that missing or invalid references are advisory, while transient reads defer resource/status updates and preserve the existing resources and condition. The PR body also calls out this difference from the sibling validators.
| } | ||
| if err := mgr.GetFieldIndexer().IndexField( | ||
| context.Background(), &mcpv1beta1.VirtualMCPServer{}, | ||
| virtualMCPServerOIDCConfigRefIndex, indexVirtualMCPServerByOIDCConfigRef, |
There was a problem hiding this comment.
This is the same extractor that MCPOIDCConfigReconciler already registers on VirtualMCPServer as vmcpOIDCConfigRefIndexKey (mcpoidcconfig_controller.go:399), so we end up with two identical indexes over every vMCP. The comment says it's for when the OIDC config controller isn't registered, but app.go always registers it in the same manager, so that only happens in tests. Could the mapper query vmcpOIDCConfigRefIndexKey instead and this registration go away? The new MCPOIDCConfig -> ConfigMap index above is needed, that one makes sense.
There was a problem hiding this comment.
Addressed in 73345c7. Removed the duplicate index key and registration. The mapper and fake-client fixtures now reuse vmcpOIDCConfigRefIndexKey, which the OIDC controller registers before vMCP setup. The new ConfigMap-to-OIDC index remains.
| require.NoError(t, c.Get(t.Context(), key, current)) | ||
| statusManager := virtualmcpserverstatus.NewStatusManager(current) | ||
| err := r.validateCABundleRef(t.Context(), current, statusManager) | ||
| if tt.readError == "configmap" { |
There was a problem hiding this comment.
These cases call validateCABundleRef directly and then apply status themselves, so if the call were removed from runValidations they'd all still pass, and the path where Reconcile bails out without writing status isn't covered. Could you add one test that goes through runValidations (or Reconcile) with a ConfigMap Get interceptor returning an error, and checks the result and that the existing condition is left untouched?
There was a problem hiding this comment.
Added TestVirtualMCPServerReconcileCABundleReadErrorPreservesStatus in 73345c7. It calls Reconcile with a CA ConfigMap Get interceptor and asserts the original error reaches the caller, the existing full status and resource versions remain unchanged, and a pending Deployment replica change stays unapplied. It also checks that the CA read was reached and no resources were created. The full controller package passes with race detection.
| exist. It is `True` with reason `CABundleRefValid`, or `False` with reason | ||
| `CABundleRefNotFound` for a missing ConfigMap and `CABundleRefInvalid` for an | ||
| invalid reference or missing key. The key defaults to `ca.crt`. This condition is | ||
| advisory: it adds diagnostics without changing deployment or TLS behavior or |
There was a problem hiding this comment.
The arch doc is updated, but the user-facing list of condition types in docs/operator/virtualmcpserver-api.md (around line 487) doesn't mention CABundleRefValidated. Could you add a line there with the reasons? That list is already missing a few others (PodTemplateSpecValid, TelemetryConfigRefValidated, AuthServerConfigValidated), so adding those too would be nice but isn't required.
There was a problem hiding this comment.
Added CABundleRefValidated to the operator API reference in 73345c7, including CABundleRefValid, CABundleRefNotFound, and CABundleRefInvalid, the default key, cleanup, and transient-error behavior. I kept the unrelated missing condition entries out of this change.
Document that transient CA ConfigMap read failures pause resource and status updates for retry, while missing or invalid references remain advisory. Reuse the shared OIDC-reference index instead of maintaining an identical second index. Cover the actual reconcile error path and document the CA condition and reasons in the operator API reference. Signed-off-by: Sanskarzz <sanskar.gur@gmail.com>
Summary
VirtualMCPServer can generate an incoming OIDC CA volume for a missing ConfigMap or key without a specific status condition explaining the failure. Add advisory
CABundleRefValidateddiagnostics, for missing or invalid references, consistent with the other OIDC-capable workload types, and indexed ConfigMap watches so the condition recovers when the dependency changes.Fixes #5632
Type of change
Test plan
0 issues)task operator-generateandtask operator-manifests), with no generated-code or CRD-schema changesUnit coverage includes selected/default/custom/binaryData keys, missing and malformed references, namespace isolation, transient read errors, stale-condition cleanup, unrelated status preservation, unchanged-reconcile status-write idempotency, and a Reconcile-level transient-error regression asserting that persisted status and resources are untouched. The envtest scenario exercises ConfigMap creation, key removal/repair, deletion, and CA-reference removal through manager watches, while checking that a missing CA remains advisory and the Deployment is generated.
API Compatibility
v1beta1APIAdds condition/reason constants only; no new spec fields, schema tightening, or TLS changes. Missing or invalid CA references are advisory. Transient CA ConfigMap read errors preserve the previous condition and defer resource/status updates until retry, leaving existing deployed resources in place.
Changes
Does this introduce a user-facing change?
Yes. Previously, a missing incoming OIDC CA ConfigMap/key had no CA-specific condition on VirtualMCPServer. It now reports
CABundleRefValidated=Falsewith an actionable reason/message, changes toTruewhen repaired, and removes the condition when the CA reference is removed. A transient ConfigMap lookup failure preserves the prior condition and pauses pending resource/status updates until retry.Implementation plan
Implementation plan
Special notes for reviewers
The existing generic
StatusManager.SetConditionalready stampsObservedGeneration, so this avoids adding a redundant interface method/mock. The mapper reuses the vMCP OIDC-reference index already registered by the shared OIDC controller; only the ConfigMap-to-OIDC dependency requires a new index. Reference validation accepts keys indataorbinaryData, matching Kubernetes volume projection; it does not parse certificate contents.Unlike the older MCPServer/MCPRemoteProxy CA validators, this vMCP validator returns transient CA ConfigMap read errors instead of reporting them as missing resources. That gives controller-runtime a deterministic retry with backoff, but the current reconcile exits before resource reconciliation and the status flush.
Local lint is limited to the affected packages because of available memory; repository-wide CI remains necessary. No live-cluster E2E run was performed locally.