feat(SecretManagement): migrate secrets to AWS SM #107
Conversation
WalkthroughThis pull request updates the Helm chart versions for the "Agent" and "Regions" entries in multiple README files. The chart version for the "Agent" is changed from Changes
Sequence Diagram(s)sequenceDiagram
participant T as Template Engine
participant V as Values
participant K as Kubernetes Secret
T->>V: Check length of agent.authentication.existingSecretToken
alt Token provided
T->>K: Retrieve token via secretKeyRef
K-->>T: Return secret token
else No token provided
T->>V: Use agent.authentication.token value
V-->>T: Return default token
end
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
charts/agent/templates/_helpers.tpl
Outdated
| {{- if gt (len .Values.agent.authentication.existingSecretToken) 0 }} | ||
| valueFrom: | ||
| secretKeyRef: | ||
| name: {{ .Values.agent.authentication.existingSecretToken }} |
There was a problem hiding this comment.
can you rename this to existingSecret as all charts ?
charts/agent/templates/_helpers.tpl
Outdated
| {{- if gt (len .Values.agent.authentication.existingSecretToken) 0 }} | ||
| valueFrom: | ||
| secretKeyRef: | ||
| name: {{ .Values.agent.authentication.existingSecretToken }} | ||
| key: "token" |
There was a problem hiding this comment.
hardcoded, can you make the key available in values ?
![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
🧹 Nitpick comments (1)
charts/agent/templates/_helpers.tpl (1)
29-36: Consider making the secret key configurable.The implementation assumes the token is stored under the key "token" in the Kubernetes secret. Consider making this configurable to support different secret structures.
{{- if gt (len .Values.agent.authentication.existingSecretToken) 0 }} valueFrom: secretKeyRef: name: {{ .Values.agent.authentication.existingSecretToken }} - key: "token" + key: {{ .Values.agent.authentication.existingSecretTokenKey | default "token" }} {{- else }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
charts/agent/Chart.yamlis excluded by!**/*.yamlcharts/agent/values.schema.jsonis excluded by!**/*.jsoncharts/agent/values.yamlis excluded by!**/*.yamlcharts/regions/Chart.lockis excluded by!**/*.lock,!**/*.lock
📒 Files selected for processing (4)
README.md(1 hunks)charts/agent/README.md(2 hunks)charts/agent/templates/_helpers.tpl(1 hunks)charts/regions/README.md(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- README.md
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Tests
| value: {{ .Values.agent.authentication.token }} | ||
| {{- end }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for empty token value.
The fallback case should validate that the token value is not empty to prevent authentication issues.
{{- else }}
+{{- if not .Values.agent.authentication.token }}
+{{- fail "authentication.token is required when existingSecretToken is not provided" }}
+{{- end }}
value: {{ .Values.agent.authentication.token }}📝 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.
| value: {{ .Values.agent.authentication.token }} | |
| {{- end }} | |
| {{- else }} | |
| {{- if not .Values.agent.authentication.token }} | |
| {{- fail "authentication.token is required when existingSecretToken is not provided" }} | |
| {{- end }} | |
| value: {{ .Values.agent.authentication.token }} | |
| {{- end }} |
charts/agent/README.md
Outdated
| | affinity | object | `{}` | | | ||
| | agent.authentication.clientID | string | `""` | | | ||
| | agent.authentication.clientSecret | string | `""` | | | ||
| | agent.authentication.existingSecretToken | string | `""` | | |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance documentation for existingSecretToken.
The new configuration key needs a description explaining its purpose and usage. Consider adding:
- Purpose: For using existing Kubernetes secrets
- Format: Expected secret format
- Example usage
-| agent.authentication.existingSecretToken | string | `""` | |
+| agent.authentication.existingSecretToken | string | `""` | Name of an existing Kubernetes secret containing the authentication token. The secret must contain a key named 'token'. Example: "my-auth-secret" |📝 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.
| | agent.authentication.existingSecretToken | string | `""` | | | |
| | agent.authentication.existingSecretToken | string | `""` | Name of an existing Kubernetes secret containing the authentication token. The secret must contain a key named 'token'. Example: "my-auth-secret" | |
charts/regions/README.md
Outdated
| | agent.affinity | object | `{}` | | | ||
| | agent.agent.authentication.clientID | string | `""` | | | ||
| | agent.agent.authentication.clientSecret | string | `""` | | | ||
| | agent.agent.authentication.existingSecretToken | string | `""` | | |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Maintain consistent documentation with agent chart.
Update the description to match the agent chart's documentation for consistency.
-| agent.agent.authentication.existingSecretToken | string | `""` | |
+| agent.agent.authentication.existingSecretToken | string | `""` | Name of an existing Kubernetes secret containing the authentication token. The secret must contain a key named 'token'. Example: "my-auth-secret" |📝 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.
| | agent.agent.authentication.existingSecretToken | string | `""` | | | |
| | agent.agent.authentication.existingSecretToken | string | `""` | Name of an existing Kubernetes secret containing the authentication token. The secret must contain a key named 'token'. Example: "my-auth-secret" | |
charts/regions/Chart.lock
Outdated
| dependencies: | ||
| - name: agent | ||
| repository: file://../agent | ||
| version: 2.4.1 | ||
| version: 2.5.0 | ||
| - name: operator | ||
| repository: oci://ghcr.io/formancehq/helm | ||
| version: v2.6.0 | ||
| digest: sha256:72eccae222d27466817cb2dedb8cacbce157ae43255d79fc194ad4fe99133830 | ||
| generated: "2025-02-05T11:19:12.47191+01:00" |
There was a problem hiding this comment.
Can you also bump regions charts ?
sofiasimdianova
force-pushed
the
feat/sm_migrate_secrets
branch
from
ddefa2c to
e5e09f5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
README.md (1)
7-7: Ensure Agent chart update and configuration keys documentation.The Agent helm chart row has been updated to version
2.5.0, which aligns with the intended migration for secrets management using AWS SM. Please verify that the new configuration keys (such asagent.authentication.existingSecretandagent.authentication.secretKeys.secret) are clearly documented in the Agent chart's README or relevant configuration docs to help users properly utilize this feature.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
charts/agent/Chart.yamlis excluded by!**/*.yamlcharts/agent/values.schema.jsonis excluded by!**/*.jsoncharts/agent/values.yamlis excluded by!**/*.yamlcharts/regions/Chart.lockis excluded by!**/*.lock,!**/*.lockcharts/regions/Chart.yamlis excluded by!**/*.yaml
📒 Files selected for processing (4)
README.md(1 hunks)charts/agent/README.md(2 hunks)charts/agent/templates/_helpers.tpl(1 hunks)charts/regions/README.md(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- charts/agent/templates/_helpers.tpl
- charts/agent/README.md
- charts/regions/README.md
🧰 Additional context used
🪛 LanguageTool
README.md
[style] ~13-~13: Using many exclamation marks might seem excessive (in this case: 10 exclamation marks for a text that’s 3025 characters long)
Context: ...tions, users, roles, and permissions. | 
(EN_EXCESSIVE_EXCLAMATION)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Tests
🔇 Additional comments (1)
README.md (1)
15-15: Confirm Regions chart version update.The Regions helm chart row has been updated to version
2.9.4with the app version specified aslatest. Ensure that this version bump is consistent with all related documentation and any dependencies or version constraints, such as those in the Agent chart section, have been updated appropriately.
|
|
||
| dependencies: | ||
| - name: agent | ||
| version: 2.X |
charts/regions/Chart.lock
Outdated
| version: 2.5.0 | ||
| version: 2.5.0 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
README.md (1)
7-7: Agent Row Version Update & Secret Management Clarification
The Agent chart version has been updated to "2.5.0" as required. However, given the PR objective of migrating secrets to AWS Secrets Manager (with the addition ofagent.authentication.existingSecret), please verify that the accompanying documentation in./charts/agent/README.mdaccurately describes the new secret management configuration. This helps ensure that users understand how to configure the AWS SM integration.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
charts/agent/Chart.yamlis excluded by!**/*.yamlcharts/regions/Chart.lockis excluded by!**/*.lock,!**/*.lockcharts/regions/Chart.yamlis excluded by!**/*.yaml
📒 Files selected for processing (3)
README.md(1 hunks)charts/agent/README.md(2 hunks)charts/regions/README.md(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- charts/regions/README.md
- charts/agent/README.md
🧰 Additional context used
🪛 LanguageTool
README.md
[style] ~13-~13: Using many exclamation marks might seem excessive (in this case: 10 exclamation marks for a text that’s 3026 characters long)
Context: ...tions, users, roles, and permissions. |
(EN_EXCESSIVE_EXCLAMATION)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Tests
🔇 Additional comments (1)
README.md (1)
15-15: Regions Chart Version Update and Dependency Consistency
The Regions chart version is updated to "2.10.0", which aligns with the intended improvements. As with the Agent row, please ensure that any associated configuration keys—especially those related to secret management—are clearly documented in the Regions chart README if applicable. This will provide a consistent user experience across the Helm charts.
Add existingSecret value to migrate secrets to AWS SM for token authorisation