chore(nat): Improve AllocatorError variants and messages#1491
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR aims to reduce confusion when diagnosing stateful NAT allocator failures by refining AllocatorError variants and improving associated error messages.
Changes:
- Clarifies the
AllocatorError::NoFreePorterror message to reflect “no free port in a block” (not “no free block”). - Introduces a new
AllocatorError::PortReservationFailedvariant for port reservation failures. - Updates stateful NAT error-to-
DoneReasontranslation to account for the new variant.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
nat/src/stateful/allocation.rs |
Updates allocator error messages and adds PortReservationFailed. |
nat/src/stateful/mod.rs |
Extends translate_error to handle the new allocator error variant. |
Comments suppressed due to low confidence (1)
nat/src/stateful/mod.rs:517
confidence: 7
tags: [logic]
`AllocatorError::PortReservationFailed(_)` is currently translated to `DoneReason::NatFailure`, but reservation failures are typically a resource/collision condition similar to `NoFreePort(_)` (which maps to `NatOutOfResources`). If `PortReservationFailed` is meant to represent “requested port already in use / not available”, consider translating it to `NatOutOfResources` for consistent drop reason semantics (or add a brief rationale for why this should be `NatFailure`).
| StatefulNatError::AllocationFailure(
AllocatorError::PortAllocationFailed(_)
| AllocatorError::PortReservationFailed(_)
| AllocatorError::UnsupportedIcmpCategory
| AllocatorError::MissingDiscriminant
| AllocatorError::UnsupportedDiscriminant,
) => DoneReason::NatFailure,
</details>
50ef711 to
07cf6ba
Compare
AllocatorError::NoFreePort is not about no port block being available, but not ports being available in a block. The message is confusing: let's fix it. It's been so confusing that we used the error at a location where the error is not related (passing it an incorrect argument): let's add a new variant for that location, to make it easier to troubleshoot errors. Signed-off-by: Quentin Monnet <qmo@qmon.net>
07cf6ba to
fd19678
Compare
In commit 19af92e ("feat(stateful-nat): move NF to nf.rs") we moved the code for the NetworkFunction for masquerading NAT to a dedicated file, but we left the related tests behind; move them along, as it makes no sense to keep them in the near-empty mod.rs. Also remove the tracing imports from mod.rs given that there's no more code in that file. Signed-off-by: Quentin Monnet <qmo@qmon.net>
mvachhar
requested changes
Apr 28, 2026
Fredi-raspall
approved these changes
Apr 29, 2026
mvachhar
approved these changes
Apr 29, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
AllocatorError::NoFreePortis not about no port block being available, but not ports being available in a block. The message is confusing: let's fix it. It's been so confusing that we used the error at a location where the error is not related (passing it an incorrect argument): let's add a new variant for that location, to make it easier to troubleshoot errors.