feat: add proxy env var support for EDA containers#341
Open
jamesmarshall24 wants to merge 2 commits intoansible:mainfrom
Open
feat: add proxy env var support for EDA containers#341jamesmarshall24 wants to merge 2 commits intoansible:mainfrom
jamesmarshall24 wants to merge 2 commits intoansible:mainfrom
Conversation
jamesmarshall24
requested review from
dsavineau,
lucas-benedito,
mkanoor,
rooftopcellist and
ttuffin
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughAdds three optional proxy fields ( Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant K8s as Kubernetes API
participant Operator
participant Templates as Template Engine
participant Deploy as Deployments/Pods
User->>K8s: Apply/Patch EDA CR (includes proxy fields)
K8s->>Operator: Notify CR create/update
Operator->>Templates: Render manifests using CR values/defaults
Templates->>K8s: Apply Deployments with injected env vars
K8s->>Deploy: Schedule Pods from Deployments
Deploy->>Deploy: Containers receive HTTP_PROXY/https_proxy/NO_PROXY env vars
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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.
🧹 Nitpick comments (2)
config/manifests/bases/eda-server-operator.clusterserviceversion.yaml (1)
7-7: Add CSV specDescriptors for the proxy configuration fields.The CRD exposes
http_proxy,https_proxy, andno_proxyfields, but the CSV lacks matching specDescriptors in the EDA spec section. This makes the proxy configuration options invisible in the OpenShift Console form.Suggested CSV entries
- displayName: HTTP Proxy path: http_proxy x-descriptors: - urn:alm:descriptor:com.tectonic.ui:advanced - urn:alm:descriptor:com.tectonic.ui:text - displayName: HTTPS Proxy path: https_proxy x-descriptors: - urn:alm:descriptor:com.tectonic.ui:advanced - urn:alm:descriptor:com.tectonic.ui:text - displayName: No Proxy path: no_proxy x-descriptors: - urn:alm:descriptor:com.tectonic.ui:advanced - urn:alm:descriptor:com.tectonic.ui:text🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/manifests/bases/eda-server-operator.clusterserviceversion.yaml` at line 7, The CSV is missing specDescriptors for the CRD proxy fields so the OpenShift Console form doesn't show http_proxy, https_proxy, and no_proxy; update the ClusterServiceVersion spec (the EDA spec section) to add specDescriptor entries for path: http_proxy, path: https_proxy, and path: no_proxy, each with displayName ("HTTP Proxy", "HTTPS Proxy", "No Proxy") and x-descriptors including urn:alm:descriptor:com.tectonic.ui:advanced and urn:alm:descriptor:com.tectonic.ui:text so the fields become visible/editable in the console.roles/eda/defaults/main.yml (1)
151-155: Consider whether operand proxy config should auto-inherit from the operator pod environment.Currently,
http_proxy,https_proxy, andno_proxydefault to empty strings. Since templates use{% if http_proxy %}conditionals (which treat empty strings as falsy), these variables only render proxy env settings when explicitly set in the EDA CR. No mechanism exists to inherit proxy configuration from the operator pod environment—there are nolookup('env', ...)calls in the codebase.In proxied clusters, this means operand workloads will not automatically get the same proxy settings as the operator pod unless the CR is updated. This design is safe and allows independent proxy configuration, but it may be worth clarifying in documentation or considering whether auto-inheritance via
lookup()would improve usability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@roles/eda/defaults/main.yml` around lines 151 - 155, The defaults currently set http_proxy, https_proxy, and no_proxy to empty strings so template conditionals like {% if http_proxy %} skip rendering unless the EDA CR explicitly sets them; to enable auto-inheritance from the operator pod environment, change the defaults to read the env via lookup('env', ...) (e.g., set http_proxy: "{{ lookup('env','http_proxy') | default('') }}", and similarly for https_proxy and no_proxy) so templates will render proxy env vars when the operator has them while still allowing CR overrides; update the variables named http_proxy, https_proxy, and no_proxy in defaults/main.yml and ensure templates keep their existing {% if http_proxy %} checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@config/manifests/bases/eda-server-operator.clusterserviceversion.yaml`:
- Line 7: The CSV is missing specDescriptors for the CRD proxy fields so the
OpenShift Console form doesn't show http_proxy, https_proxy, and no_proxy;
update the ClusterServiceVersion spec (the EDA spec section) to add
specDescriptor entries for path: http_proxy, path: https_proxy, and path:
no_proxy, each with displayName ("HTTP Proxy", "HTTPS Proxy", "No Proxy") and
x-descriptors including urn:alm:descriptor:com.tectonic.ui:advanced and
urn:alm:descriptor:com.tectonic.ui:text so the fields become visible/editable in
the console.
In `@roles/eda/defaults/main.yml`:
- Around line 151-155: The defaults currently set http_proxy, https_proxy, and
no_proxy to empty strings so template conditionals like {% if http_proxy %} skip
rendering unless the EDA CR explicitly sets them; to enable auto-inheritance
from the operator pod environment, change the defaults to read the env via
lookup('env', ...) (e.g., set http_proxy: "{{ lookup('env','http_proxy') |
default('') }}", and similarly for https_proxy and no_proxy) so templates will
render proxy env vars when the operator has them while still allowing CR
overrides; update the variables named http_proxy, https_proxy, and no_proxy in
defaults/main.yml and ensure templates keep their existing {% if http_proxy %}
checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c0f2aa86-3077-4951-a2ee-f8f87130b124
📒 Files selected for processing (7)
config/crd/bases/eda.ansible.com_edas.yamlconfig/manifests/bases/eda-server-operator.clusterserviceversion.yamlroles/eda/defaults/main.ymlroles/eda/templates/eda-activation-worker.deployment.yaml.j2roles/eda/templates/eda-api.deployment.yaml.j2roles/eda/templates/eda-default-worker.deployment.yaml.j2roles/eda/templates/eda-event-stream.deployment.yaml.j2
jamesmarshall24
force-pushed
the
AAP-71680
branch
2 times, most recently
from
551b2bb to
d484073
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
roles/eda/templates/eda-api.deployment.yaml.j2 (1)
324-341: Proxy vars not injected for init containers (run-migrations,eda-initial-data).Same note as event-stream:
create_initial_dataandcollectstaticineda-initial-datacould in some environments need outbound access. If that’s intentionally excluded, ignore; otherwise consider mirroring the same conditional block into those init containerenvlists.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@roles/eda/templates/eda-api.deployment.yaml.j2` around lines 324 - 341, The init containers run-migrations and eda-initial-data are missing the conditional proxy environment variables block, so create_initial_data and collectstatic inside eda-initial-data (and any network-using logic in run-migrations) may not be able to reach external endpoints; copy the same conditional env block (the HTTP_PROXY/https_proxy/NO_PROXY entries currently present for the main container) into the env lists for the init containers named run-migrations and eda-initial-data so those init containers receive HTTP_PROXY, http_proxy, HTTPS_PROXY, https_proxy, NO_PROXY and no_proxy when the corresponding variables (http_proxy, https_proxy, no_proxy) are defined.molecule/default/tasks/proxy_env_var_test.yml (1)
33-129: Heavy duplication across four assert blocks — consider a loop.The four wait/assert blocks are near-identical apart from the component label and register var. A single
loop:over a list of{component, expected_http, expected_no_proxy}dicts would cut ~100 lines and make future additions trivial. Non-blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@molecule/default/tasks/proxy_env_var_test.yml` around lines 33 - 129, Replace the four nearly identical "Wait for proxy vars" + "Assert all 6 proxy vars" blocks with a single pair of tasks that iterate over a list of component definitions (e.g., items with keys component and fail_msg or name) — use one kubernetes.core.k8s_info task with loop over the components (registering a single variable like deployment_result per item) and one ansible.builtin.assert task that also loops over the same list and asserts the six env vars against the expected values using item.component to index into deployment_result.resources[0].spec.template.spec.containers[0].env; keep the same filter expressions (selectattr('name','equalto') | map(attribute='value') | first == 'http://localhost:3128' and for NO_PROXY/no_proxy) and reuse the original fail messages by referencing item.fail_msg so behavior and checks for api_deployment, default_worker_deployment, activation_worker_deployment, and event_stream_deployment are preserved but deduplicated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@molecule/default/tasks/proxy_env_var_test.yml`:
- Around line 2-13: The test currently patches the EDA CR in the task named
"Patch EDA CR to set proxy env vars" leaving spec.http_proxy, spec.https_proxy
and spec.no_proxy set to localhost:3128, which breaks later tasks; add a
follow-up cleanup task (e.g. "Revert proxy env vars on EDA CR") that uses
kubernetes.core.k8s with state: patched against the same
api_version/kind/name/namespace and sets spec.http_proxy: '', spec.https_proxy:
'' and spec.no_proxy: '' so the CR is restored after the test; ensure the
cleanup task runs after the patch (append to the same playbook or mark as last).
---
Nitpick comments:
In `@molecule/default/tasks/proxy_env_var_test.yml`:
- Around line 33-129: Replace the four nearly identical "Wait for proxy vars" +
"Assert all 6 proxy vars" blocks with a single pair of tasks that iterate over a
list of component definitions (e.g., items with keys component and fail_msg or
name) — use one kubernetes.core.k8s_info task with loop over the components
(registering a single variable like deployment_result per item) and one
ansible.builtin.assert task that also loops over the same list and asserts the
six env vars against the expected values using item.component to index into
deployment_result.resources[0].spec.template.spec.containers[0].env; keep the
same filter expressions (selectattr('name','equalto') | map(attribute='value') |
first == 'http://localhost:3128' and for NO_PROXY/no_proxy) and reuse the
original fail messages by referencing item.fail_msg so behavior and checks for
api_deployment, default_worker_deployment, activation_worker_deployment, and
event_stream_deployment are preserved but deduplicated.
In `@roles/eda/templates/eda-api.deployment.yaml.j2`:
- Around line 324-341: The init containers run-migrations and eda-initial-data
are missing the conditional proxy environment variables block, so
create_initial_data and collectstatic inside eda-initial-data (and any
network-using logic in run-migrations) may not be able to reach external
endpoints; copy the same conditional env block (the
HTTP_PROXY/https_proxy/NO_PROXY entries currently present for the main
container) into the env lists for the init containers named run-migrations and
eda-initial-data so those init containers receive HTTP_PROXY, http_proxy,
HTTPS_PROXY, https_proxy, NO_PROXY and no_proxy when the corresponding variables
(http_proxy, https_proxy, no_proxy) are defined.
🪄 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: CHILL
Plan: Pro Plus
Run ID: 40da8f3a-1984-42d0-b947-57a66b464c5a
📒 Files selected for processing (9)
config/crd/bases/eda.ansible.com_edas.yamlconfig/manifests/bases/eda-server-operator.clusterserviceversion.yamlconfig/samples/eda_v1alpha1_eda.yamlmolecule/default/tasks/proxy_env_var_test.ymlroles/eda/defaults/main.ymlroles/eda/templates/eda-activation-worker.deployment.yaml.j2roles/eda/templates/eda-api.deployment.yaml.j2roles/eda/templates/eda-default-worker.deployment.yaml.j2roles/eda/templates/eda-event-stream.deployment.yaml.j2
✅ Files skipped from review due to trivial changes (2)
- config/samples/eda_v1alpha1_eda.yaml
- config/crd/bases/eda.ansible.com_edas.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- roles/eda/templates/eda-default-worker.deployment.yaml.j2
- roles/eda/defaults/main.yml
Add http_proxy, https_proxy, and no_proxy CRD fields to the EDA spec. Add the proxy-aware OLM annotation to the CSV so that OLM injects cluster proxy configuration into the operator manager pod. Assisted by: Claude Signed-off-by: James Marshall <jamarsha@redhat.com>
jamesmarshall24
force-pushed
the
AAP-71680
branch
from
383659a to
99384bb
Compare
Add the proxy env block to all init containers that may make outbound network calls in proxy-required environments: - eda-api: run-migrations and eda-initial-data (collectstatic) - eda-event-stream: wait-for-migrations - eda-default-worker: wait-for-migrations - eda-activation-worker: wait-for-migrations The configure-bundle-ca-cert init container is intentionally excluded as it does not make network calls. Assisted by: Claude Signed-off-by: James Marshall <jamarsha@redhat.com>
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add http_proxy, https_proxy, and no_proxy CRD fields to the EDA spec.
Add the proxy-aware OLM annotation to the CSV so that OLM injects cluster proxy configuration into the operator manager pod.
Summary by CodeRabbit
New Features
Chores
Tests