feat: Checkpoint & Forti NAT support#4547
Conversation
This reverts commit af68600.
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Laennart
changed the title
Co-authored-by: Lennart Schmidt <Laennart@users.noreply.github.com>
|
There was a problem hiding this comment.
Review (by Claude Opus): PR #4547 — feat: Checkpoint & Forti NAT support
Findings mapped to already-mentioned comments
🔴 1. rule_type is never set on NAT rules — and this defeats the PR's own consistency exemption
Maps to: Copilot comments on cp_nat.py:245 (still open). New insight: ties it to the check_consistency.py change.
parse_nat_rule_transform() sets "type": "nat", but parse_single_rule derives the stored type from native_rule.get("rule_type", "access") — a different key. So NAT rules are persisted with rule_type="access".
The PR added this exemption to _check_rule_consistency:
if rule.rule_uid in seen_rule_uids and rule.rule_type != "nat":
duplicate_rule_uids.append(rule.rule_uid)
Because rule_type is "access" (not "nat"), this exemption never fires for the very rules it was written for. Copilot raised the key-mismatch twice; the consequence is now concrete and unresolved. Fix: set "rule_type": "nat" in both transform dicts (or have parse_single_rule fall back to type).
🟠 2. Cross-gateway NAT duplication — the single-management rebuttal doesn't fully cover it
Maps to: Copilot on cp_nat.py:20; Laennart replied "Checkpoint is single-management, gateways list always has one element."
The reply addresses managements, but a single Checkpoint management can drive multiple gateways/devices, and handle_nat_rules() appends one NAT rulebase to native_config_domain["nat_rulebases"] per device. normalize_nat_rules then loops every gateway × all NAT rulebases, so gateway A's NAT parent receives gateway B's rules. insert_rulebase_link dedupes the links, and Rulebase.rules is UID-keyed (so identical UIDs overwrite rather than multiply within one rulebase) — but rules still land on the wrong gateway, and the same UID across gateways feeds finding #1's duplicate path. Recommend tagging each NAT rulebase with its policy/package and matching it to the owning gateway, rather than relying on the one-gateway assumption.
🟡 3. time: "" for NAT rules
Maps to: Copilot on cp_nat.py:241 — still open.
nat_out_rule uses "time": "" and nat_in_rule defaults to "". parse_single_rule calls parse_rule_part(native_rule["time"], "time"); a string is iterable, so this risks warnings/malformed output. Use []. Minor but trivial to fix.
🟡 4. Policy (package) UID used as ordered-layer/rulebase UID
Maps to: Copilot on fwcommon.py get_ordered_layer_uids/define_initial_rulebase_links; Laennart: "intentional — present Policy as Rulebase in the report." Accepting as intentional. Note the related Copilot duplicate-append concern is now fixed via list(dict.fromkeys(...)).
🟡 5. Missing unit tests for the NAT pipeline
Maps to: Copilot on cp_nat.py:20; ErikPre: "fw_module specific tests are in todo." Still open. SonarQube reports 0.0% coverage on new code. There are +15 lines in test_checkpoint_file_import.py and +151 in test_fortiadom5ff.py, so the Forti side has some coverage; the Checkpoint cp_nat link/typing/xlate logic does not. Given findings #1–#3, a focused test would have caught the rule_type mismatch.
🟡 6. 9.0.xx.sql placeholder filename
Maps to: Copilot on 9.0.xx.sql:1; Laennart: "placeholder until final version name is set." Acknowledged — must be renamed to a concrete numeric version before merge or the upgrade automation's ([\d.]+).sql regex skips/breaks it. Merge-blocker, not a logic bug.
New observations (not in existing comments)
DynGraphqlQuery.cs is not in the changed-files list, yet Copilot left a finding on it about a missing separator before stm_link_type producing invalid GraphQL. That change appears to have been dropped/reverted — worth confirming the RulebaseLinkWhereStatement concatenation in the current head is correct, since the nat link type still needs to round-trip through reports.
A stray agents file (+1 line) was added at repo root — looks accidental (config/symlink). Verify and remove if unintended.
Verdict
Solid feature and the team has already worked through most of Copilot's structural feedback. Blocking before merge: finding #1 (the rule_type mismatch, which silently breaks the NAT duplicate-UID exemption), and the SQL rename (#6). Findings #2/#3/#5 should be addressed or consciously deferred with a tracking note; #4 is accepted as intentional.
closes #4009