Skip to content

fix: Decouple AppArmor nsjail installer DaemonSet from workflows.enabled - #356

Merged
mako-taco merged 2 commits into
mainfrom
jake/apparmor-decouple-workflows-gate
Jul 31, 2026
Merged

fix: Decouple AppArmor nsjail installer DaemonSet from workflows.enabled#356
mako-taco merged 2 commits into
mainfrom
jake/apparmor-decouple-workflows-gate

Conversation

@mako-taco

Copy link
Copy Markdown
Contributor

Summary

Fixes an AppArmor installer gate mismatch flagged by Greptile on PR #355 (#355 (comment)): when rr.jsExecutor.enabled=true and codeExecutor.appArmorProfileInstaller=true but workflows.enabled=false, the js-executor pod's wait-for-apparmor init container polls forever for /host-run/retool-executor.loaded, but the DaemonSet that creates that file (apparmor_nsjail_daemonset.yaml) was gated on retool.workflows.enabled and never rendered — so js-executor pods hang in Init forever.

rr.jsExecutor is an independent RR component that does not require workflows.enabled to be true, so gating the shared AppArmor installer DaemonSet/ConfigMap on workflows.enabled alone was incorrect; it needs to render whenever EITHER of its two consumers needs it (legacy code-executor, which is workflows-gated, OR RR js-executor, which is not).

Changes

  • Added a new helper retool.appArmorNsjailInstaller.enabled in _helpers.tpl that returns "1" when codeExecutor.appArmorProfileInstaller is true AND either retool.workflows.enabled OR rr.jsExecutor is enabled.
  • apparmor_nsjail_daemonset.yaml and apparmor_nsjail_configmap.yaml now gate on this new helper instead of the old workflows.enabled + appArmorProfileInstaller double-if.
  • Updated doc comments in values.yaml and charts/retool/ci/test-apparmor-nsjail-option.yaml to describe the corrected gating logic.
  • No other AppArmor config was affected — rr.agentSandbox.appArmorProfileInstaller (used by the separate gVisor-based agent-sandbox node-installer DaemonSet) was already self-contained and gated purely on its own component-enabled check, so it did not have this bug.

Testing

  • helm lint and helm template against all existing charts/retool/ci/*option.yaml overlays pass with no changes in behavior for existing configurations.
  • Verified via helm template that the new gate correctly handles all 4 combinations of (workflows.enabled, rr.jsExecutor.enabled) x (codeExecutor.appArmorProfileInstaller true/false) — renders only when the flag is true AND at least one consumer is enabled; unaffected when neither consumer needs it.
  • Live end-to-end validation on a real AKS cluster (Ubuntu 24.04, kernel.apparmor_restrict_unprivileged_userns=1): deployed just the DaemonSet + ConfigMap + js-executor Deployment into an isolated test namespace with workflows.enabled: false. Confirmed the DaemonSet still rolls out and loads the AppArmor profile on every node (AppArmor profiles loaded successfully in the install init container logs), and the js-executor pod's wait-for-apparmor init container completed instantly (Terminated/Completed, not hung) because the profile file was already present — the pod reached 1/1 Running with the container.apparmor.security.beta.kubernetes.io/js-executor: localhost/retool-executor annotation correctly applied (confined, not unconfined). Test namespace was torn down afterward; the real deployed retool Helm release on that cluster was untouched throughout.

Add a new retool.appArmorNsjailInstaller.enabled helper that renders the
shared AppArmor nsjail installer ConfigMap/DaemonSet whenever
codeExecutor.appArmorProfileInstaller is true AND either
retool.workflows.enabled or rr.jsExecutor is enabled, instead of gating on
workflows.enabled alone. Previously, when jsExecutor was enabled but
workflows was not, the installer DaemonSet never rendered, leaving
js-executor's wait-for-apparmor init container polling forever.

Co-authored-by: Cursor <cursoragent@cursor.com>
@greptile-apps

greptile-apps Bot commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
charts/retool/templates/_helpers.tpl Adds a shared helper that combines the installer flag with the two supported consumer-enablement paths.
charts/retool/templates/apparmor_nsjail_configmap.yaml Replaces the workflows-only rendering conditions with the shared installer helper.
charts/retool/templates/apparmor_nsjail_daemonset.yaml Uses the shared installer helper so the DaemonSet can render for an independently enabled js-executor.
charts/retool/values.yaml Documents the corrected installer rendering conditions for both consumers.
charts/retool/Chart.yaml Increments the Helm chart patch version for the rendering fix.

Reviews (2): Last reviewed commit: "chore: sync root values.yaml comment and..." | Re-trigger Greptile

Comment thread charts/retool/ci/test-apparmor-nsjail-option.yaml
- Mirror the appArmorProfileInstaller doc comment update into the
  top-level values.yaml (kept in sync with charts/retool/values.yaml
  per .github/workflows/values-yaml-synced.yaml).
- Bump charts/retool/Chart.yaml to 6.11.14 (ct lint requires a version
  bump for chart changes).

Co-authored-by: Cursor <cursoragent@cursor.com>
@mako-taco
mako-taco marked this pull request as ready for review July 31, 2026 21:20
@mako-taco
mako-taco merged commit 371750b into main Jul 31, 2026
14 checks passed
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.

2 participants