Skip to content

iptables: Remove '--nowildcard' from socket match#12248

Merged
joestringer merged 3 commits intomasterfrom
pr/jrajahalme/fix-iptables-socket-match
Jun 29, 2020
Merged

iptables: Remove '--nowildcard' from socket match#12248
joestringer merged 3 commits intomasterfrom
pr/jrajahalme/fix-iptables-socket-match

Conversation

@jrajahalme
Copy link
Copy Markdown
Member

@jrajahalme jrajahalme commented Jun 23, 2020

'--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

@jrajahalme jrajahalme added kind/bug This is a bug in the Cilium logic. release-note/bug This PR fixes an issue in a previous release of Cilium. labels Jun 23, 2020
@jrajahalme jrajahalme requested a review from a team June 23, 2020 23:42
@jrajahalme jrajahalme marked this pull request as draft June 23, 2020 23:42
@jrajahalme
Copy link
Copy Markdown
Member Author

test-me-please

@coveralls
Copy link
Copy Markdown

coveralls commented Jun 24, 2020

Coverage Status

Coverage increased (+0.004%) to 36.939% when pulling bd620cb on pr/jrajahalme/fix-iptables-socket-match into 48f8e79 on master.

@jrajahalme
Copy link
Copy Markdown
Member Author

retest-net-next

1 similar comment
@jrajahalme
Copy link
Copy Markdown
Member Author

retest-net-next

@jrajahalme
Copy link
Copy Markdown
Member Author

test-only --k8s_version=1.18 --focus="K8s.*AutoDirectNodeRoutes" --kernel_version=net-next

@jrajahalme jrajahalme force-pushed the pr/jrajahalme/fix-iptables-socket-match branch from bb07b3e to cd503fe Compare June 24, 2020 23:07
@jrajahalme jrajahalme marked this pull request as ready for review June 24, 2020 23:07
@jrajahalme jrajahalme requested a review from a team as a code owner June 24, 2020 23:07
Copy link
Copy Markdown
Member

@nebril nebril left a 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).

Comment thread test/helpers/kubectl.go Outdated
Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will remove.

Comment thread test/helpers/kubectl.go Outdated
Copy link
Copy Markdown
Member

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.

Copy link
Copy Markdown
Member Author

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

Comment thread test/helpers/kubectl.go Outdated
@jrajahalme jrajahalme force-pushed the pr/jrajahalme/fix-iptables-socket-match branch from cd503fe to 65a5cb0 Compare June 25, 2020 17:04
@jrajahalme
Copy link
Copy Markdown
Member Author

test-me-please

@jrajahalme jrajahalme requested review from nebril and qmonnet June 25, 2020 17:04
Copy link
Copy Markdown
Member

@qmonnet qmonnet left a 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!

@joestringer joestringer self-requested a review June 25, 2020 19:33
@jrajahalme jrajahalme force-pushed the pr/jrajahalme/fix-iptables-socket-match branch from 65a5cb0 to 12b5f2a Compare June 25, 2020 20:04
@jrajahalme
Copy link
Copy Markdown
Member Author

test-me-please

@jrajahalme
Copy link
Copy Markdown
Member Author

test-gke

@jrajahalme
Copy link
Copy Markdown
Member Author

Fixed a white-space formatting error.

jrajahalme added a commit that referenced this pull request Jul 8, 2020
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>
jrajahalme added a commit that referenced this pull request Jul 10, 2020
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>
brb pushed a commit that referenced this pull request Jul 15, 2020
[ 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>
christarazi pushed a commit that referenced this pull request Jul 20, 2020
[ 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/bug This is a bug in the Cilium logic. release-note/bug This PR fixes an issue in a previous release of Cilium.

Projects

None yet

7 participants