feat: Allow Setting Image Registry and Pull Secrets Globally #266
Conversation
|
|
WalkthroughUpdates Helm chart dependency version. Adds helper templates to centralize image and pullSecret rendering. Refactors deployment, job, and test templates to use new helpers. Expands values schema and defaults to support global registry, pull secrets, image registry/digest, and initContainer pull policy. Removes root-level imagePullSecrets from schema. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as Helm User
participant H as Helm Renderer
participant T as openfga.templates
participant C as Common Chart Helpers
U->>H: helm template/install
H->>T: Render deployment/job/test templates
T->>T: include "openfga.pullSecrets"/"jobPullSecrets"
T->>T: include "openfga.image"/"openfga.initContainer.image"
T->>C: common.images.image(imageRoot, global, chart)
C-->>T: Resolved image (registry/repo:tag or @digest)
T-->>H: Manifests with images & imagePullSecrets
H-->>U: Final Kubernetes manifests
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
charts/openfga/Chart.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
charts/openfga/Chart.yaml(1 hunks)charts/openfga/templates/_helpers.tpl(1 hunks)charts/openfga/templates/deployment.yaml(4 hunks)charts/openfga/templates/job.yaml(2 hunks)charts/openfga/templates/tests/test-connection.yaml(1 hunks)charts/openfga/values.schema.json(4 hunks)charts/openfga/values.yaml(2 hunks)
🔇 Additional comments (13)
charts/openfga/values.yaml (2)
1-13: LGTM: Well-structured hierarchical image configuration.The addition of global and component-specific image configuration follows Helm best practices:
- Global defaults (lines 1-3) for consistency across components
- Component-level overrides (lines 8, 12-13) for flexibility
- Support for both tag-based and digest-based image selection
The empty defaults ensure backward compatibility.
57-63: LGTM: Consistent initContainer configuration.The initContainer configuration mirrors the main image configuration structure, providing the same level of flexibility for registry, digest, and pull secrets.
charts/openfga/templates/deployment.yaml (2)
33-33: LGTM: Centralized pull secret management.The replacement of the manual
imagePullSecretsblock with theopenfga.pullSecretshelper centralizes secret management and ensures consistency across all pods.
43-43: LGTM: Consistent image resolution through helpers.The template now consistently uses:
openfga.initContainer.image(line 43) for the wait-for-migration containeropenfga.image(lines 56, 85) for both the migrate-database initContainer and main containerThis centralization enables global registry configuration and digest-based image selection.
Also applies to: 56-56, 85-85
charts/openfga/templates/tests/test-connection.yaml (1)
10-10: LGTM: Test pod aligned with deployment pattern.The test pod now uses the same helper templates (
openfga.pullSecretsandopenfga.image) as the deployment, ensuring consistency across all pod specifications.Also applies to: 13-13
charts/openfga/templates/job.yaml (2)
28-28: LGTM: Proper use of job-specific pull secrets.The job correctly uses
openfga.jobPullSecretsinstead ofopenfga.pullSecrets. Since the migration job doesn't use the initContainer image, excluding its pull secrets follows the principle of least privilege.
38-38: LGTM: Consistent image helper usage.The migration job now uses the
openfga.imagehelper, enabling centralized image configuration.charts/openfga/templates/_helpers.tpl (2)
157-159: LGTM: Correct image helper implementation.The
openfga.imagehelper correctly passes.context tocommon.images.imageand includes all required parameters (imageRoot, global, chart).
161-163: LGTM: Correct initContainer image helper implementation.The
openfga.initContainer.imagehelper correctly passes.context and required parameters. Note that it intentionally omits thechartparameter as initContainer images typically don't need chart-specific processing.charts/openfga/values.schema.json (3)
6-17: LGTM: Well-documented global configuration schema.The schema additions for
global.imageRegistryandglobal.imagePullSecretsinclude clear descriptions that explain their purpose and provide examples of how they interact with component-specific settings.
22-47: LGTM: Comprehensive image configuration schema.The schema properly documents:
image.registryfor component-specific registry overrideimage.digestwith clear precedence over tagimage.pullSecretsfor component-specific secretsThe descriptions reference official Kubernetes documentation for imagePullSecrets, which is helpful.
1130-1156: LGTM: Consistent initContainer schema additions.The initContainer schema additions mirror the main image configuration, providing:
- Registry override capability
- Digest-based image selection
- Component-specific pull secrets
- Clear documentation for each field
charts/openfga/Chart.yaml (1)
28-28: Manual verification required: upstream fetch failed Please confirm that Bitnami common chart v2.31.4 providescommon.images.imageandcommon.images.renderPullSecretshelpers and review its changelog for breaking changes since v2.13.3.
| {{- define "openfga.pullSecrets" -}} | ||
| {{- include "common.images.renderPullSecrets" (dict "images" (list .Values.image .Values.initContainer) "context" $) -}} | ||
| {{- end }} |
There was a problem hiding this comment.
Fix incorrect context parameter in helper template.
The openfga.pullSecrets helper passes $ (root context at template definition) instead of . (current context) to common.images.renderPullSecrets. This will cause the helper to receive the wrong context and fail to properly resolve image pull secrets.
Apply this diff to fix the context parameter:
{{- define "openfga.pullSecrets" -}}
-{{- include "common.images.renderPullSecrets" (dict "images" (list .Values.image .Values.initContainer) "context" $) -}}
+{{- include "common.images.renderPullSecrets" (dict "images" (list .Values.image .Values.initContainer) "context" .) -}}
{{- end }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {{- define "openfga.pullSecrets" -}} | |
| {{- include "common.images.renderPullSecrets" (dict "images" (list .Values.image .Values.initContainer) "context" $) -}} | |
| {{- end }} | |
| {{- define "openfga.pullSecrets" -}} | |
| {{- include "common.images.renderPullSecrets" (dict "images" (list .Values.image .Values.initContainer) "context" .) -}} | |
| {{- end }} |
🤖 Prompt for AI Agents
In charts/openfga/templates/_helpers.tpl around lines 145 to 147, the helper
uses `$` (root context) when calling common.images.renderPullSecrets which
causes the wrong context to be passed; change the context argument from `$` to
`.` so the current template context is forwarded to renderPullSecrets, ensuring
image and initContainer values resolve correctly.
| {{- define "openfga.jobPullSecrets" -}} | ||
| {{- include "common.images.renderPullSecrets" (dict "images" (list .Values.image) "context" $) -}} | ||
| {{- end }} |
There was a problem hiding this comment.
Fix incorrect context parameter in job helper template.
Same issue as in openfga.pullSecrets: the helper passes $ instead of . for the context parameter.
Apply this diff:
{{- define "openfga.jobPullSecrets" -}}
-{{- include "common.images.renderPullSecrets" (dict "images" (list .Values.image) "context" $) -}}
+{{- include "common.images.renderPullSecrets" (dict "images" (list .Values.image) "context" .) -}}
{{- end }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {{- define "openfga.jobPullSecrets" -}} | |
| {{- include "common.images.renderPullSecrets" (dict "images" (list .Values.image) "context" $) -}} | |
| {{- end }} | |
| {{- define "openfga.jobPullSecrets" -}} | |
| {{- include "common.images.renderPullSecrets" (dict "images" (list .Values.image) "context" .) -}} | |
| {{- end }} |
🤖 Prompt for AI Agents
In charts/openfga/templates/_helpers.tpl around lines 153 to 155, the
jobPullSecrets helper is passing the wrong context variable ($) to the include
of common.images.renderPullSecrets; update the include call to pass the current
context (.) instead of $ so the correct scope/values are available when
rendering pull secrets.
1 participant
Description
What problem is being solved?
The Helm chart lacked support for global image registry and pull secrets configuration. Users couldn't:
This made it difficult for organizations with private registries or specific image pull requirements to deploy OpenFGA without maintaining custom forks.
How is it being solved?
The solution leverages the Bitnami common chart library (upgraded from 2.13.3 to 2.31.4) which provides standardized helper functions for:
This follows Kubernetes and Helm best practices by supporting a hierarchy:
What changes are made to solve it?
charts/openfga/values.yaml:53-62
charts/openfga/templates/_helpers.tpl:144-162
charts/openfga/templates/*.yaml
charts/openfga/values.schema.json
charts/openfga/Chart.yaml
Review Checklist
mainSummary by CodeRabbit
New Features
Chores