Print full labelset for all identities in 'cilium ip list' output #28425
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
joestringer
added
release-note/bug
maintainer-s-little-helper
bot
added
dont-merge/needs-release-note-label
joestringer
added
affects/v1.12
|
/test |
christarazi
added
the
area/cli
squeed
approved these changes
Oct 6, 2023
derailed
reviewed
Oct 6, 2023
maintainer-s-little-helper
bot
added
the
ready-to-merge
derailed
approved these changes
Oct 6, 2023
|
All required tests passing and reviews are in. I'll fix up the minor nit from Fernand and repush. Should be good to merge after that. |
joestringer
force-pushed
the
pr/joe/fix-ip-list-host-labels
branch
from
8e7c22a
to
a9d7166
Compare
If the 'cilium ip list' command cannot fetch identities for some numeric identities, then print an error and continue. This way it should still be possible to view most of the ips and at least their numeric identity counterparts even if one of the identities' labels cannot be queried. Signed-off-by: Joe Stringer <joe@cilium.io>
No need to iterate the labels here and individually append them to the slice, there's GetPrintableModel() for this purpose. Signed-off-by: Joe Stringer <joe@cilium.io>
In particular the host identity can have multiple labels that are not hardcoded directly from the number, so we should just always fetch the full labelset from the agent to ensure that the CLI prints what the agent knows. No special cases for reserved identities. The main subtlety here is that the GetIdentityIDParams() input is always the numeric identity as a string, and never the stringified name of that identity. For instance if client-side the CLI thinks that it can map identity "1" to "host" then it will still make the query with the parameter "1" (since that's how the identity get API works). If the query fails, then it'll attempt to fall back to printing a stringified name like "host" if available client-side, or otherwise just print the number. Signed-off-by: Joe Stringer <joe@cilium.io>
A single IP can only have exactly one identity. Each identity can have a set of labels associated with it. This code was confusingly using the variable name 'identities' for individual labels within a single identity. Rename it for clarity. (Strictly speaking the labels may not actually be labels but may be the numeric identity if the user decided not to pretty-print the labels, but I think this variable naming is still more clear than what preceded it). Signed-off-by: Joe Stringer <joe@cilium.io>
joestringer
force-pushed
the
pr/joe/fix-ip-list-host-labels
branch
from
a9d7166
to
5173b4c
Compare
|
/test |
|
All required tests are passing, button is green. Merging. |
|
Oh wait, I'll let @joestringer do the honors. |
tklauser
added
backport-pending/1.14
tklauser
added
backport-done/1.14
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The host identity can have multiple labels that are not hardcoded directly from
the number, so we should just always fetch the full labelset from the agent for
all identities to ensure that the CLI prints what the agent knows. No need for
special cases for reserved identities.
Technically this bug goes way back and impacts any version since the kube-apiserver
entity policy feature has existed, but it's a minor bug so I'm only proposing this
for 1.14 at this time.