Skip to content

[INF-7190] allow disable of seccomp profile installation#346

Draft
ryanartecona wants to merge 1 commit into
mainfrom
ra/seccomp-no-install
Draft

[INF-7190] allow disable of seccomp profile installation#346
ryanartecona wants to merge 1 commit into
mainfrom
ra/seccomp-no-install

Conversation

@ryanartecona

Copy link
Copy Markdown
Contributor

Requested for scenarios where the operator prefers to manage custom seccomp profiles out of band, so pods don't need sensitive host mounts.

@greptile-apps

greptile-apps Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR adds an installSeccompProfile flag to both codeExecutor and rr.jsExecutor so cluster operators who manage seccomp profiles out of band (node image, DaemonSet, etc.) can skip the install-seccomp initContainer, its ConfigMap, and the sensitive hostPath mount while still having pods reference the profile by path.

  • codeExecutor: a new $installSeccomp variable gates the initContainer, ConfigMap, and hostPath volume; the seccompProfile reference in the container spec is controlled by the pre-existing $useSecComp flag, so the two concerns are cleanly separated.
  • rr.jsExecutor: equivalent gating added for the initContainer, ConfigMap, and volumes; a CI test overlay exercises both executors with installSeccompProfile: false.

Confidence Score: 4/5

Safe to merge; the default value of installSeccompProfile: true preserves existing behavior for all current users.

The logic is correct and the new flag is cleanly separated from the existing useSeccompProfile gate. The two minor findings in deployment_js_executor.yaml — an unconditional checksum/seccomp annotation that can cause spurious rollouts for out-of-band users, and a volumes: key that can render without items — do not affect correctness or the default deployment path.

charts/retool/templates/deployment_js_executor.yaml — the checksum/seccomp annotation and the volumes: rendering when installSeccompProfile: false are worth a second look.

Important Files Changed

Filename Overview
charts/retool/templates/deployment_js_executor.yaml Adds outer if or installSeccompProfile initContainers guard, wraps seccomp volumes in if installSeccompProfile; unconditional checksum/seccomp annotation and possible empty volumes: key are minor issues.
charts/retool/templates/deployment_code_executor.yaml Introduces $installSeccomp derived from $useSecComp && installSeccompProfile; initContainer, its ConfigMap volume, and hostPath volume are all gated on it consistently.
charts/retool/templates/configmap_code_executor.yaml Correctly adds installSeccompProfile to the ConfigMap guard so the ConfigMap is omitted when install is disabled.
charts/retool/templates/configmap_js_executor.yaml Wraps ConfigMap in if installSeccompProfile; straightforward and correct.
charts/retool/values.yaml Adds installSeccompProfile: true defaults with clear comments explaining the out-of-band profile management use-case for both codeExecutor and rr.jsExecutor.
charts/retool/ci/test-seccomp-install-disabled-option.yaml New CI test overlay exercising installSeccompProfile: false for both executors; covers the new feature path in helm lint.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[useSeccompProfile?] -- false --> P[privileged: true\nno seccomp]
    A -- true --> B{installSeccompProfile?}
    B -- true --> C[Render ConfigMap\nRender initContainer\nRender hostPath volume\nReference seccompProfile in container]
    B -- false --> D[Skip ConfigMap\nSkip initContainer\nSkip hostPath volume\nReference seccompProfile in container\n⚠️ Profile must exist on node]

    E[jsExecutor\nAlways uses seccomp] --> F{installSeccompProfile?}
    F -- true --> G[Render ConfigMap\nRender initContainer\nRender hostPath volume\nReference seccompProfile in container]
    F -- false --> H[Skip ConfigMap\nSkip initContainer\nSkip hostPath volume\nReference seccompProfile in container\n⚠️ Profile must exist on node]
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A[useSeccompProfile?] -- false --> P[privileged: true\nno seccomp]
    A -- true --> B{installSeccompProfile?}
    B -- true --> C[Render ConfigMap\nRender initContainer\nRender hostPath volume\nReference seccompProfile in container]
    B -- false --> D[Skip ConfigMap\nSkip initContainer\nSkip hostPath volume\nReference seccompProfile in container\n⚠️ Profile must exist on node]

    E[jsExecutor\nAlways uses seccomp] --> F{installSeccompProfile?}
    F -- true --> G[Render ConfigMap\nRender initContainer\nRender hostPath volume\nReference seccompProfile in container]
    F -- false --> H[Skip ConfigMap\nSkip initContainer\nSkip hostPath volume\nReference seccompProfile in container\n⚠️ Profile must exist on node]
Loading

Comments Outside Diff (1)

  1. charts/retool/templates/deployment_js_executor.yaml, line 26 (link)

    P2 Unconditional checksum/seccomp annotation when profile install is disabled

    checksum/seccomp is emitted regardless of installSeccompProfile, so every time files/nsjail-seccomp.json changes in the chart the pod will roll — even for users who set installSeccompProfile: false and manage the profile entirely out of band. Those users never consume this ConfigMap or volume, so the rollout is spurious. Compare deployment_code_executor.yaml where the equivalent annotation is wrapped in {{- if $useSecComp }}. Consider gating this line on .Values.rr.jsExecutor.installSeccompProfile as well.

Reviews (1): Last reviewed commit: "allow disable of seccomp profile install..." | Re-trigger Greptile

Comment on lines 162 to +171
volumes:
{{- if .Values.rr.jsExecutor.installSeccompProfile }}
- name: seccomp-profile
configMap:
name: {{ template "retool.fullname" . }}-js-executor-seccomp
- name: host-seccomp
hostPath:
path: /var/lib/kubelet/seccomp
type: DirectoryOrCreate
{{- end }}

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.

P2 volumes: key can render with no items

Before this PR the seccomp-profile and host-seccomp entries were always emitted, so volumes: always had at least two items. With installSeccompProfile: false and no user-defined rr.jsExecutor.volumes, extraConfigMapMounts, or extraVolumes, the volumes: key is now rendered with nothing beneath it (volumes: null in the parsed YAML). The Kubernetes API treats null as an empty list and will accept the pod spec, so this won't break deployments, but it is inconsistent with the explicit-list convention used elsewhere. Consider wrapping the volumes: key itself in an outer conditional, or at minimum emitting volumes: [] in the empty case.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

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.

1 participant