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
Fix client IP preservation for NodePort service with protocol SCTP #104756
Conversation
@tnqn: This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
{kubeNodePortLocalSetSCTP, string(KubeNodePortChain), "RETURN", "dst,dst", utilipset.ProtocolSCTP}, | ||
{kubeNodePortSetSCTP, string(KubeNodePortChain), string(KubeMarkMasqChain), "dst,dst", utilipset.ProtocolSCTP}, |
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.
I'm not familiar with the ipset implementation, why sctp matchType
is dst,dst
but udp and tcp ar eonly dst
?
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.
It's because bitmap:port
ipset doesn't support matching SCTP port: #74341
https://ipset.netfilter.org/ipset.man.html#lbAV:
The set match and SET target netfilter kernel modules interpret the stored numbers as TCP or UDP port numbers.
pkg/proxy/ipvs/proxier_test.go
Outdated
string(kubeServicesChain): {{ | ||
JumpChain: string(KubeNodePortChain), MatchSet: "", | ||
}}, | ||
string(KubeNodePortChain): { |
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.
why is this test affected if we only have changed the SCTP rules order?
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.
Previously the test only checked whether the target iptables rule exists or not and didn't care about the extra rules got from the system and their order, by using checkIptables
. If we just add the two sctp related rules to expectedIptablesChains
, it's not going to validate one rule is after the other and can be broken in the future without any test failure. So I add a new method checkIptablesInOrder
which validates the expectedIptablesChains
have the same number and order of rules as the ones got from the system. As a result, we need to add missing chains of this test accordingly.
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.
hmm, shouldn't that be the default behavior of checkIptables
?
that method is used in other places and order matters, i.e. we can't have a RETURN as first rule and mark the test as valid XD
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.
I was trying to touch less code. But making checkIptables
check rule order for all tests makes sense to me. I have updated checkIptables
and other tests accordingly.
/assign @andrewsykim @uablrek |
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.
@aojea thanks for the review. Answered your questions.
{kubeNodePortLocalSetSCTP, string(KubeNodePortChain), "RETURN", "dst,dst", utilipset.ProtocolSCTP}, | ||
{kubeNodePortSetSCTP, string(KubeNodePortChain), string(KubeMarkMasqChain), "dst,dst", utilipset.ProtocolSCTP}, |
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.
It's because bitmap:port
ipset doesn't support matching SCTP port: #74341
https://ipset.netfilter.org/ipset.man.html#lbAV:
The set match and SET target netfilter kernel modules interpret the stored numbers as TCP or UDP port numbers.
pkg/proxy/ipvs/proxier_test.go
Outdated
string(kubeServicesChain): {{ | ||
JumpChain: string(KubeNodePortChain), MatchSet: "", | ||
}}, | ||
string(KubeNodePortChain): { |
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.
Previously the test only checked whether the target iptables rule exists or not and didn't care about the extra rules got from the system and their order, by using checkIptables
. If we just add the two sctp related rules to expectedIptablesChains
, it's not going to validate one rule is after the other and can be broken in the future without any test failure. So I add a new method checkIptablesInOrder
which validates the expectedIptablesChains
have the same number and order of rules as the ones got from the system. As a result, we need to add missing chains of this test accordingly.
The iptables rule that matches kubeNodePortLocalSetSCTP must be inserted before the one matches kubeNodePortSetSCTP, otherwise all SCTP traffic would be masqueraded regardless of whether its ExternalTrafficPolicy is Local or not. To cover the case in tests, the patch adds rule order validation to checkIptables.
e468fcb
to
9ee3ae7
Compare
/lgtm I have tested with and without the PR on IPv4 and IPv6 and it works as expected. And for completeness I also tested that the src was preserved for |
/test pull-kubernetes-e2e-gci-gce-ipvs |
@aojea @uablrek @andrewsykim if there is no new comment, could you please approve it? |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aojea, tnqn The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass. This bot retests PRs for certain kubernetes repos according to the following rules:
You can:
/retest |
1.21 EOL is |
@aojea thanks for the information. I created PRs to backport it to 1.21 and 1.22. |
What type of PR is this?
/kind bug
What this PR does / why we need it:
The iptables rule that matches kubeNodePortLocalSetSCTP must be inserted before the one matches kubeNodePortSetSCTP, otherwise all SCTP traffic would be masqueraded regardless of whether its ExternalTrafficPolicy is Local or not.
Besides, it adds unit test to verify the iptables rules are in expected order.
Which issue(s) this PR fixes:
Fixes #104755
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: