Extract tunnel options to simplify override, and inject them through hive #29051

giorio94 · 2023-11-08T11:02:53Z

Certain features (e.g., egress gateway, high-scale ipcache, ...) require
the configuration of the tunnel device also then the primary routing
mode is set to native routing, as some traffic may need to flow through
it. Currently, every affected code path needs ad-hoc checks, which is
getting more and more complex with the addition of new features.

This commit consolidates the tunnel-related options, that is protocol,
port, device name and whether it requires MTU adaptation, into a single
struct, initialized based on the user configuration and the requests of
additional features, and propagated through hive. Specifically, different
modules can now request the configuration of the tunnel device (regardless
of the primary routing mode) through the injection of the appropriate
enabler object. Additional validation constraints can be specified if a
given feature is compatible only with a specific encapsulation protocol.

This change introduces a small functional difference, because now the
tunnel protocol flag is always respected, and no longer defaulted to
geneve when Cilium is configured in native routing mode, and DSR
Geneve is enabled. This prevents causing unexpected tunnel changes
when turning on/off other features (DSR Geneve in this case).

The primary routing mode is still represented by the TunnelingEnabled()
function, both to reduce the scope of this change and because it is
currently accessed also before the initialization of the hive.

Fixes: #25769
~~Depends on: #29053~~

giorio94 · 2023-11-08T11:21:49Z

/test

giorio94 · 2023-11-08T14:13:26Z

/test

giorio94 · 2023-11-08T15:52:01Z

Marking ready for review, to start gathering feedback. Please ignore the first commit, as it is a stripped down version of #29053, which should be merged first.

christarazi

ci-structure LGTM

jschwinger233

LGTM, thanks! Left a comment but don't let it block merging.

daemon/cmd/kube_proxy_replacement.go

lmb

Loader changes LGTM.

julianwiedmann

One log message nitpick, and one question wrt to upgrade behaviour. Both could be addressed as follow-on PR if needed. Otherwise lgtm, thank you very much Marco!

daemon/cmd/kube_proxy_replacement.go

test/k8s/services.go

daemon/cmd/daemon.go

ysksuzuki

LGTM! Just a non-blocking newbie question.

derailed

@giorio94 Nice work Marco!

pkg/datapath/loader/netlink.go

pkg/datapath/tunnel/cell.go

pkg/datapath/tunnel/tunnel.go

giorio94 · 2023-11-10T11:08:49Z

Rebased onto main and dropped the temporary commit. Additionally added a new commit extending the upgrade notes to document the behavioral difference with respect to Cilium configured in native routing mode with DSR Geneve enabled.

giorio94 · 2023-11-10T11:10:38Z

/test

giorio94 · 2023-11-10T12:56:42Z

Fixed a test failing due to a mismatch of integer types.

giorio94 · 2023-11-10T12:56:48Z

/ci-runtime

giorio94 · 2023-11-10T13:33:53Z

/test

Certain features (e.g., egress gateway, high-scale ipcache, ...) require the configuration of the tunnel device also then the primary routing mode is set to native routing, as some traffic may need to flow through it. Currently, every affected code path needs ad-hoc checks, which is getting more and more complex with the addition of new features. This commit consolidates the tunnel-related options, that is protocol, port, device name and whether it requires MTU adaptation, into a single struct, initialized based on the user configuration and the requests of additional features, and propagated through hive. Specifically, different modules can now request the configuration of the tunnel device (regardless of the primary routing mode) through the injection of the appropriate enabler object. Additional validation constraints can be specified if a given feature is compatible only with a specific encapsulation protocol. This change introduces a small functional difference, because now the tunnel protocol flag is always respected, and no longer defaulted to geneve when Cilium is configured in native routing mode, and DSR Geneve is enabled. This prevents causing unexpected tunnel changes when turning on/off other features (DSR Geneve in this case). The primary routing mode is still represented by the TunnelingEnabled() function, both to reduce the scope of this change and because it is currently accessed also before the initialization of the hive. Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>

Historically, the tunnel protocol was defaulted to geneve when Cilium was configured in native routing mode and DSR Geneve was enabled. This is no longer the case since the last commit, and the tunnel protocol flag is now always respected (possibly triggering an error if incompatible), so that we don't cause unexpected tunnel changes when turning on/off other features. Hence, let's document in the upgrade notes that users now need to explicitly configure the tunnel protocol to geneve when using DSR Geneve. Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>

giorio94 · 2023-11-13T17:13:57Z

Rebased onto main to fix conflict

giorio94 · 2023-11-13T17:14:07Z

/test

derailed

@giorio94 Thanks for the updates!

julianwiedmann · 2023-11-20T07:55:29Z

Additionally added a new commit extending the upgrade notes to document the behavioral difference with respect to Cilium configured in native routing mode with DSR Geneve enabled.

Note that Marco wrote up a draft for how we could do auto-selection of the tunnel protocol in some scenarios: #29267. Let's continue any discussion there.

Reflect the config change from cilium#29051 in the kubeproxy-free docs for DSR-Geneve. Fixes: cilium#30845 Signed-off-by: Julian Wiedmann <jwi@isovalent.com>

Reflect the config change from #29051 in the kubeproxy-free docs for DSR-Geneve. Fixes: #30845 Signed-off-by: Julian Wiedmann <jwi@isovalent.com>

[ upstream commit 8791007 ] Reflect the config change from #29051 in the kubeproxy-free docs for DSR-Geneve. Fixes: #30845 Signed-off-by: Julian Wiedmann <jwi@isovalent.com>

