Skip to content

identity: remove labels on CID #39552

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
May 20, 2025
Merged

identity: remove labels on CID #39552

merged 1 commit into from
May 20, 2025

Conversation

marseel
Copy link
Contributor

@marseel marseel commented May 15, 2025 •
edited
Loading

As we don't use labels from CiliumIdentities, let's remove them.
Labels have restriction of 63 characters and when someone created a
serviceaccount with name longer than 63, we were trying to add it to
labels of CiliumIdentity. This resulted in failure of creating CID and
failure to start pod.
We only keep namespace label as it's used when listing ciliumidentities
through kubectl, example:

$ k get ciliumidentities.cilium.io 
NAME    NAMESPACE            AGE
21391   kube-system          45m
22578   local-path-storage   45m
9551    default              45m

Fixes: #16579

Creating pod with long (>63 characters) serviceaccount name works now

@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 May 15, 2025
@github-actions github-actions bot added the sig/policy Impacts whether traffic is allowed or denied based on user-defined policies. label May 15, 2025
@marseel marseel force-pushed the pr/marseel/remove_labels branch 2 times, most recently from b5fbcb1 to 331a4df Compare May 15, 2025 10:32
@marseel marseel added the release-note/bug This PR fixes an issue in a previous release of Cilium. label May 15, 2025
@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 May 15, 2025
@marseel
Copy link
Contributor Author

marseel commented May 15, 2025

/test

@marseel marseel marked this pull request as ready for review May 15, 2025 11:39
@marseel marseel requested review from a team as code owners May 15, 2025 11:39
@marseel marseel requested review from joamaki and bimmlerd May 15, 2025 11:39
This was referenced May 15, 2025
Closed
Closed
sypakine
sypakine approved these changes May 15, 2025
View reviewed changes
Copy link
Contributor

@sypakine sypakine left a comment

Choose a reason for hiding this comment

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

Thanks for digging into this!

@marseel marseel force-pushed the pr/marseel/remove_labels branch from 331a4df to 5e3c86a Compare May 15, 2025 13:19
bimmlerd
bimmlerd approved these changes May 15, 2025
View reviewed changes
Copy link
Member

@bimmlerd bimmlerd left a comment

Choose a reason for hiding this comment

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

LGTM code-wise, but is this a breaking change for folks somehow consuming labels on CiliumIdentity? Does it need to be mentioned in the docs somewhere?

@marseel
Copy link
Contributor Author

marseel commented May 15, 2025

/test

@marseel
Copy link
Contributor Author

marseel commented May 15, 2025

LGTM code-wise, but is this a breaking change for folks somehow consuming labels on CiliumIdentity? Does it need to be mentioned in the docs somewhere?

I don't have a strong opinion, I can add it if you have strong opinion on it. CiliumIdentity is kind of an implementation detail ;)

squeed
squeed reviewed May 19, 2025
View reviewed changes
@squeed
Copy link
Contributor

squeed commented May 19, 2025

This seems extremely reasonable! I agree that there could be someone relying on this, but I don't see it as published API. That said, we should probably add a release note and not backport this, on the off chance that is relied on.

(There may be some kubectl get -l XXX scripts out there).

@marseel
identity: remove labels on CID
e528f3e 
As we don't use labels from CiliumIdentities, let's remove them.
Labels have restriction of 63 characters and when someone created a
serviceaccount with name longer than 63, we were trying to add it to
labels of CiliumIdentity. This resulted in failure of creating CID and
failure to start pod.
We only keep namespace label as it's used when listing ciliumidentities
through kubectl.

Fixes: #16579

Signed-off-by: Marcel Zieba <marcel.zieba@isovalent.com>
@marseel marseel force-pushed the pr/marseel/remove_labels branch from 5e3c86a to e528f3e Compare May 19, 2025 11:26
@marseel marseel requested a review from a team as a code owner May 19, 2025 11:26
@marseel marseel requested a review from qmonnet May 19, 2025 11:26
@marseel
Copy link
Contributor Author

marseel commented May 19, 2025

This seems extremely reasonable! I agree that there could be someone relying on this, but I don't see it as published API. That said, we should probably add a release note and not backport this, on the off chance that is relied on.

Done.

@marseel
Copy link
Contributor Author

marseel commented May 19, 2025

/test

qmonnet
qmonnet approved these changes May 20, 2025
View reviewed changes
Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

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

OK for the docs, thanks!

@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 May 20, 2025
@joestringer joestringer added this pull request to the merge queue May 20, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 20, 2025
@joestringer joestringer added this pull request to the merge queue May 20, 2025
Merged via the queue into main with commit e34b68c May 20, 2025
333 of 338 checks passed
@joestringer joestringer deleted the pr/marseel/remove_labels branch May 20, 2025 19:44
@julianwiedmann
Copy link
Member

@marseel given it's labeled as release-note/bug, should this be backported? Or is the impact sufficiently low severity?

@marseel
Copy link
Contributor Author

marseel commented May 22, 2025

given it's labeled as release-note/bug, should this be backported? Or is the impact sufficiently low severity?

@julianwiedmann too low severity.
Also see Casey's comment, which I fully agree with:

This seems extremely reasonable! I agree that there could be someone relying on this, but I don't see it as published API. That said, we should probably add a release note and not backport this, on the off chance that is relied on.

@julianwiedmann julianwiedmann added the severity/low Impact is limited, only affecting unusual configs, or all configs with low impact in prod. label May 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@squeed squeed squeed left review comments

@joamaki joamaki joamaki approved these changes

@bimmlerd bimmlerd bimmlerd approved these changes

@qmonnet qmonnet qmonnet approved these changes

+1 more reviewer

@sypakine sypakine sypakine approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/bug This PR fixes an issue in a previous release of Cilium. severity/low Impact is limited, only affecting unusual configs, or all configs with low impact in prod. sig/policy Impacts whether traffic is allowed or denied based on user-defined policies.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cannot create CiliumIdentity for ServiceAccount names longer than 63 chars
8 participants
@marseel @squeed @julianwiedmann @joamaki @sypakine @bimmlerd @qmonnet @joestringer