Skip to content

Conversation

@PlagueCZ
Copy link
Contributor

Currently portmap table entries use ICMP identifier in place of TCP/UDP source port. But it is already used as TCP/UDP destination port.

All other parts of dpservice use the same destination port replacement, but replace source port with ICMP type instead.

To easily implement #697 (dpservice HA synchronization), the ICMP type needs to be part of portmap/portoverload tables.


Unless I am mistaken, changing this has only one impact on current dpservice and that is the way portmap entries will be overloaded.

Currently for each ICMP flow (identifier) a new NAT portmap entry is created, thus a new NAT port is being chosen.

After this change the portmap entry will be always the same for a given VM (and ICMP type, but in practice most ICMP calls will be to Echo). This in turn will reuse already present NAT port.

This in turn can bring more flow reusal (in theory one infinitely reused flow if some VM has a PING every 25s for example), which I am not sure how good/bad it is for real-world application.

@PlagueCZ PlagueCZ requested a review from a team as a code owner July 31, 2025 16:18
@github-actions github-actions bot added size/XS enhancement New feature or request labels Jul 31, 2025
@PlagueCZ PlagueCZ marked this pull request as draft July 31, 2025 16:19
@byteocean
Copy link
Contributor

To summarise the discussion results, there is a good reason to use icmp_identifier as part of the portmap key. It allows differentiation of two concurrent icmp sessions (e.g., two running ping processes) on the server side by using two different NAT ports as new icmp identifiers.

@guvenc guvenc merged commit 261c105 into main Aug 6, 2025
7 checks passed
@guvenc guvenc deleted the feature/snat_icmp_type branch August 6, 2025 10:03
@github-project-automation github-project-automation bot moved this to Done in Roadmap Aug 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

5 participants