feat(helm)!: Replace hostPath-based aws_config_directory with chart-managed Secret (resolves #2162).#2164
feat(helm)!: Replace hostPath-based aws_config_directory with chart-managed Secret (resolves #2162).#2164junhaoliao wants to merge 8 commits intoy-scope:mainfrom
hostPath-based aws_config_directory with chart-managed Secret (resolves #2162).#2164Conversation
…rt-managed Secret (resolves y-scope#2162).
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughChart version bumped; AWS config input renamed from ChangesChart Version
AWS config secret & mount
Sequence Diagram(s)sequenceDiagram
participant Helm as Helm renderer
participant K8sAPI as Kubernetes API
participant Pod as Pod / Container
Helm->>Helm: Read `.Values.clpConfig.aws_config`
alt aws_config set
Helm->>K8sAPI: Render & apply Secret `{{ include "clp.fullname" . }}-aws-config`
Helm->>K8sAPI: Render Deployments referencing Secret volume
K8sAPI->>Pod: Mount Secret at /opt/clp/.aws
Pod->>Pod: Application reads AWS config from /opt/clp/.aws
else aws_config not set
Helm->>K8sAPI: Do not render Secret or mounts
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tools/deployment/package-helm/values.yaml`:
- Around line 259-265: Add a new Helm Secret template named
aws-config-secret.yaml that mirrors the pattern used by database-secret.yaml,
queue-secret.yaml, and redis-secret.yaml: read clpConfig.aws_config.credentials
and clpConfig.aws_config.config values and create a Secret (named using the
release name, e.g., {{ include "clp.fullname" . }}-aws-config) with keys for
credentials and config so the clp.awsConfigVolume helper can mount them into
pods; ensure the template is conditional on .Values.clpConfig.aws_config not
being null and uses proper tpl/quote handling for multi-line file content.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ae49a1b6-74b2-4506-9b0e-e4a84879baf6
📒 Files selected for processing (9)
tools/deployment/package-helm/Chart.yamltools/deployment/package-helm/templates/_helpers.tpltools/deployment/package-helm/templates/compression-scheduler-deployment.yamltools/deployment/package-helm/templates/compression-worker-deployment.yamltools/deployment/package-helm/templates/configmap.yamltools/deployment/package-helm/templates/garbage-collector-deployment.yamltools/deployment/package-helm/templates/query-worker-deployment.yamltools/deployment/package-helm/templates/webui-deployment.yamltools/deployment/package-helm/values.yaml
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tools/deployment/package-helm/templates/aws-config-secret.yaml`:
- Around line 8-14: When .Values.clpConfig.aws_config is present but neither
.Values.clpConfig.aws_config.credentials nor .Values.clpConfig.aws_config.config
is provided, the two with-blocks in the stringData section (credentials and
config) are skipped producing an empty stringData; add a Helm validation check
that detects this case and fails early with a clear message (e.g., using an if
that tests .Values.clpConfig.aws_config and the absence of both
.Values.clpConfig.aws_config.credentials and
.Values.clpConfig.aws_config.config, then call fail with a descriptive message)
so templates/aws-config-secret.yaml does not render an empty secret.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: adb2ccca-9f56-4fa3-b055-208d60aca59b
📒 Files selected for processing (1)
tools/deployment/package-helm/templates/aws-config-secret.yaml
hoophalab
left a comment
hoophalab
left a comment
There was a problem hiding this comment.
Overall good. Some comments.
| {{- define "clp.awsConfigVolumeMount" -}} | ||
| name: "aws-config" | ||
| mountPath: {{ .Values.clpConfig.aws_config_directory | quote }} | ||
| mountPath: "/opt/clp/.aws" |
There was a problem hiding this comment.
Can we comment that this variable needs to be in sync with CONTAINER_AWS_CONFIG_DIRECTORY in clp_py_utils/clp_config.py?
There was a problem hiding this comment.
i created a named template helper to avoid the magic string: clp.awsConfigMountPath
| {{- if .Values.clpConfig.aws_config_directory }} | ||
| aws_config_directory: {{ .Values.clpConfig.aws_config_directory | quote }} | ||
| {{- if .Values.clpConfig.aws_config }} | ||
| aws_config_directory: "/opt/clp/.aws" |
There was a problem hiding this comment.
Since the package doesn't actually handle the volume mount in Helm, this variable could be set to any string, null, or even omitted entirely, right?
There was a problem hiding this comment.
the package still read the awsConfigMount volume - it's just that the volume content (i.e., config) no longer comes from some file on some host's filesystem, but rather the k8s secret, which composes these files in the containers when launched:
- /opt/clp/.aws/credentials
- /opt/clp/.aws/config
|
Shall we update |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tools/deployment/package-helm/Chart.yaml`:
- Line 3: The Chart.yaml chart version was only incremented from "0.3.2-dev.1"
to "0.3.2-dev.2" but the PR changes the values schema in a breaking way
(renaming aws_config_directory → aws_config); update the chart's version to the
next breaking-major series (e.g. change the version field value in Chart.yaml to
"0.4.0-dev.1" or another appropriate 0.4.x semantic-major bump) so operators and
upgrade tooling detect the breaking change.
In `@tools/deployment/package-helm/templates/_helpers.tpl`:
- Around line 366-370: The AWS secret volume definition "clp.awsConfigVolume"
currently omits a defaultMode, leaving files with Kubernetes' default 0644;
update the template for clp.awsConfigVolume to add defaultMode: 0400 under the
secret stanza so the mounted AWS credential file is owner-read only (0400),
preserving readOnly mounts and using the existing include "clp.fullname" for
secretName.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: dd2ec6f1-7d79-4806-b493-802b58d5d466
📒 Files selected for processing (9)
tools/deployment/package-helm/Chart.yamltools/deployment/package-helm/templates/_helpers.tpltools/deployment/package-helm/templates/compression-scheduler-deployment.yamltools/deployment/package-helm/templates/compression-worker-deployment.yamltools/deployment/package-helm/templates/configmap.yamltools/deployment/package-helm/templates/garbage-collector-deployment.yamltools/deployment/package-helm/templates/query-worker-deployment.yamltools/deployment/package-helm/templates/webui-deployment.yamltools/deployment/package-helm/values.yaml
| apiVersion: "v2" | ||
| name: "clp" | ||
| version: "0.3.2-dev.1" | ||
| version: "0.3.2-dev.2" |
There was a problem hiding this comment.
Use a breaking-level chart version bump for this values schema change.
Line 3 bumps 0.3.2-dev.1 → 0.3.2-dev.2, but this PR introduces a breaking values contract change (aws_config_directory → aws_config). Please bump to the next breaking-compatible series (for example 0.4.0-dev.1) so upgrade tooling and operators can detect risk correctly.
Suggested change
-version: "0.3.2-dev.2"
+version: "0.4.0-dev.1"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/deployment/package-helm/Chart.yaml` at line 3, The Chart.yaml chart
version was only incremented from "0.3.2-dev.1" to "0.3.2-dev.2" but the PR
changes the values schema in a breaking way (renaming aws_config_directory →
aws_config); update the chart's version to the next breaking-major series (e.g.
change the version field value in Chart.yaml to "0.4.0-dev.1" or another
appropriate 0.4.x semantic-major bump) so operators and upgrade tooling detect
the breaking change.
| {{- define "clp.awsConfigVolume" -}} | ||
| name: "aws-config" | ||
| hostPath: | ||
| path: {{ .Values.clpConfig.aws_config_directory | quote }} | ||
| type: "Directory" | ||
| secret: | ||
| secretName: {{ include "clp.fullname" . }}-aws-config | ||
| {{- end }} |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Add defaultMode: 0400 to restrict AWS credential file permissions.
The Secret volume uses Kubernetes' default mode (0644), making credential files group- and world-readable within the container. Since the mount is already readOnly: true, setting defaultMode: 0400 (owner read-only) aligns with AWS SDK best-practice and eliminates the world-readable warning that AWS tooling emits.
🔒 Proposed fix
{{- define "clp.awsConfigVolume" -}}
name: "aws-config"
secret:
secretName: {{ include "clp.fullname" . }}-aws-config
+ defaultMode: 0400
{{- end }}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/deployment/package-helm/templates/_helpers.tpl` around lines 366 - 370,
The AWS secret volume definition "clp.awsConfigVolume" currently omits a
defaultMode, leaving files with Kubernetes' default 0644; update the template
for clp.awsConfigVolume to add defaultMode: 0400 under the secret stanza so the
mounted AWS credential file is owner-read only (0400), preserving readOnly
mounts and using the existing include "clp.fullname" for secretName.
…mplate; update docs - Add `clp.awsConfigMountPath` helper to define the AWS config mount path in a single place, eliminating the need for sync comments between _helpers.tpl and configmap.yaml. - Update `clp.awsConfigVolumeMount` and configmap.yaml to use the new helper instead of a hardcoded string. - Add a note in clp-config.md that Helm users should provide AWS config via --set-file rather than setting aws_config_directory manually.
…ofile section Replace the Helm deployment note with a Docker Compose / Kubernetes (Helm) tab-set, matching the pattern used in clp-json.md. This gives each deployment method its own clear instructions instead of burying Helm details in a note.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/src/user-docs/guides-using-object-storage/aws-s3/clp-config.md (1)
181-184: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winClarify the note for deployment-specific context.
The note references
aws_config_directorygenerically, but the required action differs between Docker Compose and Helm deployments:
- Docker Compose: Users should comment out or set
aws_config_directorytonullwhen not using profiles.- Kubernetes (Helm): Users should simply omit the
--set-file clpConfig.aws_config.*flags when not using profiles; they don't directly configureaws_config_directory.Consider updating the note to reflect this distinction, similar to the tabbed approach used above, to avoid confusion for Helm users.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/src/user-docs/guides-using-object-storage/aws-s3/clp-config.md` around lines 181 - 184, Update the generic note about aws_config_directory to distinguish deployment types: instruct Docker Compose users to comment out or set aws_config_directory to null when not using profiles, and instruct Kubernetes/Helm users to omit the --set-file clpConfig.aws_config.* flags (instead of editing aws_config_directory) when not using profiles; reference the aws_config_directory setting and the --set-file clpConfig.aws_config.* Helm flags so readers can quickly find the relevant configuration block.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/src/user-docs/guides-using-object-storage/aws-s3/clp-config.md`:
- Around line 169-170: Replace the vague sentence "aws_config_directory is set
automatically by the chart" with a clearer statement that the chart manages the
AWS config and that users should supply the AWS config file contents via
--set-file; mention that the chart creates a Secret and mounts it at
/opt/clp/.aws and the ConfigMap references that mount (referencing
aws_config_directory, /opt/clp/.aws, ConfigMap, and --set-file to locate the
change).
- Line 157: Add a comma after the linking adverb "Typically" at the start of the
sentence (i.e., change "Typically X" to "Typically, X"); locate the sentence
that begins with "Typically" in clp-config.md and insert the comma immediately
after the word to correct the grammar.
---
Outside diff comments:
In `@docs/src/user-docs/guides-using-object-storage/aws-s3/clp-config.md`:
- Around line 181-184: Update the generic note about aws_config_directory to
distinguish deployment types: instruct Docker Compose users to comment out or
set aws_config_directory to null when not using profiles, and instruct
Kubernetes/Helm users to omit the --set-file clpConfig.aws_config.* flags
(instead of editing aws_config_directory) when not using profiles; reference the
aws_config_directory setting and the --set-file clpConfig.aws_config.* Helm
flags so readers can quickly find the relevant configuration block.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: af8ae836-f7bc-4d54-97f4-530953419337
📒 Files selected for processing (1)
docs/src/user-docs/guides-using-object-storage/aws-s3/clp-config.md
| ::::{tab-set} | ||
| :::{tab-item} Docker Compose | ||
| :sync: docker | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Add comma after linking adverb for clarity.
The linking adverb "Typically" at the start of the sentence should be followed by a comma for proper grammar.
✏️ Proposed grammar fix
-Typically `~/.aws`:
+Typically, `~/.aws`:🧰 Tools
🪛 LanguageTool
[uncategorized] ~157-~157: A comma may be missing after the conjunctive/linking adverb ‘Typically’.
Context: ...tab-item} Docker Compose :sync: docker Typically ~/.aws: ```yaml aws_config_directory...
(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/src/user-docs/guides-using-object-storage/aws-s3/clp-config.md` at line
157, Add a comma after the linking adverb "Typically" at the start of the
sentence (i.e., change "Typically X" to "Typically, X"); locate the sentence
that begins with "Typically" in clp-config.md and insert the comma immediately
after the word to correct the grammar.
…lm chart installation details
2 participants
Description
The Helm chart's
aws_config_directoryvolume useshostPath, which mounts a directory from the Kubernetes node's filesystem. This doesn't work on managed Kubernetes clusters (e.g., EKS) wherehostPathis either unavailable or restricted by PodSecurityStandards.This PR replaces the
hostPath-basedaws_config_directoryvalue with a newaws_configobject.Users provide their AWS config file contents via Helm's
--set-fileflag, and the chart creates a Kubernetes Secret from those contents. The Secret is then mounted as a volume in all pods that need AWS config access (compression-scheduler, compression-worker, query-worker, garbage-collector, webui).Usage:
Changes:
values.yaml: Replacedaws_config_directory: nullwithaws_config: nulland documented--set-fileusage.templates/aws-config-secret.yaml(new): Creates a Secret fromaws_config.credentialsandaws_config.configfile contents.templates/_helpers.tpl:clp.awsConfigVolumenow emits asecretvolume instead ofhostPath;clp.awsConfigVolumeMounthardcodes/opt/clp/.awsas the mount path.templates/configmap.yaml: Condition and rendered path updated to useaws_config.aws_config_directorytoaws_config.Checklist
breaking change.
Validation performed
helm templaterenders no AWS resources whenaws_configis null (default)Task: Verify that when
aws_configis not set (defaultnull), no AWS-related resources arerendered.
Command:
Output:
No AWS resources are emitted when
aws_configis disabled — all deployments omit the volume and volumeMount, and no Secret is created.helm templaterenders Secret and mounts correctly withaws_configTask: Verify that setting
aws_configvia--set-filecreates a Secret and mounts it in all 5 deployments.Command:
Output:
All 5 deployments (compression-scheduler, compression-worker, query-worker, garbage-collector,
webui) reference the chart-managed Secret as a volume source.
Command (verify configmap sets
aws_config_directory):Output:
The configmap correctly hardcodes the
aws_config_directorypath whenaws_configis provided.Single-node cluster: S3 compress, search, and stream extraction
Task: Deploy with
set-up-test.sh, upgrade with S3 +aws_config, and verify compress, search, and stream extraction all work.Command:
Output:
Command (upgrade with S3 + aws_config):
Output:
Command (verify Secret exists and contains expected keys):
Output:
Command (submit S3 compression job):
Output:
Command (check compression worker logs):
Output:
Explanation: The compression worker successfully read the AWS credentials from the Secret-mounted volume at
/opt/clp/.aws, connected to S3, and uploaded the archive.Command (search via Playwright):
Search for
postgresin the WebUI.Result:
Success - search job 3 found 10 results in 0.301 seconds (13.4 kB/s)Command (stream extraction via Playwright):
Click "Original file" link on a search result.
Result: New tab opens showing the decompressed stream file contents from S3 — confirms the
webui correctly reads AWS config from the Secret-mounted volume.
Multi-node shared cluster: S3 compress and search
Task: Deploy with
set-up-multi-shared-test.sh, upgrade with S3 +aws_config, and verifycompress and search work across worker nodes.
Command:
Output:
Command (upgrade + compress + search):
Output:
Result: S3 compression completed successfully. Search found 10 results in 0.352 seconds.
Multi-node dedicated cluster: S3 compress, search, and stream extraction
Task: Deploy with
set-up-multi-dedicated-test.sh, upgrade with S3 +aws_config(including nodeSelector), and verify compress, search, and stream extraction work on dedicated worker nodes.Command:
Output:
Command (upgrade with nodeSelector):
Output:
Result: S3 compression completed successfully. Search found 10 results in 0.317 seconds. Stream extraction via "Original file" link opened a new tab showing the decompressed file contents from S3.
Summary by CodeRabbit
Chores
Documentation