-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Use direct routing device only when tunneling is disabled and BPF Host Routing or NodePort are enabled. #18815
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
Conversation
a518f53
to
5612531
Compare
/test |
5612531
to
7b7b33d
Compare
/test |
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.
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.
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.
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>
26ac911
to
8034fd5
Compare
/test Job 'Cilium-PR-K8s-1.23-kernel-net-next' failed: Click to show.Test Name
Failure Output
If it is a flake and a GitHub issue doesn't already exist to track it, comment |
/test-1.23-net-next |
Travis CI failure seems relevant to my changes. Let me check. |
I shouldn't skip 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 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>
8034fd5
to
88da2ee
Compare
/test |
Please see the commit for details.
Fixes: #17480