ROSAENG-130 | feat: add support for AWS podidentity#67
ROSAENG-130 | feat: add support for AWS podidentity#67cdoan1 wants to merge 3 commits intoopenshift-hyperfleet:mainfrom
Conversation
Add support for using AWS Pod Identity with Secrets Store CSI driver to mount database credentials from AWS Secrets Manager, eliminating the need for Kubernetes Secrets. This includes a new SecretProviderClass template, file-based credential flags for the application, and comprehensive Helm chart configuration options. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Move AWS secretName to aws subsection and clean up adapter configuration comments for better clarity. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @cdoan1. Thanks for your PR. I'm waiting for a openshift-hyperfleet member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
WalkthroughThe changes add optional AWS Secrets Store CSI integration for external database credentials and conditionalize init/main container arguments, secret mount paths, and volumes based on .Values.database.external.enabled and .Values.database.external.usePodIdentity. A new SecretProviderClass template is introduced (rendered when external DB, pod identity, and createSecretProviderClass are enabled). When pod identity is enabled, the chart mounts CSI-provided secret files; when disabled, it continues to mount a Kubernetes Secret by name. Values.yaml gains configuration for the external AWS secrets flow. Sequence Diagram(s)sequenceDiagram
actor Helm
participant K8s_API as "Kubernetes API"
participant SPC as "SecretProviderClass (CSI)"
participant Pod as "Pod (init + main)"
participant CSI as "Secrets Store CSI Driver"
participant AWS as "AWS Secrets Manager"
participant K8s_Secret as "Kubernetes Secret (opaque)"
Helm->>K8s_API: Render & create resources (Deployment, SPC if enabled)
K8s_API->>SPC: Register SecretProviderClass
K8s_API->>Pod: Create Pod (initContainer + main)
Pod->>CSI: Node CSI mounts requested secret provider
CSI->>AWS: Request secret using pod identity (if enabled)
AWS->>CSI: Return secret payload
CSI->>Pod: Project secret files into mount path
alt usePodIdentity == true
Pod->>Pod: initContainer reads mounted secret files and runs migration
Pod->>Pod: main container reads mounted secret files to connect to DB
else usePodIdentity == false
K8s_API->>K8s_Secret: Provide Secret by name
K8s_Secret->>Pod: Mount secret into pod (legacy path)
Pod->>Pod: initContainer/main read secret files from secret mount
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Important Merge conflicts detected (Beta)
✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@charts/templates/deployment.yaml`:
- Around line 43-50: The --db-sslmode flag is only rendered inside the
pod-identity branch so .Values.database.external.sslMode is ignored when using
external DB with secret mount mode; update charts/templates/deployment.yaml to
ensure the --db-sslmode={{ .Values.database.external.sslMode | default "require"
}} argument is emitted whenever .Values.database.external.enabled is true
(either move it outside the usePodIdentity conditional or add a separate
conditional for external enabled and not usePodIdentity), referencing the
existing conditional keys .Values.database.external.enabled and
.Values.database.external.usePodIdentity and the flag name --db-sslmode to
locate where to add/move the line.
- Around line 145-151: The CSI volumeAttributes currently always renders a
secretProviderClass even when
.Values.database.external.createSecretProviderClass is false and
.Values.database.external.secretProviderClass is empty; update the template
around the csi block to conditionally render the secretProviderClass key only
when the computed value is non-empty (check
.Values.database.external.createSecretProviderClass OR
.Values.database.external.secretProviderClass) so the CSI config is omitted if
no provider class is provided; locate the ternary expression that builds the
name (uses include "hyperfleet-api.fullname",
.Values.database.external.secretProviderClass,
.Values.database.external.createSecretProviderClass) and wrap or gate the
secretProviderClass output with an if that ensures the rendered string is not
empty.
In `@charts/templates/secretproviderclass.yaml`:
- Around line 13-16: The SecretProviderClass template currently renders
potentially empty values for region and objectName which defers failure to
runtime; update the template to validate and fail fast by wrapping
.Values.database.external.aws.region and
.Values.database.external.aws.secretName with the Helm required function (e.g.,
required "aws.region is required" .Values.database.external.aws.region) so
template rendering fails when those inputs are empty; ensure the required checks
are applied where region and objectName are inserted in the SecretProviderClass
objects block to prevent creating a pod-mount-time error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0a3e0604-0fcb-4da0-b5bf-1c29e58c6e3f
📒 Files selected for processing (3)
charts/templates/deployment.yamlcharts/templates/secretproviderclass.yamlcharts/values.yaml
| {{- if and .Values.database.external.enabled .Values.database.external.usePodIdentity }} | ||
| csi: | ||
| driver: secrets-store.csi.k8s.io | ||
| readOnly: true | ||
| volumeAttributes: | ||
| secretProviderClass: {{ ternary (printf "%s-db-secrets" (include "hyperfleet-api.fullname" .)) .Values.database.external.secretProviderClass .Values.database.external.createSecretProviderClass | quote }} | ||
| {{- else }} |
There was a problem hiding this comment.
Add a render-time guard for empty secretProviderClass.
At Line 150, if createSecretProviderClass=false and database.external.secretProviderClass is empty, the pod renders with an invalid CSI config and fails at startup.
🔧 Proposed fix
- name: secrets
{{- if and .Values.database.external.enabled .Values.database.external.usePodIdentity }}
+ {{- $spcName := ternary (printf "%s-db-secrets" (include "hyperfleet-api.fullname" .)) .Values.database.external.secretProviderClass .Values.database.external.createSecretProviderClass }}
+ {{- if empty $spcName }}
+ {{- fail "database.external.secretProviderClass is required when usePodIdentity=true and createSecretProviderClass=false" }}
+ {{- end }}
csi:
driver: secrets-store.csi.k8s.io
readOnly: true
volumeAttributes:
- secretProviderClass: {{ ternary (printf "%s-db-secrets" (include "hyperfleet-api.fullname" .)) .Values.database.external.secretProviderClass .Values.database.external.createSecretProviderClass | quote }}
+ secretProviderClass: {{ $spcName | quote }}
{{- else }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@charts/templates/deployment.yaml` around lines 145 - 151, The CSI
volumeAttributes currently always renders a secretProviderClass even when
.Values.database.external.createSecretProviderClass is false and
.Values.database.external.secretProviderClass is empty; update the template
around the csi block to conditionally render the secretProviderClass key only
when the computed value is non-empty (check
.Values.database.external.createSecretProviderClass OR
.Values.database.external.secretProviderClass) so the CSI config is omitted if
no provider class is provided; locate the ternary expression that builds the
name (uses include "hyperfleet-api.fullname",
.Values.database.external.secretProviderClass,
.Values.database.external.createSecretProviderClass) and wrap or gate the
secretProviderClass output with an if that ensures the rendered string is not
empty.
There was a problem hiding this comment.
@cdoan1, sounds good! The proposed fix above adds the $spcName variable and the {{- if empty $spcName }} guard to fail early at render time, so it should be straightforward to drop in. Feel free to ping me once it's in and I'll take another look. 👍
🧠 Learnings used
Learnt from: rafabene
Repo: openshift-hyperfleet/hyperfleet-api PR: 43
File: charts/templates/servicemonitor.yaml:1-1
Timestamp: 2026-02-02T18:38:24.725Z
Learning: In the openshift-hyperfleet/hyperfleet-api repo, Helm template files under charts/templates/ use Go template syntax (e.g., {{ .Values.xxx }}). Do not lint or flag yamllint errors for these files, as standard YAML linters do not understand Helm Go templates. If you must lint, configure yamllint to skip these files or use a Helm-aware linter.
| region: {{ .Values.database.external.aws.region | quote }} | ||
| objects: | | ||
| - objectName: {{ .Values.database.external.aws.secretName | quote }} | ||
| objectType: "secretsmanager" |
There was a problem hiding this comment.
Fail fast when AWS SecretProviderClass inputs are empty.
At Line 13 and Line 15, empty region or objectName can render successfully but fail later at pod mount/runtime. Guard these with required so misconfiguration is caught during template rendering.
🔧 Proposed fix
{{- if and .Values.database.external.enabled .Values.database.external.usePodIdentity .Values.database.external.createSecretProviderClass }}
+{{- $awsRegion := required "database.external.aws.region is required when createSecretProviderClass=true" .Values.database.external.aws.region }}
+{{- $awsSecretName := required "database.external.aws.secretName is required when createSecretProviderClass=true" .Values.database.external.aws.secretName }}
apiVersion: secrets-store.csi.x-k8s.io/v1
kind: SecretProviderClass
metadata:
@@
spec:
provider: aws
parameters:
usePodIdentity: "true"
- region: {{ .Values.database.external.aws.region | quote }}
+ region: {{ $awsRegion | quote }}
objects: |
- - objectName: {{ .Values.database.external.aws.secretName | quote }}
+ - objectName: {{ $awsSecretName | quote }}
objectType: "secretsmanager"📝 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.
| region: {{ .Values.database.external.aws.region | quote }} | |
| objects: | | |
| - objectName: {{ .Values.database.external.aws.secretName | quote }} | |
| objectType: "secretsmanager" | |
| {{- if and .Values.database.external.enabled .Values.database.external.usePodIdentity .Values.database.external.createSecretProviderClass }} | |
| {{- $awsRegion := required "database.external.aws.region is required when createSecretProviderClass=true" .Values.database.external.aws.region }} | |
| {{- $awsSecretName := required "database.external.aws.secretName is required when createSecretProviderClass=true" .Values.database.external.aws.secretName }} | |
| apiVersion: secrets-store.csi.x-k8s.io/v1 | |
| kind: SecretProviderClass | |
| metadata: | |
| name: {{ .Values.database.external.secretProviderClassName }} | |
| spec: | |
| provider: aws | |
| parameters: | |
| usePodIdentity: "true" | |
| region: {{ $awsRegion | quote }} | |
| objects: | | |
| - objectName: {{ $awsSecretName | quote }} | |
| objectType: "secretsmanager" | |
| {{- end }} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@charts/templates/secretproviderclass.yaml` around lines 13 - 16, The
SecretProviderClass template currently renders potentially empty values for
region and objectName which defers failure to runtime; update the template to
validate and fail fast by wrapping .Values.database.external.aws.region and
.Values.database.external.aws.secretName with the Helm required function (e.g.,
required "aws.region is required" .Values.database.external.aws.region) so
template rendering fails when those inputs are empty; ensure the required checks
are applied where region and objectName are inserted in the SecretProviderClass
objects block to prevent creating a pod-mount-time error.
There was a problem hiding this comment.
@cdoan1, sounds good! Here's a quick recap of the proposed change for reference when you implement it:
{{- if and .Values.database.external.enabled .Values.database.external.usePodIdentity .Values.database.external.createSecretProviderClass }}
{{- $awsRegion := required "database.external.aws.region is required when createSecretProviderClass=true" .Values.database.external.aws.region }}
{{- $awsSecretName := required "database.external.aws.secretName is required when createSecretProviderClass=true" .Values.database.external.aws.secretName }}
apiVersion: secrets-store.csi.x-k8s.io/v1
kind: SecretProviderClass
metadata:
name: {{ include "hyperfleet-api.fullname" . }}-db-secrets
namespace: {{ .Release.Namespace }}
labels:
{{- include "hyperfleet-api.labels" . | nindent 4 }}
spec:
provider: aws
parameters:
usePodIdentity: "true"
region: {{ $awsRegion | quote }}
objects: |
- objectName: {{ $awsSecretName | quote }}
objectType: "secretsmanager"Using required ensures helm template / helm install fails immediately with a clear error message if either value is missing, rather than silently producing a broken SecretProviderClass that only fails at pod mount time.
🧠 Learnings used
Learnt from: rafabene
Repo: openshift-hyperfleet/hyperfleet-api PR: 43
File: charts/templates/servicemonitor.yaml:1-1
Timestamp: 2026-02-02T18:38:24.725Z
Learning: In the openshift-hyperfleet/hyperfleet-api repo, Helm template files under charts/templates/ use Go template syntax (e.g., {{ .Values.xxx }}). Do not lint or flag yamllint errors for these files, as standard YAML linters do not understand Helm Go templates. If you must lint, configure yamllint to skip these files or use a Helm-aware linter.
|
/ok-to-test |
ciaranRoche
left a comment
ciaranRoche
left a comment
There was a problem hiding this comment.
Overall: The feature is needed, but the implementation is too coupled to AWS. This chart should stay cloud-agnostic — we don't want to maintain database.external.aws.*, database.external.gcp.*, database.external.azure.* etc. as we onboard more providers.
The core issue: The chart doesn't need to know how secrets get into the volume — it just needs to know where to find them. The --db-*-file flags already handle the rest.
Suggested approach:
- Drop
secretproviderclass.yamlentirely — users create their ownSecretProviderClass(AWS, GCP, Azure, Vault) outside the chart and pass the name in - Replace
usePodIdentity/aws.*/createSecretProviderClasswith a single genericsecretVolumeoverride:
database:
external:
enabled: false
secretName: "" # existing K8s Secret (current behavior)
sslMode: require # keep this - generic across clouds
secretMountPath: /mnt/secrets-store # keep this - generic
# Override the volume source for secrets (e.g., CSI driver, projected volume)
# When set, replaces the default Kubernetes Secret volume
secretVolume: {}
# csi:
# driver: secrets-store.csi.k8s.io
# readOnly: true
# volumeAttributes:
# secretProviderClass: my-aws-spc- In
deployment.yaml, the volume section becomes:
- name: secrets
{{- if .Values.database.external.secretVolume }}
{{- toYaml .Values.database.external.secretVolume | nindent 8 }}
{{- else }}
secret:
secretName: {{ .Values.database.external.secretName }}
{{- end }}- The
--db-*-fileargs should be passed wheneverdatabase.external.enabledis true (not gated onusePodIdentity), since file-based config works for both K8s secrets and CSI volumes.
Benefits:
- Works with AWS Secrets Manager, GCP Secret Manager, Azure Key Vault, HashiCorp Vault — anything that provides a CSI volume or projected volume
- Zero AWS-specific code in the chart
- Users document their
SecretProviderClassin their own infra repo where it belongs sslModeandsecretMountPathare kept — they're genuinely generic
The other changes in this PR (adapter comment cleanup, sslMode) are fine to keep.
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
charts/templates/secretproviderclass.yaml (1)
1-1:⚠️ Potential issue | 🟠 MajorFail fast for required AWS inputs in this template.
region(Line 13) andobjectName(Line 15) can render as empty values, which defers failure to mount/runtime. Add Helmrequiredchecks right after theifguard and use those vars inregion/objectName.🔧 Proposed fix
{{- if and .Values.database.external.enabled .Values.database.external.usePodIdentity .Values.database.external.createSecretProviderClass }} +{{- $awsRegion := required "database.external.aws.region is required when createSecretProviderClass=true" .Values.database.external.aws.region }} +{{- $awsSecretName := required "database.external.aws.secretName is required when createSecretProviderClass=true" .Values.database.external.aws.secretName }} apiVersion: secrets-store.csi.x-k8s.io/v1 kind: SecretProviderClass metadata: name: {{ include "hyperfleet-api.fullname" . }}-db-secrets @@ parameters: usePodIdentity: "true" - region: {{ .Values.database.external.aws.region | quote }} + region: {{ $awsRegion | quote }} objects: | - - objectName: {{ .Values.database.external.aws.secretName | quote }} + - objectName: {{ $awsSecretName | quote }} objectType: "secretsmanager"As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
Also applies to: 13-15
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/templates/secretproviderclass.yaml` at line 1, Add Helm required checks immediately after the existing if guard in the charts/templates/secretproviderclass.yaml template to fail fast when mandatory AWS inputs are missing: call required with a clear message for the region and for the objectName values (the same .Values database.external keys used later) so the template fails during chart rendering instead of at mount/runtime; keep using the existing region and objectName symbols in the region and objectName fields after validating them with required.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@charts/templates/secretproviderclass.yaml`:
- Line 1: Add Helm required checks immediately after the existing if guard in
the charts/templates/secretproviderclass.yaml template to fail fast when
mandatory AWS inputs are missing: call required with a clear message for the
region and for the objectName values (the same .Values database.external keys
used later) so the template fails during chart rendering instead of at
mount/runtime; keep using the existing region and objectName symbols in the
region and objectName fields after validating them with required.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 62120a97-48a9-4cde-a6dd-964ba1985da6
📒 Files selected for processing (1)
charts/templates/secretproviderclass.yaml
|
@cdoan1: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
3 participants
On AWS, the prod deployment can use AWS MQ and AWS RDS
With eks clusters, we can generate the secrets in aws secret manager, and sync these secrets down to the pod via pod identity and secretprovderclass.
Summary by CodeRabbit
New Features
Chores