Plane-EE: Add extraEnv to support proxy endpoints#168
Conversation
- Introduced `extraEnv` configuration in values.yaml to allow users to specify additional environment variables for all workloads. - Updated deployment templates to conditionally include the `extraEnv` variables in the environment section of each workload. - Cleaned up unnecessary SSL environment variables from the API deployment.
…o add-https-proxy
WalkthroughAdds a global Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as Helm User
participant H as Helm (Templates)
participant K as Kubernetes API
participant P as Pod/Container
U->>H: helm install/upgrade with values.yaml
H->>H: Evaluate .Values.extraEnv
alt extraEnv provided
H->>K: Render manifests including env: (extraEnv)
else no extraEnv
H->>K: Render manifests without extra env block
end
K->>P: Start containers with resulting env configuration
sequenceDiagram
autonumber
participant V as Values
participant T as api.deployment.yaml
participant K as Kubernetes
participant C as API Container
V->>T: airgapped=true and s3 secret present?
alt airgapped && s3 secret present
T->>K: Include CA-related env vars (SSL_CERT_*, REQUESTS_CA_BUNDLE, CURL_CA_BUNDLE)
else
T->>K: Do not include CA-related env vars
end
T->>K: Optionally include extraEnv if set
K->>C: Run API container with final env
sequenceDiagram
autonumber
participant H as Helm
participant K as Kubernetes
participant J as MinIO Bucket Job
participant M as MinIO
H->>K: Apply Job with updated container (image_mc, command/args)
K->>J: Start Job pod
J->>J: Load envFrom secrets and optional extraEnv
J->>M: Execute mc commands (config, bucket)
J-->>K: Job completes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
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
♻️ Duplicate comments (2)
charts/plane-enterprise/templates/workloads/redis.stateful.yaml (1)
48-51: LGTM – correct conditional env block/indentation.Same note as elsewhere: ensure
.Values.extraEnvis a list of EnvVar items.charts/plane-enterprise/templates/workloads/rabbitmq.stateful.yaml (1)
53-56: LGTM – safe conditional env insertion.Same schema caveat for
.Values.extraEnvas noted earlier.
🧹 Nitpick comments (3)
charts/plane-enterprise/templates/workloads/iframely.deployment.yaml (1)
57-60: Conditional extra env injection looks correct (indentation, placement).Please ensure
.Values.extraEnvis a list of Kubernetes EnvVar objects (- name: FOO\n value: bar). If it’s a map, K8s will reject the manifest. Consider adding a brief example in values.yaml and optionally supportingextraEnvFromfor ConfigMap/Secret refs, or a shared helper (e.g.,include "plane.extraEnv").charts/plane-enterprise/templates/workloads/silo.deployment.yaml (1)
86-89: LGTM – consistent with pattern across workloads.Minor: consider whether initContainers (e.g., wait-for-rabbitmq) also need proxy env; if yes, mirror
extraEnvthere.charts/plane-enterprise/templates/workloads/api.deployment.yaml (1)
113-126: Avoid emittingenv: nullwhen no entriesIf both the airgapped CA block and
extraEnvare absent, this template still rendersenv:with no list items, which serializes toenv: null. While Kubernetes usually tolerates it, it is cleaner (and safer for validation tooling) to omit the field altogether. Wrap the whole block in a single guard and reuse the computed condition inside.- env: - {{- if and .Values.airgapped.enabled .Values.airgapped.s3SecretName .Values.airgapped.s3SecretKey }} + {{- $hasAirgapCA := and .Values.airgapped.enabled .Values.airgapped.s3SecretName .Values.airgapped.s3SecretKey }} + {{- if or $hasAirgapCA .Values.extraEnv }} + env: + {{- if $hasAirgapCA }} - name: SSL_CERT_FILE value: "/etc/ssl/certs/ca-certificates.crt" @@ - {{- if .Values.extraEnv }} - {{- toYaml .Values.extraEnv | nindent 10 }} - {{- end }} + {{- if .Values.extraEnv }} + {{- toYaml .Values.extraEnv | nindent 10 }} + {{- end }} + {{- end }}
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
charts/plane-enterprise/templates/workloads/admin.deployment.yaml(1 hunks)charts/plane-enterprise/templates/workloads/api.deployment.yaml(1 hunks)charts/plane-enterprise/templates/workloads/automation-consumer.deployment.yaml(1 hunks)charts/plane-enterprise/templates/workloads/beat-worker.deployment.yaml(1 hunks)charts/plane-enterprise/templates/workloads/email.deployment.yaml(1 hunks)charts/plane-enterprise/templates/workloads/iframely.deployment.yaml(1 hunks)charts/plane-enterprise/templates/workloads/live.deployment.yaml(1 hunks)charts/plane-enterprise/templates/workloads/migrator.job.yaml(1 hunks)charts/plane-enterprise/templates/workloads/minio.stateful.yaml(2 hunks)charts/plane-enterprise/templates/workloads/monitor.stateful.yaml(1 hunks)charts/plane-enterprise/templates/workloads/outbox-poller.deployment.yaml(1 hunks)charts/plane-enterprise/templates/workloads/postgres.stateful.yaml(1 hunks)charts/plane-enterprise/templates/workloads/rabbitmq.stateful.yaml(1 hunks)charts/plane-enterprise/templates/workloads/redis.stateful.yaml(1 hunks)charts/plane-enterprise/templates/workloads/silo.deployment.yaml(1 hunks)charts/plane-enterprise/templates/workloads/space.deployment.yaml(1 hunks)charts/plane-enterprise/templates/workloads/web.deployment.yaml(1 hunks)charts/plane-enterprise/templates/workloads/worker.deployment.yaml(1 hunks)charts/plane-enterprise/values.yaml(1 hunks)
🔇 Additional comments (4)
charts/plane-enterprise/templates/workloads/postgres.stateful.yaml (1)
49-54: PGDATA + extraEnv merged under env is good.Layout is valid;
PGDATAprecedes optionalextraEnventries with correct nindent. No issues.charts/plane-enterprise/templates/workloads/admin.deployment.yaml (1)
56-59: LGTM – clean extraEnv injection.Indentation and placement are correct; no conflicts with existing envFrom.
charts/plane-enterprise/templates/workloads/email.deployment.yaml (1)
79-82: LGTM – proper conditional env block.If proxies are the main use case, ensure
NO_PROXYdefaults cover cluster domains (e.g.,.svc,.cluster.local, namespace svc names).charts/plane-enterprise/templates/workloads/worker.deployment.yaml (1)
50-53: LGTM – matches the global pattern.No ordering issues with envFrom; indentation is correct.
….yaml and update API deployment template to conditionally include extraEnv. Removed default proxy settings and streamlined the handling of additional environment variables.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
charts/plane-enterprise/values.yaml (1)
391-399: Good default:extraEnvempty with commented examplesPrevents accidental proxying on fresh installs. Consider adding a note in README suggesting typical NO_PROXY entries (kubernetes.default.svc,.svc,.cluster.local,) and lowercase variants (http_proxy/https_proxy/no_proxy) for broader tooling compatibility.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
charts/plane-enterprise/templates/workloads/api.deployment.yaml(1 hunks)charts/plane-enterprise/values.yaml(1 hunks)
| {{- if or .Values.extraEnv (and .Values.airgapped.enabled .Values.airgapped.s3SecretName .Values.airgapped.s3SecretKey) }} | ||
| env: | ||
| {{- if and .Values.airgapped.enabled .Values.airgapped.s3SecretName .Values.airgapped.s3SecretKey }} | ||
| - name: SSL_CERT_FILE | ||
| value: "/etc/ssl/certs/ca-certificates.crt" | ||
| - name: SSL_CERT_DIR | ||
| value: "/etc/ssl/certs" | ||
| - name: REQUESTS_CA_BUNDLE | ||
| value: "/etc/ssl/certs/ca-certificates.crt" | ||
| - name: CURL_CA_BUNDLE | ||
| value: "/etc/ssl/certs/ca-certificates.crt" | ||
| {{- end }} | ||
| {{- if .Values.extraEnv }} | ||
| {{- toYaml .Values.extraEnv | nindent 10 }} | ||
| {{- end }} | ||
| {{- end }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Align airgapped gating; keep env merge behavior intentional
- The
envblock correctly merges CA vars with.Values.extraEnv. Note:enventries overrideenvFromon name collisions (intended?). - Inconsistency:
envgating usesand ... s3SecretName s3SecretKeybut the install-CA command earlier checks onlys3SecretName. Align both to require the key as well to avoid running the CA setup when only the secret name is set.
Apply this diff to the script gating (earlier in the file):
- {{- if and .Values.airgapped.enabled .Values.airgapped.s3SecretName }}
+ {{- if and .Values.airgapped.enabled .Values.airgapped.s3SecretName .Values.airgapped.s3SecretKey }}📝 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.
| {{- if or .Values.extraEnv (and .Values.airgapped.enabled .Values.airgapped.s3SecretName .Values.airgapped.s3SecretKey) }} | |
| env: | |
| {{- if and .Values.airgapped.enabled .Values.airgapped.s3SecretName .Values.airgapped.s3SecretKey }} | |
| - name: SSL_CERT_FILE | |
| value: "/etc/ssl/certs/ca-certificates.crt" | |
| - name: SSL_CERT_DIR | |
| value: "/etc/ssl/certs" | |
| - name: REQUESTS_CA_BUNDLE | |
| value: "/etc/ssl/certs/ca-certificates.crt" | |
| - name: CURL_CA_BUNDLE | |
| value: "/etc/ssl/certs/ca-certificates.crt" | |
| {{- end }} | |
| {{- if .Values.extraEnv }} | |
| {{- toYaml .Values.extraEnv | nindent 10 }} | |
| {{- end }} | |
| {{- end }} | |
| {{- if and .Values.airgapped.enabled .Values.airgapped.s3SecretName .Values.airgapped.s3SecretKey }} |
🤖 Prompt for AI Agents
In charts/plane-enterprise/templates/workloads/api.deployment.yaml around lines
114 to 129, the airgapped gating for injecting CA env vars uses both
.Values.airgapped.s3SecretName and .Values.airgapped.s3SecretKey, but an earlier
install-CA command only checks .Values.airgapped.s3SecretName; update that
earlier conditional to require both s3SecretName and s3SecretKey (i.e., change
its guard from checking only s3SecretName to and .Values.airgapped.s3SecretName
.Values.airgapped.s3SecretKey) so the CA setup and env injection are aligned,
and leave the env merge behavior (explicit env entries plus .Values.extraEnv)
unchanged.
…o add-https-proxy
…monitor, API, silo, beatworker, email_service, outbox_poller, automation_consumer, and iframely deployments
…port global configurations across deployments
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
charts/plane-enterprise/README.md (1)
493-498: Document the expected env var object structure.Consider adding a brief example (
- name: HTTP_PROXY,value: https://...) so users know the list should containname/value(orvalueFrom) maps. That removes guesswork when wiring proxy variables.
… to include examples for better guidance
2 participants
Description
Type of Change
Screenshots and Media (if applicable)
Test Scenarios
References
Summary by CodeRabbit
New Features
Refactor
Chores