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.

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

Remove non fatal errors from SPIRE client in operator #28698

Merged
merged 1 commit into from
Jan 11, 2024
Merged

Remove non fatal errors from SPIRE client in operator #28698

merged 1 commit into from
Jan 11, 2024

Conversation

meyskens
Copy link
Member

This removed level=error errors from the spire-client system in the operator. As it is expected to fail on initial startup of SPIRE to have connection issues Cilium will retry.

Some errors also came from the SPIFFE library itself which is why a wrapper to log them as warnings instead was added.

Fixes: #27842

Remove non fatal errors from SPIRE client in the operator

@meyskens meyskens requested review from a team and mhofstetter and removed request for a team October 19, 2023 12:39
@meyskens meyskens requested a review from a team as a code owner October 19, 2023 12:39
@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 Oct 19, 2023
@meyskens meyskens added release-note/bug This PR fixes an issue in a previous release of Cilium. needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch labels Oct 19, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Oct 19, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in 1.14.4 Oct 19, 2023
@meyskens meyskens added dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. sig/agent Cilium agent related. feature/authentication labels Oct 19, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Oct 19, 2023
mhofstetter
mhofstetter approved these changes Oct 20, 2023
View reviewed changes
Copy link
Member

@mhofstetter mhofstetter left a comment

Choose a reason for hiding this comment

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

Nice @meyskens

Only one non-blocking nit in a comment.

operator/auth/spire/log_wrapper.go Outdated Show resolved Hide resolved
@mhofstetter mhofstetter added the area/servicemesh GH issues or PRs regarding servicemesh label Oct 20, 2023
pippolo84
pippolo84 reviewed Oct 20, 2023
View reviewed changes
Copy link
Member

@pippolo84 pippolo84 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 your PR!
I've left a couple of questions inline.

operator/auth/spire/client.go Show resolved Hide resolved
operator/auth/spire/log_wrapper.go Show resolved Hide resolved
@thorn3r thorn3r added this to Needs backport from main in 1.14.5 Nov 9, 2023
@thorn3r thorn3r removed this from Needs backport from main in 1.14.4 Nov 9, 2023
@github-actions GitHub Actions
Copy link

This pull request has been automatically marked as stale because it
has not had recent activity. It will be closed if no further activity
occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Nov 20, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in 1.14.4 Nov 20, 2023
@meyskens meyskens removed the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Nov 20, 2023
@nebril nebril added this to Needs backport from main in 1.14.6 Dec 11, 2023
@nebril nebril removed this from Needs backport from main in 1.14.5 Dec 11, 2023
@aanm aanm requested a review from pippolo84 December 11, 2023 14:48
@aanm aanm removed this from Needs backport from main in 1.14.4 Dec 11, 2023
@meyskens
Copy link
Member Author

/test

@meyskens
Remove non fatal errors from SPIRE client in operator
3b2b3d0 
This removed level=error errors from the spire-client system in the
operator. As it is expected to fail on initial startup of SPIRE to have
connection issues Cilium will retry.

Some errors also came from the SPIFFE library itself which is why a
wrapper to log them as warnings instead was added.

Signed-off-by: Maartje Eyskens <maartje.eyskens@isovalent.com>
@meyskens
Copy link
Member Author

meyskens commented Jan 4, 2024

/test

@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 Jan 5, 2024
@dylandreimerink
Copy link
Member

@meyskens can all open conversations be considered resolved?

@meyskens
Copy link
Member Author

@dylandreimerink judging from the thumbs ups i think they can

@pippolo84
Copy link
Member

@meyskens can all open conversations be considered resolved?

Regarding my comments everything is fine, I marked them as resolved. 👍

@aanm aanm added this pull request to the merge queue Jan 11, 2024
Merged via the queue into cilium:main with commit 284b41d Jan 11, 2024
62 checks passed
@jibi jibi mentioned this pull request Jan 12, 2024
Merged
32 tasks
@jibi jibi added backport-pending/1.15 The backport for Cilium 1.15.x for this PR is in progress. and removed needs-backport/1.15 This PR / issue needs backporting to the v1.15 branch labels Jan 12, 2024
@gandro gandro mentioned this pull request Jan 16, 2024
Merged
5 tasks
@gandro gandro added backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. and removed needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch labels Jan 16, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from main to Backport pending to v1.15 in v1.15.0-rc.1 Jan 16, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from main to Backport pending to v1.14 in 1.14.6 Jan 16, 2024
@aanm aanm mentioned this pull request Jan 16, 2024
Merged
@github-actions github-actions bot added backport-done/1.14 The backport for Cilium 1.14.x for this PR is done. and removed backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. labels Jan 17, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot removed this from Backport pending to v1.14 in 1.14.6 Jan 17, 2024
@gentoo-root gentoo-root mentioned this pull request Jan 17, 2024
Merged
@giorio94 giorio94 added backport-done/1.15 The backport for Cilium 1.15.x for this PR is done. and removed backport-pending/1.15 The backport for Cilium 1.15.x for this PR is in progress. labels Jan 29, 2024
@aanm aanm added this to Backport pending to v1.15 in 1.15.1 Jan 31, 2024
@aanm aanm removed this from Backport pending to vv1.15.0-rc in v1.15.0-rc.1 Jan 31, 2024
@aanm aanm removed this from Backport pending to v1.15 in 1.15.1 Jan 31, 2024
@aanm aanm added this to Backport done to v1.15 in v1.15.0-rc.1 Jan 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pippolo84 pippolo84 pippolo84 approved these changes

@mhofstetter mhofstetter mhofstetter approved these changes

Assignees
No one assigned
Labels
area/servicemesh GH issues or PRs regarding servicemesh backport-done/1.14 The backport for Cilium 1.14.x for this PR is done. backport-done/1.15 The backport for Cilium 1.15.x for this PR is done. feature/authentication 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. sig/agent Cilium agent related.
Projects
No open projects
v1.15.0-rc.1
Backport done to v1.15
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Operator emits lots of level=error msg="Failed to watch the Workload API" log messages
8 participants
@meyskens @aanm @giorio94 @dylandreimerink @pippolo84 @mhofstetter @gandro @jibi