Cleanup: remove send_redirects sysctl configuration #1364

tnqn · 2020-10-13T05:34:41Z

With L3 flows, Pod-to-Pod traffic will no longer go through gateway interfaces. For Pod-to-Service traffic, Linux doesn't allow sending redirect packets for NAT'd connections. Therefore, setting send_redirects is no longer needed.

For #1363

antrea-bot · 2020-10-13T05:34:58Z

Thanks for your PR.
Unit tests and code linters are run automatically every time the PR is updated.
E2e, conformance and network policy tests can only be triggered by a member of the vmware-tanzu organization. Regular contributors to the project should join the org.

The following commands are available:

/test-e2e: to trigger e2e tests.
/skip-e2e: to skip e2e tests.
/test-conformance: to trigger conformance tests.
/skip-conformance: to skip conformance tests.
/test-whole-conformance: to trigger all conformance tests on linux.
/skip-whole-conformance: to skip all conformance tests on linux.
/test-networkpolicy: to trigger networkpolicy tests.
/skip-networkpolicy: to skip networkpolicy tests.
/test-windows-conformance: to trigger windows conformance tests.
/skip-windows-conformance: to skip windows conformance tests.
/test-windows-networkpolicy: to trigger windows networkpolicy tests.
/skip-windows-networkpolicy: to skip windows networkpolicy tests.
/test-hw-offload: to trigger ovs hardware offload test.
/skip-hw-offload: to skip ovs hardware offload test.
/test-all: to trigger all tests (except whole conformance).
/skip-all: to skip all tests (except whole conformance).

tnqn · 2020-10-13T05:35:14Z

/test-all

codecov-io · 2020-10-13T05:39:12Z

Codecov Report

Merging #1364 into master will increase coverage by 0.10%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1364      +/-   ##
==========================================
+ Coverage   64.36%   64.46%   +0.10%     
==========================================
  Files         159      159              
  Lines       12674    12654      -20     
==========================================
  Hits         8157     8157              
+ Misses       3665     3648      -17     
+ Partials      852      849       -3

Flag	Coverage Δ
#integration-tests	`45.05% <ø> (+0.19%)`	⬆️
#kind-e2e-tests	`50.24% <ø> (-0.15%)`	⬇️
#unit-tests	`42.12% <ø> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/agent/route/route_linux.go	`59.88% <ø> (+1.59%)`	⬆️
pkg/apis/controlplane/v1beta1/sets.go	`36.76% <0.00%> (-2.95%)`	⬇️
pkg/agent/controller/networkpolicy/priority.go	`87.85% <0.00%> (-2.15%)`	⬇️
pkg/apiserver/storage/ram/store.go	`78.94% <0.00%> (-1.51%)`	⬇️
...ntroller/networkpolicy/networkpolicy_controller.go	`80.56% <0.00%> (-0.27%)`	⬇️
pkg/agent/proxy/proxier.go	`74.80% <0.00%> (ø)`
...agent/controller/traceflow/traceflow_controller.go	`0.00% <0.00%> (ø)`
pkg/agent/controller/networkpolicy/cache.go	`84.98% <0.00%> (+0.39%)`	⬆️
pkg/ovs/openflow/ofctrl_builder.go	`76.17% <0.00%> (+0.43%)`	⬆️
pkg/agent/controller/networkpolicy/reconciler.go	`69.03% <0.00%> (+0.59%)`	⬆️
... and 4 more

tnqn · 2020-10-13T06:57:24Z

/test-windows-conformance

jianjuns

Question - but when AntreaProxy is not enabled, do we still need send_redirect for Service traffic?

antoninbas

Thanks for taking care of this, LGTM

antoninbas · 2020-10-14T00:12:26Z

@tnqn nit: in the commit message / PR description: you wrote "capacities" instead of "capabilities"

tnqn · 2020-10-14T02:26:18Z

Question - but when AntreaProxy is not enabled, do we still need send_redirect for Service traffic?

@jianjuns No, I have verified no redirect packets are generated for such traffic. It should make sense that a DNATed packet may be sent out from the interface which it comes in so linux must have handled it. I could also try to confirm from code.

@tnqn nit: in the commit message / PR description: you wrote "capacities" instead of "capabilities"

Thanks @antoninbas, corrected.

tnqn · 2020-10-14T02:27:30Z

/test-all

tnqn · 2020-10-14T02:28:11Z

