Add sidecar injection webhook proposal

[tombentley]

Assisted-by: Claude Opus 4.6 noreply@anthropic.com

[k-wall]

Discussed at Community Call 23rd April. Tom is looking for feedback.

[k-wall]

Sounds very sensible

[tombentley]

Not really. Because the proxy configuration is built from the KroxyliciousSidecarConfig at injection time it means that any subsequent changes to the KroxyliciousSidecarConfig are ignored until the next time the pod get created. That creates a lot of problems:

• The user who has RBAC on the KroxyliciousSidecarConfig might not have RBAC on the Pod, Deployment etc. So it assumes some kind of communication between those users. Such communication might not be possible in practice.
• Erroneous changes to the KroxyliciousSidecarConfig are only discovered at some unbounded time later.

The right way to solve this would be for the proxy to phone home to obtain a configuration from a control plane, and to receive updates to that configuration without the need for Pod restarts. But I really want to avoid boiling the ocean here by blocking this alpha sidecar feature on having such a control plane. I think the control plane piece is something we could add later (also as an alpha feature) and then maybe they would become stable features together later on.

[k-wall]

Thanks @tombentley, looks like a solid proposal. I've left a few comments to be considered.

[tombentley]

Thanks @k-wall @robobario, I think I've addresses your comments, though with a watering down of some of the initial ambition wrt trust boundaries. PTAL.

[tombentley]

Not really. Because the proxy configuration is built from the KroxyliciousSidecarConfig at injection time it means that any subsequent changes to the KroxyliciousSidecarConfig are ignored until the next time the pod get created. That creates a lot of problems:

• The user who has RBAC on the KroxyliciousSidecarConfig might not have RBAC on the Pod, Deployment etc. So it assumes some kind of communication between those users. Such communication might not be possible in practice.
• Erroneous changes to the KroxyliciousSidecarConfig are only discovered at some unbounded time later.

The right way to solve this would be for the proxy to phone home to obtain a configuration from a control plane, and to receive updates to that configuration without the need for Pod restarts. But I really want to avoid boiling the ocean here by blocking this alpha sidecar feature on having such a control plane. I think the control plane piece is something we could add later (also as an alpha feature) and then maybe they would become stable features together later on.

[k-wall]

Hi! We've updated the proposal numbering system to use PR numbers as proposal identifiers.

Action required: Please rebase your PR on main.

Once you rebase, you'll need to rename your proposal file and update the title:

git mv proposals/100-sidecar-injection-webhook.md proposals/100-injection-webhook.md
# Update title: remove any old number prefix and add PR number
sed -i.bak '0,/^# /{s/^# \([0-9]\{3\}\|xxx\|nnn\|000\) - /# 100 - /; t; s/^# /# 100 - /}' proposals/100-injection-webhook.md && rm proposals/100-injection-webhook.md.bak
git add proposals/100-injection-webhook.md
git commit -m "Rename proposal to use PR number"
git push

The GitHub workflow will automatically check your proposal file naming after you push and update this PR description if any corrections are still needed.

See proposals/README.md for the updated workflow.

[k-wall]

Correction: The previous notification had an incorrect filename. Here are the correct commands:

git mv proposals/100-sidecar-injection-webhook.md proposals/100-sidecar-injection-webhook.md
# Update title: remove any old number prefix and add PR number
sed -i.bak '0,/^# /{s/^# \([0-9]\{3\}\|xxx\|nnn\|000\) - /# 100 - /; t; s/^# /# 100 - /}' proposals/100-sidecar-injection-webhook.md && rm proposals/100-sidecar-injection-webhook.md.bak
git add proposals/100-sidecar-injection-webhook.md
git commit -m "Rename proposal to use PR number"
git push

Sorry for the confusion!

[tombentley]

@robobario @SamBarker @k-wall I've tried to address your previous comments, and also bring this into live with the implementation PR. Please take another look.

[robobario]

Suggested change

	The webhook admin creates one per namespace. The following edge cases are handled:
	The webhook admin creates one or more per namespace. The following edge cases are handled:

Sounds a bit like you can only create one currently, but then the edge cases cover the multi case.

[robobario]

should we mention that initially we will only support PEMs with PKCS8 RSA/EC private keys, and whatever the cert format is we can parse.

[robobario]

should probably mention multi-replica, PDB we've added for availability

[SamBarker]

Three suggestions on the injection-skipped label:

1. Add opted-out for explicitly opted-out pods.
The sidecar.kroxylicious.io/injection: disabled label expresses intent (set by
the app owner); injection-skipped is observed state (set by the webhook). They
are semantically different: without an ack label there's no way to distinguish
"the webhook honoured the opt-out" from "the webhook hasn't processed this pod
yet." Adding injection-skipped=opted-out also means operators get a single
uniform query for all non-injected pods regardless of reason:

kubectl get pods -l sidecar.kroxylicious.io/injection-skipped

2. Revisit the label values for clarity without the webhook's context.
For an SRE encountering these at 2am:

Current	Suggested	Rationale
no-config	no-sidecar-config	Makes clear it's the KroxyliciousSidecarConfig CR that's absent, not some internal webhook config
multiple-configs	ambiguous-sidecar-config	"Multiple" describes the cause; "ambiguous" describes the problem the operator needs to resolve
already-injected	container-name-conflict	The current name implies a previous injection pass; the actual cause is a pre-existing container with a conflicting name

