feat: runtime NetworkPolicies for EDA operator workloads - #362
Conversation
|
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: Enterprise Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. 📝 WalkthroughWalkthroughThis change adds NetworkPolicies for EDA workloads and managed PostgreSQL. It updates RBAC permissions, applies policies through Ansible tasks, handles optional UI policy removal, and adds Molecule checks for policy existence and selectors. ChangesNetworkPolicy enforcement
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🟡 Moderate · up to When the UI is disabled, the policy deletion task may fail because it combines deletion with server-side apply, preventing successful reconciliation. This should be corrected before merge. Sequence Diagram(s)sequenceDiagram
participant AnsibleRoles
participant KubernetesAPI
participant EDAWorkloads
participant PostgreSQLPods
AnsibleRoles->>KubernetesAPI: apply EDA NetworkPolicies
AnsibleRoles->>KubernetesAPI: apply managed PostgreSQL NetworkPolicy
KubernetesAPI->>EDAWorkloads: enforce ingress and egress rules
KubernetesAPI->>PostgreSQLPods: enforce PostgreSQL ingress and DNS egress
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (3)
roles/eda/defaults/main.yml (1)
11-12: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse one shared source for
supported_pg_version.The EDA NetworkPolicies use this value in five PostgreSQL selectors. A version mismatch can block database egress for four policies; the activation-worker policy also allows unrestricted egress. Move the value to play-level or
group_varsdata.🤖 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 `@roles/eda/defaults/main.yml` around lines 11 - 12, Move supported_pg_version out of roles/eda/defaults/main.yml into shared play-level or group_vars data, then update the EDA NetworkPolicies and roles/postgres usage to reference that single shared variable. Remove the duplicated role default while preserving the current PostgreSQL version value and all five selector references.roles/eda/tasks/networkpolicy.yml (1)
14-18: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueRemove
apply: truefrom the deletion task.ui_disabledhas a role default offalse.state: absentdeletes the NetworkPolicy identified by its metadata name and namespace.applyonly affects updates.🤖 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 `@roles/eda/tasks/networkpolicy.yml` around lines 14 - 18, Remove the apply: true parameter from the kubernetes.core.k8s task applying the EDA UI NetworkPolicy, while preserving the existing conditional state based on ui_disabled and the templated definition.molecule/default/tasks/eda_test.yml (1)
57-73: 🔒 Security & Privacy | 🔵 Trivial | 🏗️ Heavy liftValidate workload policy rules, not only resource existence.
This loop accepts any policy with the expected name. It does not validate selectors, policy types, peers, or ports. An empty or overly broad policy can therefore pass while violating least-privilege isolation. Add per-workload assertions for the intended rules.
As per path instructions, this comment addresses a major security validation gap.
🤖 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 `@molecule/default/tasks/eda_test.yml` around lines 57 - 73, Extend the “Verify per-workload NetworkPolicies exist” task to assert each policy’s intended selectors, policyTypes, ingress/egress peers, and ports rather than only checking resources | length. Use workload-specific expected rules for every loop item, and fail when any policy is empty or broader than the least-privilege configuration.Source: Path instructions
🤖 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 `@molecule/default/tasks/eda_test.yml`:
- Around line 67-73: Update the policy-check loop in the EDA test to
conditionally exclude the ui entry when ui_disabled is true, while retaining all
other policy checks. Add an assertion for the ui_disabled path that the UI
policy is absent, and preserve the existing policy-existence assertions when the
UI is enabled.
- Around line 52-54: Update the PostgreSQL NetworkPolicy assertions in the EDA
test to validate the complete database selector, including the PostgreSQL name
and instance labels alongside component=database, and verify the ingress rule’s
protocol is TCP and its port equals eda_postgres_port. Preserve the existing eda
source-selector validation while ensuring an overly broad selector or all-port
rule cannot pass.
- Around line 29-31: Update the default-deny selector assertions to require
operator == 'In' and ensure the values collection contains exactly the expected
deployment_type-derived value, rather than merely containing 'eda-operator'.
Preserve the existing key and single-expression checks in the np_default_deny
assertions.
- Line 20: Define test variables for the EDA resource name (`eda-sample`) and
`supported_pg_version` in eda_test.yml, then update the NetworkPolicy lookups at
molecule/default/tasks/eda_test.yml lines 20, 43, and 62 to use the EDA
resource-name variable instead of namespace-derived names; use the
supported_pg_version variable wherever the PostgreSQL version is referenced.
In `@roles/eda/templates/eda-activation-worker.networkpolicy.yaml.j2`:
- Around line 36-65: The unrestricted `- {}` egress rule in the activation
worker policy supersedes the DNS, managed database, and gateway rules. Replace
it with an `ipBlock` rule allowing external destinations while excluding the
cluster and service CIDRs, preserving the existing granular internal egress
rules and the comment explaining user-defined event-source destinations.
In `@roles/eda/templates/eda-api.networkpolicy.yaml.j2`:
- Around line 70-75: Update the external-database egress rule in the network
policy template to include a `to.ipBlock` destination using the configured
database host’s stable CIDR(s), while retaining the existing TCP port selection.
Use the already-available `managed_database` configuration to source the
destination; do not attempt to place a DNS hostname in `ipBlock`, and preserve
the managed-database branch behavior.
- Around line 50-59: Make DNS egress cluster-aware by creating one configurable
partial and reusing it in all seven network policies:
roles/eda/templates/eda-api.networkpolicy.yaml.j2 lines 50-59,
roles/eda/templates/eda-default-worker.networkpolicy.yaml.j2 lines 27-36,
roles/eda/templates/eda-activation-worker.networkpolicy.yaml.j2 lines 26-35,
roles/eda/templates/eda-event-stream.networkpolicy.yaml.j2 lines 48-57,
roles/eda/templates/eda-ui.networkpolicy.yaml.j2 lines 47-56,
roles/eda/templates/db-management.networkpolicy.yaml.j2 lines 17-26, and
roles/postgres/templates/postgres.networkpolicy.yaml.j2. Select openshift-dns on
port 5353 for OpenShift and kube-system on port 53 for Kubernetes, and remove
the activation-worker DNS rule where its unrestricted - {} egress rule already
permits traffic.
In `@roles/eda/templates/eda-ui.networkpolicy.yaml.j2`:
- Around line 19-45: Update both ingress port entries in the network policy
template’s router/ingress-controller and gateway-operator rules to use the UI
listener port 8080 instead of api_nginx_port. Leave the UI egress rule using
api_nginx_port for API access.
---
Nitpick comments:
In `@molecule/default/tasks/eda_test.yml`:
- Around line 57-73: Extend the “Verify per-workload NetworkPolicies exist” task
to assert each policy’s intended selectors, policyTypes, ingress/egress peers,
and ports rather than only checking resources | length. Use workload-specific
expected rules for every loop item, and fail when any policy is empty or broader
than the least-privilege configuration.
In `@roles/eda/defaults/main.yml`:
- Around line 11-12: Move supported_pg_version out of
roles/eda/defaults/main.yml into shared play-level or group_vars data, then
update the EDA NetworkPolicies and roles/postgres usage to reference that single
shared variable. Remove the duplicated role default while preserving the current
PostgreSQL version value and all five selector references.
In `@roles/eda/tasks/networkpolicy.yml`:
- Around line 14-18: Remove the apply: true parameter from the
kubernetes.core.k8s task applying the EDA UI NetworkPolicy, while preserving the
existing conditional state based on ui_disabled and the templated definition.
🪄 Autofix
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: Enterprise
Run ID: a8a4df3e-76e9-4cbb-8a6c-08f7e64234e4
📒 Files selected for processing (15)
config/rbac/role.yamlmolecule/default/tasks/eda_test.ymlroles/eda/defaults/main.ymlroles/eda/tasks/main.ymlroles/eda/tasks/networkpolicy.ymlroles/eda/templates/db-management.networkpolicy.yaml.j2roles/eda/templates/default-deny.networkpolicy.yaml.j2roles/eda/templates/eda-activation-worker.networkpolicy.yaml.j2roles/eda/templates/eda-api.networkpolicy.yaml.j2roles/eda/templates/eda-default-worker.networkpolicy.yaml.j2roles/eda/templates/eda-event-stream.networkpolicy.yaml.j2roles/eda/templates/eda-ui.networkpolicy.yaml.j2roles/postgres/tasks/main.ymlroles/postgres/tasks/networkpolicy.ymlroles/postgres/templates/postgres.networkpolicy.yaml.j2
|
Responding to the three nitpick comments from the CodeRabbit review summary:
Validate workload policy rules, not only resource existence — Declined per the test plan for AAP-73976 (MR !280), which explicitly delegates rule-level connectivity validation to ATF regression and scopes molecule to existence validation and key selector checks. The |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
roles/eda/templates/eda-api.networkpolicy.yaml.j2 (1)
62-77: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftAn empty
- {}egress rule removes all egress isolation and makes the DNS rule dead configuration. In all four policies the- {}rule permits every destination on every port, including the Kubernetes API server, other namespaces, and any in-cluster service. The precedingopenshift-dnsrule therefore has no effect, andpolicyTypes: [Egress]no longer restricts anything. A reader can wrongly conclude that these workloads are egress-restricted.Pick one approach and apply it consistently:
- Keep
- {}and delete the superseded DNS rule, keeping only the explanatory comment. This is honest but leaves in-cluster egress fully open.- Replace
- {}with anipBlockfor0.0.0.0/0that hasexceptentries for the cluster pod CIDR and the service CIDR, and keep the granular DNS rule plus explicit rules for postgres and the gateway. This preserves the isolation goal of the PR while still permitting user-defined external destinations.Approach 2 is preferred for the API and event-stream policies, which have known in-cluster destinations.
roles/eda/templates/eda-api.networkpolicy.yaml.j2#L62-L77: replace- {}with a CIDR-scoped external rule and restore explicit postgres and gateway egress rules, or delete the DNS rule at Lines 63-72.roles/eda/templates/eda-default-worker.networkpolicy.yaml.j2#L26-L41: apply the same choice; delete the DNS rule at Lines 27-36 if- {}stays.roles/eda/templates/eda-activation-worker.networkpolicy.yaml.j2#L25-L40: apply the same choice; delete the DNS rule at Lines 26-35 if- {}stays.roles/eda/templates/eda-event-stream.networkpolicy.yaml.j2#L47-L63: apply the same choice; delete the DNS rule at Lines 48-57 if- {}stays.🤖 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 `@roles/eda/templates/eda-api.networkpolicy.yaml.j2` around lines 62 - 77, The empty egress rule makes the DNS allow rule ineffective and removes all egress isolation. Prefer the CIDR-scoped external egress approach: in roles/eda/templates/eda-api.networkpolicy.yaml.j2 lines 62-77, roles/eda/templates/eda-default-worker.networkpolicy.yaml.j2 lines 26-41, roles/eda/templates/eda-activation-worker.networkpolicy.yaml.j2 lines 25-40, and roles/eda/templates/eda-event-stream.networkpolicy.yaml.j2 lines 47-63, replace - {} with an ipBlock for 0.0.0.0/0 excluding the cluster pod and service CIDRs, retain DNS access, and add explicit postgres and gateway rules where applicable. Apply the same policy consistently across all four templates.
🤖 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.
Duplicate comments:
In `@roles/eda/templates/eda-api.networkpolicy.yaml.j2`:
- Around line 62-77: The empty egress rule makes the DNS allow rule ineffective
and removes all egress isolation. Prefer the CIDR-scoped external egress
approach: in roles/eda/templates/eda-api.networkpolicy.yaml.j2 lines 62-77,
roles/eda/templates/eda-default-worker.networkpolicy.yaml.j2 lines 26-41,
roles/eda/templates/eda-activation-worker.networkpolicy.yaml.j2 lines 25-40, and
roles/eda/templates/eda-event-stream.networkpolicy.yaml.j2 lines 47-63, replace
- {} with an ipBlock for 0.0.0.0/0 excluding the cluster pod and service CIDRs,
retain DNS access, and add explicit postgres and gateway rules where applicable.
Apply the same policy consistently across all four templates.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 74a31ffd-2f96-4329-837b-73c5790f127f
📒 Files selected for processing (8)
molecule/default/tasks/eda_test.ymlroles/eda/tasks/networkpolicy.ymlroles/eda/templates/eda-activation-worker.networkpolicy.yaml.j2roles/eda/templates/eda-api.networkpolicy.yaml.j2roles/eda/templates/eda-default-worker.networkpolicy.yaml.j2roles/eda/templates/eda-event-stream.networkpolicy.yaml.j2roles/eda/templates/eda-ui.networkpolicy.yaml.j2roles/postgres/templates/postgres.networkpolicy.yaml.j2
💤 Files with no reviewable changes (1)
- roles/eda/tasks/networkpolicy.yml
🚧 Files skipped from review as they are similar to previous changes (3)
- roles/eda/templates/eda-ui.networkpolicy.yaml.j2
- molecule/default/tasks/eda_test.yml
- roles/postgres/templates/postgres.networkpolicy.yaml.j2
rooftopcellist
left a comment
rooftopcellist
left a comment
There was a problem hiding this comment.
Two prior major findings are still open on the worker NPs, and two new findings surfaced in this round. Details are inline.
| - from: | ||
| - podSelector: | ||
| matchLabels: | ||
| app.kubernetes.io/managed-by: '{{ deployment_type }}-operator' |
There was a problem hiding this comment.
Remove this ingress block entirely. EDA workers use a pull model — the activation-worker connects out to poll for work and never receives inbound connections. The 'task dispatch' comment isn't accurate here: dispatch happens via the worker calling out to the API or Redis, not via inbound pushes to the worker.
Change policyTypes to [Egress] only and drop the ingress: section. This is the same fix that was applied to galaxy-worker in galaxy-operator PR #278.
| - from: | ||
| - podSelector: | ||
| matchLabels: | ||
| app.kubernetes.io/managed-by: '{{ deployment_type }}-operator' |
There was a problem hiding this comment.
Remove this ingress block. The postgres LISTEN/NOTIFY mechanism works over the worker's existing outbound connection to postgres — postgres does not open a new inbound connection back to the worker. No port needs to be open on the worker for this to function.
Change policyTypes to [Egress] only and drop the ingress: section. A portless ingress rule allowing all traffic from any operator-managed pod on all ports defeats the purpose of the NetworkPolicy.
| app.kubernetes.io/managed-by: '{{ deployment_type }}-operator' | ||
| app.kubernetes.io/component: '{{ deployment_type }}' | ||
| policyTypes: | ||
| - Egress |
There was a problem hiding this comment.
Should be policyTypes: [Ingress, Egress] with no ingress: rules. Declaring only Egress leaves ingress governance entirely to the catch-all default-deny NP. If the default-deny is ever absent — maintenance window, namespace migration, disaster recovery — this pod would have no ingress protection at all. Each NP should be self-contained in what it governs.
| @@ -81,13 +81,24 @@ rules: | |||
| - watch | |||
| - apiGroups: | |||
| - apps | |||
There was a problem hiding this comment.
Duplicate apps apiGroup rule — there's an identical block earlier in the file. This looks like a leftover from splitting the networking.k8s.io rule into its own block. Kubernetes ignores duplicate rules at runtime but it makes the ClusterRole harder to audit. Remove this block.
|
rooftopcellist
left a comment
rooftopcellist
left a comment
There was a problem hiding this comment.
All open findings addressed. Activation-worker and default-worker are now correctly pure-egress with no ingress rules. db-management policyTypes now covers both Ingress and Egress. Duplicate RBAC rule removed. apply: true is consistent across all NP tasks. Approved.
…ivation jobs When activation_job_namespace is set, activation Job pods run in a different namespace. The default-deny NetworkPolicies from ansible#362 use bare podSelector which only matches pods in the same namespace, blocking websocket, API, and pg_notify traffic from cross-namespace job pods. Add conditional namespaceSelector to eda-api and postgres NetworkPolicy templates so ingress rules match activation job pods regardless of which namespace they run in. Co-authored-by: Cursor <cursoragent@cursor.com>
…ivation jobs When activation_job_namespace is set, activation Job pods run in a different namespace. The default-deny NetworkPolicies from ansible#362 use bare podSelector which only matches pods in the same namespace, blocking websocket, API, and pg_notify traffic from cross-namespace job pods. Add conditional namespaceSelector to eda-api and postgres NetworkPolicy templates so ingress rules match activation job pods regardless of which namespace they run in. Co-authored-by: Cursor <cursoragent@cursor.com>
* feat: add configurable namespace for activation job pods Add activation_job_namespace field to the activation_worker section of the EDA CRD. When set, the operator injects EDA_ACTIVATION_JOB_NAMESPACE into the activation worker ConfigMap and creates a Role + RoleBinding in the target namespace so the EDA ServiceAccount can manage Jobs, Pods, Secrets, and Services there. Changes: - CRD: new optional string field activation_worker.activation_job_namespace - Role defaults: activation_job_namespace defaults to empty string - ConfigMap template: conditionally sets EDA_ACTIVATION_JOB_NAMESPACE - New RBAC template for cross-namespace Role and RoleBinding - Deploy task: applies/removes cross-namespace RBAC conditionally - ClusterRole + ClusterRoleBinding for operator to manage RBAC in the target namespace Closes #344 Signed-off-by: Alexey Masolov <amasolov@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com> * ci: add activation_job_namespace scenario to PR workflow Add a CI test scenario for the new activation_job_namespace parameter. Creates a dedicated CR fixture and pre-creates the target namespace before applying the CR. Signed-off-by: Alexey Masolov <amasolov@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com> * fix: capture previous namespace before ConfigMap apply and handle namespace changes Move the lookup of _previous_activation_job_namespace to run before the ConfigMap is applied, so the old value is read before it gets overwritten. Also widen the RBAC cleanup condition to trigger whenever the previous namespace differs from the new one (not just when the new value is empty), so changing from namespace A to B correctly removes orphaned Role and RoleBinding from A. Signed-off-by: Alexey Masolov <amasolov@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com> * fix: add escalate verb to ClusterRole and fix RBAC task ordering Add the escalate verb on the roles resource in the eda-activation-job-namespace-manager ClusterRole. Kubernetes RBAC escalation prevention blocks creating a Role that grants permissions the creator doesn't hold. The escalate verb is the standard mechanism to permit this without granting the operator broad cluster-wide access to pods, secrets, services, and jobs. Also moves the _previous_activation_job_namespace lookup to run before the ConfigMap is applied (so the old value is captured before overwrite) and widens the RBAC cleanup condition to trigger on any namespace change, not just removal. Signed-off-by: Alexey Masolov <amasolov@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com> * fix: add bind verb to ClusterRole for RoleBinding creation The escalate verb fixed Role creation, but creating a RoleBinding that references a Role with elevated permissions also requires the bind verb on the roles resource. Without it, Kubernetes RBAC escalation prevention blocks the RoleBinding creation. Signed-off-by: Alexey Masolov <amasolov@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com> * fix: remove ignore_errors from RBAC cleanup task The k8s module with state=absent already handles 404 gracefully (treats "not found" as "already absent"), so ignore_errors is unnecessary and could mask legitimate permission or connectivity failures. Signed-off-by: Alexey Masolov <amasolov@redhat.com> * fix: add namespaceSelector to NetworkPolicies for cross-namespace activation jobs When activation_job_namespace is set, activation Job pods run in a different namespace. The default-deny NetworkPolicies from #362 use bare podSelector which only matches pods in the same namespace, blocking websocket, API, and pg_notify traffic from cross-namespace job pods. Add conditional namespaceSelector to eda-api and postgres NetworkPolicy templates so ingress rules match activation job pods regardless of which namespace they run in. Co-authored-by: Cursor <cursoragent@cursor.com> * fix: apply ConfigMap after RBAC migration to preserve rollback state Move the env-properties ConfigMap update to run after the cross-namespace RBAC create/delete tasks. Previously, the ConfigMap was updated first, so if the RBAC operations failed the next reconciliation would read the new namespace as _previous_activation_job_namespace and skip cleanup of the old Role and RoleBinding. Co-authored-by: Cursor <cursoragent@cursor.com> --------- Signed-off-by: Alexey Masolov <amasolov@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
3 participants
Create NetworkPolicy templates and tasks for all EDA operator-managed
workloads: api, ui, default-worker, activation-worker, event-stream,
postgres, and db-management (backup/restore pods).
Pattern follows the gateway operator reference, default-deny first
using match Expressions, per-workload least-privilege allow policies,
conditional managed/external database logic, DNS egress on all
egress-bearing policies, and NP tasks ordered before deployment tasks.
Key EDA-specific decisions:
pods that connect via pg_notify (not operator-managed, no managed-by
label)
Add networkpolicies to networking.k8s.io RBAC rule and replace molecule
FIXME stub with real NP assertions.
https://redhat.atlassian.net/browse/AAP-73976
Summary by CodeRabbit