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 CAPI endpoints for Certificates Refresh #699

Merged
merged 5 commits into from
Sep 30, 2024

Conversation

mateoflorido
Copy link
Member

Overview

Add new CAPI endpoints to manage certificates refreshes

Rationale

In this pull request, we have added a new set of handlers that allow access from CAPI to the refresh certificates endpoints using the node token as the authentication mechanism. Additionally, we have modified the handler to return the certificate refresh response earlier, deferring the restart until after the response has been flushed. Also, added a couple of drive-by fixes related to the RPC endpoints for Certificates Refreshes.

@mateoflorido mateoflorido requested a review from a team as a code owner September 25, 2024 22:57
Copy link
Contributor

@HomayoonAlimohammadi HomayoonAlimohammadi left a comment

Choose a reason for hiding this comment

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

Thanks a lot @mateoflorido! Left some minor comments.

src/k8s/pkg/k8sd/api/certificates_refresh.go Outdated Show resolved Hide resolved
src/k8s/pkg/k8sd/api/endpoints.go Outdated Show resolved Hide resolved
Copy link
Contributor

@bschimke95 bschimke95 left a comment

Choose a reason for hiding this comment

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

Great work overall. +1 on Hues comments and please add a short comment on why we wait before restarting. This is IMHO not obvious because k8sd is not restarted as part of RestartControlPlaneServices

src/k8s/pkg/k8sd/api/endpoints.go Outdated Show resolved Hide resolved
@mateoflorido
Copy link
Member Author

@HomayoonAlimohammadi @bschimke95 I have created a new PR for including this endpoints. I will cut a release and update the endpoints once this is merged: canonical/k8s-snap-api#10

Copy link
Member

@berkayoz berkayoz left a comment

Choose a reason for hiding this comment

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

LGTM, one question on the endpoints. These are per-node endpoints that are called right? For any of the control plane(k8sd cluster) we should use capi-auth-token instead of the node token.

Copy link
Contributor

@bschimke95 bschimke95 left a comment

Choose a reason for hiding this comment

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

LGTM, just a final comment. Feel free to ignore if you think this is not a problem

src/k8s/pkg/k8sd/api/certificates_refresh.go Outdated Show resolved Hide resolved
@mateoflorido mateoflorido merged commit 2b9d49a into main Sep 30, 2024
20 checks passed
@mateoflorido mateoflorido deleted the KU-1513/capi-refresh-certs branch September 30, 2024 19:12
evilnick pushed a commit to evilnick/k8s-snap that referenced this pull request Nov 14, 2024
Refactor Certificates Refresh endpoints to flush the response early and restart the services asynchronously
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants