Skip to content

misc masquerade fixes - #1640

Merged
Fredi-raspall merged 2 commits into
mainfrom
pr/fredi/misc_masquerade_fixes
Jul 18, 2026
Merged

misc masquerade fixes#1640
Fredi-raspall merged 2 commits into
mainfrom
pr/fredi/misc_masquerade_fixes

Conversation

@Fredi-raspall

Copy link
Copy Markdown
Contributor
  • Contains logging improvements and clean-up in masquerading code
  • Improves logic to admit / deny existing masquerading flows on reconfigs.

- remove unused error variant
- polish logs to provide extra information
- rename variables

Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
@Fredi-raspall
Fredi-raspall requested a review from qmonnet July 17, 2026 12:23
@Fredi-raspall
Fredi-raspall requested a review from a team as a code owner July 17, 2026 12:23
Copilot AI review requested due to automatic review settings July 17, 2026 12:23
@coderabbitai

coderabbitai Bot commented Jul 17, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 28aba09a-9395-4b38-84f1-2cc23f0fcc80

📥 Commits

Reviewing files that changed from the base of the PR and between ea41ce1 and 123b742.

📒 Files selected for processing (1)
  • nat/src/masquerade/flows.rs
🚧 Files skipped from review as they are similar to previous changes (1)
  • nat/src/masquerade/flows.rs

📝 Walkthrough

Walkthrough

Changes

Masquerade allocation now uses destination VPC discriminants consistently, reports missing pools distinctly, adjusts reservation logging, and validates both NAT coverage and source-IP inclusion when checking existing masquerading flows.

Masquerade allocation and flow validation

Layer / File(s) Summary
Allocation errors and pool-key identity
nat/src/masquerade/allocation.rs, nat/src/masquerade/apalloc/{mod.rs,display.rs,test_alloc.rs}
Adds NoPoolFound, updates error classification, and uses dst_vpcd consistently for pool keys, display, lookup, and tests.
Pool reservation handling
nat/src/masquerade/apalloc/{mod.rs,port_alloc.rs}
IPv4 and IPv6 reservations return NoPoolFound when pools are absent, with consolidated reservation and updated denial logging.
Masquerading flow compatibility
nat/src/masquerade/flows.rs
Re-reservation logs include additional context, and expose checks require both masquerade-range coverage and source-IP inclusion.

Possibly related PRs

Suggested reviewers: qmonnet

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Title check ❓ Inconclusive The title is related but too generic to identify the main change. Rename it to describe the primary change, such as masquerading flow reconfiguration fixes.
✅ Passed checks (4 passed)
Check name Status Explanation
Description check ✅ Passed The description matches the PR's logging and masquerading-flow reconfiguration changes.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

Comment @coderabbitai help to get the list of available commands.

@Fredi-raspall Fredi-raspall added ci:+release Enable VLAB release tests ci:+vlab Enable VLAB tests labels Jul 17, 2026
@Fredi-raspall Fredi-raspall reopened this Jul 17, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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 `@nat/src/masquerade/flows.rs`:
- Around line 121-130: The masquerade flow-retention check in the peering
`valexp()` loop must honor negative ranges before retaining a flow. Update the
`nat.as_range` and `expose.ips()` matching logic to use the effective expose
matcher or otherwise reject IPs covered by `nat.not_as` and source IPs covered
by `expose.nots`, while preserving the existing positive-range requirements.
🪄 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: ab46ef3f-33cc-48f4-8fda-d389a5498a04

📥 Commits

Reviewing files that changed from the base of the PR and between fcb9dd8 and a5511d6.

📒 Files selected for processing (6)
  • nat/src/masquerade/allocation.rs
  • nat/src/masquerade/apalloc/display.rs
  • nat/src/masquerade/apalloc/mod.rs
  • nat/src/masquerade/apalloc/port_alloc.rs
  • nat/src/masquerade/apalloc/test_alloc.rs
  • nat/src/masquerade/flows.rs

Comment thread nat/src/masquerade/flows.rs

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refines the NAT masquerade subsystem’s reconfiguration behavior and observability, focusing on clearer logging and more accurate validation of whether existing masqueraded flows remain admissible after config changes.

Changes:

  • Adjust flow revalidation logic to require both the existing masquerade IP and the flow source IP to still be permitted by a matching expose during reconfig.
  • Improve/standardize allocator and flow logging, and add a dedicated NoPoolFound allocator error for re-reserve paths.
  • Rename allocator pool table key field dst_iddst_vpcd for clarity/consistency (including tests and display output).

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
nat/src/masquerade/flows.rs Tightens reconfig validation for existing masquerade flows and improves related logging.
nat/src/masquerade/apalloc/test_alloc.rs Updates tests to match dst_vpcd field rename.
nat/src/masquerade/apalloc/port_alloc.rs Minor logging tweak for explicit well-known port reservation denial.
nat/src/masquerade/apalloc/mod.rs Renames pool key field; improves re-reserve behavior with NoPoolFound and warn-level logging.
nat/src/masquerade/apalloc/display.rs Updates display output to match dst_vpcd rename.
nat/src/masquerade/allocation.rs Adjusts error messages and introduces NoPoolFound mapped to DoneReason::Filtered.

Comment thread nat/src/masquerade/flows.rs
Comment thread nat/src/masquerade/flows.rs Outdated
@Fredi-raspall
Fredi-raspall force-pushed the pr/fredi/misc_masquerade_fixes branch from a5511d6 to ea41ce1 Compare July 17, 2026 12:31
Comment thread nat/src/masquerade/flows.rs Outdated
Comment thread nat/src/masquerade/flows.rs Outdated
}
}

if compatible_expose_found {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be cleaner with early exit here instead:

        if !alloced_ip_valid {
            debug!("Flow is no longer valid for masquerading between {src_vpcd} -- {dst_vpcd}");
            flow_info.invalidate_pair();
            return;
        }
        if !compatible_expose_found {
            debug!("Masquerade ip {ip} is no longer allowed over peering {src_vpcd} -- {dst_vpcd}");
            flow_info.invalidate_pair();
            return;
        }

Then less indent for the last block - the code would be less nested, the conditions easier to follow.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

Rewrite the logic to admit a masqueraded flow after a config change.
The prior logic checked if the masquerading address was still a
valid one under the new configuration but did not check if the
source of the flow was still allowed in the new configuration; a
check that was indirectly made by the allocator.

Make the restriction explicit, instead of relying on the allocator
to do so.

Signed-off-by: Fredi Raspall <fredi@githedgehog.com>
@Fredi-raspall
Fredi-raspall force-pushed the pr/fredi/misc_masquerade_fixes branch from ea41ce1 to 123b742 Compare July 17, 2026 18:27
@Fredi-raspall
Fredi-raspall added this pull request to the merge queue Jul 17, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jul 17, 2026
@Fredi-raspall
Fredi-raspall added this pull request to the merge queue Jul 18, 2026
Merged via the queue into main with commit 1842c63 Jul 18, 2026
27 checks passed
@Fredi-raspall
Fredi-raspall deleted the pr/fredi/misc_masquerade_fixes branch July 18, 2026 11:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci:+release Enable VLAB release tests ci:+vlab Enable VLAB tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants