fix(masquerade): fix flow genid for masqueraded flows

[Fredi-raspall]

We were setting the flow genid from the genid of the allocator. That idea was not bad, but had an issue: a config change that would not change the masquerading peerings would not create a new NAT allocator. So, its genid will fall behind the genid of the config. Fix this by keeping the genid in the Masquerade NF instead and update it even if the allocator does not change so that it always represents the main config generation id.

Fixes: https://github.com/githedgehog/internal/issues/463

[coderabbitai]

📝 Walkthrough

Walkthrough

Masquerade generation IDs now belong to NatAllocator state instead of MasqueradeConfig or individual allocations. Allocator updates, flow creation, validation, management wiring, and tests use the revised APIs.

Changes

Masquerade generation ID refactor

Layer / File(s)	Summary
Allocator configuration and generation ID contract nat/src/masquerade/allocator_writer.rs, nat/src/masquerade/apalloc/mod.rs	MasqueradeConfig no longer stores GenId. update_nat_allocator receives it separately. NatAllocator stores and exposes the value through atomic state.
Allocation and flow generation handling nat/src/masquerade/apalloc/*, nat/src/masquerade/flows.rs, nat/src/masquerade/nf.rs	Allocation values and AllocatedPort no longer carry generation IDs. Flow validation and creation read the ID from NatAllocator. recheck_flow handles allocator removal, replacement, and generation changes.
Caller and test migration mgmt/src/processor/proc.rs, acl-filter/src/tests.rs, nat/src/masquerade/test.rs, nat/src/masquerade/test_alloc.rs, nat/src/test.rs	Management code and test setup use the revised constructor and allocator update signatures. Tests verify generation ID updates and flow rechecks during reconfiguration.

Possibly related PRs

• githedgehog/dataplane#1598: Modifies masquerade allocator configuration and allocator-writer APIs.
• githedgehog/dataplane#1638: Modifies NAT, ACL flow-filter tests, and management paths that consume masquerade APIs.
• githedgehog/dataplane#1696: Modifies masquerade allocator generation and state handling.

Suggested reviewers: daniel-noland, qmonnet, mvachhar

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly identifies the main change: correcting generation IDs for masqueraded flows.
Description check	✅ Passed	The description explains the generation ID issue, the fix, and the affected allocator behavior.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
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.}

[Copilot]

Pull request overview

This PR fixes masqueraded-flow generation ID (genid) handling so flow genids track the main config generation, even when NAT peerings don’t change and the NAT allocator instance is reused.

Changes:

• Removes genid from MasqueradeConfig and passes genid separately through allocator update paths.
• Stores/serves genid on NatAllocator (with set_genid) and updates it even when the allocator is kept.
• Updates flow genid comparisons to use NatAllocator::genid() rather than MasqueradeConfig::genid().

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
nat/src/test.rs	Updates test pipeline setup to the new MasqueradeConfig::new and update_nat_allocator(..., genid, ...) APIs.
nat/src/masquerade/test.rs	Adjusts masquerade tests for the new genid plumbing and allocator update signature.
nat/src/masquerade/nf.rs	Uses allocator-served genid when creating flow pairs (instead of deriving it from allocations).
nat/src/masquerade/flows.rs	Switches flow-vs-config generation checks to compare against allocator.genid().
nat/src/masquerade/apalloc/test_alloc.rs	Updates allocator construction in tests to pass genid explicitly.
nat/src/masquerade/apalloc/port_alloc.rs	Removes per-allocation genid tracking from allocated port objects (now allocator-owned).
nat/src/masquerade/apalloc/mod.rs	Adds allocator-level genid storage and accessors; removes allocation-level genid setters/getters.
nat/src/masquerade/allocator_writer.rs	Changes config equality semantics (genid removed) and updates allocator update API to accept genid separately and advance it when reusing allocator.
mgmt/src/processor/proc.rs	Updates management-plane config application to pass genid into allocator updates separately from peerings config.
acl-filter/src/tests.rs	Updates integration tests to the new allocator update API.

[Fredi-raspall]

That's fair and is precisely what the previous code granted. Will fix.

[qmonnet]

Looks good. Do we have a non-regression test (if not, could you maybe create one please?) to ensure that the genid gets incremented even when no allocators change on a config bump?

[coderabbitai]

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

[coderabbitai]

🧹 Nitpick comments (1)

nat/src/masquerade/nf.rs (1)

429-464: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win

Add replacement-allocator regression coverage. check_masquerading_flow calls set_genid_pair(current.genid()) after successful re-reservation. Test a replacement allocator that accepts the existing allocation and assert that both flows use its generation ID.

🤖 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 `@nat/src/masquerade/nf.rs` around lines 429 - 464, Add regression coverage for
recheck_flow with a replacement allocator that accepts the existing allocation.
Verify check_masquerading_flow successfully re-reserves the flow and that both
paired flows have the replacement allocator’s generation ID via genid().

Source: Coding guidelines

🤖 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.

Nitpick comments:
In `@nat/src/masquerade/nf.rs`:
- Around line 429-464: Add regression coverage for recheck_flow with a
replacement allocator that accepts the existing allocation. Verify
check_masquerading_flow successfully re-reserves the flow and that both paired
flows have the replacement allocator’s generation ID via genid().

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 237d9ce9-a154-4f60-a144-c8c85b8f6db7

📥 Commits

Reviewing files that changed from the base of the PR and between 410f47f and f17911b.

📒 Files selected for processing (2)

• nat/src/masquerade/nf.rs
• nat/src/masquerade/test.rs

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.