/test-whole-conformance

jianjuns

Ok. Let us remove it then.

tnqn · 2020-10-14T04:13:52Z

/test-conformance

srikartati · 2020-10-16T10:00:51Z

pkg/agent/route/route_linux.go

@@ -350,16 +335,6 @@ func writeLine(buf *bytes.Buffer, words ...string) {
 	}
 }

-func disableICMPSendRedirects(intfName string) error {
-	cmdStr := fmt.Sprintf("echo 0 > /proc/sys/net/ipv4/conf/%s/send_redirects", intfName)


@tnqn As part of the flow exporter, we make sure a couple of netfilter/conntrack parameters to be set.
https://github.com/vmware-tanzu/antrea/blob/5f65bba1b771b6fd1f1b548a3458bd3647c74489/pkg/agent/flowexporter/connections/conntrack_linux.go#L151
I think the change of privileges for the antrea-agent container will affect those checks, right?

thanks @srikartati for raising this, I didn't know we have other places to perform sysctl. Is the conntrack configuration must-have to make flow exporter work? If so, I'm thinking two approaches:

Keep privilege in the upstream yaml and use customized manifest for clusters that do not allow privileged pod (but flow exporter and other features that require sysctl configuration won't be available in these clusters).

Try performing sysctl in antrea-agent and if it fails, logging a warning but do not exit the program if an env variable like IGNORE_SYSCTL_ERR is set. This means user doesn't give antrea-agent pod privilege, assuming sysctl will be configured out-of-band.

@antoninbas @jianjuns @srikartati any idea?

Thanks for catching this @srikartati

these parameters are not must-have, but they do provide additional information. The code already does not fail and exit if the kernel parameters cannot be set, so maybe IGNORE_SYSCTL_ERR is not required if we go with Quan's idea. Although the problem with always ignoring it is we do not catch issues when testing, as was the case here.

a possibility is to use an initContainer that's privileged in order to set these kernel parameters; and run the Agent as unprivileged. The initContainer can easily be removed from the manifest when needed.

could we consider setting the sysctls in the Pod spec as that doesn't require privileged mode (https://kubernetes.io/docs/tasks/administer-cluster/sysctl-cluster/#setting-sysctls-for-a-pod)? That relies on container runtime support to set the kernel parameters.

@tnqn Setting up the conntrack parameters is not a must per say for Flow Exporter, but the flow visibility would not be complete especially because of the lack of flow stats. Currently, if there is no write access to sysctl parameters and the values are not desired, then we just log an error and continue. In this case, we just provide flow records without stats and timestamps.

Since having no privileges doesn't break functionality, IMO we can go for option 2 to configure them out-of-band and describe the importance of setting required sysctl parameters in the flow exporter documentation.
I would like to hear others opinion.

The 3rd option doesn't work as setting unsafe-sysctls need extra argument --allowed-unsafe-sysctls set to kubelet and it's not allowed to set net namespace related kernel parameter for a host network Pod: https://github.com/kubernetes/kubernetes/blob/2046f4212a355a3381da20c4d800283b98e9a081/pkg/kubelet/sysctl/whitelist.go#L89-L91.

I think the initContainer approach is feasible, we may need to measure how long would it add to the start time if having another initContainer. Anyway I could reduce the scope of this PR to remove send_redirects configuration only first.

What I like about the initContainer approach is we can group all operations that require privileged mode and we ensure that the antrea-agent container itself can run without privileged mode since the default YAML itself will not include privileged: true. But yes, that's a separate question that can be resolved in a future PR. After merging this PR, one can already edit the manifest and remove privileged: true, which will cause a warning to be logged if FlowExporter is enabled, but will not terminate the agent.

tnqn · 2020-10-19T14:48:41Z

Question - but when AntreaProxy is not enabled, do we still need send_redirect for Service traffic?

@jianjuns @antoninbas I have confirmed from code that redirect packets for a NATed connection is not allowed for Linux, it's dropped by a NAT hook:
https://github.com/torvalds/linux/blob/a811c1fa0a02c062555b54651065899437bacdbe/net/netfilter/nf_nat_proto.c#L585-L586
So we are safe to remove the sysctl configuration.

With L3 flows, Pod-to-Pod traffic will no longer go through gateway interfaces. For Pod-to-Service traffic, Linux doesn't allow sending redirect packets for NAT'd connections. Therefore, configuring send_redirects is no longer needed.

tnqn · 2020-10-19T15:35:43Z

/test-all
/test-whole-conformance

vmwclabot added the cla-not-required label Oct 13, 2020

tnqn requested review from antoninbas and jianjuns October 13, 2020 06:57

jianjuns reviewed Oct 13, 2020

View reviewed changes

antoninbas previously approved these changes Oct 14, 2020

View reviewed changes

tnqn dismissed antoninbas’s stale review via 9057a24 October 14, 2020 02:05

tnqn force-pushed the sysctl branch from 9d9e63d to 9057a24 Compare October 14, 2020 02:05

antoninbas previously approved these changes Oct 14, 2020

View reviewed changes

jianjuns previously approved these changes Oct 14, 2020

View reviewed changes

srikartati reviewed Oct 16, 2020

View reviewed changes

Cleanup: remove send_redirects sysctl configuration

0961130

With L3 flows, Pod-to-Pod traffic will no longer go through gateway interfaces. For Pod-to-Service traffic, Linux doesn't allow sending redirect packets for NAT'd connections. Therefore, configuring send_redirects is no longer needed.

tnqn dismissed stale reviews from jianjuns and antoninbas via 0961130 October 19, 2020 15:34

tnqn force-pushed the sysctl branch from 9057a24 to 0961130 Compare October 19, 2020 15:34

tnqn changed the title ~~Cleanup: remove send_redirects sysctl configuration and reduce privilege~~ Cleanup: remove send_redirects sysctl configuration Oct 19, 2020

antoninbas approved these changes Oct 19, 2020

View reviewed changes

tnqn merged commit 6f698ee into antrea-io:master Oct 20, 2020

antoninbas mentioned this pull request Nov 12, 2020

Remove unnecessary sysctl configuration and reduce privilege #1363

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup: remove send_redirects sysctl configuration #1364

Cleanup: remove send_redirects sysctl configuration #1364

tnqn commented Oct 13, 2020 •

edited

Loading

antrea-bot commented Oct 13, 2020

tnqn commented Oct 13, 2020

codecov-io commented Oct 13, 2020 •

edited

Loading

tnqn commented Oct 13, 2020

jianjuns left a comment

antoninbas left a comment

antoninbas commented Oct 14, 2020

tnqn commented Oct 14, 2020

tnqn commented Oct 14, 2020

tnqn commented Oct 14, 2020

jianjuns left a comment

tnqn commented Oct 14, 2020

srikartati Oct 16, 2020 •

edited

Loading

tnqn Oct 16, 2020

antoninbas Oct 16, 2020

srikartati Oct 16, 2020 •

edited

Loading

tnqn Oct 19, 2020

antoninbas Oct 19, 2020

tnqn commented Oct 19, 2020

tnqn commented Oct 19, 2020

Cleanup: remove send_redirects sysctl configuration #1364

Cleanup: remove send_redirects sysctl configuration #1364

Conversation

tnqn commented Oct 13, 2020 • edited Loading

antrea-bot commented Oct 13, 2020

tnqn commented Oct 13, 2020

codecov-io commented Oct 13, 2020 • edited Loading

Codecov Report

tnqn commented Oct 13, 2020

jianjuns left a comment

Choose a reason for hiding this comment

antoninbas left a comment

Choose a reason for hiding this comment

antoninbas commented Oct 14, 2020

tnqn commented Oct 14, 2020

tnqn commented Oct 14, 2020

tnqn commented Oct 14, 2020

jianjuns left a comment

Choose a reason for hiding this comment

tnqn commented Oct 14, 2020

srikartati Oct 16, 2020 • edited Loading

Choose a reason for hiding this comment

tnqn Oct 16, 2020

Choose a reason for hiding this comment

antoninbas Oct 16, 2020

Choose a reason for hiding this comment

srikartati Oct 16, 2020 • edited Loading

Choose a reason for hiding this comment

tnqn Oct 19, 2020

Choose a reason for hiding this comment

antoninbas Oct 19, 2020

Choose a reason for hiding this comment

tnqn commented Oct 19, 2020

tnqn commented Oct 19, 2020

tnqn commented Oct 13, 2020 •

edited

Loading

codecov-io commented Oct 13, 2020 •

edited

Loading

srikartati Oct 16, 2020 •

edited

Loading

srikartati Oct 16, 2020 •

edited

Loading