Skip to content

Conversation

YutaroHayakawa
Copy link
Member

@YutaroHayakawa YutaroHayakawa commented Feb 15, 2022

Please see the commit for details.

Fixes: #17480

Use direct routing device only when tunneling is disabled and BPF Host Routing or NodePort are enabled.

@YutaroHayakawa YutaroHayakawa requested a review from a team February 15, 2022 18:07
@YutaroHayakawa YutaroHayakawa requested review from a team as code owners February 15, 2022 18:07
@YutaroHayakawa YutaroHayakawa requested review from a team and ldelossa February 15, 2022 18:07
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Feb 15, 2022
@YutaroHayakawa YutaroHayakawa marked this pull request as draft February 15, 2022 18:07
@YutaroHayakawa
Copy link
Member Author

/test

@YutaroHayakawa
Copy link
Member Author

/test

Copy link
Contributor

@ldelossa ldelossa left a comment

Choose a reason for hiding this comment

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

Early review - This generally looks good to me. Small nit on conditional statement.

The change seems good, there is small potential that we changed code paths in agent init, so I'd lean on tests to make sure its an OK change.

Copy link
Member

@pchaigno pchaigno left a comment

Choose a reason for hiding this comment

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

Mostly nits from me, with one question regarding potential impact.

Don't forget to set the labels 😉

The Direct Routing Device is currently used by BPF Host
Routing and BPF Node Port. With both features, we don't
need it for tunneling mode. The DirectRoutingDeviceRequired
helper function checks the above conditions and returns if
the Direct Routing Device is required for the given
configuration.

Upcoming commits will fix several places that use Direct
Routing Device without tunneling or BPF Host Routing in
an account using DirectRoutingDeviceRequired.

Signed-off-by: Yutaro Hayakawa <yutaro.hayakawa@isovalent.com>
@YutaroHayakawa YutaroHayakawa force-pushed the issue-17480-1 branch 2 times, most recently from 26ac911 to 8034fd5 Compare February 21, 2022 08:11
@YutaroHayakawa YutaroHayakawa added release-note/minor This PR changes functionality that users may find relevant to operating Cilium. area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. labels Feb 21, 2022
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Feb 21, 2022
@YutaroHayakawa YutaroHayakawa changed the title Use direct routing device only when needed Use direct routing device only when tunneling is disabled and BPF Host Routing or NodePort are enabled. Feb 21, 2022
@YutaroHayakawa YutaroHayakawa marked this pull request as ready for review February 21, 2022 09:27
@YutaroHayakawa
Copy link
Member Author

YutaroHayakawa commented Feb 21, 2022

/test

Job 'Cilium-PR-K8s-1.23-kernel-net-next' failed:

Click to show.

Test Name

K8sDatapathConfig Host firewall With VXLAN

Failure Output

FAIL: Failed to reach 10.0.0.112:80 from testclient-host-stgtp

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.23-kernel-net-next so I can create one.

@YutaroHayakawa YutaroHayakawa added area/daemon Impacts operation of the Cilium daemon. kind/enhancement This would improve or streamline existing functionality. labels Feb 21, 2022
@YutaroHayakawa
Copy link
Member Author

/test-1.23-net-next

@YutaroHayakawa
Copy link
Member Author

Travis CI failure seems relevant to my changes. Let me check.

@YutaroHayakawa
Copy link
Member Author

I shouldn't skip InitDefaultPrefix in here even if the DirectRoutingDevice is missing.

https://github.com/cilium/cilium/pull/18815/files#diff-9143180298a1f5ba5dc10fd98d87945d1381ef45b7347a639f85e13e84285ca2R363-R367

Actually, in the case of the IPv6, we are handling the missing case here.

https://github.com/cilium/cilium/blob/8034fd57180a7ebc8284a9b099d17db7404e45c9/pkg/node/address.go#L113
Background: 41805e4

I'll revert this change.

Currently, the usage of the Direct Routing Device in Cilium
has the following problems.

1. It only checks whether NodePort is enabled when it uses
   Direct Routing Device. We should also check if the BPF
   Host Routing and Native Routing are enabled (= tunneling
   is disabled) or not.

2. It uses the option.Config.DirectRoutingDevice without
   checking if it is empty. It would be safer to always use
   it with a check and return errors when it is required but
   empty.

Signed-off-by: Yutaro Hayakawa <yutaro.hayakawa@isovalent.com>
Some of the tests using default tunneling mode to test Direct Routing
Device related features like neighbor discovery or device detection.
Due to the fix in the previous commit, now Direct Routing Device is
only used when the tunneling is disabled. Thus, those tests will fail.
Fix them to use native routing mode explicitly.

Signed-off-by: Yutaro Hayakawa <yutaro.hayakawa@isovalent.com>
@YutaroHayakawa
Copy link
Member Author

/test

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Feb 21, 2022
@nebril nebril merged commit 6237e74 into cilium:master Feb 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/daemon Impacts operation of the Cilium daemon. area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. kind/enhancement This would improve or streamline existing functionality. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

datapath: Do not require direct routing device in tunneling mode
4 participants