New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
iptables: Remove '--nowildcard' from socket match #12248
Conversation
test-me-please |
retest-net-next |
1 similar comment
retest-net-next |
test-only --k8s_version=1.18 --focus="K8s.*AutoDirectNodeRoutes" --kernel_version=net-next |
bb07b3e
to
cd503fe
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM overall, one small change request (ExpectWithOffset comment).
test/helpers/kubectl.go
Outdated
res := kub.CiliumExecContext(context.TODO(), ciliumPod, pickDNSProxyPort) | ||
if !res.WasSuccessful() { | ||
ginkgoext.Failf("Cannot find DNS proxy port on %s", ciliumPod) | ||
return 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: this line will never get executed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will remove.
test/helpers/kubectl.go
Outdated
return 0 | ||
} | ||
portStr := res.GetStdOut().String() | ||
gomega.Expect(portStr).ShouldNot(gomega.BeEmpty(), "No DNS proxy port found on %s", ciliumPod) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use ExpectWithOffset
, this will make test failure point to parent call instead of this line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, so the offset value of 1 will point the failure to caller, 2 would point the failure to the caller's caller etc.?
cd503fe
to
65a5cb0
Compare
test-me-please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
Since we address NodePort in k8s2 using the DNS proxy port of k8s2 as the source port from k8s1, one round is enough regardless of the backend selection, as in both cases the replies are reverse NATted at k8s2 (where the port conflict was happening before it was fixed by #12248). Fixes: #12336 Signed-off-by: Jarno Rajahalme <jarno@covalent.io>
Since we address NodePort in k8s2 using the DNS proxy port of k8s2 as the source port from k8s1, one round is enough regardless of the backend selection, as in both cases the replies are reverse NATted at k8s2 (where the port conflict was happening before it was fixed by #12248). Fixes: #12336 Signed-off-by: Jarno Rajahalme <jarno@covalent.io>
[ upstream commit 657295c ] Since we address NodePort in k8s2 using the DNS proxy port of k8s2 as the source port from k8s1, one round is enough regardless of the backend selection, as in both cases the replies are reverse NATted at k8s2 (where the port conflict was happening before it was fixed by #12248). Fixes: #12336 Signed-off-by: Jarno Rajahalme <jarno@covalent.io> Signed-off-by: Martynas Pumputis <m@lambda.lt>
[ upstream commit 657295c ] Since we address NodePort in k8s2 using the DNS proxy port of k8s2 as the source port from k8s1, one round is enough regardless of the backend selection, as in both cases the replies are reverse NATted at k8s2 (where the port conflict was happening before it was fixed by #12248). Fixes: #12336 Signed-off-by: Jarno Rajahalme <jarno@covalent.io> Signed-off-by: Martynas Pumputis <m@lambda.lt>
'--no-wildcard' allows the socket match to find zero-bound (listening)
sockets, which we do not want, as this may intercept traffic intended
for other nodes, for example, reply traffic when an ephemeral source
port number allocated in one node happens to be the same as the
allocated proxy port number in the node doing the iptables socket
match changed here.
Note to backporters: The test suite changes need not be backported
to older releases (e.g., 1.6) if these are non-trivial merge conflicts.
Fixes: #12281
Fixes: #12127
Fixes: #8945
Fixes: #10669
Fixes: #11867
Fixes: #10118
Fixes: #11313
Fixes: #12241
Fixes: #10231
Related: #8864
Signed-off-by: Jarno Rajahalme jarno@covalent.io