Plane-EE: Support multiple custom S3 CA certificates #179
Conversation
WalkthroughThe pull request refactors the Plane Enterprise Helm chart to replace two separate S3 secret fields with an array-based structure. The chart version bumps to 1.6.6, and updates propagate across documentation, configuration templates, deployment manifests, and default values. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–25 minutes
Poem
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: 1
🧹 Nitpick comments (1)
charts/plane-enterprise/values.yaml (1)
13-19: Default values structure properly reflects new multi-secret model.The
s3Secrets: []default is correct—no certificates are mounted unless explicitly provided. Users must add their secret references here. Consider that the helpful comment on lines 15-18 about boto could be expanded to include a usage example showing the expected array structure.Optional: add an example comment showing the expected structure:
airgapped: enabled: false # s3Secrets: # - name: my-s3-ca # key: ca.crt # - name: my-s3-ca-intermediate # key: intermediate.crt
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
charts/plane-enterprise/Chart.yaml(1 hunks)charts/plane-enterprise/README.md(4 hunks)charts/plane-enterprise/questions.yml(1 hunks)charts/plane-enterprise/templates/config-secrets/app-env.yaml(2 hunks)charts/plane-enterprise/templates/workloads/api.deployment.yaml(5 hunks)charts/plane-enterprise/templates/workloads/worker.deployment.yaml(5 hunks)charts/plane-enterprise/values.yaml(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: mguptahub
Repo: makeplane/helm-charts PR: 98
File: charts/plane-enterprise/templates/certs/email-certs.yaml:1-14
Timestamp: 2025-05-22T09:40:55.645Z
Learning: In the makeplane/helm-charts repository, the Certificate resources follow a consistent pattern of only specifying the 'name' field in 'issuerRef' without explicit 'kind' or 'group' fields, using a common cert issuer approach across the application.
🔇 Additional comments (9)
charts/plane-enterprise/Chart.yaml (1)
8-8: Version bump is appropriate for this refactoring.The chart version increment to 1.6.6 is suitable for this feature enhancement, though maintainers should ensure release notes prominently feature the breaking changes in the airgapped secret configuration.
charts/plane-enterprise/questions.yml (1)
90-97: Array indexing in UI questionnaire is a reasonable limitation.The questions file exposes only the first array element (
[0]) for the UI questionnaire. Users needing multiple certificates should configure them directly in values.yaml. This is an acceptable trade-off between UI simplicity and feature capability.charts/plane-enterprise/templates/config-secrets/app-env.yaml (1)
35-37: AWS_CA_BUNDLE path updated to system standard correctly.The new path
/etc/ssl/certs/ca-certificates.crtis the correct system CA bundle location on Linux distributions. This aligns well with the startup script logic that callsupdate-ca-certificates. The dual condition check (and .Values.airgapped.enabled .Values.airgapped.s3Secrets) ensures the variable is only set when both are true.charts/plane-enterprise/templates/workloads/worker.deployment.yaml (3)
20-32: Projected volumes approach for multiple secrets is correct.The use of Kubernetes projected volumes with a range loop over
airgapped.s3Secretsis the right pattern for mounting multiple secrets into a single volume. Each secret entry is properly unpacked with.nameand.keyaccessible within the loop scope.
58-75: Startup script multi-file CA installation logic is sound.The refactored script properly handles multiple certificate files:
- Checks for any files in
/s3-custom-causingls -A- Copies all files to
/usr/local/share/ca-certificates/- Runs
update-ca-certificatesto integrate with the system CA store- Provides clear feedback in both success and skip cases
This approach works correctly even with multiple certificates and chains.
95-110: Environment variables properly configured for system CA path.Lines 97-106 set all four CA-related environment variables (
SSL_CERT_FILE,SSL_CERT_DIR,REQUESTS_CA_BUNDLE,CURL_CA_BUNDLE) to point to/etc/ssl/certs/ca-certificates.crt. This ensures boto3, requests, curl, and other tools correctly find the system-updated CA bundle. The conditional check usingand .Values.airgapped.enabled .Values.airgapped.s3Secretsis appropriate.charts/plane-enterprise/templates/workloads/api.deployment.yaml (3)
43-55: Projected volumes in API deployment mirror worker correctly.The API deployment's volumes section (lines 43-55) is consistent with the worker deployment, using the same projected volume pattern with range loop over
airgapped.s3Secrets. This consistency across workloads is good for maintainability.
81-98: API deployment startup script matches worker CA installation logic.The startup script (lines 81-98) in the API deployment correctly mirrors the worker deployment's multi-file CA certificate installation approach. Consistency across deployments is maintained.
118-133: Environment variable configuration consistent across API and worker.Lines 118-133 properly configure the SSL and CA environment variables, with the same system path references used in the worker deployment. The conditional gating on
airgapped.s3Secretsis correct.
| ### Air-gapped Settings | ||
|
|
||
| | Setting | Default | Required | Description | | ||
| | ---------------------- | :-----: | :------: | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | ||
| | airgapped.enabled | false | No | Specifies the airgapped mode the Plane API runs in. | | ||
| | airgapped.s3SecretName | "" | No | Name of the Secret that contains the CA certificate (.crt). The Secret must include a data key whose filename matches the basename of `airgapped.s3SecretKey`. Used to override S3’s CA when `airgapped.enabled=true`. Applying this secret looks like: `kubectl -n plane create secret generic plane-s3-ca \ --from-file=s3-custom-ca.crt=/path/to/your/ca.crt` | | ||
| | airgapped.s3SecretKey | "" | No | Key name of the secret to load the Custom Root CA from `airgapped.s3SecretName` | | ||
| | Setting | Default | Required | Description | | ||
| |-----------------------|:-------:| :------: |----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| | ||
| | airgapped.enabled | false | No | Specifies the airgapped mode the Plane API runs in. | | ||
| | airgapped.s3Secrets | [] | No | List of Kubernetes Secrets containing CA certificates to install. Each item in the list must contain a `name` (the Secret name) and a `key` (the file key inside the Secret). Applying a secret looks like: `kubectl -n plane create secret generic plane-s3-ca \ --from-file=s3-custom-ca.crt=/path/to/your/ca.crt` | |
There was a problem hiding this comment.
Documentation for new s3Secrets structure is clear but lacks breaking change warning.
The table update and kubectl example are helpful. However, the documentation does not explicitly highlight that this is a breaking change from the old s3SecretName/s3SecretKey fields. Users upgrading from older versions will have deployment failures if they don't update their values.
Add a prominent "Breaking Changes" section in the README (before the configuration tables) noting:
- Old fields
airgapped.s3SecretNameandairgapped.s3SecretKeyhave been removed - Migration required: convert to the new
airgapped.s3Secrets[]list structure - Example migration path for users
Alternatively, ensure this is clearly documented in release notes/CHANGELOG.
🤖 Prompt for AI Agents
In charts/plane-enterprise/README.md around lines 99 to 104, the new
airgapped.s3Secrets[] documentation does not warn that this is a breaking change
from the old airgapped.s3SecretName/airgapped.s3SecretKey fields; add a
prominent "Breaking Changes" section before the configuration tables that states
the old fields were removed, instructs users to migrate to the new
airgapped.s3Secrets[] list structure, and include a concise example showing how
to convert a single s3SecretName/s3SecretKey pair into the new list format plus
a note that failing to migrate will cause deployments to fail; alternatively,
reference and link to the exact release notes/CHANGELOG entry where the
migration steps are documented.
|
Should we add a |
|
Thanks @hajowieland for the PR. We are looking at it and will merge after testing. |
2 participants
Description
With #153 and #171 support was added for custom S3 CA certificates. But it only allows one single certificate to be added.
It does not work with a certificate chain as the logs show this warning:
This PR fixes this by refactoring the values and script related to the custom S3 certificates.
Now multilpe certificates can be added like this:
This breaks existing configurations, as the previous
s3SecretNameands3SecretKeyvalues have been removed.Also the environment variable
AWS_CA_BUNDLEwas updated to point to the generated/etc/ssl/certs/ca-certificates.crtfile instead of the file in/s3-custom-caas we want it to point to the system CA bundle.Type of Change
Test Scenarios
Installing custom CA certificates... Found certificates in /s3-custom-ca. Installing... CA certificates installed successfullyInside the api and worker Pods:
/s3-custom-ca # ls ca.crt another.crtThe certificates can also be found in the resulting
/etc/ssl/certs/ca-certificates.crtfile.Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.