Fix client IP preservation for NodePort service with protocol SCTP #104756

tnqn · 2021-09-03T15:11:51Z

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?

Fixed client IP preservation for NodePort service with protocol SCTP in ipvs mode

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot · 2021-09-03T15:11:58Z

@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 triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

aojea · 2021-09-03T21:44:32Z

pkg/proxy/ipvs/proxier.go

 	{kubeNodePortLocalSetSCTP, string(KubeNodePortChain), "RETURN", "dst,dst", utilipset.ProtocolSCTP},
+	{kubeNodePortSetSCTP, string(KubeNodePortChain), string(KubeMarkMasqChain), "dst,dst", utilipset.ProtocolSCTP},


I'm not familiar with the ipset implementation, why sctp matchType is dst,dst but udp and tcp ar eonly dst?

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.

aojea · 2021-09-03T21:46:38Z

pkg/proxy/ipvs/proxier_test.go

-				string(kubeServicesChain): {{
-					JumpChain: string(KubeNodePortChain), MatchSet: "",
-				}},
+				string(KubeNodePortChain): {


why is this test affected if we only have changed the SCTP rules order?

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.

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

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.

aojea · 2021-09-03T21:48:45Z

/assign @andrewsykim @uablrek

tnqn

@aojea thanks for the review. Answered your questions.

tnqn · 2021-09-06T09:13:44Z

pkg/proxy/ipvs/proxier.go

 	{kubeNodePortLocalSetSCTP, string(KubeNodePortChain), "RETURN", "dst,dst", utilipset.ProtocolSCTP},
+	{kubeNodePortSetSCTP, string(KubeNodePortChain), string(KubeMarkMasqChain), "dst,dst", utilipset.ProtocolSCTP},


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.

tnqn · 2021-09-06T09:24:03Z

pkg/proxy/ipvs/proxier_test.go

-				string(kubeServicesChain): {{
-					JumpChain: string(KubeNodePortChain), MatchSet: "",
-				}},
+				string(KubeNodePortChain): {


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.

uablrek · 2021-09-06T12:06:30Z

/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 type: LoadBalancer and it works with and without this PR.

aojea · 2021-09-06T12:28:53Z

/test pull-kubernetes-e2e-gci-gce-ipvs

tnqn · 2021-09-09T16:22:11Z

@aojea @uablrek @andrewsykim if there is no new comment, could you please approve it?

aojea · 2021-09-09T16:44:57Z

/approve

k8s-ci-robot · 2021-09-09T16:45:34Z

[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:

~~pkg/proxy/ipvs/OWNERS~~ [aojea]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-triage-robot · 2021-09-09T20:20:36Z

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:

The PR does have any do-not-merge/* labels
The PR does not have the needs-ok-to-test label
The PR is mergeable (does not have a needs-rebase label)
The PR is approved (has cncf-cla: yes, lgtm, approved labels)
The PR is failing tests required for merge

You can:

Review the full test history for this PR
Prevent this bot from retesting with /lgtm cancel or /hold
Help make our tests less flaky by following our Flaky Tests Guide

/retest

JornShen · 2022-05-17T12:38:26Z

@tnqn shall we cherry-pick this to 1.22 or even earlier branch ? cc @aojea

tnqn · 2022-05-20T15:46:13Z

@tnqn shall we cherry-pick this to 1.22 or even earlier branch ? cc @aojea

@JornShen I could backport it to 1.22, but will there be new patch releases for 1.21? @aojea

aojea · 2022-05-23T08:39:07Z

1.21 EOL is 2022-06-28 based on https://kubernetes.io/releases/

tnqn · 2022-05-24T16:28:03Z

@aojea thanks for the information. I created PRs to backport it to 1.21 and 1.22.

k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 3, 2021

k8s-ci-robot requested review from aojea and freehan September 3, 2021 15:12

k8s-ci-robot added area/ipvs sig/network Categorizes an issue or PR as relevant to SIG Network. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Sep 3, 2021

aojea reviewed Sep 3, 2021

View reviewed changes

k8s-ci-robot assigned andrewsykim and uablrek Sep 3, 2021

tnqn commented Sep 6, 2021

View reviewed changes

tnqn force-pushed the ipvs-sctp-masquerade branch from e468fcb to 9ee3ae7 Compare September 6, 2021 10:58

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 6, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 9, 2021

k8s-ci-robot merged commit a402f17 into kubernetes:master Sep 9, 2021

k8s-ci-robot added this to the v1.23 milestone Sep 9, 2021

tnqn deleted the ipvs-sctp-masquerade branch September 10, 2021 03:53

This was referenced May 24, 2022

Automated cherry pick of #104756: Fix client IP preservation for NodePort service with protocol #110198

Closed

Automated cherry pick of #104756: Fix client IP preservation for NodePort service with protocol #110199

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix client IP preservation for NodePort service with protocol SCTP #104756

Fix client IP preservation for NodePort service with protocol SCTP #104756

tnqn commented Sep 3, 2021

k8s-ci-robot commented Sep 3, 2021

aojea Sep 3, 2021

tnqn Sep 6, 2021

aojea Sep 3, 2021

tnqn Sep 6, 2021

aojea Sep 6, 2021

tnqn Sep 6, 2021

aojea commented Sep 3, 2021

tnqn left a comment

tnqn Sep 6, 2021

tnqn Sep 6, 2021

uablrek commented Sep 6, 2021

aojea commented Sep 6, 2021

tnqn commented Sep 9, 2021

aojea commented Sep 9, 2021

k8s-ci-robot commented Sep 9, 2021

k8s-triage-robot commented Sep 9, 2021

JornShen commented May 17, 2022 •

edited

tnqn commented May 20, 2022

aojea commented May 23, 2022

tnqn commented May 24, 2022

		{kubeNodePortLocalSetSCTP, string(KubeNodePortChain), "RETURN", "dst,dst", utilipset.ProtocolSCTP},
		{kubeNodePortSetSCTP, string(KubeNodePortChain), string(KubeMarkMasqChain), "dst,dst", utilipset.ProtocolSCTP},

Fix client IP preservation for NodePort service with protocol SCTP #104756

Fix client IP preservation for NodePort service with protocol SCTP #104756

Conversation

tnqn commented Sep 3, 2021

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot commented Sep 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aojea commented Sep 3, 2021

tnqn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uablrek commented Sep 6, 2021

aojea commented Sep 6, 2021

tnqn commented Sep 9, 2021

aojea commented Sep 9, 2021

k8s-ci-robot commented Sep 9, 2021

k8s-triage-robot commented Sep 9, 2021

JornShen commented May 17, 2022 • edited

tnqn commented May 20, 2022

aojea commented May 23, 2022

tnqn commented May 24, 2022

JornShen commented May 17, 2022 •

edited