Skip to content
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

cilium-dbg: Expose Cilium network routing status #32036

Merged
merged 1 commit into from
Apr 19, 2024

Conversation

joestringer
Copy link
Member

@joestringer joestringer commented Apr 17, 2024

Expose the routing mode currently active in the cilium-dbg status CLI.

Example:

$ cilium-dbg status
...
Routing:                              Network: Tunnel [vxlan]   Host: Legacy
...

Expose the routing mode currently active in the cilium-dbg status CLI.

Signed-off-by: Joe Stringer <joe@cilium.io>
@joestringer joestringer requested review from a team as code owners April 17, 2024 23:59
@joestringer joestringer added the release-note/misc This PR makes changes that have no direct user impact. label Apr 17, 2024
@joestringer
Copy link
Member Author

/test

Copy link
Contributor

@learnitall learnitall left a comment

Choose a reason for hiding this comment

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

Thanks Joe! I'm surprised we don't have this already.

The changes to the API are breaking, but since this API resource is only consumed by utilities which are built alongside the API, we shouldn't have to worry about breaking compatibility. It's not expected for an external consumer to rely on the existing of the HostRouting object, as external consumers of Cilium's API aren't expected in general, and we don't package different Cilium client and server versions into the same image.

@brb
Copy link
Member

brb commented Apr 19, 2024

Host: Legacy

A bit of bikeshedding, but ... IMO the BPF host routing is still an experimental feature (e.g., in the ENI mode it's discouraged). For me "legacy" reads as if something is bad / not wanted. Maybe we could improve the wording by just saying BPF routing: (on | off).

@joestringer
Copy link
Member Author

@brb I think that's fair criticism. Note we've been using this terminology for several releases now and I think it's actually been pretty effective at nudging users to try to upgrade their kernels and get BPF host routing enabled. That said I didn't have a strong opinion on this. @borkmann would you like to chime in on the above comment from Martynas?

Copy link
Contributor

@thorn3r thorn3r left a comment

Choose a reason for hiding this comment

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

Nice addition 👍 Agreed that off sounds a bit nicer than legacy if there are some cases where it will never be supported. If that wording has inspired FOMO and encouraged some folks to upgrade, I'm guessing it will also inspire the same for EKS users

@joestringer joestringer added this pull request to the merge queue Apr 19, 2024
@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 Apr 19, 2024
Merged via the queue into main with commit 302a8b1 Apr 19, 2024
265 of 268 checks passed
@joestringer joestringer deleted the pr/joe/expose-routing-mode branch April 19, 2024 18:20
@joestringer
Copy link
Member Author

@brb I opened #32131 to follow up on that.

Comment on lines +496 to +500
if sr.Routing.InterHostRoutingMode == models.RoutingInterHostRoutingModeTunnel {
status = status + " [" + sr.Routing.TunnelProtocol + "]"
}
status = status + "\tHost: " + sr.Routing.IntraHostRoutingMode

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, that's been on my list for quite a while 🙂. Did you consider also exposing the tunnel port?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants