Skip to content

Consolidate sidecar injection into shared webhook logic#6

Merged
pdettori merged 6 commits into
rossoctl:mainfrom
cwiklik:add-agent-cr-webhook
Oct 27, 2025
Merged

Consolidate sidecar injection into shared webhook logic#6
pdettori merged 6 commits into
rossoctl:mainfrom
cwiklik:add-agent-cr-webhook

Conversation

@cwiklik

@cwiklik cwiklik commented Oct 27, 2025

Copy link
Copy Markdown
Collaborator

Description

This PR consolidates duplicate sidecar injection code by moving Agent CR injection from the kagenti operator into the kagenti-extensions webhook, where it shares common code with the existing MCPServer CR injection logic.

Problem Statement

Previously, sidecar injection logic existed in two places:

  • kagenti operator: Directly injected sidecars into Agent CRs during reconciliation

  • kagenti-extensions webhook: Injected sidecars into MCPServer CRs via admission webhook

This duplication created maintenance overhead, increased the risk of implementation drift, and made it harder to ensure consistent behavior across both resource types.

Solution

This PR implements a unified webhook-based approach:

  • Created shared injection library (pkg/injection) containing common sidecar injection logic

  • Refactored MCPServer webhook to use the shared library

  • Added new Agent webhook that uses the same shared library

Both Agent and MCPServer CRs now receive identical sidecar injection through a consistent, maintainable codebase.

Flexible Injection Control

The webhook supports flexible injection control via namespace labels and annotations, similar to Istio's sidecar injection pattern. This provides fine-grained control over which resources receive sidecar injection. In deployments where the namespace is not specifically labeled to force sidecar injection, the Agent/MCPServer can opt-in via an annotation:

apiVersion: toolhive.stacklok.dev/v1alpha1
kind: MCPServer
metadata:
  name: force-injection-server
  namespace: other-namespace  # No label
  annotations:
    kagenti.dev/inject: "true"  # Explicit opt-in

If a namespace is labeled to force sidecar injection, the Agent/MCPServer can opt-out via an annotation:

apiVersion: v1
kind: Namespace
metadata:
  name: my-apps
  labels:
    kagenti-enabled: "true"  # All Agent/MCPServer CRs in this namespace get sidecars
apiVersion: toolhive.stacklok.dev/v1alpha1
kind: MCPServer
metadata:
  name: skip-injection-server
  namespace:   my-apps # All Agent/MCPServer CRs in this namespace get sidecars
  annotations:
    kagenti.dev/inject: "false"  # Explicit opt-out

Fixes #2

Jarek Cwiklik added 5 commits October 17, 2025 19:33
Signed-off-by: Jarek Cwiklik <cwiklik@mac.lan>
Signed-off-by: Jarek Cwiklik <cwiklik@mac.lan>
Signed-off-by: Jarek Cwiklik <cwiklik@mac.lan>
Signed-off-by: Jarek Cwiklik <cwiklik@mac.lan>
Signed-off-by: Jarek Cwiklik <cwiklik@mac.lan>
@cwiklik
cwiklik requested review from mrsabath and pdettori October 27, 2025 17:59
@cwiklik cwiklik changed the title Consolidate sidecar injection into shared webhook logicAdd agent cr webhook Consolidate sidecar injection into shared webhook logic Oct 27, 2025

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull Request Overview

This PR consolidates duplicate sidecar injection logic by creating a shared mutation library used by both MCPServer and Agent webhooks. The changes eliminate code duplication between the kagenti operator and the webhook, implementing a unified webhook-based approach with Istio-style namespace injection controls.

Key Changes:

  • Introduced shared pkg/injection library containing common pod mutation logic
  • Refactored MCPServer webhook to use the new shared library
  • Added new Agent webhook using the same shared mutation engine

Reviewed Changes

