Skip to content

labelsfilter: Add serviceaccount label in the default labels#38017

Merged
joestringer merged 1 commit intocilium:mainfrom
liyihuang:pr/liyihuang/add_service_account_in_default_labels
Apr 7, 2025
Merged

labelsfilter: Add serviceaccount label in the default labels#38017
joestringer merged 1 commit intocilium:mainfrom
liyihuang:pr/liyihuang/add_service_account_in_default_labels

Conversation

@liyihuang
Copy link
Copy Markdown
Contributor

@liyihuang liyihuang commented Mar 5, 2025

  • 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
Copy Markdown
Contributor Author

liyihuang commented Mar 5, 2025

/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
Copy Markdown
Contributor Author

liyihuang commented Mar 5, 2025

/test

@liyihuang
Copy link
Copy Markdown
Contributor Author

liyihuang commented Mar 5, 2025

/ci-runtime

@liyihuang
Copy link
Copy Markdown
Contributor Author

liyihuang commented Mar 5, 2025

/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 a user, derailed and ysksuzuki March 6, 2025 00:47
ghost
ghost approved these changes Mar 6, 2025
View reviewed changes
Copy link
Copy Markdown

@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
Copy Markdown
Member

ysksuzuki commented Mar 7, 2025

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
Copy Markdown
Contributor Author

liyihuang commented Mar 7, 2025

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
Copy Markdown
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
Copy Markdown
Contributor Author

liyihuang commented Mar 27, 2025

/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
Copy Markdown
Contributor Author

liyihuang commented Apr 1, 2025

/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
Copy Markdown
Contributor Author

liyihuang commented Apr 1, 2025

/test

@joestringer
Copy link
Copy Markdown
Member

joestringer commented Apr 2, 2025

@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
Copy Markdown
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
Copy Markdown
Contributor Author

liyihuang commented Apr 2, 2025

/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
Copy Markdown
Contributor Author

liyihuang commented Apr 2, 2025

/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
Copy Markdown
Contributor Author

liyihuang commented Apr 4, 2025

/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
Copy Markdown
Member

joestringer commented Apr 7, 2025

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

7 participants

@liyihuang @ysksuzuki @joestringer @derailed @marseel @msune @julianwiedmann