Improve multi-protocol support for NPL #2903
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2903 +/- ##
==========================================
- Coverage 61.55% 52.42% -9.13%
==========================================
Files 283 283
Lines 23644 23693 +49
==========================================
- Hits 14555 12422 -2133
- Misses 7511 9882 +2371
+ Partials 1578 1389 -189
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| for _, npData := range pt.NodePortTable { | ||
| protocols := make([]string, 0, len(supportedProtocols)) | ||
| for _, protocol := range npData.Protocols { | ||
| protocols = append(protocols, protocol.Protocol) |
There was a problem hiding this comment.
Should it only append the protocol when it's in use? otherwise pt.PodPortRules.AddAllRules would always install an iptables rule for it even it's not used and cause inconsistency between the protocolSocketState and the actual state?
There was a problem hiding this comment.
good catch, let me fix that
antoninbas
force-pushed
the
fix-multi-protocol-implementation-for-NPL
branch
from
d490051 to
f80873c
Compare
|
@tnqn thanks for the attentive review, I addressed your comments in a new commit |
| } | ||
| nplPorts = append(nplPorts, rules.PodNodePort{ | ||
| NodePort: npData.NodePort, | ||
| PodPort: npData.PodPort, | ||
| PodIP: npData.PodIP, | ||
| Protocols: protocols, | ||
| }) | ||
| protocols = protocols[:0] // reuse slice |
There was a problem hiding this comment.
I suspected this will impact the protocols that are already stored in previous PodNodePort as they will share the array and I did a test to verify:
(play.golang.org removed share function so I'm pasting the test code):
package main
import (
"fmt"
)
type Collection struct {
strs []string
}
func main() {
var collections []Collection
strs := make([]string, 0, 2)
for i:=0;i<3;i++ {
strs = append(strs, fmt.Sprintf("%d", i))
collections = append(collections, Collection{strs: strs})
strs = strs[:0]
}
fmt.Println(collections)
}
It got [{[2]} {[2]} {[2]}].
I think this is not expected?
There was a problem hiding this comment.
you are right, this is definitely incorrect.
I fixed it and I also added one unit test, as I feel this code (rule restoration) is a bit under-tested. I confirmed that the test was failing when re-using the array.
There is an edge case, which is not handled well by the current NPL controller implementation: if a given Pod port needs to be exposed for both TCP and UDP, and the first available Node port is only available for TCP, it will be still be selected and NPL rule installation will succeed for TCP but not for UDP. We have 2 options: 1. reserve the port for both protocols ahead of time, even if the port is only needed for one protocol initially. 2. support using different Node ports for different protocols, even when the Pod port is the same. In this patch, we go with option 1) to preserve the "nice" property that a give Pod port maps to a unique Node port. Fixes antrea-io#2894 Signed-off-by: Antonin Bas <[email protected]>
Signed-off-by: Antonin Bas <[email protected]>
antoninbas
force-pushed
the
fix-multi-protocol-implementation-for-NPL
branch
2 times, most recently
from
c0e63ae to
b25bbde
Compare
Signed-off-by: Antonin Bas <[email protected]>
antoninbas
force-pushed
the
fix-multi-protocol-implementation-for-NPL
branch
from
b25bbde to
ce6bd11
Compare
|
/test-all |
There is an edge case, which is not handled well by the current NPL
controller implementation: if a given Pod port needs to be exposed for
both TCP and UDP, and the first available Node port is only available
for TCP, it will be still be selected and NPL rule installation will
succeed for TCP but not for UDP.
We have 2 options:
is only needed for one protocol initially.
the Pod port is the same.
In this patch, we go with option 1) to preserve the "nice" property that
a give Pod port maps to a unique Node port.
Fixes #2894
Signed-off-by: Antonin Bas [email protected]