Copilot reviewed 64 out of 86 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
kagenti-webhook/internal/webhook/injector/pod_mutator.go Core mutation engine implementing namespace-aware injection logic
kagenti-webhook/internal/webhook/injector/container_builder.go Builds standardized sidecar containers (spiffe-helper, client-registration)
kagenti-webhook/internal/webhook/injector/volume_builder.go Creates required volumes for sidecar containers
kagenti-webhook/internal/webhook/injector/namespace_checker.go Checks namespace labels/annotations for injection enablement
kagenti-webhook/internal/webhook/v1alpha1/mcpserver_webhook.go Refactored MCPServer webhook using shared mutator
kagenti-webhook/internal/webhook/v1alpha1/agent_webhook.go New Agent webhook implementation using shared mutator
kagenti-webhook/cmd/main.go Updated to instantiate shared mutator and register both webhooks
kagenti-webhook/go.mod Added kagenti-operator dependency and k8s.io/utils
toolhive-webhook/* Removed old toolhive-webhook files (replaced by kagenti-webhook)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread kagenti-webhook/internal/webhook/injector/pod_mutator.go Outdated
Signed-off-by: Jarek Cwiklik <cwiklik@mac.lan>

@pdettori pdettori left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

/LGTM

@pdettori
pdettori merged commit 83e3b7c into rossoctl:main Oct 27, 2025
1 check passed
huang195 added a commit to huang195/kagenti-extensions that referenced this pull request May 7, 2026
Must-fix:
- rossoctl#1+rossoctl#7: Plugins spawned credential watchers with initCtx (60s), so a
  slow client-registration orphaned the plugin after Start's deadline
  and OnRequest returned 503 for the pod's lifetime. Watchers now run
  on process-lifetime contexts created in Init via context.Background()
  + context.WithCancel; the cancel func is stored on the plugin and
  fired in Shutdown. Both JWTValidation and TokenExchange now
  implement pipeline.Shutdowner. Removed the orphaned
  context.WithCancel at main.go:89 that looked like it was meant to
  be that context.

- rossoctl#2: testutil.go moved out of the plugins package into a dedicated
  sub-package authlib/plugins/plugintesting/ so it can't be imported
  by the production binary. Rewrote the helpers as stub plugin
  adapters that mimic jwt-validation / token-exchange OnRequest but
  don't touch plugin internals — listener tests inject an
  *auth.Auth via plugintesting.NewJWTValidation / NewTokenExchange,
  wire through plugintesting.BuildPipeline, and never see the real
  Configure path. All four listener tests updated.

- rossoctl#11: Removed identity sub-validation that applyDefaults made
  unreachable; class-of-error compensated by logging a boot-time
  WARN when Configure's best-effort ReadCredentialFile fails (see
  also rossoctl#12).

Suggestions:
- rossoctl#3: pollCredentials no longer mutates p.cfg. Reads credential
  values into locals and feeds them through auth.UpdateIdentity. cfg
  is now immutable after Configure returns. Added a
  buildClientAuthFrom helper that takes explicit args so the
  goroutine doesn't read p.cfg either.

- rossoctl#4: Rewrote the NoTokenPolicy doc comment to state the current
  behavior ("deny in all modes; operators who need allow or
  client-credentials must set it explicitly") instead of referring
  to the removed config.NoTokenPolicyForMode.

- rossoctl#5: Deleted unused DefaultInboundPlugins/DefaultOutboundPlugins
  and the whole defaults.go file. Fixed the stale PipelineConfig
  docstring in config.go that said "defaults kick in when
  pipeline section is omitted" — they don't.

- rossoctl#6: PluginEntry.UnmarshalYAML normalizes explicit `config: null`
  (and any other !!null-tagged scalar under `config:`) to a nil
  RawMessage, so Build's "plugin does not accept configuration"
  gate doesn't fire spuriously on the four bytes "null".

- rossoctl#12: Deleted unreachable validate() branches: static-mode's
  audience emptiness check, spiffe's missing-path check,
  client-secret's missing-id / missing-secret checks. applyDefaults
  fills the matching field in every case; the branches were dead
  code that obscured what the plugin actually validated.

Nits:
- rossoctl#8: Renamed TestPluginEntry_IDDefaultsToName →
  TestPluginEntry_IDOmittedStaysEmpty.

- rossoctl#9: TestBuild_ConfigForNonConfigurablePlugin now asserts the error
  text contains "does not accept configuration" — operator-facing
  contract.

- rossoctl#10: Renamed TestConfigurable_ErrorAborts →
  TestConfigurable_ErrorPropagates and noted in the comment that
  Build-level abort coverage lives in plugins_test.go.

- rossoctl#13: Documented on jwtValidationConfig.AudienceFile that empty-
  string is treated as "unset" and triggers the default. Operators
  who want no file backing must supply an explicit Audience.

- rossoctl#14: Added TestAuthbridgeCombinedYAML_Loads — parses the in-repo
  authbridge-combined.yaml with env vars set, asserts both inbound
  and outbound pipelines Build cleanly. A future rename of any
  default constant that the YAML relies on breaks this test in CI
  rather than silently shipping a broken image.

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Hai Huang <huang195@gmail.com>
huang195 added a commit to huang195/kagenti-extensions that referenced this pull request May 7, 2026
rossoctl#1 atomic.Pointer for bgCancel:
Both JWTValidation and TokenExchange now hold bgCancel in an
atomic.Pointer[context.CancelFunc] rather than a plain field. Today's
callers (pipeline.Start / pipeline.Stop) are serialized so there's no
actual race, but a future caller invoking Shutdown from a different
goroutine would have been latent-racing the Init assignment. Shutdown
uses Swap(nil) so it's also idempotent.

-race passes clean on pipeline, plugins, config packages.

rossoctl#2 Widen NoTokenPolicy WARN:
Previously only fired when default_policy was "exchange". That missed
envoy-sidecar deployers relying on the old client-credentials default
for routed hosts. WARN now fires whenever no_token_policy was
defaulted, with the message listing all three pre-PR mode-specific
defaults so operators can see which one their deployment used to get.

rossoctl#3 audience_file WARN mentions defaulted origin:
When audience_file was defaulted to /shared/client-id.txt (Kagenti
convention) and the file isn't readable, the new WARN spells that
out — so non-Kagenti deployers notice their config didn't put the
plugin where it thinks it is. Explicit-path case keeps the shorter
original message.

rossoctl#4 Drop client_id from credential-loaded log:
OAuth spec treats client_id as non-secret, but some operators treat
it as sensitive. The signal (credentials loaded) doesn't need the
identifier.

rossoctl#5 TODO markers for /stats and Ready():
Grep-able TODO(follow-up) comments at the stub sites in main.go with
sketch notes for the future work. These sit next to the existing
behavior comments so anyone reading those surfaces finds the
follow-up plan.

rossoctl#6 Pipeline.Start failure unwind:
On Init error at plugin N, Start now calls Shutdown on plugins
[0..N-1] in reverse order before returning the error. Prevents leaking
goroutines spawned by earlier plugins' Init when a downstream peer
rejects its config at boot. Not a correctness bug for production
(log.Fatalf then exit, kernel cleans up) but matters for any embedded
or multi-tenant caller.

New test TestPipelineStart_UnwindsSuccessfulInitsOnFailure asserts
the reverse-order Shutdown on the subset that Init'd, and that plugins
after the failing one are neither Init'd nor Shutdown'd.

rossoctl#8 Document strictness asymmetry in CONVENTIONS.md:
Plugin config is strict (DisallowUnknownFields); top-level runtime
YAML is forgiving (unknown keys silently dropped). Note explains the
forward-compat rationale and points at config.Validate's
empty-pipeline guard as the mechanism that closes the obvious
stale-schema gap.

rossoctl#7 is procedural (kind smoke test) — not in code.

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Hai Huang <huang195@gmail.com>
huang195 added a commit to huang195/kagenti-extensions that referenced this pull request May 7, 2026
Must-fix:
- rossoctl#1+rossoctl#7: Plugins spawned credential watchers with initCtx (60s), so a
  slow client-registration orphaned the plugin after Start's deadline
  and OnRequest returned 503 for the pod's lifetime. Watchers now run
  on process-lifetime contexts created in Init via context.Background()
  + context.WithCancel; the cancel func is stored on the plugin and
  fired in Shutdown. Both JWTValidation and TokenExchange now
  implement pipeline.Shutdowner. Removed the orphaned
  context.WithCancel at main.go:89 that looked like it was meant to
  be that context.

- rossoctl#2: testutil.go moved out of the plugins package into a dedicated
  sub-package authlib/plugins/plugintesting/ so it can't be imported
  by the production binary. Rewrote the helpers as stub plugin
  adapters that mimic jwt-validation / token-exchange OnRequest but
  don't touch plugin internals — listener tests inject an
  *auth.Auth via plugintesting.NewJWTValidation / NewTokenExchange,
  wire through plugintesting.BuildPipeline, and never see the real
  Configure path. All four listener tests updated.

- rossoctl#11: Removed identity sub-validation that applyDefaults made
  unreachable; class-of-error compensated by logging a boot-time
  WARN when Configure's best-effort ReadCredentialFile fails (see
  also rossoctl#12).

Suggestions:
- rossoctl#3: pollCredentials no longer mutates p.cfg. Reads credential
  values into locals and feeds them through auth.UpdateIdentity. cfg
  is now immutable after Configure returns. Added a
  buildClientAuthFrom helper that takes explicit args so the
  goroutine doesn't read p.cfg either.

- rossoctl#4: Rewrote the NoTokenPolicy doc comment to state the current
  behavior ("deny in all modes; operators who need allow or
  client-credentials must set it explicitly") instead of referring
  to the removed config.NoTokenPolicyForMode.

- rossoctl#5: Deleted unused DefaultInboundPlugins/DefaultOutboundPlugins
  and the whole defaults.go file. Fixed the stale PipelineConfig
  docstring in config.go that said "defaults kick in when
  pipeline section is omitted" — they don't.

- rossoctl#6: PluginEntry.UnmarshalYAML normalizes explicit `config: null`
  (and any other !!null-tagged scalar under `config:`) to a nil
  RawMessage, so Build's "plugin does not accept configuration"
  gate doesn't fire spuriously on the four bytes "null".

- rossoctl#12: Deleted unreachable validate() branches: static-mode's
  audience emptiness check, spiffe's missing-path check,
  client-secret's missing-id / missing-secret checks. applyDefaults
  fills the matching field in every case; the branches were dead
  code that obscured what the plugin actually validated.

Nits:
- rossoctl#8: Renamed TestPluginEntry_IDDefaultsToName →
  TestPluginEntry_IDOmittedStaysEmpty.

- rossoctl#9: TestBuild_ConfigForNonConfigurablePlugin now asserts the error
  text contains "does not accept configuration" — operator-facing
  contract.

- rossoctl#10: Renamed TestConfigurable_ErrorAborts →
  TestConfigurable_ErrorPropagates and noted in the comment that
  Build-level abort coverage lives in plugins_test.go.

- rossoctl#13: Documented on jwtValidationConfig.AudienceFile that empty-
  string is treated as "unset" and triggers the default. Operators
  who want no file backing must supply an explicit Audience.

- rossoctl#14: Added TestAuthbridgeCombinedYAML_Loads — parses the in-repo
  authbridge-combined.yaml with env vars set, asserts both inbound
  and outbound pipelines Build cleanly. A future rename of any
  default constant that the YAML relies on breaks this test in CI
  rather than silently shipping a broken image.

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Hai Huang <huang195@gmail.com>
huang195 added a commit to huang195/kagenti-extensions that referenced this pull request May 7, 2026
rossoctl#1 atomic.Pointer for bgCancel:
Both JWTValidation and TokenExchange now hold bgCancel in an
atomic.Pointer[context.CancelFunc] rather than a plain field. Today's
callers (pipeline.Start / pipeline.Stop) are serialized so there's no
actual race, but a future caller invoking Shutdown from a different
goroutine would have been latent-racing the Init assignment. Shutdown
uses Swap(nil) so it's also idempotent.

-race passes clean on pipeline, plugins, config packages.

rossoctl#2 Widen NoTokenPolicy WARN:
Previously only fired when default_policy was "exchange". That missed
envoy-sidecar deployers relying on the old client-credentials default
for routed hosts. WARN now fires whenever no_token_policy was
defaulted, with the message listing all three pre-PR mode-specific
defaults so operators can see which one their deployment used to get.

rossoctl#3 audience_file WARN mentions defaulted origin:
When audience_file was defaulted to /shared/client-id.txt (Kagenti
convention) and the file isn't readable, the new WARN spells that
out — so non-Kagenti deployers notice their config didn't put the
plugin where it thinks it is. Explicit-path case keeps the shorter
original message.

rossoctl#4 Drop client_id from credential-loaded log:
OAuth spec treats client_id as non-secret, but some operators treat
it as sensitive. The signal (credentials loaded) doesn't need the
identifier.

rossoctl#5 TODO markers for /stats and Ready():
Grep-able TODO(follow-up) comments at the stub sites in main.go with
sketch notes for the future work. These sit next to the existing
behavior comments so anyone reading those surfaces finds the
follow-up plan.

rossoctl#6 Pipeline.Start failure unwind:
On Init error at plugin N, Start now calls Shutdown on plugins
[0..N-1] in reverse order before returning the error. Prevents leaking
goroutines spawned by earlier plugins' Init when a downstream peer
rejects its config at boot. Not a correctness bug for production
(log.Fatalf then exit, kernel cleans up) but matters for any embedded
or multi-tenant caller.

New test TestPipelineStart_UnwindsSuccessfulInitsOnFailure asserts
the reverse-order Shutdown on the subset that Init'd, and that plugins
after the failing one are neither Init'd nor Shutdown'd.

rossoctl#8 Document strictness asymmetry in CONVENTIONS.md:
Plugin config is strict (DisallowUnknownFields); top-level runtime
YAML is forgiving (unknown keys silently dropped). Note explains the
forward-compat rationale and points at config.Validate's
empty-pipeline guard as the mechanism that closes the obvious
stale-schema gap.

rossoctl#7 is procedural (kind smoke test) — not in code.

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Hai Huang <huang195@gmail.com>
huang195 added a commit to huang195/kagenti-extensions that referenced this pull request May 15, 2026
Replace the two broken-after-rossoctl#411 standalone demos
(`single-target/`, `multi-target/`) with a single configuration-only
reference at `demos/token-exchange-routes/`. Both old demos used the
pre-rossoctl#411 multi-sidecar pattern (envoy-proxy + authbridge-light +
client-registration + standalone spiffe-helper) with images that no
longer publish; rewriting their YAMLs to the combined-sidecar shape
is real per-demo work that needs runtime testing. The route-config
content is the part that's still useful and isn't covered well by
other demos, so extract that.

New `demos/token-exchange-routes/README.md` covers:
- How AuthBridge's outbound `token-exchange` plugin matches the
  request `Host` header against `authproxy-routes` entries and
  performs RFC 8693 exchange.
- ConfigMap shape: per-route `host` / `target_audience` /
  `token_scopes` fields, with `kubectl create configmap` example.
- Single-target example (one route entry, weather-tool style).
- Multi-target example (three routes; the existing
  `routes.yaml` is moved into this directory unchanged as a
  worked example).
- Mixing exchange and passthrough (default policy);
  tightening to `default_policy: exchange` for tool-only
  namespaces.
- Glob pattern semantics — `filepath.Match` quirks around `**`,
  short-name matching, etc.
- Troubleshooting: `Host` mismatches, missing scopes on the
  agent's own Keycloak client, audience errors at the target,
  routes-not-reloaded.
- Cross-references to `weather-agent/demo-ui-advanced.md` and
  `webhook/README.md` for the deployment side.

Removed:
- `demos/single-target/` (entire directory) — covered by webhook +
  weather-agent demos plus the new routes reference; manual
  multi-sidecar deployment YAMLs were the broken bit.
- `demos/multi-target/{demo.md,run-demo-commands.sh,teardown-demo.sh,k8s/}`
  — the script-driven flow tied to the dead `agent` Deployment in
  the `authbridge` namespace; not worth migrating.

Renamed:
- `demos/multi-target/` → `demos/token-exchange-routes/`
  (`git mv` preserves history on `routes.yaml`).

Cross-references updated everywhere:
- `demos/README.md` table: drop the strikethrough rows + warning
  paragraph; add Token-Exchange Routes / MCP Parser / abctl
  Walkthrough as Reference-tier entries; the recommended-path
  step 3 points at the new routes guide.
- `demos/github-issue/{demo.md,demo-ui.md,demo-manual.md}`,
  `demos/weather-agent/demo-ui.md` — "Multi-Target Demo" links
  retargeted to `../token-exchange-routes/README.md` with updated
  link labels.
- `authbridge/README.md` overview link bullet + the demos list at
  the bottom collapsed to one Token-Exchange Routes entry.
- `authbridge/CLAUDE.md` — directory tree + demo descriptions +
  Keycloak setup-script table updated; gotcha rossoctl#6 (hardcoded SPIFFE
  ID in `single-target/setup_keycloak.py`) removed since the file
  is gone, remaining gotchas renumbered.
- `CLAUDE.md` (top-level) — directory listing line + manual-
  deployment learning-path bullet retargeted at the new routes
  guide.
- `demos/webhook/setup_keycloak.py` — comment that pointed at
  `single-target/k8s/authbridge-deployment.yaml` for the
  `envoy-config` ConfigMap retargeted at the kagenti-system
  release namespace.

Existing single/multi-target sweep banners from earlier in this PR
(broken-after-rossoctl#411 YAML headers) are deleted with the YAMLs.

Net diff: -1,500 lines (the two broken demos), +210 lines (the new
consolidated reference + cross-ref updates). Repo-wide grep for
`single-target/` and `multi-target/` now matches only on prose
that uses those words descriptively (e.g. "single-target route
pattern"), not as path components.

Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com>
Signed-off-by: Hai Huang <huang195@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Feature: Consolidate Agent and MCPServer Webhooks to Avoid Code Duplication

3 participants