Skip to content

labelsfilter: Add serviceaccount label in the default labels #38017

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
Apr 7, 2025
Merged

labelsfilter: Add serviceaccount label in the default labels #38017

merged 1 commit into from
Apr 7, 2025

Conversation

liyihuang
Copy link
Contributor

  • Add serviceaccount label in the default labels list
  • Update the unit test
  • Update the doc

Fixes: #36923

Add serviceaccount label in the default labels list

@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 Mar 5, 2025
@liyihuang liyihuang force-pushed the pr/liyihuang/add_service_account_in_default_labels branch 4 times, most recently from 8eb10b5 to ec45a35 Compare March 5, 2025 18:51
@liyihuang
Copy link
Contributor Author

/test

@liyihuang liyihuang force-pushed the pr/liyihuang/add_service_account_in_default_labels branch 2 times, most recently from 929e75e to 8b4445d Compare March 5, 2025 19:49
@liyihuang
Copy link
Contributor Author

/test

@liyihuang
Copy link
Contributor Author

/ci-runtime

@liyihuang
Copy link
Contributor Author

/ci-eks

@liyihuang liyihuang marked this pull request as ready for review March 6, 2025 00:46
@liyihuang liyihuang requested review from a team as code owners March 6, 2025 00:47
@liyihuang liyihuang requested review from derailed, a user and ysksuzuki March 6, 2025 00:47
ghost
ghost approved these changes Mar 6, 2025
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

docs look good

@ghost ghost added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Mar 6, 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 Mar 6, 2025
@ysksuzuki
Copy link
Member

From CFP

When using the following including label docs to configure the labels, the label io.cilium.k8s.policy.serviceaccount is not included by default, which will cause the cilium connectivity test fails and we have to include this label manually.

Could you elaborate on why the cilium connectivity test fails without io.cilium.k8s.policy.serviceaccount?

@liyihuang
Copy link
Contributor Author

From CFP

When using the following including label docs to configure the labels, the label io.cilium.k8s.policy.serviceaccount is not included by default, which will cause the cilium connectivity test fails and we have to include this label manually.

Could you elaborate on why the cilium connectivity test fails without io.cilium.k8s.policy.serviceaccount?

Because we have the test case for it.

For example:

cilium/cilium-cli/connectivity/builder/manifests/client-egress-to-echo-service-account.yaml

Line 17 in dd2295f

io.cilium.k8s.policy.serviceaccount: echo-same-node

As I mentioned before, this label was added by us without user configuring it. I think we should include as the default label.

ysksuzuki
ysksuzuki approved these changes Mar 21, 2025
View reviewed changes
Copy link
Member

@ysksuzuki ysksuzuki left a comment

Choose a reason for hiding this comment

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

Thank you for clarifying it! LGTM

@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 Mar 21, 2025
@joestringer joestringer added the upgrade-impact This PR has potential upgrade or downgrade impact. label Mar 21, 2025
joestringer
joestringer reviewed Mar 21, 2025
View reviewed changes
@joestringer joestringer added dont-merge/discussion A discussion is ongoing and should be resolved before merging, regardless of reviews & tests status. and removed ready-to-merge This PR has passed all tests and received consensus from code owners to merge. labels Mar 21, 2025
@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 Mar 21, 2025
@joestringer joestringer removed the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Mar 21, 2025
@joestringer joestringer requested a review from marseel March 26, 2025 19:36
@liyihuang liyihuang force-pushed the pr/liyihuang/add_service_account_in_default_labels branch from 8b4445d to 88f51dc Compare March 27, 2025 14:00
@liyihuang
Copy link
Contributor Author

/test

marseel
marseel reviewed Mar 31, 2025
View reviewed changes
@joestringer joestringer removed the dont-merge/discussion A discussion is ongoing and should be resolved before merging, regardless of reviews & tests status. label Mar 31, 2025
@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 Mar 31, 2025
@liyihuang liyihuang force-pushed the pr/liyihuang/add_service_account_in_default_labels branch 2 times, most recently from d6a92fa to 4243e68 Compare April 1, 2025 15:31
@liyihuang
Copy link
Contributor Author

/test

@liyihuang liyihuang force-pushed the pr/liyihuang/add_service_account_in_default_labels branch from 4243e68 to 7c8e42b Compare April 1, 2025 23:38
@liyihuang
Copy link
Contributor Author

/test

@joestringer
Copy link
Member

@liyihuang could you file an issue for the failing test? At a glance of the latest push events for the e2e-upgrade workflow (link) it seems like we may have introduced a regression into the main branch related to this test. I see that upon multiple re-runs the same test is failing for the same reason, so I don't think we can treat this as an unreliable failure. Rather, we should be recognizing that the test framework is telling us something is broken, file an issue, notify the relevant code owners and we can follow up to investigate and locate the root cause of the failure.

@liyihuang
Copy link
Contributor Author

liyihuang commented Apr 2, 2025 •
edited
Loading

Sure. btw, I could be related to changes recently since I did a rebase yesterday because of other issues.

I can see the main is broken for this e2e-upgrade 10 since another PR having the same issue.

The issue is known and resolved by the #38566

@liyihuang liyihuang mentioned this pull request Apr 2, 2025
Closed
@liyihuang liyihuang force-pushed the pr/liyihuang/add_service_account_in_default_labels branch from 7c8e42b to 087dc98 Compare April 2, 2025 13:56
@liyihuang
Copy link
Contributor Author

/test

@liyihuang liyihuang force-pushed the pr/liyihuang/add_service_account_in_default_labels branch from 087dc98 to eec2d86 Compare April 2, 2025 17:15
@liyihuang
Copy link
Contributor Author

/test

@liyihuang
labelsfilter: Add serviceaccount label in the default labels
fb3cbb6 
- Add serviceaccount label in the default labels list
- Update the unit test
- Update the doc

Signed-off-by: Liyi Huang <liyi.huang@isovalent.com>
@liyihuang liyihuang force-pushed the pr/liyihuang/add_service_account_in_default_labels branch from d2840ec to fb3cbb6 Compare April 4, 2025 18:51
@liyihuang
Copy link
Contributor Author

/test

@liyihuang liyihuang mentioned this pull request Apr 6, 2025
Merged
@julianwiedmann julianwiedmann removed the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Apr 7, 2025
@joestringer
Copy link
Member

FQDN failures should be fixed by #38754 .

@joestringer joestringer merged commit a1e9a5d into cilium:main Apr 7, 2025
66 of 67 checks passed
@liyihuang liyihuang deleted the pr/liyihuang/add_service_account_in_default_labels branch August 16, 2025 02:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joestringer joestringer joestringer left review comments

@marseel marseel marseel left review comments

@derailed derailed derailed approved these changes

@ysksuzuki ysksuzuki ysksuzuki approved these changes

+1 more reviewer
Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
release-note/minor This PR changes functionality that users may find relevant to operating Cilium. upgrade-impact This PR has potential upgrade or downgrade impact.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

CFP: Add serviceaccount label in the default labels list
6 participants
@liyihuang @ysksuzuki @joestringer @derailed @marseel @julianwiedmann