giorio94 added kind/cleanup This includes no functional changes. area/daemon Impacts operation of the Cilium daemon. release-note/misc This PR makes changes that have no direct user impact. labels Nov 8, 2023

giorio94 force-pushed the mio/tunnel-config-cell branch from ec18ab2 to 7a6ed14 Compare November 8, 2023 14:12

giorio94 added the dont-merge/blocked Another PR must be merged before this one. label Nov 8, 2023

giorio94 marked this pull request as ready for review November 8, 2023 15:52

giorio94 requested review from a team as code owners November 8, 2023 15:52

giorio94 requested review from ysksuzuki, christarazi, derailed, jschwinger233 and lmb November 8, 2023 15:52

christarazi approved these changes Nov 8, 2023

View reviewed changes

jschwinger233 approved these changes Nov 9, 2023

View reviewed changes

daemon/cmd/kube_proxy_replacement.go Show resolved Hide resolved

lmb approved these changes Nov 9, 2023

View reviewed changes

julianwiedmann approved these changes Nov 9, 2023

View reviewed changes

daemon/cmd/kube_proxy_replacement.go Outdated Show resolved Hide resolved

test/k8s/services.go Show resolved Hide resolved

julianwiedmann added the upgrade-impact This PR has potential upgrade or downgrade impact. label Nov 9, 2023

ysksuzuki reviewed Nov 9, 2023

View reviewed changes

daemon/cmd/daemon.go Show resolved Hide resolved

ysksuzuki approved these changes Nov 9, 2023

View reviewed changes

derailed reviewed Nov 9, 2023

View reviewed changes

pkg/datapath/loader/netlink.go Show resolved Hide resolved

pkg/datapath/loader/netlink.go Show resolved Hide resolved

pkg/datapath/tunnel/cell.go Show resolved Hide resolved

pkg/datapath/tunnel/tunnel.go Show resolved Hide resolved

giorio94 force-pushed the mio/tunnel-config-cell branch from 7a6ed14 to e1f729b Compare November 10, 2023 11:07

giorio94 requested a review from a team as a code owner November 10, 2023 11:07

giorio94 requested a review from qmonnet November 10, 2023 11:07

giorio94 removed the dont-merge/blocked Another PR must be merged before this one. label Nov 10, 2023

giorio94 force-pushed the mio/tunnel-config-cell branch from e1f729b to 7ee3b3a Compare November 10, 2023 12:56

giorio94 mentioned this pull request Nov 10, 2023

CI: Conformance Ginkgo (f19-update) - found error endpoint regeneration failed #29108

Closed

giorio94 requested a review from derailed November 10, 2023 16:41

giorio94 added 2 commits November 13, 2023 18:13

giorio94 force-pushed the mio/tunnel-config-cell branch from 7ee3b3a to 1397972 Compare November 13, 2023 17:13

derailed approved these changes Nov 13, 2023

View reviewed changes

qmonnet approved these changes Nov 14, 2023

View reviewed changes

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 Nov 14, 2023

julianwiedmann merged commit 1cc77a1 into cilium:main Nov 14, 2023
61 checks passed

giorio94 mentioned this pull request Nov 20, 2023

tunnel: add support for tunnel protocol defaulting #29267

Closed

julianwiedmann mentioned this pull request Feb 19, 2024

docs: kpr: DSR-Geneve with native-routing requires tunnelProtocol #30854

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract tunnel options to simplify override, and inject them through hive #29051

Extract tunnel options to simplify override, and inject them through hive #29051

giorio94 commented Nov 8, 2023 •

edited

giorio94 commented Nov 8, 2023

giorio94 commented Nov 8, 2023

giorio94 commented Nov 8, 2023

christarazi left a comment

jschwinger233 left a comment

lmb left a comment

julianwiedmann left a comment

ysksuzuki left a comment

derailed left a comment

giorio94 commented Nov 10, 2023

giorio94 commented Nov 10, 2023

giorio94 commented Nov 10, 2023

giorio94 commented Nov 10, 2023

giorio94 commented Nov 10, 2023

giorio94 commented Nov 13, 2023

giorio94 commented Nov 13, 2023

derailed left a comment

julianwiedmann commented Nov 20, 2023

Extract tunnel options to simplify override, and inject them through hive #29051

Extract tunnel options to simplify override, and inject them through hive #29051

Conversation

giorio94 commented Nov 8, 2023 • edited

giorio94 commented Nov 8, 2023

giorio94 commented Nov 8, 2023

giorio94 commented Nov 8, 2023

christarazi left a comment

Choose a reason for hiding this comment

jschwinger233 left a comment

Choose a reason for hiding this comment

lmb left a comment

Choose a reason for hiding this comment

julianwiedmann left a comment

Choose a reason for hiding this comment

ysksuzuki left a comment

Choose a reason for hiding this comment

derailed left a comment

Choose a reason for hiding this comment

giorio94 commented Nov 10, 2023

giorio94 commented Nov 10, 2023

giorio94 commented Nov 10, 2023

giorio94 commented Nov 10, 2023

giorio94 commented Nov 10, 2023

giorio94 commented Nov 13, 2023

giorio94 commented Nov 13, 2023

derailed left a comment

Choose a reason for hiding this comment

julianwiedmann commented Nov 20, 2023

giorio94 commented Nov 8, 2023 •

edited