feat: add a Helm chart and annotation-driven sidecar injection - #612
Conversation
bind leaves it umask-derived 0755, which denies the group write and so admits only the binding uid — a same-pod attach could not share a socket without sharing a uid.
TASKITO_WEBHOOK_LISTEN serves a mutating admission webhook that adds an executor to an annotated pod, reusing the pod's own image so nothing new is pulled. Webhook-only deployments need no DSN.
Scheduler, dashboard and injector in one release, with a self-signed webhook certificate by default and cert-manager as an opt-in.
Guards the caBundle against the served certificate — genCA is random, so a helper called once per template would break TLS in a way only a live cluster shows.
The chart mounts no volume for it, so the database would vanish with the pod and a second replica would not share it.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds webhook-only runtime support, annotation-driven executor sidecar injection, a Helm chart, certificate handling, Unix socket permissions, chart validation, and CI coverage. ChangesTaskito webhook runtime
Helm chart deployment
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Kubernetes
participant WebhookRouter
participant AnnotationParser
participant SidecarInjector
Kubernetes->>WebhookRouter: POST /mutate AdmissionReview
WebhookRouter->>AnnotationParser: parse pod annotations
AnnotationParser->>SidecarInjector: return InjectionSpec
SidecarInjector->>WebhookRouter: return JSONPatch or no patch
WebhookRouter->>Kubernetes: return AdmissionReviewResponse
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
pratyush618
changed the title
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (7)
.github/workflows/ci-chart.yml (1)
79-89: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAnchor the field regex to a YAML key.
field()searches the whole rendered document and returns the first unanchored match. Two cases can select the wrong value:
- A longer key that ends with the searched name matches. For example
client-ca.crt: <other>matchesca.crt.- If the chart later renders a second Secret that carries
tls.crt, the first occurrence wins silently.The current role combination renders only the webhook Secret, so the check passes today. Anchor the match to make the assertion stable as the chart grows.
♻️ Proposed change to anchor the key match
def field(name): - match = re.search(rf"{re.escape(name)}: (\S+)", rendered) + match = re.search(rf"^\s*{re.escape(name)}: (\S+)$", rendered, re.M) if not match: sys.exit(f"{name} is missing from the rendered chart") return match.group(1)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci-chart.yml around lines 79 - 89, Update the field() regex in the chart-rendering validation to match only an exact YAML key at the beginning of a line, allowing normal indentation, instead of matching the searched name anywhere in the document. Preserve the existing missing-field failure and value extraction behavior for ca.crt, caBundle, and tls.crt.crates/taskito-server/src/config/webhook.rs (1)
72-80: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe temporary certificate files are never removed.
touchwrites into the system temp directory and no test deletes the file. Each test run leaves PEM-named files behind. The paths are keyed by label and process id, so tests stay correct, but the files accumulate.Remove the files at the end of each test, or use a scoped temporary directory.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/taskito-server/src/config/webhook.rs` around lines 72 - 80, Update the test helper touch and its callers so temporary certificate files are cleaned up after each test, removing every path created under the taskito-webhook naming scheme; preserve the existing certificate setup and test behavior.crates/taskito-server/src/config/mod.rs (1)
172-177: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for the webhook-only path that omits the DSN.
The tests cover both rejection paths. The new acceptance path —
TASKITO_WEBHOOK_LISTENset,TASKITO_DSNabsent,Ok(config)returned — has no test. That path is the contract this PR adds. A regression there would only surface at deploy time.The test needs real certificate files on disk, because
required_pathcallspath.is_file().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/taskito-server/src/config/mod.rs` around lines 172 - 177, Add a test alongside an_attach_listener_still_needs_a_dsn covering webhook-only configuration: create real temporary certificate and key files, set TASKITO_WEBHOOK_LISTEN without TASKITO_DSN, call Config::from_map, and assert it returns Ok(config). Ensure the certificate paths point to files so required_path succeeds.crates/taskito-server/src/webhook/inject.rs (2)
103-139: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftThe sidecar copies no
resourcesand nosecurityContextfrom the pod.
build_sidecarcopiesimage,imagePullPolicy,env, andenvFrom. It sets noresourcesand nosecurityContext. Two consequences follow.First, a pod whose containers all declare equal requests and limits has QoS class
Guaranteed. Adding a container with noresourcesdowngrades the pod toBurstable. That changes eviction order for an existing workload, without the operator changing anything except the annotations.Second, a cluster that enforces the
restrictedPod Security Standard requiresrunAsNonRoot,allowPrivilegeEscalation: false,capabilities.drop: ["ALL"], andseccompProfileon every container. If those are set per-container on the app rather than at pod level, the injected sidecar has none of them and the API server rejects the pod. The rejection message names the injected container, which is confusing because that container is not in the submitted manifest.Copy
securityContextfrom the source container, and add annotations for the sidecar's resource requests and limits.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/taskito-server/src/webhook/inject.rs` around lines 103 - 139, Update build_sidecar to copy the source container’s securityContext into the generated sidecar. Also add sidecar resource requests and limits sourced from the injection annotations, using the existing annotation/configuration symbols and preserving omission when values are not provided.
43-49: 🩺 Stability & Availability | 🔵 TrivialConsider the pod-completion effect for Job and CronJob workloads.
The patch appends the executor to
/spec/containers. A regular container that runs until shutdown prevents aJobpod from reachingSucceeded, because a Job pod completes only when every regular container exits. If aJoborCronJobpod template carries the injection annotations, the Job never finishes.Kubernetes 1.29 and later support native sidecars: an entry in
initContainerswithrestartPolicy: Always. That form does not block Job completion.Document the limitation, or select the target list based on the workload. The chart can also restrict the webhook's object selector so that only the intended workloads are matched.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/taskito-server/src/webhook/inject.rs` around lines 43 - 49, The injection logic constructing ops must account for Job and CronJob pod-completion semantics: avoid adding the long-running sidecar to /spec/containers for those workloads. Select an initContainers entry with restartPolicy Always for Kubernetes 1.29+ or restrict the webhook’s matched workloads accordingly, and document any remaining limitation using the relevant webhook injection symbols.crates/taskito-server/src/webhook/mod.rs (1)
87-132: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
mutateignores the request operation and object kind.The handler applies
inject::patch_fortorequest.objectfor every review it receives. It does not checkrequest.operationorrequest.kind. Correctness depends entirely on theMutatingWebhookConfigurationrules that the chart installs.If a rule is later widened, or an operator registers the endpoint manually, a non-pod object reaches
patch_for.patch_forthen reads/spec/containers, finds nothing, and returnsOk(None), so the object is admitted unchanged. The failure is safe, but it is silent.Consider checking
request.kind.kind == "Pod"and returningallowwith a warning otherwise. That keeps the handler correct independently of the registration.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/taskito-server/src/webhook/mod.rs` around lines 87 - 132, Update mutate to validate request.kind.kind before calling inject::patch_for, allowing the review unchanged and logging a warning when the object is not a Pod. Preserve the existing injection flow for Pod requests and use the existing request UID and review response helpers for the allow response.crates/taskito-server/src/runtime/mod.rs (1)
132-145: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winA failing dashboard cancels the webhook without its graceful drain.
tokio::try_join!returns on the first error and drops the other future. If the dashboard fails first, the webhook future is dropped.crate::webhook::serveinstalls a shutdown watcher that callshandle.graceful_shutdown(Some(5s)), but that watcher only fires whenshutdownis triggered. A dropped future never triggers it. In-flight admissions are then cut, which is the exact case the 5-second drain exists for.
shutdown.trigger()runs only afterblock_onreturns, which is after the drop.Trigger shutdown inside the async block before you stop awaiting, so both servers drain.
♻️ Proposed change
- let result = match (dashboard, webhook) { - (Some(dashboard), Some(webhook)) => tokio::try_join!(dashboard, webhook).map(|_| ()), + let result = match (dashboard, webhook) { + (Some(dashboard), Some(webhook)) => { + // Signal shutdown as soon as either side fails, so the other + // drains through its own handler instead of being dropped. + let outcome = tokio::try_join!(dashboard, webhook).map(|_| ()); + shutdown.trigger(); + outcome + }That still drops the surviving future immediately. For a true drain, spawn each server as a task, select on the join handles, trigger shutdown, then await both handles.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/taskito-server/src/runtime/mod.rs` around lines 132 - 145, Update the concurrent server handling around dashboard and webhook so a failure triggers shutdown before either server task is dropped. Spawn each present server as a task, select on the join handles for the first completion or error, call shutdown.trigger(), then await both handles so crate::webhook::serve can perform its graceful drain. Preserve the existing listener-only behavior and propagate the relevant server result.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/taskito-server/src/config/webhook.rs`:
- Around line 5-8: Rewrite the module documentation sentence around the
certificate file reads to state directly that the files are read once at
startup, not on each accepted connection, and that certificate rotation requires
a restart. Preserve the existing reference to the chart checksum annotation.
In `@crates/taskito-server/src/main.rs`:
- Around line 35-36: Update the TASKITO_DSN entry in the environment-variable
help text to indicate it is required except for webhook-only deployments,
matching the qualification in the surrounding deployment requirements text.
In `@crates/taskito-server/src/runtime/listener.rs`:
- Around line 156-175: Update the socket creation flow around UnixListener::bind
to temporarily set the process umask so the socket is created no more permissive
than SOCKET_MODE, then restore the previous umask on every success and error
path via an RAII guard that cannot be bypassed by ?. Protect the process-wide
umask change with the appropriate mutex when concurrent file creation is
possible, and remove the claim in restrict_socket that the post-bind window is
harmless.
In `@crates/taskito-server/src/webhook/annotations.rs`:
- Around line 99-112: Normalize boolean annotation values to a consistent case
before matching in both the inherit_env calculation and opted_in function, so
values such as “True” and “False” behave like their lowercase equivalents.
Preserve the existing recognized-value semantics and default behavior unless the
surrounding API already provides validation for unrecognized values.
In `@crates/taskito-server/src/webhook/inject.rs`:
- Around line 143-160: Update socket_dir to reject socket paths whose parent
directory is the filesystem root, in addition to the existing empty-parent
check. Ensure unix:/attach.sock fails with the same absolute-path validation
error instead of returning "/" as the mount directory, while preserving valid
non-root absolute paths.
In `@crates/taskito-server/src/webhook/mod.rs`:
- Around line 52-60: The webhook TLS configuration in
crates/taskito-server/src/webhook/mod.rs:52-60 must reload certificates when the
mounted Secret changes: clone RustlsConfig and run a background watcher or
polling task that calls reload_from_pem_file with the configured certificate and
key paths. Update crates/taskito-server/src/config/webhook.rs:5-8 to accurately
describe this reload behavior and fix the garbled sentence.
In `@crates/taskito-server/tests/webhook_injection.rs`:
- Around line 159-163: Update the test
a_body_that_is_not_an_admission_review_is_rejected so its payload is
syntactically invalid JSON and therefore exercises the serde_json parsing-error
branch in the webhook handler, while retaining the expected BAD_REQUEST status.
Keep the test name aligned with the behavior it actually verifies.
In `@deploy/helm/taskito-server/templates/deployment.yaml`:
- Around line 152-175: Update the probe conditionals in the deployment template
to cover every enabled listener, including TCP liveness and readiness probes for
the attach port when attach.enabled is the only listener. When both
dashboard.enabled and webhook.enabled are true, configure each probe against the
existing health endpoint that reports both listeners rather than dashboard-only
/health, while preserving HTTPS for webhook checks.
In `@deploy/helm/taskito-server/templates/NOTES.txt`:
- Around line 24-36: The webhook annotation guidance must not show a chart-local
attach address when attach.enabled is false. In
deploy/helm/taskito-server/templates/NOTES.txt lines 24-36, condition the
chart-local example on attach.enabled and otherwise instruct users to provide an
external taskito.dev/attach endpoint plus its token Secret. In
deploy/helm/taskito-server/README.md lines 39-50, state that the existing
example requires attach.enabled=true and add a webhook-only example using an
external attach endpoint.
---
Nitpick comments:
In @.github/workflows/ci-chart.yml:
- Around line 79-89: Update the field() regex in the chart-rendering validation
to match only an exact YAML key at the beginning of a line, allowing normal
indentation, instead of matching the searched name anywhere in the document.
Preserve the existing missing-field failure and value extraction behavior for
ca.crt, caBundle, and tls.crt.
In `@crates/taskito-server/src/config/mod.rs`:
- Around line 172-177: Add a test alongside an_attach_listener_still_needs_a_dsn
covering webhook-only configuration: create real temporary certificate and key
files, set TASKITO_WEBHOOK_LISTEN without TASKITO_DSN, call Config::from_map,
and assert it returns Ok(config). Ensure the certificate paths point to files so
required_path succeeds.
In `@crates/taskito-server/src/config/webhook.rs`:
- Around line 72-80: Update the test helper touch and its callers so temporary
certificate files are cleaned up after each test, removing every path created
under the taskito-webhook naming scheme; preserve the existing certificate setup
and test behavior.
In `@crates/taskito-server/src/runtime/mod.rs`:
- Around line 132-145: Update the concurrent server handling around dashboard
and webhook so a failure triggers shutdown before either server task is dropped.
Spawn each present server as a task, select on the join handles for the first
completion or error, call shutdown.trigger(), then await both handles so
crate::webhook::serve can perform its graceful drain. Preserve the existing
listener-only behavior and propagate the relevant server result.
In `@crates/taskito-server/src/webhook/inject.rs`:
- Around line 103-139: Update build_sidecar to copy the source container’s
securityContext into the generated sidecar. Also add sidecar resource requests
and limits sourced from the injection annotations, using the existing
annotation/configuration symbols and preserving omission when values are not
provided.
- Around line 43-49: The injection logic constructing ops must account for Job
and CronJob pod-completion semantics: avoid adding the long-running sidecar to
/spec/containers for those workloads. Select an initContainers entry with
restartPolicy Always for Kubernetes 1.29+ or restrict the webhook’s matched
workloads accordingly, and document any remaining limitation using the relevant
webhook injection symbols.
In `@crates/taskito-server/src/webhook/mod.rs`:
- Around line 87-132: Update mutate to validate request.kind.kind before calling
inject::patch_for, allowing the review unchanged and logging a warning when the
object is not a Pod. Preserve the existing injection flow for Pod requests and
use the existing request UID and review response helpers for the allow response.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7447dcb4-f4c3-4a7c-a975-eee06a6574be
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (31)
.github/workflows/ci-chart.yml.github/workflows/ci.ymlCHANGELOG.mdcrates/taskito-server/Cargo.tomlcrates/taskito-server/src/config/mod.rscrates/taskito-server/src/config/webhook.rscrates/taskito-server/src/lib.rscrates/taskito-server/src/main.rscrates/taskito-server/src/runtime/listener.rscrates/taskito-server/src/runtime/mod.rscrates/taskito-server/src/webhook/admission.rscrates/taskito-server/src/webhook/annotations.rscrates/taskito-server/src/webhook/inject.rscrates/taskito-server/src/webhook/mod.rscrates/taskito-server/tests/webhook_injection.rsdeploy/helm/taskito-server/.helmignoredeploy/helm/taskito-server/Chart.yamldeploy/helm/taskito-server/README.mddeploy/helm/taskito-server/templates/NOTES.txtdeploy/helm/taskito-server/templates/_helpers.tpldeploy/helm/taskito-server/templates/_validate.tpldeploy/helm/taskito-server/templates/deployment.yamldeploy/helm/taskito-server/templates/ingress.yamldeploy/helm/taskito-server/templates/secret.yamldeploy/helm/taskito-server/templates/service.yamldeploy/helm/taskito-server/templates/serviceaccount.yamldeploy/helm/taskito-server/templates/webhook.yamldeploy/helm/taskito-server/values.yamldocs/content/docs/resources/changelog.mdxdocs/content/docs/shared/guides/operations/deployment.mdxscripts/version.mjs
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
ByteVeda/taskito(manual)
A chmod after bind left it accepting at 0777 under a permissive umask. Bind on a staging path, chmod there, then rename into place.
cert-manager renews a Secret in place, so a pair read once at boot would be served until it expired.
A pod annotated "True" was read as not opted in, skipping the sidecar silently.
Its parent is /, so the volume mounted over the container root and hid the binary the sidecar was told to run.
Every envelope field is optional, so the old body parsed fine and passed for the wrong reason.
It had no probe at all; a TCP connect stands in where there is no HTTP listener.
A webhook-only install creates no attach Service, so the sample annotations pointed at nothing.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
deploy/helm/taskito-server/README.md (2)
123-130: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCorrect the probe description for attach-only releases.
When only
attach.enabledis enabled, the chart uses TCP probes on theattachport. It does not use/healthin that mode. Document that/healthapplies to dashboard or webhook roles, and that/readinessrequires a post-render Deployment patch because the chart does not configure it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/helm/taskito-server/README.md` around lines 123 - 130, Update the “Probes” section to describe attach-only releases accurately: when only attach.enabled is enabled, both probes use TCP checks on the attach port rather than /health. State that /health applies to dashboard or webhook roles, and that probing /readiness requires a post-render Deployment patch because the chart does not configure it.
119-121: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftAdd a restart trigger for cert-manager renewals.
The checksum changes only during Helm rendering. A cert-manager Secret update does not change the Deployment, while the server reads the PEM files only at startup. Add a Secret-change restart mechanism or document the required rollout after renewal.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/helm/taskito-server/README.md` around lines 119 - 121, Update the certificate rotation documentation to address cert-manager Secret renewals: either describe and configure a Secret-change restart mechanism for the Deployment, or explicitly document that a rollout is required after renewal so the server rereads the PEM files.
🧹 Nitpick comments (1)
crates/taskito-server/src/runtime/listener.rs (1)
344-370: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
libc::umaskand isolate the process-wide umask changeThe workspace uses edition 2021, so
extern "C"does not requireunsafe. The hand-writtenu32signature is not portable becauseumaskuses target-specificlibc::mode_t. Addlibcas a direct dev-dependency and calllibc::umask.
umaskaffects the whole test process. Serialize this test with tests that create files or change the umask, or avoid changing the process umask in a parallel test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/taskito-server/src/runtime/listener.rs` around lines 344 - 370, Update the test the_socket_is_never_visible_at_a_wider_mode to use libc::umask with the target-specific mode_t signature, removing the hand-written extern declaration. Add libc as a direct dev-dependency, and serialize this process-wide umask mutation with other file-creating or umask-changing tests, restoring the previous mask safely before the test exits.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/taskito-server/src/runtime/listener.rs`:
- Around line 129-140: Update the documentation above the Unix socket binding
flow to remove the claim that no peer can observe the wide staging mode. State
that the deterministic staging path is briefly reachable in the same destination
directory before set_permissions narrows it, so the window is reduced but not
eliminated; do not implement the suggested private-subdirectory redesign unless
required elsewhere.
---
Outside diff comments:
In `@deploy/helm/taskito-server/README.md`:
- Around line 123-130: Update the “Probes” section to describe attach-only
releases accurately: when only attach.enabled is enabled, both probes use TCP
checks on the attach port rather than /health. State that /health applies to
dashboard or webhook roles, and that probing /readiness requires a post-render
Deployment patch because the chart does not configure it.
- Around line 119-121: Update the certificate rotation documentation to address
cert-manager Secret renewals: either describe and configure a Secret-change
restart mechanism for the Deployment, or explicitly document that a rollout is
required after renewal so the server rereads the PEM files.
---
Nitpick comments:
In `@crates/taskito-server/src/runtime/listener.rs`:
- Around line 344-370: Update the test
the_socket_is_never_visible_at_a_wider_mode to use libc::umask with the
target-specific mode_t signature, removing the hand-written extern declaration.
Add libc as a direct dev-dependency, and serialize this process-wide umask
mutation with other file-creating or umask-changing tests, restoring the
previous mask safely before the test exits.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bb7afae7-9f74-4b89-9e08-8aaed1fcffcb
📒 Files selected for processing (10)
crates/taskito-server/src/config/webhook.rscrates/taskito-server/src/main.rscrates/taskito-server/src/runtime/listener.rscrates/taskito-server/src/webhook/annotations.rscrates/taskito-server/src/webhook/inject.rscrates/taskito-server/src/webhook/mod.rscrates/taskito-server/tests/webhook_injection.rsdeploy/helm/taskito-server/README.mddeploy/helm/taskito-server/templates/NOTES.txtdeploy/helm/taskito-server/templates/deployment.yaml
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
ByteVeda/taskito(manual)
🚧 Files skipped from review as they are similar to previous changes (5)
- crates/taskito-server/src/config/webhook.rs
- crates/taskito-server/src/main.rs
- deploy/helm/taskito-server/templates/deployment.yaml
- crates/taskito-server/tests/webhook_injection.rs
- crates/taskito-server/src/webhook/annotations.rs
A staging path only shortened the window: it sat in the same directory and its name was predictable. A 0700 directory removes the search permission needed to reach it at all.
It was ASCII art in a code block, so it read as a code sample and ignored light/dark theming.
2 participants
Closes #556. S10 — the last phase of the executor-attach plan, plus the socket-permission follow-up that fell out of #611.
What lands
A Helm chart —
deploy/helm/taskito-server. One release runs the scheduler, the dashboard, and the injector, because they are one binary and one image. Turn off what you don't want.Annotation-driven sidecar injection —
TASKITO_WEBHOOK_LISTENserves a mutating admission webhook. An annotated pod gets an executor sidecar with no manifest edit beyond its own metadata:The injected container reuses the pod's own image reference, so nothing new is pulled and the pattern works for any language. It inherits the app container's
env/envFromtoo, so a handler reading the same config as the app keeps working; the injector's ownTASKITO_*always win over an inherited value.fix: chmod the attach socket to 0660—bindleft it umask-derived0755, which denies the group write, andconnect(2)needs write. Only the binding uid could attach, so a same-pod Unix-socket attach required two containers to share a uid. At0660a sharedfsGroupis enough, which is the thing you can actually configure. The docs shipped in #611 moved fromrunAsUsertofsGroupaccordingly.Decisions worth flagging
taskito-server, not a new crate. One image, one publish pipeline, one thing to version. The cost is thatConfig.dsnbecomesOption<String>— the webhook rewrites pod specs and reads no jobs, so it is the one role that runs without a database. Every other role still refuses to start without one.webhook.certManager.enabled=trueas the opt-in. The chart installs with no external dependency, and cert-manager stays available for anyone who wants real rotation.failurePolicy: Ignoreby default. An injector that is down must not stop pod creation cluster-wide. Move toFailonce a workload depends on the sidecar.One trap, guarded in CI
genCA/genSignedCertare random. A cert helper called from more than one template mints a different CA per call, so thecaBundleon theMutatingWebhookConfigurationstops signing the certificate the pod serves — and every admission call fails TLS verification, in a way nothing but a live cluster reveals. The helper memoises into.Values, andci-chart.ymldecodes both and runsopenssl verifyso it cannot regress.Verification
cargo test --workspace— 0 failures. 22 new tests: annotation parsing, patch building, the admission envelope, socket mode, and a router-level suite intests/webhook_injection.rs.cargo clippy --workspace --all-targetsclean;--features postgresand--features redischeck; the pyo3 tripwire stays empty for core, workflows and mesh.helm lintplus every role combination rendered, and every invalid combination confirmed refused.curl --cacertadmission call against the Service SAN hostname — first with the debug binary, then with the distroless image built fromdocker/scheduler.Dockerfile. Both returned the expected patch; the denial path returns the annotation name instatus.message.PT_INTERPcheck passes) and reports its version. Content size 11.2 MB → 11.9 MB for the TLS stack.scripts/version.mjsnow mirrorsChart.yaml, verified by--checkand by a--setround trip.Summary by CodeRabbit