feat: runtime NetworkPolicies for EDA operator workloads - #362
Conversation
|
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)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe operator adds NetworkPolicies for EDA workloads and managed PostgreSQL. It updates RBAC and Ansible task wiring, supports managed or external databases, conditionally removes the UI policy, and adds Molecule validation. ChangesNetworkPolicy enforcement
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant EDAAnsibleRole
participant PostgreSQLRole
participant KubernetesAPI
participant EDAWorkloads
participant PostgreSQLPods
EDAAnsibleRole->>KubernetesAPI: Apply EDA NetworkPolicies
KubernetesAPI->>EDAWorkloads: Enforce workload ingress and egress rules
PostgreSQLRole->>KubernetesAPI: Apply PostgreSQL NetworkPolicy
KubernetesAPI->>PostgreSQLPods: Enforce database ingress and DNS egress
sequenceDiagram
participant Molecule
participant KubernetesAPI
participant NetworkPolicies
Molecule->>KubernetesAPI: Retrieve expected NetworkPolicies
KubernetesAPI->>NetworkPolicies: Return policy resources
Molecule->>NetworkPolicies: Validate selectors, rules, and UI presence
Suggested reviewers: 🚥 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
|
|
|
||
| - name: Apply EDA UI NetworkPolicy | ||
| kubernetes.core.k8s: | ||
| state: "{{ 'present' if not ui_disabled else 'absent' }}" |
There was a problem hiding this comment.
should we also add an apply here for consistency? it doesn't necessarily change anything, and state does the job correctly... but still, we're doing it for every other resource, so... I think we should put it here as well:
| state: "{{ 'present' if not ui_disabled else 'absent' }}" | |
| state: "{{ 'present' if not ui_disabled else 'absent' }}" | |
| apply: "{{ omit if ui_disabled else true }}" |
| ui_disabled: false | ||
|
|
||
| # Keep in sync with roles/postgres/vars/main.yml | ||
| supported_pg_version: 15 |
There was a problem hiding this comment.
I think this is OK for now, but we could accidentally drift in case the postgres role changes its value. Why don't we load this value from the postgres role instead? We could include_vars before calling calling Apply PostgreSQL NetworkPolicy. WDYT?
2 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