Fix: harden controllers against stale objects, orphaned resources, an…#424
Merged
pdettori merged 1 commit intoJun 10, 2026
Merged
Conversation
Ygnas
changed the title
Ygnas
force-pushed
the
fix/controller-robustness-improvements
branch
2 times, most recently
from
June 10, 2026 12:44
7ba660d to
6617e0d
Compare
Ygnas
marked this pull request as ready for review
June 10, 2026 13:41
Contributor
Author
|
cc: @varshaprasad96 |
varshaprasad96
left a comment
varshaprasad96
left a comment
Contributor
There was a problem hiding this comment.
/lgtm
No blocking issues. The code changes are correct, well-tested (unit + integration pass), and follow established patterns in the codebase. The E2E failure is pre-existing and addressed by #423. This is ready to merge.
varshaprasad96
approved these changes
Jun 10, 2026
…d non-determinism Address high-severity findings from static analysis and adversarial review across multiple controllers: OwnerReference on bootstrap resources: - otel.go: set OwnerReference (operator Deployment) on ingress CA and collector ConfigMaps to prevent orphaning on uninstall - sharedtrust_controller.go: set OwnerReference (intermediate cert Secret) on cacerts Secrets for proper GC on cert rotation - agentruntime_controller.go: document why shared namespace ConfigMaps intentionally omit OwnerReference (multi-tenant safety) Status update conflict resilience: - agentruntime_controller.go: wrap all four bare Status().Update calls in retry.RetryOnConflict with fresh Get, matching the established pattern in agentcard_controller.go - Set conditions individually on the fresh object inside each retry closure instead of bulk-copying rt.Status.Conditions Unnecessary update elimination: - mlflow_controller.go: track annotation changes alongside env var changes; skip r.Update when nothing actually changed Signature verification correctness: - verifier.go: removeEmptyFields now recurses into array elements, cleaning nested maps and stripping empty strings to ensure canonical JSON consistency between signer and verifier Pod security: - kind-with-registry.yaml: add runAsNonRoot, seccompProfile, and drop ALL capabilities on dev registry Deployment Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Ignas Baranauskas <ibaranau@redhat.com>
Ygnas
force-pushed
the
fix/controller-robustness-improvements
branch
from
June 10, 2026 18:17
6617e0d to
bcde375
Compare
Member
|
LGTM - e2e CI has been fixed so finally all green |
pdettori
approved these changes
Jun 10, 2026
This was referenced Jun 15, 2026
kevincogan
pushed a commit
to kevincogan/kagenti-operator
that referenced
this pull request
Jun 18, 2026
The webhook creates the spire-agent-socket POD-level volume (CSI driver csi.spiffe.io) when spire-enabled is true, but never mounts it into either authbridge container. The bundled spiffe-helper inside the combined authbridge images dials unix:///spiffe-workload-api/spire-agent.sock (per defaults.go SocketPath) and sits in a silent dial-loop forever because the directory doesn't exist in the container. Hidden until now because no in-production workload actually consumes SPIRE-issued material: - token-exchange uses /shared/client-secret.txt (Keycloak client_credentials), not JWT-SVIDs. - jwt-validation uses JWKS, not SPIRE. - /opt/svid*.pem and /opt/jwt_svid.token sit empty in every spire-enabled pod (verified — weather-agent ran 3+ days with kagenti.io/spire=enabled and /opt empty the whole time). kagenti-extensions PR rossoctl#424 (mTLS between authbridge sidecars via SPIRE X.509 SVIDs) is the first workload that actually consumes SPIRE material. Without this fix, the mTLS feature can't function because tls.ServerConfig errors at construction when /opt/svid.pem is missing. This completes the bundled-spiffe-helper migration alluded to in injection_decision.go's TODO — when the standalone spiffe-helper sidecar was removed and the helper moved into the combined authbridge images, the volume mount got lost. Adds the mount to both: - BuildEnvoyProxyContainerWithSpireOption (envoy-sidecar mode) - BuildProxySidecarContainerWithPorts (proxy-sidecar / lite modes) Mount path /spiffe-workload-api matches the SocketPath in defaults.go. ReadOnly is correct (the CSI volume itself is already read-only at the Pod level). Tests added: TestBuildEnvoyProxyContainer_SpireEnabled_HasSocketMount and TestBuildProxySidecarContainer_SpireEnabled_HasSocketMount lock the mount presence + path + ReadOnly when SPIRE is on; the matching SpireDisabled tests assert the mount is absent when SPIRE is off. Verified: all unit tests in internal/webhook/injector/ pass. Operator image rebuilt and rollout-restarted; mtls demo pods now have the new mount in their spec (kubelet attempts to mount, which is the correct change). Full end-to-end mTLS demo verification deferred to the kagenti-extensions PR rossoctl#424 reviewer once the SPIRE agent + CSI driver are restored on the test cluster — those were absent during local verification, blocking the mount-success path but not the operator's own correctness. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
kevincogan
pushed a commit
to kevincogan/kagenti-operator
that referenced
this pull request
Jun 18, 2026
Introduces a MTLSMode field on AgentRuntimeSpec and plumbs it through the existing CR > namespace > default resolution chain so the admission webhook can render an mtls: block into the per-agent authbridge ConfigMap. With this in place, kagenti-extensions PR rossoctl#424 (mTLS between authbridge sidecars on the proxy-sidecar / lite paths) can be enabled declaratively per workload instead of by hand-editing the namespace ConfigMap. Behavior: * MTLSMode enum: disabled (default) | permissive | strict. Resolution: AgentRuntime CR > namespace authbridge-runtime-config mtls.mode > "disabled". Mirrors the existing AuthBridgeMode chain. * Per-agent ConfigMap renders top-level mtls: {mode: <value>} when resolved mode is non-disabled. Cert paths are intentionally not emitted — they default to authbridge's bundled-spiffe-helper convention (/opt/svid.pem, /opt/svid_key.pem, /opt/svid_bundle.pem), so surfacing them here would couple the operator to authbridge's internal layout for no benefit. Stale mtls blocks in the base YAML are scrubbed when toggling back to disabled. * mtlsMode != disabled implicitly requires SPIRE because the bundled spiffe-helper writes the X.509 SVID files mTLS reads. The pod mutator auto-enables SPIRE for the workload when mtlsMode is set — operators don't need to add the spiffe-helper-inject label separately. The operator does not mutate the namespace authbridge-config ConfigMap (which would race with the kagenti meta-chart for ownership); it just changes the in-memory injection decision. * The AgentRuntime validating webhook rejects mtlsMode != disabled when authBridgeMode is envoy-sidecar. Envoy SDS isn't currently configured by the kagenti envoy-config, so accepting the combo would silently produce a workload running plaintext on the wire while the user believed they had strict mTLS. The error message points to the supported modes (proxy-sidecar / lite) and flags this as a current limitation, not a permanent one — when the envoy-config gains SDS support, the rejection just goes away. * Spec changes to AuthBridgeMode and MTLSMode now flow into the AgentRuntime controller's resolved-config hash. Editing either field re-stamps the kagenti.io/config-hash pod-template annotation and the Deployment rolls automatically. (Pre-existing gap fixed along the way: AuthBridgeMode changes also weren't triggering rollouts before this commit.) Out of scope: * envoy-sidecar mTLS via Envoy SDS — separate piece of work in the kagenti meta-chart's envoy-config template. * Namespace-level authbridge-runtime-config edits triggering rollouts. Today the namespace ConfigMap is consumed by the admission webhook only; namespace-level mtls.mode changes won't auto-roll existing workloads. Operators editing the namespace config should kubectl rollout restart manually until that's addressed. * Chart-side CRD at charts/kagenti-operator/crds/ is intentionally not touched. That copy has been drifting from config/crd/bases/ since commit 677f63d (predates `lite` mode entirely). Recent CRD-touching commits (4eb62af, 21a2258) follow the same "config is source of truth" pattern; this PR matches. Cleaning up the chart-side drift is a separate concern. Tests added: * ExtractMTLSMode parser cases (empty, missing block, unknown mode, malformed YAML, mtls coexisting with other top-level blocks). * ensurePerAgentConfigMap: strict and permissive emit the block; disabled and "" omit the block; stale-block scrubbing on toggle back to disabled. * Validating webhook: 11-case matrix of (mode, mtls) combinations on Create and Update; envoy-sidecar + non-disabled rejected, error message points to the follow-up. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Hai Huang <huang195@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Address high-severity findings from static analysis and adversarial review across multiple controllers:
OwnerReference on bootstrap resources:
Status update conflict resilience:
Unnecessary update elimination:
Signature verification correctness:
Pod security:
Assisted-By: Claude (Anthropic AI) noreply@anthropic.com
Related issue(s)
(Optional) Testing Instructions
Fixes #