Skip to content

webhook: add idempotency check and configurable unix socket path - #31

Open
pmady wants to merge 1 commit into
dragonflyoss:mainfrom
pmady:refactor/idempotency-configurable-socket-metrics
Open

webhook: add idempotency check and configurable unix socket path#31
pmady wants to merge 1 commit into
dragonflyoss:mainfrom
pmady:refactor/idempotency-configurable-socket-metrics

Conversation

@pmady

@pmady pmady commented Mar 25, 2026

Copy link
Copy Markdown

What this PR does

Part of dragonflyoss/dragonfly#4416. Adds idempotency check (dragonfly.io/injected annotation) to prevent double injection, makes unix socket path configurable via InjectConf, and adds tests.

Ref: dragonflyoss/dragonfly#4416

@pmady
pmady requested review from a team as code owners March 25, 2026 22:04
@github-actions
github-actions Bot requested a review from jim3ma March 25, 2026 22:04
@pmady
pmady force-pushed the refactor/idempotency-configurable-socket-metrics branch from a64cf25 to 25dd45f Compare March 26, 2026 17:46
@pmady

pmady commented Apr 6, 2026

Copy link
Copy Markdown
Author

Hi friendly ping on this PR. It's been open for about 2 weeks now.

Quick summary of what it does (ref: dragonflyoss/dragonfly#4416):

  • Idempotency: Adds dragonfly.io/injected annotation to prevent double injection on re-admitted pods
  • Configurable unix socket path: Makes the dfdaemon socket path configurable via Config.UnixSockPath (falls back to default /var/run/dragonfly/dfdaemon.sock)
  • Tests: Adds tests for both features (custom/default socket path, skip-already-injected, annotation-set-after-inject, no-double-inject)

The branch is cleanly rebased on the latest main (c77b133). All 48 injector unit tests pass. The changes are minimal (+159/-2 lines across 8 files).

Is there anything you'd like me to change or split up before merging? Happy to address any feedback.

@pmady

pmady commented May 4, 2026

Copy link
Copy Markdown
Author

@gaius-qi bump on this one too when you get a chance - adds the idempotency check we discussed plus makes the unix socket path configurable

@pmady

pmady commented May 22, 2026

Copy link
Copy Markdown
Author

hey @gaius-qi, just checking in — this one and #39 have been sitting for a bit. no rush, just lmk if theres anything you want changed or if the approach looks off. happy to adjust

- Add dragonfly.io/injected annotation to prevent double injection
- Make unix socket path configurable via Config.UnixSockPath field
- Fall back to default /var/run/dragonfly/dfdaemon.sock when empty
- Add tests for configurable socket path and idempotency
- Update default ConfigMap with unixSockPath field

Ref: dragonflyoss/dragonfly#4416
Signed-off-by: pmady <pavan4devops@gmail.com>
@pmady
pmady force-pushed the refactor/idempotency-configurable-socket-metrics branch from 25dd45f to 306e178 Compare September 8, 2026 17:36
@pmady

pmady commented Sep 8, 2026

Copy link
Copy Markdown
Author

@gaius-qi rebased, make test green. can you add the enhancement label so classify passes - i cant label from a fork. same reason as #39 for wanting this in before november

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟢 Approval recommended

The changes are cohesive, appear correct for the stated goals (idempotency + configurable socket path), and include targeted tests covering the new behavior.

Pull request overview

This PR enhances the Kubernetes mutating webhook injection flow by adding an explicit idempotency guard (via a dragonfly.io/injected annotation) to prevent double-injection, and by making the dfdaemon unix socket path configurable through the injector Config.

Changes:

  • Skip injection early when a Pod is already marked as injected, and mark Pods as injected after successful mutation.
  • Add unixSockPath to injector configuration and use it when injecting the unix socket HostPath volume + mount (with a default fallback).
  • Add/extend Ginkgo tests covering idempotency behavior and custom unix socket path configuration.
File summaries
File Description
internal/webhook/v1/pod_webhook.go Adds idempotency check and sets an injected marker annotation after injection.
internal/webhook/v1/pod_webhook_test.go Adds tests ensuring injection is skipped when already injected and that the marker is set after injection.
internal/webhook/v1/injector/unix_socket.go Uses Config.UnixSockPath (or default) for HostPath volume and container mount path.
internal/webhook/v1/injector/unix_socket_test.go Adds tests validating custom socket path behavior and default fallback when empty.
internal/webhook/v1/injector/constant.go Introduces injected marker annotation constants.
internal/webhook/v1/injector/config.go Extends injector config struct and default config with UnixSockPath.
internal/webhook/v1/injector/config_test.go Verifies DefaultConfig() includes the default unix socket path.
config/webhook/config-map.yaml Adds unixSockPath to the deployed injector config map.
Review details
  • Files reviewed: 8/8 changed files
  • Comments generated: 0
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

}

// Idempotency: skip pods that have already been injected.
if d.isAlreadyInjected(pod) {

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.

I find the semantic design here a bit ambiguous. Adding an injected flag to the annotation is not clear enough, as it does not distinguish between successful and failed injections.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fair. None of the injectors can fail right now, Inject() has no error return and the annotation only gets written after all of them run, so there is no failed state to record today. Agree a bare bool is thin though.

Two ways to go, which do you prefer:

  1. Make it a status like istio's sidecar.istio.io/status, e.g. dragonfly.io/injection-status with a small json value listing which injectors ran and the socket path. Also useful when debugging a pod.
  2. Same key, enum value instead (injected for now, room for failed / skipped later), keep the check as-is.

Leaning to 1 since it also records what got injected. Happy to go either way, I will update the PR once you pick

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.

3 participants