3. Reinvocation should be silent.
The configuration drift detection section describes a second skip path: the
webhook skips injection if the sidecar.kroxylicious.io/config-generation
annotation is already present (idempotency guard on reinvocation). The current
text implies this would set injection-skipped=already-injected, but reinvocation
is expected behaviour — adding the label would pollute the signal for operators
querying -l sidecar.kroxylicious.io/injection-skipped. Reinvocation should
produce no label.

[tombentley]

On (1): opted-out pods are excluded by objectSelector on the MutatingWebhookConfiguration; they never reach the webhook so it can't label them. Changing the exclusion mechanism (e.g. moving opt-out from objectSelector to in-webhook logic) would mean every pod creation in the namespace hits the webhook regardless. I think the current approach is the right trade-off.

On (2): Yes, we can use better values; I'll revise.

On (3): You're right, reinvocation is expected behaviour and shouldn't set the label. I'll make that explicit.

[SamBarker]

This paragraph is back-to-front: it cites the current implementation to justify
the design rather than stating the property the design requires. The proposal
should describe the requirement, and leave it to the implementation to satisfy it.

Something like: for annotation comparison to be a reliable idempotency check,
config generation must be deterministic — the same KroxyliciousSidecarConfigSpec
must always produce byte-identical serialized output within a given webhook
version. The implementation must guarantee this; how it does so is not a concern
of this proposal.

The version caveat also deserves calling out explicitly as a constraint: a webhook
upgrade that changes serialization output means existing pods' annotations no
longer match what the webhook would generate, making them appear as configuration
drift even though the KroxyliciousSidecarConfig itself hasn't changed. The
proposal should state this consequence — and note that remediation requires pods
to cycle, since nothing proactively re-injects running pods.

[tombentley]

I'll rewrite this to state the determinism requirement without referencing implementation details, and add an explicit note about the webhook-upgrade consequence: changed serialisation output means existing pods appear as drift, requiring pod cycling to remediate.

[SamBarker]

The proposal only defines Ready=True. If the webhook observes a config and can
determine it is invalid (e.g. missing required fields, cases 1–3 from the edge
cases section), setting Ready=False with a descriptive reason and message would
surface the problem directly on the CRD — where an operator would naturally look
— rather than requiring them to find the relevant log line. As it stands, a
misconfigured KroxyliciousSidecarConfig is either acknowledged (Ready=True)
or invisible; there is no condition state that says "I have seen this and it is
wrong."

[tombentley]

Good point. I'll add Ready=False (reason Invalid, with a descriptive message) for configs that the webhook can determine are invalid at observation time. This naturally covers structural problems detectable by the webhook's own validation beyond what the CRD schema catches (e.g. referencing a non-existent Secret).

For the per-namespace edge cases (no-config, multiple-configs): these aren't properties of a single CRD, so they don't fit naturally into CRD status. They're already covered by the injection-skipped label on affected pods. I think that's the right place for those.

[SamBarker]

FEATURE_GATES is ambiguous — it reads as though it controls the webhook's own
internal feature flags rather than describing which Kubernetes API server feature
gates are active on the cluster. K8S_FEATURE_GATES would be clearer about the
subject.

It would also be worth stating explicitly that this is an escape hatch for when
version-based detection is insufficient, not the recommended default. Deployers
who set it are taking on the responsibility of keeping it in sync with their
cluster configuration — the proposal should acknowledge that risk.

[tombentley]

I'll rename to K8S_FEATURE_GATES and add a note that this is an escape hatch for clusters where version-based detection is insufficient, with deployers responsible for keeping it in sync.

[SamBarker]

The proposal only addresses one direction: the sidecar's security context is never
weakened below what the webhook would set. It doesn't address the reverse — a pod
with a weaker pod-level security context (e.g. seccompProfile: Unconfined to
support io_uring). Since the webhook doesn't set a container-level seccomp profile
on the sidecar, the sidecar would silently inherit a permissive pod-level one.

Not a blocker for alpha, but worth keeping in mind: the proxy itself is a candidate
for io_uring in the future (for performance), so we may actually want a way to
configure the sidecar's seccomp profile explicitly rather than inheriting from the
pod. The KroxyliciousSidecarConfig spec is the natural place for that.

[tombentley]

Good catch. For alpha, I think we should set seccompProfile: RuntimeDefault explicitly on the sidecar container, so it doesn't inherit a permissive pod-level profile. Container-level seccomp overrides pod-level, so this won't affect the app container.

Agreed that KroxyliciousSidecarConfig is the right place for explicit seccomp configuration if/when we need it (e.g. for io_uring). I'll add a note about that as a future consideration.

[SamBarker]

secretMounts[].secretName is a cross-resource reference — without validating
that the referenced Secret exists in the pod's namespace, a
KroxyliciousSidecarConfig with a bad reference passes admission silently and
only fails when Kubernetes tries to mount the volume at scheduling time. This
validation needs to be part of the alpha implementation, not deferred. It is also
exactly the case where Ready=False would be most valuable — see separate
comment on § Status.

The operator has validation logic for LocalObjectReference fields that handles
this; the webhook should follow the same pattern.

[SamBarker]

Really like the forward-looking list structure here. Active/passive DR is a
compelling use case — the sidecar exposes both live and DR clusters, and failover
is a config change plus pod restart. Could be a great showcase for the project.