MGMT-24178, MGMT-24196: Netris CaaS fixes / improvements - #268
openshift-merge-bot[bot] merged 1 commit into
Conversation
|
@danmanor: This pull request references MGMT-24178 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. DetailsIn response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
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:
WalkthroughThe PR changes multiple Ansible roles and templates to reconcile existing resources instead of unconditionally deleting/recreating them. Netris roles (nat, server_cluster, cluster_infra, external_access) now GET existing NAT/DNAT/SNAT and server cluster objects by name to skip creation or perform PUT updates; NAT/DNAT IPAM allocation is conditional. NMState config is created for all agents but applied only to newly added agents. A new wait_for nodes task, defaults, and template step were added and integrated into OCP install flow. Integration tests use KIND with the Podman provider and Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
collections/ansible_collections/netris/controller/roles/nat/tasks/create.yaml (1)
57-82:⚠️ Potential issue | 🟠 MajorDon't treat a name match as a converged NAT rule.
This now makes
nat_state: presentmean “present by name” only. If the rule already exists but any of the desired fields changed (dnatToIP, ports, source/destination CIDRs, pool, comment, etc.), the role skips the controller write and leaves stale NAT state behind. That is a correctness regression for callers that recompute NAT parameters between runs.Please compare the desired
_nat_bodyagainst_existing_natand update the rule when drift is detected instead of unconditionally skipping on name match.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@collections/ansible_collections/netris/controller/roles/nat/tasks/create.yaml` around lines 57 - 82, The task currently treats any name match as converged; change it to compare the desired _nat_body against the found _existing_nat and only skip creation if they are identical. After the "Find NAT rule by name" and "Set nat facts from existing rule" steps, add logic to compute whether fields drift (compare keys like dnatToIP, ports, source/destination CIDRs, pool, comment, etc. between _nat_body and _existing_nat) and set a boolean (e.g., _nat_needs_update). Then, instead of skipping when _existing_nat is present, branch: if _nat_needs_update is true call ansible.builtin.uri to update the existing rule (use method PUT or PATCH against {{ netris_controller_url }}/api/v2/nat/{{ nat_id }} with body _nat_body), else mark nat_already_existed and skip creation; ensure nat_id and nat_existing_rule remain set.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@collections/ansible_collections/netris/controller/roles/server_cluster/tasks/create.yaml`:
- Around line 63-76: The vpc field in the _request_body set by the "Build
request body (base)" task currently always chooses the existing VPC or defaults
to {id:0,name:'Create New'}, ignoring caller-supplied
server_cluster_vpc_id/server_cluster_vpc_name; modify the vpc logic to first
preserve and use the caller-supplied server_cluster_vpc_id and
server_cluster_vpc_name when provided (non-empty), otherwise fall back to the
existing _existing_server_cluster.vpc (if defined) and only then default to
{id:0, name:'Create New'}; update the vpc expression used when setting
_request_body so functions/consumers relying on server_cluster_vpc_id get the
intended VPC.
In
`@collections/ansible_collections/netris/steps/roles/external_access/tasks/create.yaml`:
- Around line 58-79: The current task lumps both DNAT facts and IPAM allocation
together, causing both netris_dnat_api_ip and netris_dnat_ingress_ip to be
overwritten when only one rule is missing; change logic so each discovered
floating IP fact is set independently from _existing_api_dnat and
_existing_ingress_http_dnat (and check _existing_ingress_https_dnat if
applicable) and invoke the netris.controller.ipam role only for the number of
missing IPs (compute ipam_count = 0/1/2 based on which of _existing_api_dnat and
_existing_ingress_http_dnat are undefined/null), then map allocated ips to only
the missing facts (e.g., assign ipam_allocated_ips[0] to the first missing fact
and [1] to the second) so existing rules’ IPs are preserved and only missing
addresses are provisioned.
---
Outside diff comments:
In
`@collections/ansible_collections/netris/controller/roles/nat/tasks/create.yaml`:
- Around line 57-82: The task currently treats any name match as converged;
change it to compare the desired _nat_body against the found _existing_nat and
only skip creation if they are identical. After the "Find NAT rule by name" and
"Set nat facts from existing rule" steps, add logic to compute whether fields
drift (compare keys like dnatToIP, ports, source/destination CIDRs, pool,
comment, etc. between _nat_body and _existing_nat) and set a boolean (e.g.,
_nat_needs_update). Then, instead of skipping when _existing_nat is present,
branch: if _nat_needs_update is true call ansible.builtin.uri to update the
existing rule (use method PUT or PATCH against {{ netris_controller_url
}}/api/v2/nat/{{ nat_id }} with body _nat_body), else mark nat_already_existed
and skip creation; ensure nat_id and nat_existing_rule remain set.
🪄 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
Run ID: 2cbe91d8-81c4-4f99-854b-df894421312d
📒 Files selected for processing (4)
collections/ansible_collections/netris/controller/roles/nat/tasks/create.yamlcollections/ansible_collections/netris/controller/roles/server_cluster/tasks/create.yamlcollections/ansible_collections/netris/steps/roles/cluster_infra/tasks/create.yamlcollections/ansible_collections/netris/steps/roles/external_access/tasks/create.yaml
bd77f42 to
bb69514
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
collections/ansible_collections/netris/steps/roles/cluster_infra/tasks/create.yaml (1)
168-201:⚠️ Potential issue | 🟠 MajorDon't skip SNAT reconciliation just because the name already exists.
After Line 168 sets
netris_nat_snat_ipfrom_existing_snat, Lines 173-201 bypass the NAT role completely. That means changes tonat_source_address,nat_vpc_id,nat_vpc_name, or the desired SNAT IP are never applied once the rule exists. For a scale/reconfigure flow, this can leave the cluster using an old SNAT rule indefinitely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@collections/ansible_collections/netris/steps/roles/cluster_infra/tasks/create.yaml` around lines 168 - 201, The current flow sets netris_nat_snat_ip from _existing_snat and then skips the netris.controller.nat role when _existing_snat is present, preventing reconciliation; change the logic so the include_role name: netris.controller.nat always runs (no when gated by _existing_snat) and relies on netris_nat_snat_ip which is either set from _existing_snat or from the IPAM allocation; keep the two set_fact tasks that populate netris_nat_snat_ip but remove the when condition from the NAT include (or invert to always true) so nat_source_address, nat_vpc_id, nat_vpc_name and nat_snat_to_ip are always applied and the rule is reconciled/updated via the nat role.collections/ansible_collections/netris/steps/roles/external_access/tasks/create.yaml (1)
81-97:⚠️ Potential issue | 🟠 MajorExisting DNAT rules can keep stale backend targets.
These
whenclauses skip the NAT role solely because a rule with the same name exists. If the API service IP/port or the MetalLB ingress IP changes,dnatToIP/dnatToPortwill never be reconciled and traffic keeps flowing to the old backend. Please let the NAT role reconcilepresentstate, or explicitly compare the existing rule and update it on drift.Also applies to: 161-177, 179-195
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@collections/ansible_collections/netris/steps/roles/external_access/tasks/create.yaml` around lines 81 - 97, The task "Create DNAT rule for API endpoint" currently skips including the netris.controller.nat role whenever _existing_api_dnat exists, which prevents reconciliation of nat_dnat_to_ip/port drift; change the logic so the role is always executed to enforce nat_state: present (or modify the when to compare _existing_api_dnat.dnatToIP / _existing_api_dnat.dnatToPort / nat_protocol / nat_destination_port against the desired vars and only skip when they match). Specifically update the task that includes role netris.controller.nat (and the similarly structured tasks referenced at the other ranges) to either remove the when: _existing_api_dnat... guard or replace it with a conditional that verifies equality between _existing_api_dnat fields and the variables nat_dnat_to_ip, nat_dnat_to_port (and nat_destination_port/nat_protocol as appropriate) so the role will run and reconcile when any backend target has changed.collections/ansible_collections/netris/controller/roles/nat/tasks/create.yaml (1)
57-90:⚠️ Potential issue | 🟠 MajorDon't treat a same-name NAT rule as already converged.
Once
_existing_natis found, this role skips Line 69 entirely and never checks whether the current controller rule still matches_nat_body. That leaves stale NAT config in place after backend/IP/port/VPC changes, which is exactly the kind of drift apresentrole should reconcile instead of silently accepting. Please compare the existing rule to the desired payload and update/recreate when they differ.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@collections/ansible_collections/netris/controller/roles/nat/tasks/create.yaml` around lines 57 - 90, The role currently treats a same-name NAT rule as converged without comparing its fields; change the flow so that after finding _existing_nat you compare its relevant fields to the desired payload _nat_body (normalize keys/types as needed) and only mark nat_already_existed true if they are equal; if they differ, call the API to update the existing rule (use the existing nat id from _existing_nat.id and a PUT/PATCH to /api/v2/nat/{{ nat_id }} or recreate as appropriate) and then set nat_id and nat_already_existed=false (or nat_updated flag) from the update response; update the tasks named "Find NAT rule by name", "Set nat facts from existing rule (skip creation)", and add an "Update NAT rule" URI task (or conditional "Create NAT rule") that runs when comparison shows differences, ensuring subsequent "Set nat facts from create response" logic uses the correct response variable names.
♻️ Duplicate comments (2)
collections/ansible_collections/netris/controller/roles/server_cluster/tasks/create.yaml (1)
85-93:⚠️ Potential issue | 🟠 MajorCaller-supplied VPC is still ignored on create path.
Line 92 is still hardcoded to
{'id': 0, 'name': 'Create New'}, soserver_cluster_vpc_id/server_cluster_vpc_nameare not honored when creating a new cluster.Suggested fix
- name: Build create body (base) ansible.builtin.set_fact: _create_body: name: "{{ server_cluster_name }}" site: "{{ {'id': server_cluster_site_id | int} }}" servers: "{{ _server_cluster_servers | default([]) }}" tags: "{{ server_cluster_tags | default([]) }}" - vpc: "{{ {'id': 0, 'name': 'Create New'} }}" + vpc: >- + {{ + {'id': server_cluster_vpc_id | int, 'name': server_cluster_vpc_name | default('')} + if (server_cluster_vpc_id is defined) + else {'id': 0, 'name': 'Create New'} + }} when: _existing_server_cluster is not defined or _existing_server_cluster is none🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@collections/ansible_collections/netris/controller/roles/server_cluster/tasks/create.yaml` around lines 85 - 93, The vpc field in the _create_body set_fact (in tasks/create.yaml, building variable _create_body) is hardcoded to {'id': 0, 'name': 'Create New'} so caller-supplied server_cluster_vpc_id/server_cluster_vpc_name are ignored; update the vpc assignment in the _create_body to use server_cluster_vpc_id and server_cluster_vpc_name when provided (and fall back to {'id': 0, 'name': 'Create New'} when they are undefined or null), ensuring you reference the same variable names (server_cluster_vpc_id, server_cluster_vpc_name) and preserve the existing when condition that only sets _create_body when _existing_server_cluster is not defined or none.collections/ansible_collections/netris/steps/roles/external_access/tasks/create.yaml (1)
58-79:⚠️ Potential issue | 🔴 CriticalPreserve existing DNAT IPs independently instead of reallocating both.
If only one DNAT already exists, this block still allocates two fresh IPs and overwrites both facts. The later create tasks then skip the existing rule, so DNS can end up pointing at an IP that no controller rule owns. HTTPS is also ignored when deciding whether the ingress floating IP can be reused. Set each discovered floating IP independently, allocate only the missing address(es), and map new IPAM results only onto the missing facts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@collections/ansible_collections/netris/steps/roles/external_access/tasks/create.yaml` around lines 58 - 79, Existing DNAT facts (netris_dnat_api_ip, netris_dnat_ingress_ip) must be preserved individually: change the when logic so you only call the ipam role (netris.controller.ipam) with ipam_count equal to the number of missing addresses (compute missing from _existing_api_dnat and _existing_ingress_http_dnat), and only set the corresponding fact(s) from ipam_allocated_ips for the missing entry(ies); in other words, test _existing_api_dnat and _existing_ingress_http_dnat separately, allocate only the required count, and map ipam_allocated_ips[0]/[1] only to the fact(s) that were not already defined.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@collections/ansible_collections/netris/controller/roles/server_cluster/tasks/create.yaml`:
- Around line 70-83: The "Update existing server cluster" task (ansible task
with register _update_resp, conditional when _existing_server_cluster is
defined) is leaking the session cookie via request logs; add no_log: true to
this task to prevent output of headers/body on failure/verbosity, and apply the
same no_log: true to any other tasks that send the Cookie header (e.g., other
ansible.builtin.uri tasks that use connect.sid or register variables) to ensure
auth-bearing URIs and responses are not logged.
---
Outside diff comments:
In
`@collections/ansible_collections/netris/controller/roles/nat/tasks/create.yaml`:
- Around line 57-90: The role currently treats a same-name NAT rule as converged
without comparing its fields; change the flow so that after finding
_existing_nat you compare its relevant fields to the desired payload _nat_body
(normalize keys/types as needed) and only mark nat_already_existed true if they
are equal; if they differ, call the API to update the existing rule (use the
existing nat id from _existing_nat.id and a PUT/PATCH to /api/v2/nat/{{ nat_id
}} or recreate as appropriate) and then set nat_id and nat_already_existed=false
(or nat_updated flag) from the update response; update the tasks named "Find NAT
rule by name", "Set nat facts from existing rule (skip creation)", and add an
"Update NAT rule" URI task (or conditional "Create NAT rule") that runs when
comparison shows differences, ensuring subsequent "Set nat facts from create
response" logic uses the correct response variable names.
In
`@collections/ansible_collections/netris/steps/roles/cluster_infra/tasks/create.yaml`:
- Around line 168-201: The current flow sets netris_nat_snat_ip from
_existing_snat and then skips the netris.controller.nat role when _existing_snat
is present, preventing reconciliation; change the logic so the include_role
name: netris.controller.nat always runs (no when gated by _existing_snat) and
relies on netris_nat_snat_ip which is either set from _existing_snat or from the
IPAM allocation; keep the two set_fact tasks that populate netris_nat_snat_ip
but remove the when condition from the NAT include (or invert to always true) so
nat_source_address, nat_vpc_id, nat_vpc_name and nat_snat_to_ip are always
applied and the rule is reconciled/updated via the nat role.
In
`@collections/ansible_collections/netris/steps/roles/external_access/tasks/create.yaml`:
- Around line 81-97: The task "Create DNAT rule for API endpoint" currently
skips including the netris.controller.nat role whenever _existing_api_dnat
exists, which prevents reconciliation of nat_dnat_to_ip/port drift; change the
logic so the role is always executed to enforce nat_state: present (or modify
the when to compare _existing_api_dnat.dnatToIP / _existing_api_dnat.dnatToPort
/ nat_protocol / nat_destination_port against the desired vars and only skip
when they match). Specifically update the task that includes role
netris.controller.nat (and the similarly structured tasks referenced at the
other ranges) to either remove the when: _existing_api_dnat... guard or replace
it with a conditional that verifies equality between _existing_api_dnat fields
and the variables nat_dnat_to_ip, nat_dnat_to_port (and
nat_destination_port/nat_protocol as appropriate) so the role will run and
reconcile when any backend target has changed.
---
Duplicate comments:
In
`@collections/ansible_collections/netris/controller/roles/server_cluster/tasks/create.yaml`:
- Around line 85-93: The vpc field in the _create_body set_fact (in
tasks/create.yaml, building variable _create_body) is hardcoded to {'id': 0,
'name': 'Create New'} so caller-supplied
server_cluster_vpc_id/server_cluster_vpc_name are ignored; update the vpc
assignment in the _create_body to use server_cluster_vpc_id and
server_cluster_vpc_name when provided (and fall back to {'id': 0, 'name':
'Create New'} when they are undefined or null), ensuring you reference the same
variable names (server_cluster_vpc_id, server_cluster_vpc_name) and preserve the
existing when condition that only sets _create_body when
_existing_server_cluster is not defined or none.
In
`@collections/ansible_collections/netris/steps/roles/external_access/tasks/create.yaml`:
- Around line 58-79: Existing DNAT facts (netris_dnat_api_ip,
netris_dnat_ingress_ip) must be preserved individually: change the when logic so
you only call the ipam role (netris.controller.ipam) with ipam_count equal to
the number of missing addresses (compute missing from _existing_api_dnat and
_existing_ingress_http_dnat), and only set the corresponding fact(s) from
ipam_allocated_ips for the missing entry(ies); in other words, test
_existing_api_dnat and _existing_ingress_http_dnat separately, allocate only the
required count, and map ipam_allocated_ips[0]/[1] only to the fact(s) that were
not already 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
Run ID: e641f31b-3c2b-4eb9-9601-90aefa7d22ba
📒 Files selected for processing (5)
.gitignorecollections/ansible_collections/netris/controller/roles/nat/tasks/create.yamlcollections/ansible_collections/netris/controller/roles/server_cluster/tasks/create.yamlcollections/ansible_collections/netris/steps/roles/cluster_infra/tasks/create.yamlcollections/ansible_collections/netris/steps/roles/external_access/tasks/create.yaml
✅ Files skipped from review due to trivial changes (1)
- .gitignore
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
bb013aa to
f7417f8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
collections/ansible_collections/netris/controller/roles/server_cluster/tasks/create.yaml (2)
70-83:⚠️ Potential issue | 🟠 MajorHide auth-bearing update calls with
no_log.Line 75 sends
connect.sid, but this PUT task can still emit headers and body on failures or higher verbosity. Please addno_log: truehere to avoid leaking session material.Suggested fix
- name: Update existing server cluster ansible.builtin.uri: url: "{{ netris_controller_url }}/api/v2/server-cluster/{{ _existing_server_cluster.id }}" method: PUT headers: Cookie: "connect.sid={{ netris_session_cookie }}" Content-Type: "application/json" body_format: json body: "{{ _update_body }}" return_content: true status_code: [200] validate_certs: "{{ netris_validate_certs | default(true) }}" register: _update_resp + no_log: true when: _existing_server_cluster is defined and _existing_server_cluster is not none🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@collections/ansible_collections/netris/controller/roles/server_cluster/tasks/create.yaml` around lines 70 - 83, The "Update existing server cluster" Ansible task leaks authentication data via headers/body on errors; add no_log: true to that task so the Cookie header containing {{ netris_session_cookie }} (and the request body {{ _update_body }}) are never printed to logs or failure output; ensure the no_log: true is applied to the same task that registers _update_resp and is conditional on _existing_server_cluster so all outputs for that PUT call are suppressed.
85-93:⚠️ Potential issue | 🟠 MajorHonor caller-supplied VPC on create.
Lines 85-93 still hardcode
Create New, soserver_cluster_vpc_id/server_cluster_vpc_nameare ignored on the create path. That breaks callers that already resolved the target VPC and expect this role to attach the cluster there.Suggested fix
- name: Build create body (base) ansible.builtin.set_fact: _create_body: name: "{{ server_cluster_name }}" site: "{{ {'id': server_cluster_site_id | int} }}" servers: "{{ _server_cluster_servers | default([]) }}" tags: "{{ server_cluster_tags | default([]) }}" - vpc: "{{ {'id': 0, 'name': 'Create New'} }}" + vpc: >- + {{ + {'id': server_cluster_vpc_id | int, 'name': server_cluster_vpc_name | default('')} + if (server_cluster_vpc_id is defined) + else {'id': 0, 'name': 'Create New'} + }} when: _existing_server_cluster is not defined or _existing_server_cluster is none🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@collections/ansible_collections/netris/controller/roles/server_cluster/tasks/create.yaml` around lines 85 - 93, The create body currently hardcodes vpc to {'id': 0, 'name': 'Create New'} and ignores caller-supplied server_cluster_vpc_id/server_cluster_vpc_name; update the _create_body.vpc assignment in the "Build create body (base)" task to use the provided VPC when server_cluster_vpc_id is defined (e.g. set vpc to a Jinja expression that yields {'id': (server_cluster_vpc_id | int), 'name': server_cluster_vpc_name | default('')} when server_cluster_vpc_id is defined, otherwise fall back to {'id': 0, 'name': 'Create New'}), keeping the rest of the _create_body keys unchanged.collections/ansible_collections/netris/steps/roles/external_access/tasks/create.yaml (1)
60-81:⚠️ Potential issue | 🟠 MajorPreserve existing DNAT IPs when only one rule is missing.
If only one DNAT rule already exists, Lines 68-81 still allocate two fresh IPs and overwrite both facts. The later
whenclauses then skip recreating the existing rule, so DNS can end up pointing at an address with no controller rule behind it. The HTTPS lookup also is not used to preservenetris_dnat_ingress_ipwhen HTTP is missing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@collections/ansible_collections/netris/steps/roles/external_access/tasks/create.yaml` around lines 60 - 81, The task currently always allocates two new IPs and overwrites both netris_dnat_api_ip and netris_dnat_ingress_ip even when one DNAT rule exists; change the logic to preserve existing facts and only allocate/assign missing IPs: check _existing_api_dnat and _existing_ingress_http_dnat individually, call the role netris.controller.ipam with ipam_count equal to the number of missing IPs (1 or 2), and then set netris_dnat_api_ip = ipam_allocated_ips[0] only if _existing_api_dnat is undefined/none (otherwise keep existing value) and similarly set netris_dnat_ingress_ip = ipam_allocated_ips[...] only if _existing_ingress_http_dnat is undefined/none; use those variable names (_existing_api_dnat, _existing_ingress_http_dnat, ipam_allocated_ips, netris_dnat_api_ip, netris_dnat_ingress_ip) to locate where to change the include_role and set_fact blocks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@collections/ansible_collections/netris/steps/roles/cluster_infra/tasks/create.yaml`:
- Around line 182-185: The Set SNAT IP from existing rule task currently sets
netris_nat_snat_ip to the host-only address (using _existing_snat.snatToIP |
ansible.utils.ipaddr('host')) which strips the mask; change the set_fact so
netris_nat_snat_ip stays in CIDR form on the reuse path (add a /32 when the
source has no mask or otherwise preserve the mask from _existing_snat.snatToIP)
so its shape matches the new-allocation branch; update the task that references
_existing_snat.snatToIP and the set_fact for netris_nat_snat_ip accordingly.
---
Duplicate comments:
In
`@collections/ansible_collections/netris/controller/roles/server_cluster/tasks/create.yaml`:
- Around line 70-83: The "Update existing server cluster" Ansible task leaks
authentication data via headers/body on errors; add no_log: true to that task so
the Cookie header containing {{ netris_session_cookie }} (and the request body
{{ _update_body }}) are never printed to logs or failure output; ensure the
no_log: true is applied to the same task that registers _update_resp and is
conditional on _existing_server_cluster so all outputs for that PUT call are
suppressed.
- Around line 85-93: The create body currently hardcodes vpc to {'id': 0,
'name': 'Create New'} and ignores caller-supplied
server_cluster_vpc_id/server_cluster_vpc_name; update the _create_body.vpc
assignment in the "Build create body (base)" task to use the provided VPC when
server_cluster_vpc_id is defined (e.g. set vpc to a Jinja expression that yields
{'id': (server_cluster_vpc_id | int), 'name': server_cluster_vpc_name |
default('')} when server_cluster_vpc_id is defined, otherwise fall back to
{'id': 0, 'name': 'Create New'}), keeping the rest of the _create_body keys
unchanged.
In
`@collections/ansible_collections/netris/steps/roles/external_access/tasks/create.yaml`:
- Around line 60-81: The task currently always allocates two new IPs and
overwrites both netris_dnat_api_ip and netris_dnat_ingress_ip even when one DNAT
rule exists; change the logic to preserve existing facts and only
allocate/assign missing IPs: check _existing_api_dnat and
_existing_ingress_http_dnat individually, call the role netris.controller.ipam
with ipam_count equal to the number of missing IPs (1 or 2), and then set
netris_dnat_api_ip = ipam_allocated_ips[0] only if _existing_api_dnat is
undefined/none (otherwise keep existing value) and similarly set
netris_dnat_ingress_ip = ipam_allocated_ips[...] only if
_existing_ingress_http_dnat is undefined/none; use those variable names
(_existing_api_dnat, _existing_ingress_http_dnat, ipam_allocated_ips,
netris_dnat_api_ip, netris_dnat_ingress_ip) to locate where to change the
include_role and set_fact blocks.
🪄 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
Run ID: d2e4a517-7a3a-4897-b88c-94a597f38932
📒 Files selected for processing (12)
.gitignorecollections/ansible_collections/netris/controller/roles/nat/tasks/create.yamlcollections/ansible_collections/netris/controller/roles/server_cluster/tasks/create.yamlcollections/ansible_collections/netris/steps/roles/cluster_infra/tasks/create.yamlcollections/ansible_collections/netris/steps/roles/external_access/tasks/create.yamlcollections/ansible_collections/osac/service/roles/wait_for/defaults/main.yamlcollections/ansible_collections/osac/service/roles/wait_for/tasks/wait_for_cluster_operators.yamlcollections/ansible_collections/osac/service/roles/wait_for/tasks/wait_for_network_cluster_operator.yamlcollections/ansible_collections/osac/service/roles/wait_for/tasks/wait_for_nodes.yamlcollections/ansible_collections/osac/templates/roles/ocp_4_17_small/tasks/install.yamltests/integration/setup_test_env.shtests/integration/teardown_test_env.sh
✅ Files skipped from review due to trivial changes (2)
- .gitignore
- collections/ansible_collections/osac/service/roles/wait_for/defaults/main.yaml
f7417f8 to
7d6e267
Compare
danmanor
changed the title
danmanor
changed the title
7d6e267 to
8c35fe9
Compare
danmanor
changed the title
|
@danmanor: This pull request references MGMT-24178 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. This pull request references MGMT-24196 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
8c35fe9 to
2ee0a80
Compare
tzvatot
left a comment
tzvatot
left a comment
There was a problem hiding this comment.
Review: MGMT-24178, MGMT-24196: Netris CaaS fixes / improvements
Solid idempotency improvement — the core design of "lookup existing resource before create, reuse if found" is the right pattern for making the CaaS create flow re-runnable. The NAT reuse, server cluster PUT-update, and NMState scoping to new agents are all well-motivated. The wait_for_nodes addition fills a real gap in the install sequence.
| Category | Count |
|---|---|
| 🔴 Critical | 1 |
| 🟡 Important | 3 |
| 💡 Suggestion | 2 |
🔴 DNAT IP allocation logic breaks when exactly one of two rules exists
File: netris/steps/roles/external_access/tasks/create.yaml:79-117
The IPAM allocation count is computed as the sum of missing DNAT rules (0, 1, or 2). But the IP assignment logic has four branches based on _missing_api_dnat and _missing_ingress_dnat combinations, and two of them assign ipam_allocated_ips[0] — which is correct when only one IP is allocated. However, the "both missing" branch assigns [0] and [1], and the "only one missing" branches assign [0]. This works.
The actual bug: when _dnat_ipam_count is 0 (both rules exist), the IPAM role is skipped, but ipam_allocated_ips is still referenced in the conditional branches. If Ansible evaluates those set_fact tasks even with when: false, the variable reference could fail on some Ansible versions. More critically: the when conditions on the "Set API DNAT IP from IPAM" tasks use _missing_api_dnat | bool and _missing_ingress_dnat | bool, but these are set as strings ("True" / "False" from Jinja), and "False" | bool is True in Ansible because non-empty strings are truthy.
Recommendation: Use native booleans, not string booleans:
- name: Count missing DNAT IPs
ansible.builtin.set_fact:
_missing_api_dnat: >-
{{ _existing_api_dnat is not defined or _existing_api_dnat is none }}This produces the string "True" or "False", and "False" | bool evaluates to false in Ansible — actually this works because Ansible's bool filter handles the strings "True"/"False" correctly. But it's fragile. Test with ansible -m debug -a "msg={{ 'False' | bool }}" to confirm on your Ansible version.
Better: set them as actual booleans using | bool at assignment time:
_missing_api_dnat: "{{ (_existing_api_dnat is not defined or _existing_api_dnat is none) | bool }}"🟡 netris_nat_snat_ip inconsistent format — existing rule has no /32 suffix
File: netris/steps/roles/cluster_infra/tasks/create.yaml:184-185
When reusing an existing SNAT rule:
netris_nat_snat_ip: "{{ _existing_snat.snatToIP }}"When allocating a new IP:
netris_nat_snat_ip: "{{ ipam_allocated_ips[0] }}/32"The new-allocation path appends /32 but the reuse path takes the raw value from the API, which may or may not include the mask. The downstream consumer (nat_snat_to_ip) receives inconsistent formats. CodeRabbit flagged this too.
Recommendation: Normalize on the reuse path:
netris_nat_snat_ip: "{{ _existing_snat.snatToIP | ansible.utils.ipaddr('host/prefix') }}"Or strip /32 from the new-allocation path if the API doesn't return it.
🟡 Server cluster update body only sends servers and tags — may reset other fields
File: netris/controller/roles/server_cluster/tasks/create.yaml:63-67
_update_body:
servers: "{{ _server_cluster_servers | default([]) }}"
tags: "{{ server_cluster_tags | default([]) }}"The PUT request sends only servers and tags. If the Netris API treats PUT as a full replacement (not a merge), other fields like template, admin, and vpc could be reset to defaults. This would silently break the cluster.
Recommendation: Either:
- Confirm that the Netris API does a merge-style PUT (only updates provided fields), or
- Include all fields from
_existing_server_clusterin the update body, overriding onlyserversandtags:
_update_body: "{{ _existing_server_cluster | combine({'servers': _server_cluster_servers, 'tags': server_cluster_tags}) }}"🟡 no_log: true missing on server cluster update task
File: netris/controller/roles/server_cluster/tasks/create.yaml:74-84
The update task sends a request with connect.sid cookie but doesn't have no_log: true, while the create task (line 121) and all NAT tasks do. Auth cookies will appear in Ansible logs and CI output.
Recommendation: Add no_log: true to the update task.
💡 NMState split into "create CRs for all" + "apply live on new only" — intent is clear but naming could be clearer
File: netris/steps/roles/cluster_infra/tasks/create.yaml:232-249
The first call creates NMStateConfig CRs with nmstate_config_apply_live: false, the second applies live changes only to _newly_added_agents. The role must support this apply_live flag — worth verifying it exists and is respected.
💡 .claude/ added to .gitignore — may conflict with PR #245
File: .gitignore:179
PR #245 adds CLAUDE.md and .claude/rules/ to the repo. This PR adds .claude/ to .gitignore, which would make those files invisible to git. If #245 merges first, this line would cause the rules files to be ignored. If this PR merges first, #245 would need to add exceptions.
Recommendation: Coordinate with PR #245. If .claude/rules/ should be tracked, use:
.claude/*
!.claude/rules/
1f98059 to
6d5f2b3
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
collections/ansible_collections/netris/steps/roles/external_access/tasks/create.yaml (1)
65-77:⚠️ Potential issue | 🔴 CriticalReuse the ingress VIP when either ingress DNAT already exists.
netris_dnat_ingress_ipand_missing_ingress_dnatstill only consult_existing_ingress_http_dnat. If HTTPS already exists but HTTP does not, this rerun allocates a new VIP, creates only the HTTP rule on that new address, and then moves wildcard DNS to an IP where 443 is still missing. Treat ingress as present when either HTTP or HTTPS exists, and derive the shared VIP from whichever rule is already there.Suggested fix
- name: Set ingress DNAT IP from existing rule ansible.builtin.set_fact: - netris_dnat_ingress_ip: "{{ _existing_ingress_http_dnat.destinationAddress | ansible.utils.ipaddr('address') }}" - when: _existing_ingress_http_dnat is defined and _existing_ingress_http_dnat is not none + netris_dnat_ingress_ip: >- + {{ + ( + _existing_ingress_http_dnat.destinationAddress + if (_existing_ingress_http_dnat is defined and _existing_ingress_http_dnat is not none) + else _existing_ingress_https_dnat.destinationAddress + ) | ansible.utils.ipaddr('address') + }} + when: + - (_existing_ingress_http_dnat is defined and _existing_ingress_http_dnat is not none) or + (_existing_ingress_https_dnat is defined and _existing_ingress_https_dnat is not none) - name: Count missing DNAT IPs ansible.builtin.set_fact: _missing_api_dnat: "{{ (_existing_api_dnat is not defined or _existing_api_dnat is none) | bool }}" - _missing_ingress_dnat: "{{ (_existing_ingress_http_dnat is not defined or _existing_ingress_http_dnat is none) | bool }}" + _missing_ingress_dnat: >- + {{ + ( + (_existing_ingress_http_dnat is not defined or _existing_ingress_http_dnat is none) and + (_existing_ingress_https_dnat is not defined or _existing_ingress_https_dnat is none) + ) | bool + }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@collections/ansible_collections/netris/steps/roles/external_access/tasks/create.yaml` around lines 65 - 77, The ingress VIP logic only checks _existing_ingress_http_dnat; update the set_fact and missing-flag logic to treat ingress as present if either _existing_ingress_http_dnat or _existing_ingress_https_dnat exists and to derive netris_dnat_ingress_ip from whichever rule is defined (HTTP first, otherwise HTTPS). Specifically, change the netris_dnat_ingress_ip assignment to pick the destinationAddress from _existing_ingress_http_dnat or fallback to _existing_ingress_https_dnat, and change the _missing_ingress_dnat boolean to check both _existing_ingress_http_dnat and _existing_ingress_https_dnat; ensure _dnat_ipam_count uses the updated _missing_ingress_dnat.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@collections/ansible_collections/netris/controller/roles/nat/tasks/create.yaml`:
- Around line 45-55: The NAT GET and POST ansible.builtin.uri tasks are missing
a timeout and can hang; update both the "Get existing NAT rules" GET task
(registered as _nat_list_resp) and the corresponding POST task to include
timeout: "{{ netris_timeout | default(30) }}", adding the timeout parameter to
the ansible.builtin.uri call in each task while keeping existing headers,
return_content, status_code and no_log behavior unchanged.
In
`@collections/ansible_collections/netris/controller/roles/server_cluster/tasks/create.yaml`:
- Around line 7-17: The three ansible.builtin.uri tasks ("Get existing server
clusters", the PUT task that updates a server cluster, and the POST task that
creates a server cluster) lack a per-request timeout; add timeout: "{{
netris_timeout | default(30) }}" to each of these uri tasks so every call (GET,
PUT, POST) uses the same configurable 30s default and avoids hanging on a slow
controller. Locate the tasks by their task names in create.yaml and insert the
timeout line under the uri parameters for each relevant task.
---
Duplicate comments:
In
`@collections/ansible_collections/netris/steps/roles/external_access/tasks/create.yaml`:
- Around line 65-77: The ingress VIP logic only checks
_existing_ingress_http_dnat; update the set_fact and missing-flag logic to treat
ingress as present if either _existing_ingress_http_dnat or
_existing_ingress_https_dnat exists and to derive netris_dnat_ingress_ip from
whichever rule is defined (HTTP first, otherwise HTTPS). Specifically, change
the netris_dnat_ingress_ip assignment to pick the destinationAddress from
_existing_ingress_http_dnat or fallback to _existing_ingress_https_dnat, and
change the _missing_ingress_dnat boolean to check both
_existing_ingress_http_dnat and _existing_ingress_https_dnat; ensure
_dnat_ipam_count uses the updated _missing_ingress_dnat.
🪄 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
Run ID: 8591dd74-f17c-4fdc-9988-bab810de9009
📒 Files selected for processing (13)
.gitignorecollections/ansible_collections/netris/controller/roles/nat/tasks/create.yamlcollections/ansible_collections/netris/controller/roles/server_cluster/tasks/create.yamlcollections/ansible_collections/netris/steps/roles/cluster_infra/tasks/create.yamlcollections/ansible_collections/netris/steps/roles/external_access/tasks/create.yamlcollections/ansible_collections/osac/service/roles/wait_for/defaults/main.yamlcollections/ansible_collections/osac/service/roles/wait_for/tasks/wait_for_cluster_operators.yamlcollections/ansible_collections/osac/service/roles/wait_for/tasks/wait_for_network_cluster_operator.yamlcollections/ansible_collections/osac/service/roles/wait_for/tasks/wait_for_nodes.yamlcollections/ansible_collections/osac/templates/roles/ocp_4_17_small/tasks/install.yamltests/integration/setup_test_env.shtests/integration/targets/cluster_create/tasks/baseline.ymltests/integration/teardown_test_env.sh
✅ Files skipped from review due to trivial changes (3)
- tests/integration/teardown_test_env.sh
- .gitignore
- collections/ansible_collections/osac/service/roles/wait_for/tasks/wait_for_nodes.yaml
🚧 Files skipped from review as they are similar to previous changes (4)
- collections/ansible_collections/osac/service/roles/wait_for/tasks/wait_for_network_cluster_operator.yaml
- collections/ansible_collections/osac/service/roles/wait_for/tasks/wait_for_cluster_operators.yaml
- collections/ansible_collections/osac/service/roles/wait_for/defaults/main.yaml
- tests/integration/setup_test_env.sh
6d5f2b3 to
8f08055
Compare
8f08055 to
bb0738d
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danmanor, tzvatot The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
3 participants
Summary
Summary by CodeRabbit
New Features
Bug Fixes
Chores