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.

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

Add SPIRE connection to status #26896

Merged
merged 1 commit into from Aug 11, 2023
Merged

Conversation

meyskens
Copy link
Member

This adds the SPIRE connection to cilium status, this then can be used by the CLI tool to surface errors and/or wait for SPIRE to be ready. If Auth is disabled it will surface the disabled status.

Fixes: cilium/cilium-cli#1821

Add SPIRE connection to `cilium status`

@meyskens meyskens added release-note/minor This PR changes functionality that users may find relevant to operating Cilium. area/servicemesh GH issues or PRs regarding servicemesh feature/authentication labels Jul 18, 2023
@meyskens meyskens requested review from a team as code owners July 18, 2023 13:26
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.

@meyskens Thanks for the PR. I like the idea of leveraging the status for the cilium status instead of checking the SPIRE deployment directly.

I left some thoughts inline.

Overall i think the concept of the recently introduced HealthReporter would fit here quite well. It looks really promising. It already provides an easy way to report the health status of a hive module by injecting an instance of a HealthReporter. AFAIK, until now, it only lacks support for exposing the reported health status (e.g. via cilium status API).

But the bits for fetching the data are already prepared in the Health interface. IMO it should be possible that the daemon depends on the Health interface and adds a status probe for all registered modules by calling All(). This way the daemon doesn't have to be directly dependent on something "auth-related" at all.

I'm aware of the fact that this should land in v1.14. But I thought it's worth to bring this up and I'd like to have the input of @tommyp1ckles (maybe there's already something in the pipeline)

pkg/auth/spire/certificate_provider.go Outdated Show resolved Hide resolved
daemon/cmd/status.go Outdated Show resolved Hide resolved
daemon/cmd/daemon.go Outdated Show resolved Hide resolved
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.

some small optimizations regarding comments.

But it looks like there's a lint-error -> Please run make generate-k8s-api & make manifests and submit your changes

daemon/cmd/daemon.go Outdated Show resolved Hide resolved
pkg/auth/cell.go Outdated Show resolved Hide resolved
@tommyp1ckles
Copy link
Contributor

@meyskens @mhofstetter From the changes this does look a lot like a module health status, the only difference being that the status is checked when the status endpoint is called, the health reporter status would likely be emitted when there is a change of health status state or on some periodic loop (preferably on a reconciliation loop/controller).

Right now the status of the health reporting is that we don't have the new Agent API health reporter endpoint in v1.14.

However the health status reporter updates can still be accessed via debug logging so there may be value in doing that (we could make the case for bumping that to info level in that case).

If you are interested in eventually adding health reporting lmk and I can provide more details. For the time being I probably recommend doing the changes in this PR and having the health reporting be supplementary.

@meyskens meyskens added the needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch label Jul 25, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in 1.14.0 Jul 25, 2023
@aanm aanm added this to Needs backport from main in 1.14.1 Jul 26, 2023
@aanm aanm removed this from Needs backport from main in 1.14.0 Jul 26, 2023
@meyskens
Copy link
Member Author

/test

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.

LGTM, thanks for the changes @meyskens

@mhofstetter mhofstetter removed the request for review from tommyp1ckles July 27, 2023 07:29
This adds the SPIRE connection to cilium status, this then can be used
by the CLI tool to surface errors and/or wait for SPIRE to be ready.
If Auth is disabled it will surface the disabled status.

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

Rebased for the conflict with ipcahce changes on main

@meyskens
Copy link
Member Author

/test

@nebril nebril added this to Needs backport from main in 1.14.2 Aug 10, 2023
@nebril nebril removed this from Needs backport from main in 1.14.1 Aug 10, 2023
@youngnick
Copy link
Contributor

The runtime test failed to copy the Ginkgo binary? I'll retest.

/test

@youngnick
Copy link
Contributor

/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 Aug 10, 2023
@ldelossa ldelossa merged commit 1410a66 into cilium:main Aug 11, 2023
59 checks passed
@meyskens meyskens deleted the meyskens/spire-status branch August 21, 2023 07:21
@tklauser tklauser added the backport/author The backport will be carried out by the author of the PR. label Aug 22, 2023
@tklauser
Copy link
Member

@meyskens I've tried to backport this PR to v1.14 as part of the backporter rotation but hit some non-trivial merge conflicts. Would you be ok to backport this yourself given you're more familiar with the code in question? Thanks

@tklauser tklauser mentioned this pull request Aug 22, 2023
22 tasks
@meyskens
Copy link
Member Author

Will do!

@joestringer joestringer 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 Aug 25, 2023
@michi-covalent michi-covalent added this to Backport pending to v1.14 in 1.14.3 Sep 9, 2023
@michi-covalent michi-covalent removed this from Needs backport from main in 1.14.2 Sep 9, 2023
@michi-covalent
Copy link
Contributor

i guess this didn't get moved to backport-done/1.14. trying to figure out how to manually fix it 🧠

@michi-covalent michi-covalent added this to Backport done to v1.14 in 1.14.2 Sep 9, 2023
@michi-covalent michi-covalent removed this from Backport pending to v1.14 in 1.14.3 Sep 9, 2023
@michi-covalent michi-covalent 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 Sep 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/servicemesh GH issues or PRs regarding servicemesh backport/author The backport will be carried out by the author of the PR. backport-done/1.14 The backport for Cilium 1.14.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/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
No open projects
1.14.2
Backport done to v1.14
Development

Successfully merging this pull request may close these issues.

Mutual Auth: cilium status --wait needs to wait for SPIRE installation
9 participants