Skip to content

feat(security): add security context support for containers and pod templates#1156

Open
facorazza wants to merge 2 commits into
nats-io:mainfrom
facorazza:main
Open

feat(security): add security context support for containers and pod templates#1156
facorazza wants to merge 2 commits into
nats-io:mainfrom
facorazza:main

Conversation

@facorazza

Copy link
Copy Markdown

No description provided.

@synadia-claude-reviewer

synadia-claude-reviewer Bot commented Apr 18, 2026

Copy link
Copy Markdown

Claude finished @facorazza's task in 3m 16s —— View job


Review: feat(security): add security context support for containers and pod templates

  • Gather context
  • Read changed files
  • Analyze correctness and security
  • Post review

Summary

The approach is sound — all security contexts default to {} and use {{- with }} so they're upgrade-safe and only render when set. One CI-breaking bug was found; see inline comment.


Issues

1. nindent violation in nats-box/deployment/container.yamlwill fail CI (inline comment left)

helm/charts/nats/files/nats-box/deployment/container.yaml, line 54:

securityContext:
{{- toYaml . | nindent 2 }}   ← 0 spaces before {{, but nindent 2 requires 2

The CI nindent check (test.yaml) verifies that every nindent N tag has exactly N spaces of leading whitespace in the source file. Every other nindent 2 in this chart has {{- (2 spaces). This is the only one with 0 spaces and will fail the check.

Fix:

securityContext:
  {{- toYaml . | nindent 2 }}

2. Silent relocation of natsBox.container.resources in values.yaml

-      resources: {}   ← was at 6-space indent (inside image:)
+    resources: {}     ← now at 4-space indent (inside container:)

Before this PR, the default for .Values.natsBox.container.resources was effectively nil (the key lived at .natsBox.container.image.resources). This fixes a pre-existing bug, but it's an incidental structural change bundled into this PR with no mention in the description. Worth noting for changelog/reviewers.

3. No tests added

The Go test suite in helm/charts/nats/test/ has no coverage for securityContext on any of the new fields. Given the nindent bug caught above, a test rendering a chart with container.securityContext set would have caught it immediately.


Non-issues

  • All other nindent values (nindent 4 in pod-template files, nindent 2 in other container files) are correctly aligned with their template indentation level.
  • {{- with .Values.*.securityContext }} correctly suppresses output when the value is {}, so no empty securityContext: keys will be rendered by default.
  • natsBox.podTemplate.securityContext and natsBox.container.securityContext are logically placed and follow the existing merge/patch extension pattern.

Comment thread helm/charts/nats/files/nats-box/deployment/container.yaml Outdated
Co-authored-by: synadia-claude-reviewer[bot] <265638981+synadia-claude-reviewer[bot]@users.noreply.github.com>
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