S3CSI-204: Support CA certificates via ConfigMap - #351
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Review by Claude Code |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files
... and 2 files with indirect coverage changes @@ Coverage Diff @@
## main #351 +/- ##
==========================================
+ Coverage 73.60% 74.02% +0.41%
==========================================
Files 48 48
Lines 2800 2868 +68
==========================================
+ Hits 2061 2123 +62
- Misses 607 610 +3
- Partials 132 135 +3 |
|
|
Well-structured PR. Clean separation between controller CA injection (AWS_CA_BUNDLE) and mounter pod CA injection (initContainer + shared emptyDir). Security context on the init container properly follows the restricted PodSecurity policy. Tests cover both TLS and non-TLS paths. |
Review by Claude Code |
|
Minor issues found, see inline comments for details.
Review by Claude Code |
Review by Claude Code |
e547516 to
d3784c5
Compare
|
Clean, well-structured PR. Good security context on the init container, proper error wrapping throughout, and solid test coverage for both TLS and non-TLS paths. |
d3784c5 to
32aca2d
Compare
|
Clean, well-structured PR. Two minor observations: |
32aca2d to
7aede71
Compare
|
LGTM |
7aede71 to
79b979d
Compare
79b979d to
efbf43b
Compare
|
LGTM — well-structured PR with good separation between controller (AWS_CA_BUNDLE) and mounter pod (initContainer + system trust store) TLS paths. Security contexts on the init container are properly restricted. Two minor observations: |
|
LGTM |
Add TLS configuration to the CSI controller and mounter pod creator: - New --tls-* flags on controller for CA cert ConfigMap, init container image, and resource limits (fail-fast on invalid quantities) - Init container appends custom CA bundle to system certs in a shared EmptyDir (1MiB limit) mounted at /etc/ssl/certs - Init container uses set -e for proper error propagation - AWS_CA_BUNDLE env var set on mounter container when TLS is enabled Issue: S3CSI-204
Add tls.caCertConfigMap value to configure custom CA certificate support. When set, the controller passes --tls-ca-cert-configmap to the CSI controller container, and mounts the ConfigMap as a volume (marked optional to avoid blocking pod startup). Issue: S3CSI-204
Add mage e2e:tlsAll target that deploys an HTTPS S3 proxy with custom CA, creates CA cert ConfigMaps, installs the CSI driver with TLS enabled, and runs E2E tests against the HTTPS endpoint. Add tls-e2e job to the CI workflow. Uses %w error wrapping throughout for proper error chain preservation. Issue: S3CSI-204
Add dedicated TLS configuration guide covering prerequisites, Helm values, verification steps, and troubleshooting. Update Helm chart reference with tls.* parameters. Add CA/PEM/TLS terms to glossary. Add TLS note to volume provisioning docs. Issue: S3CSI-204
efbf43b to
164fdb1
Compare
Review by Claude Code |
Ensure a newline separates the system CA bundle from the custom CA certificate when building the combined trust store. Prevents PEM corruption if the system bundle does not end with a newline. Issue: S3CSI-204
|
LGTM |
|
|
||
| ## TLS Configuration | ||
|
|
||
| <!-- markdownlint-disable MD046 --> |
There was a problem hiding this comment.
Might as well deactivate MD046 as a whole in the .markdownlint.yaml I find it very hard to support on mkdocs.
There was a problem hiding this comment.
Done in a seperate commit
only 1 file is not in the above commit, which was modified with respective code changes for helm and config maps. helm-chart-configuration-reference.md
There was a problem hiding this comment.
Pull request overview
Adds support for trusting custom TLS CA certificates when the driver talks to HTTPS S3 endpoints (controller-side AWS SDK calls and mounter pod mount-s3/s2n-tls).
Changes:
- Add mounter-pod TLS config to mount a CA ConfigMap and use an initContainer to build a combined trust bundle in
/etc/ssl/certs. - Add controller-side TLS config parsing (flags/env) and Helm wiring to mount the CA ConfigMap + set
AWS_CA_BUNDLE. - Add TLS documentation and CI/E2E workflow coverage for TLS scenarios.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/podmounter/mppod/creator.go | Introduces TLSConfig and TLS volume/initContainer wiring for mounter pods. |
| pkg/podmounter/mppod/creator_test.go | Adds unit tests validating pod spec changes with/without TLS enabled. |
| cmd/scality-csi-controller/main.go | Adds flag/env parsing to construct TLS config for spawned mounter pods. |
| charts/scality-mountpoint-s3-csi-driver/values.yaml | Adds tls.* Helm values for CA ConfigMap + initContainer settings. |
| charts/scality-mountpoint-s3-csi-driver/templates/controller.yaml | Mounts CA ConfigMap into controller and sets TLS-related env vars. |
| magefiles/e2e.go | Adds helpers/targets for TLS E2E flow and ConfigMap creation. |
| .github/workflows/e2e-tests.yaml | Adds a dedicated TLS E2E job to validate end-to-end TLS behavior in CI. |
| docs/driver-deployment/tls-configuration.md | New TLS configuration guide explaining setup and troubleshooting. |
| docs/concepts-and-reference/helm-chart-configuration-reference.md | Documents new tls.* chart values and ordering constraints. |
| docs/troubleshooting.md | Adds TLS-related troubleshooting entries. |
| docs/volume-provisioning/index.md | Links to the TLS configuration guide from provisioning docs. |
| docs/glossary.md | Adds TLS/CA-related glossary entries. |
| mkdocs.yml | Adds TLS configuration page to the MkDocs navigation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if _, err := os.Stat(caCertPath); os.IsNotExist(err) { | ||
| return fmt.Errorf("CA certificate not found at %s (run GenerateTLSCerts first)", caCertPath) |
There was a problem hiding this comment.
os.Stat errors other than IsNotExist (e.g., permission issues, transient filesystem errors) are currently ignored and will fall through to later kubectl steps with a less direct failure. Consider handling any non-nil error from os.Stat and returning it with context so failures are easier to diagnose.
| if _, err := os.Stat(caCertPath); os.IsNotExist(err) { | |
| return fmt.Errorf("CA certificate not found at %s (run GenerateTLSCerts first)", caCertPath) | |
| if _, err := os.Stat(caCertPath); err != nil { | |
| if os.IsNotExist(err) { | |
| return fmt.Errorf("CA certificate not found at %s (run GenerateTLSCerts first)", caCertPath) | |
| } | |
| return fmt.Errorf("failed to stat CA certificate at %s: %w", caCertPath, err) |
| ImagePullPolicy: c.config.TLS.InitImagePullPolicy, | ||
| Command: []string{ | ||
| "sh", "-c", | ||
| "set -e; cp /etc/ssl/certs/ca-certificates.crt /shared-certs/ca-certificates.crt; echo >> /shared-certs/ca-certificates.crt; cat /custom-ca/ca-bundle.crt >> /shared-certs/ca-certificates.crt", |
There was a problem hiding this comment.
The initContainer command assumes the init image already contains /etc/ssl/certs/ca-certificates.crt. The default init image is alpine:3.21, which may not include the ca-certificates bundle by default; if the file is missing the cp will fail and mounter pods will never start when TLS is enabled. Consider installing/refreshing CA certs in the initContainer (or switching the default init image to one that guarantees the bundle is present) and fail with a clearer message if the bundle path is missing.
| "set -e; cp /etc/ssl/certs/ca-certificates.crt /shared-certs/ca-certificates.crt; echo >> /shared-certs/ca-certificates.crt; cat /custom-ca/ca-bundle.crt >> /shared-certs/ca-certificates.crt", | |
| "set -eu; if [ ! -f /etc/ssl/certs/ca-certificates.crt ]; then echo 'ERROR: system CA bundle /etc/ssl/certs/ca-certificates.crt not found in TLS init container image; ensure the init image includes ca-certificates.' 1>&2; exit 1; fi; cp /etc/ssl/certs/ca-certificates.crt /shared-certs/ca-certificates.crt; echo >> /shared-certs/ca-certificates.crt; cat /custom-ca/ca-bundle.crt >> /shared-certs/ca-certificates.crt", |
| - name: Run TLS E2E Tests | ||
| run: | | ||
| mkdir -p test-results | ||
| S3_ENDPOINT_URL=https://s3.scality.com:8443 \ | ||
| CSI_IMAGE_TAG=${{ github.sha }} \ | ||
| CSI_IMAGE_REPOSITORY=ghcr.io/${{ github.repository }} \ | ||
| JUNIT_REPORT=./test-results/e2e-tls-tests-results.xml \ | ||
| mage e2e:tlsAll | ||
|
|
||
| - name: Stop Capture and Collect Artifacts | ||
| if: always() | ||
| run: mage e2e:stopCapture | ||
|
|
||
| - name: Upload Test Artifacts | ||
| if: always() | ||
| uses: actions/upload-artifact@v4 | ||
| with: | ||
| name: e2e-tls-test-artifacts | ||
| path: artifacts | ||
|
|
||
| - name: Upload test results to Codecov | ||
| if: ${{ always() }} | ||
| uses: codecov/test-results-action@v1 | ||
| with: | ||
| token: ${{ secrets.CODECOV_TOKEN }} | ||
| file: ./tests/e2e/test-results/e2e-tls-tests-results.xml | ||
| flags: e2e_tls_tests |
There was a problem hiding this comment.
The JUnit report is written to ./test-results/e2e-tls-tests-results.xml via JUNIT_REPORT, but the Codecov upload step points to ./tests/e2e/test-results/e2e-tls-tests-results.xml. This mismatch will cause the Codecov step to fail to find the report (or upload an empty/missing file). Align the paths (either write into ./tests/e2e/test-results/ or upload from ./test-results/).
MkDocs Material admonitions use indented code blocks which conflict with MD046 (fenced code block requirement). Disabling globally removes the need for inline disable/enable comments scattered across docs.
|
LGTM |
Add tls.caCertData Helm value that lets Helm create the CA certificate ConfigMap in both controller and mounter pod namespaces automatically. Users can now do a single-step setup with --set-file tls.caCertData=... instead of manually creating ConfigMaps in two namespaces with strict ordering. The existing manual mode (tls.caCertConfigMap only) remains as a backwards-compatible fallback. Also addresses PR review feedback: - Add ca-certificates existence check in init container command - Simplify E2E TLSAll to use Helm-managed mode - Fix os.Stat error handling in e2e.go - Fix hardcoded namespace references in troubleshooting docs - Update TLS docs with Helm-managed mode as recommended approach
326c984 to
063ad0a
Compare
|
LGTM |
tcarmet
left a comment
tcarmet
left a comment
There was a problem hiding this comment.
Some comments that is just me talking to myself, so LGTM.
| helm upgrade scality-s3-csi \ | ||
| ./charts/scality-mountpoint-s3-csi-driver \ | ||
| --namespace kube-system \ | ||
| --reuse-values \ |
There was a problem hiding this comment.
TIL about this flag, and it's not even new 😅
There was a problem hiding this comment.
yes it is pretty nifty one, maintains state
| Command: []string{ | ||
| "sh", "-c", | ||
| "set -e; cp /etc/ssl/certs/ca-certificates.crt /shared-certs/ca-certificates.crt; echo >> /shared-certs/ca-certificates.crt; cat /custom-ca/ca-bundle.crt >> /shared-certs/ca-certificates.crt", | ||
| "set -e; if [ ! -f /etc/ssl/certs/ca-certificates.crt ]; then echo 'ERROR: /etc/ssl/certs/ca-certificates.crt not found. The TLS init image must include the ca-certificates package.' >&2; exit 1; fi; cp /etc/ssl/certs/ca-certificates.crt /shared-certs/ca-certificates.crt; echo >> /shared-certs/ca-certificates.crt; cat /custom-ca/ca-bundle.crt >> /shared-certs/ca-certificates.crt", |
There was a problem hiding this comment.
Nice error handling because at first when I reviewed this PR I was gonna suggest busybox as a image, to keep it lighter since it was "just a copy" but I quickly realized it wasn't going to work, and we would have hit this issue. Should keep the user experience a bit better if somebody tries to play with the image you use on the init container 👍
Summary
Adds custom CA certificate support so customers can validate TLS certificates from their RING S3 endpoints. CA certificates are injected via Kubernetes ConfigMaps (not Secrets) since they are public configuration data, not confidential.
The controller pod mounts the CA ConfigMap and sets
AWS_CA_BUNDLEfor dynamic provisioning S3 calls. Mounter pods use an Alpine initContainer to install the CA cert into the system trust store (/etc/ssl/certs/) because mount-s3's s2n-tls only reads from there.Changes
TLSConfig,configureTLS()for ConfigMap-based CA injection in mounter pods,buildTLSConfig()in controller with flag/env parsing and defaults, unit teststls.*values, conditionalAWS_CA_BUNDLEenv var, ConfigMap volume mount, and TLS env vars in controller templateensureCACertConfigMaphelper,TLSAllMage target, ande2e-tests-tlsworkflow job for full TLS integration testingTest plan
TestCreatingMountpointPodsWithTLS,TestCreatingMountpointPodsWithoutTLS)tls.caCertConfigMape2e-tests-tlsCI job passes