Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @roderik, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request enhances the Helm chart structure by introducing a centralized mechanism for managing Kubernetes pod and container security contexts. It allows administrators to define default security settings at a global level, which subcharts can then inherit or explicitly override. This change streamlines security configuration across multiple components and improves consistency, while also updating the default persistence behavior for network nodes.
Highlights
- Global Security Contexts: Introduced a new
global.securityContextsconfiguration in the main network chart, allowing for the definition of default pod and container security contexts that can be inherited by subcharts. - Inheritance Logic for Security Contexts: Implemented logic within the Helm templates for
network-bootstrapperandnetwork-nodessubcharts to merge global security contexts with chart-specific overrides. This ensures that local chart values remain authoritative while falling back to global settings when local values are empty. - Documentation Updates: Updated
README.mdfiles andvalues.yamlcomments across affected charts to clearly document the new global security context fields and their inheritance behavior. - Persistence Default Change: Adjusted the default value for
persistence.enabledin thenetwork-nodeschart fromfalsetotrue, enabling persistent volume claims by default for ledger data.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in pull request comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- There are duplicated lines for loading and merging global securityContexts – consider extracting that into a shared helper template to avoid repetition across charts.
- The persistence.enabled default was updated to true in the network-nodes README but remains false in the values.yaml – update them consistently or revert the README change if unintended.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- There are duplicated lines for loading and merging global securityContexts – consider extracting that into a shared helper template to avoid repetition across charts.
- The persistence.enabled default was updated to true in the network-nodes README but remains false in the values.yaml – update them consistently or revert the README change if unintended.
## Individual Comments
### Comment 1
<location> `charts/network/charts/network-bootstrapper/templates/job.yaml:34-37` </location>
<code_context>
+ {{- $globalSecurityContexts := dig "securityContexts" $globalValues (dict) }}
+ {{- $podSecurityContext := mergeOverwrite (deepCopy (dig "pod" $globalSecurityContexts (dict))) (default (dict) .Values.podSecurityContext) }}
+ {{- $containerSecurityContext := mergeOverwrite (deepCopy (dig "container" $globalSecurityContexts (dict))) (default (dict) .Values.securityContext) }}
+ {{- if $podSecurityContext }}
securityContext:
- {{- toYaml . | nindent 8 }}
+ {{- toYaml $podSecurityContext | nindent 8 }}
{{- end }}
{{- with .Values.initContainers }}
</code_context>
<issue_to_address>
**suggestion:** Empty podSecurityContext objects will still render a securityContext block.
Consider adding a check to ensure $podSecurityContext is not empty before rendering the securityContext block to prevent unnecessary empty blocks in the manifest.
```suggestion
{{- if and $podSecurityContext (not (empty $podSecurityContext)) }}
securityContext:
{{- toYaml $podSecurityContext | nindent 8 }}
{{- end }}
```
</issue_to_address>
### Comment 2
<location> `charts/network/charts/network-bootstrapper/templates/job.yaml:44-47` </location>
<code_context>
containers:
- name: {{ .Chart.Name }}
- {{- with .Values.securityContext }}
+ {{- if $containerSecurityContext }}
securityContext:
- {{- toYaml . | nindent 12 }}
+ {{- toYaml $containerSecurityContext | nindent 12 }}
{{- end }}
image: "{{ .Values.image.repository }}:{{ .Values.image.tag | default .Chart.AppVersion }}"
</code_context>
<issue_to_address>
**suggestion:** Rendering empty containerSecurityContext may result in unnecessary manifest blocks.
Check that $containerSecurityContext is not empty before rendering, as with podSecurityContext, to prevent unnecessary empty blocks.
```suggestion
{{- if and $containerSecurityContext (not (empty $containerSecurityContext)) }}
securityContext:
{{- toYaml $containerSecurityContext | nindent 12 }}
{{- end }}
```
</issue_to_address>
### Comment 3
<location> `charts/network/charts/network-nodes/templates/statefulset-rpc.yaml:80-82` </location>
<code_context>
+ {{- $globalSecurityContexts := dig "securityContexts" $globalValues (dict) }}
+ {{- $podSecurityContext := mergeOverwrite (deepCopy (dig "pod" $globalSecurityContexts (dict))) (default (dict) .Values.podSecurityContext) }}
+ {{- $containerSecurityContext := mergeOverwrite (deepCopy (dig "container" $globalSecurityContexts (dict))) (default (dict) .Values.securityContext) }}
+ {{- if $podSecurityContext }}
securityContext:
- {{- toYaml . | nindent 8 }}
+ {{- toYaml $podSecurityContext | nindent 8 }}
{{- end }}
{{- with .Values.initContainers }}
</code_context>
<issue_to_address>
**suggestion:** Empty podSecurityContext objects may result in empty manifest blocks.
Check that podSecurityContext is not empty before rendering to prevent unnecessary empty manifest blocks.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| {{- if $podSecurityContext }} | ||
| securityContext: | ||
| {{- toYaml . | nindent 8 }} | ||
| {{- toYaml $podSecurityContext | nindent 8 }} | ||
| {{- end }} |
There was a problem hiding this comment.
suggestion: Empty podSecurityContext objects will still render a securityContext block.
Consider adding a check to ensure $podSecurityContext is not empty before rendering the securityContext block to prevent unnecessary empty blocks in the manifest.
| {{- if $podSecurityContext }} | |
| securityContext: | |
| {{- toYaml . | nindent 8 }} | |
| {{- toYaml $podSecurityContext | nindent 8 }} | |
| {{- end }} | |
| {{- if and $podSecurityContext (not (empty $podSecurityContext)) }} | |
| securityContext: | |
| {{- toYaml $podSecurityContext | nindent 8 }} | |
| {{- end }} |
| {{- if $containerSecurityContext }} | ||
| securityContext: | ||
| {{- toYaml . | nindent 12 }} | ||
| {{- toYaml $containerSecurityContext | nindent 12 }} | ||
| {{- end }} |
There was a problem hiding this comment.
suggestion: Rendering empty containerSecurityContext may result in unnecessary manifest blocks.
Check that $containerSecurityContext is not empty before rendering, as with podSecurityContext, to prevent unnecessary empty blocks.
| {{- if $containerSecurityContext }} | |
| securityContext: | |
| {{- toYaml . | nindent 12 }} | |
| {{- toYaml $containerSecurityContext | nindent 12 }} | |
| {{- end }} | |
| {{- if and $containerSecurityContext (not (empty $containerSecurityContext)) }} | |
| securityContext: | |
| {{- toYaml $containerSecurityContext | nindent 12 }} | |
| {{- end }} |
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable feature for inheriting global security context defaults in subcharts, which enhances consistency and simplifies configuration management. The implementation correctly allows local values to override global settings. The documentation updates are also clear and helpful. My main feedback is to refactor the duplicated logic for merging security contexts into helper templates. This will improve the long-term maintainability of the Helm charts by centralizing the logic.
| {{- $globalValues := (.Values.global | default (dict)) }} | ||
| {{- $globalSecurityContexts := dig "securityContexts" $globalValues (dict) }} | ||
| {{- $podSecurityContext := mergeOverwrite (deepCopy (dig "pod" $globalSecurityContexts (dict))) (default (dict) .Values.podSecurityContext) }} | ||
| {{- $containerSecurityContext := mergeOverwrite (deepCopy (dig "container" $globalSecurityContexts (dict))) (default (dict) .Values.securityContext) }} |
There was a problem hiding this comment.
To improve maintainability and avoid code duplication, this logic for merging global and local security contexts should be extracted into a helper template in charts/network/charts/network-bootstrapper/templates/_helpers.tpl. This same logic is repeated in the network-nodes subchart.
For example, you could create a helper named network-bootstrapper.getSecurityContexts:
{{/*
Get merged security contexts
*/}}
{{- define "network-bootstrapper.getSecurityContexts" -}}
{{- $globalValues := (.Values.global | default (dict)) -}}
{{- $globalSecurityContexts := dig "securityContexts" $globalValues (dict) -}}
{{- $podSecurityContext := mergeOverwrite (deepCopy (dig "pod" $globalSecurityContexts (dict))) (default (dict) .Values.podSecurityContext) -}}
{{- $containerSecurityContext := mergeOverwrite (deepCopy (dig "container" $globalSecurityContexts (dict))) (default (dict) .Values.securityContext) -}}
{{- $securityContexts := dict "pod" $podSecurityContext "container" $containerSecurityContext -}}
{{- toYaml $securityContexts -}}
{{- end -}}
You could then replace these lines with:
{{- $securityContexts := include "network-bootstrapper.getSecurityContexts" . | fromYaml }}
{{- $podSecurityContext := $securityContexts.pod }}
{{- $containerSecurityContext := $securityContexts.container }}
| {{- $globalValues := (.Values.global | default (dict)) }} | ||
| {{- $globalSecurityContexts := dig "securityContexts" $globalValues (dict) }} | ||
| {{- $podSecurityContext := mergeOverwrite (deepCopy (dig "pod" $globalSecurityContexts (dict))) (default (dict) .Values.podSecurityContext) }} | ||
| {{- $containerSecurityContext := mergeOverwrite (deepCopy (dig "container" $globalSecurityContexts (dict))) (default (dict) .Values.securityContext) }} |
There was a problem hiding this comment.
This logic for merging security contexts is duplicated in statefulset-validator.yaml. To improve maintainability, consider extracting it into a helper template in charts/network/charts/network-nodes/templates/_helpers.tpl.
For example, a helper nodes.getSecurityContexts could be created:
{{/*
Get merged security contexts
*/}}
{{- define "nodes.getSecurityContexts" -}}
{{- $globalValues := (.Values.global | default (dict)) -}}
{{- $globalSecurityContexts := dig "securityContexts" $globalValues (dict) -}}
{{- $podSecurityContext := mergeOverwrite (deepCopy (dig "pod" $globalSecurityContexts (dict))) (default (dict) .Values.podSecurityContext) -}}
{{- $containerSecurityContext := mergeOverwrite (deepCopy (dig "container" $globalSecurityContexts (dict))) (default (dict) .Values.securityContext) -}}
{{- $securityContexts := dict "pod" $podSecurityContext "container" $containerSecurityContext -}}
{{- toYaml $securityContexts -}}
{{- end -}}
Then, you could replace these lines in both this file and statefulset-validator.yaml with:
{{- $securityContexts := include "nodes.getSecurityContexts" . | fromYaml }}
{{- $podSecurityContext := $securityContexts.pod }}
{{- $containerSecurityContext := $securityContexts.container }}
| {{- $globalValues := (.Values.global | default (dict)) }} | ||
| {{- $globalSecurityContexts := dig "securityContexts" $globalValues (dict) }} | ||
| {{- $podSecurityContext := mergeOverwrite (deepCopy (dig "pod" $globalSecurityContexts (dict))) (default (dict) .Values.podSecurityContext) }} | ||
| {{- $containerSecurityContext := mergeOverwrite (deepCopy (dig "container" $globalSecurityContexts (dict))) (default (dict) .Values.securityContext) }} |
There was a problem hiding this comment.
This logic for merging security contexts is duplicated in statefulset-rpc.yaml. To improve maintainability, consider extracting it into a helper template in charts/network/charts/network-nodes/templates/_helpers.tpl.
For example, a helper nodes.getSecurityContexts could be created:
{{/*
Get merged security contexts
*/}}
{{- define "nodes.getSecurityContexts" -}}
{{- $globalValues := (.Values.global | default (dict)) -}}
{{- $globalSecurityContexts := dig "securityContexts" $globalValues (dict) -}}
{{- $podSecurityContext := mergeOverwrite (deepCopy (dig "pod" $globalSecurityContexts (dict))) (default (dict) .Values.podSecurityContext) -}}
{{- $containerSecurityContext := mergeOverwrite (deepCopy (dig "container" $globalSecurityContexts (dict))) (default (dict) .Values.securityContext) -}}
{{- $securityContexts := dict "pod" $podSecurityContext "container" $containerSecurityContext -}}
{{- toYaml $securityContexts -}}
{{- end -}}
Then, you could replace these lines in both this file and statefulset-rpc.yaml with:
{{- $securityContexts := include "nodes.getSecurityContexts" . | fromYaml }}
{{- $podSecurityContext := $securityContexts.pod }}
{{- $containerSecurityContext := $securityContexts.container }}
![seer-by-sentry[bot]](https://avatars.githubusercontent.com/in/801464?s=60&v=4){kind=small}
| {{- end }} | ||
| {{- with .Values.podSecurityContext }} | ||
| {{- if $podSecurityContext }} | ||
| securityContext: | ||
| {{- toYaml . | nindent 8 }} | ||
| {{- toYaml $podSecurityContext | nindent 8 }} |
There was a problem hiding this comment.
Potential bug: The change from with to if causes an empty securityContext: {} block to be rendered when it was previously omitted, altering the generated Kubernetes manifest.
-
Description: The Helm template logic was changed from using
{{- with .Values.podSecurityContext }}to{{- if $podSecurityContext }}. The new variable$podSecurityContextis the result of amergeOverwritewhich always returns a map. In Helm, an empty map is falsy for awithblock but truthy for anifblock. Consequently, when both global and chart-specific security contexts are empty (the default configuration), the template will now render an emptysecurityContext: {}block in the Kubernetes manifest. Previously, this block was omitted entirely. This changes the generated manifest, as Kubernetes treats an omitted security context differently from an explicitly empty one, which can affect how security policies are applied. -
Suggested fix: The
ifcondition should be modified to check if the merged security context dictionary is non-empty before rendering the block. For example, using{{- if gt (len $podSecurityContext) 0 }}would restore the previous behavior of only rendering thesecurityContextblock when it contains actual values, preventing an empty{}from being rendered.
severity: 0.55, confidence: 0.95
Did we get this right? 👍 / 👎 to inform future reviews.
|
To view in Slack, search for: 1758206827.609169 |
github-actions
Bot
added
qa:success
github-actions
Bot
added
status:merged
1 participant
Summary
Testing
Summary by Sourcery
Seed Helm chart pod and container security contexts with new global defaults while preserving chart-specific overrides, update default persistence setting, and refresh chart documentation.
Enhancements:
Documentation: