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

Implement Worker Certificates Renewal Endpoints #565

Merged
merged 21 commits into from
Aug 12, 2024

Conversation

mateoflorido
Copy link
Member

Overview

Implement the required endpoints for refreshing certificates for Kubernetes worker nodes.

Rationale

As stated in Proposal 002. We need to provide two endpoints to allow certificates refreshes in the cluster. This pull request implements the refresh-certs/plan and the refresh-certs/run for refreshing the worker nodes' certificates.

Testing

This pull request has been tested manually. Automated testing will be added in a future pull request.

root@kw-1:~# curl --unix-socket /var/snap/k8s/common/var/lib/k8sd/state/control.socket http://localhost/1.0/k8sd/refresh-certs/plan -XPOST
{"type":"sync","status":"Success","status_code":200,"operation":"","error_code":0,"error":"","metadata":{"seed":1978322463,"certificates_signing_requests":["k8sd-1978322463-worker-kubelet-serving","k8sd-1978322463-worker-kubelet-client","k8sd-1978322463-worker-kube-proxy-client"]}}
root@kw-1:~# curl --unix-socket /var/snap/k8s/common/var/lib/k8sd/state/control.socket http://localhost/1.0/k8sd/refresh-certs/run -XPOST -H "Content-Type: application/json" --data '{"seed":1978322463, "expiration_seconds": 10}'
{"type":"error","status":"","status_code":0,"operation":"","error_code":500,"error":"CSR k8sd-1978322463-worker-kubelet-serving is not issued","metadata":null}
root@kw-1:~# curl --unix-socket /var/snap/k8s/common/var/lib/k8sd/state/control.socket http://localhost/1.0/k8sd/refresh-certs/run -XPOST -H "Content-Type: application/json" --data '{"seed":1978322463, "expiration_seconds": 10}'
{"type":"sync","status":"Success","status_code":200,"operation":"","error_code":0,"error":"","metadata":null}

@mateoflorido mateoflorido marked this pull request as ready for review July 25, 2024 13:06
@mateoflorido mateoflorido requested a review from a team as a code owner July 25, 2024 13:06
bschimke95
bschimke95 previously approved these changes Jul 25, 2024
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.

Amazing work @mateoflorido

Added a couple of comments.

src/k8s/pkg/k8sd/api/certs_refresh.go Outdated Show resolved Hide resolved
src/k8s/pkg/k8sd/api/certs_refresh.go Outdated Show resolved Hide resolved
src/k8s/pkg/k8sd/api/certs_refresh.go Outdated Show resolved Hide resolved
src/k8s/pkg/k8sd/api/certs_refresh.go Outdated Show resolved Hide resolved
src/k8s/pkg/k8sd/api/certs_refresh.go Outdated Show resolved Hide resolved
src/k8s/pkg/k8sd/api/certs_refresh.go Outdated Show resolved Hide resolved
src/k8s/pkg/k8sd/api/certs_refresh.go Outdated Show resolved Hide resolved
src/k8s/pkg/k8sd/api/certs_refresh.go Outdated Show resolved Hide resolved
src/k8s/pkg/k8sd/api/certs_refresh.go Outdated Show resolved Hide resolved
src/k8s/pkg/utils/pki/generate.go Outdated Show resolved Hide resolved
@bschimke95 bschimke95 self-requested a review July 25, 2024 14:48
@bschimke95 bschimke95 dismissed their stale review July 25, 2024 14:49

accidentally approved

@mateoflorido mateoflorido requested a review from neoaggelos July 29, 2024 13:55
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.

another pass

src/k8s/api/v1/certificates_refresh.go Outdated Show resolved Hide resolved
src/k8s/pkg/k8sd/api/certs_refresh.go Outdated Show resolved Hide resolved
src/k8s/pkg/k8sd/api/certs_refresh.go Outdated Show resolved Hide resolved
src/k8s/pkg/k8sd/api/certs_refresh.go Outdated Show resolved Hide resolved
src/k8s/pkg/utils/file_operation.go Outdated Show resolved Hide resolved
src/k8s/pkg/utils/file_operation.go Outdated Show resolved Hide resolved
src/k8s/pkg/utils/file_operation.go Outdated Show resolved Hide resolved
Copy link
Contributor

@neoaggelos neoaggelos left a comment

Choose a reason for hiding this comment

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

Did a first pass. Good work overall, but see the requested changes

src/k8s/pkg/client/kubernetes/csr.go Outdated Show resolved Hide resolved
src/k8s/pkg/k8sd/api/certs_refresh.go Outdated Show resolved Hide resolved
src/k8s/pkg/k8sd/api/certs_refresh.go Outdated Show resolved Hide resolved
src/k8s/pkg/k8sd/api/certs_refresh.go Outdated Show resolved Hide resolved
src/k8s/pkg/utils/file_operation.go Outdated Show resolved Hide resolved
@mateoflorido mateoflorido force-pushed the KU-1130/worker-certs-plan branch 2 times, most recently from e5111ec to 15e1b3a Compare July 31, 2024 00:03
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, great work

src/k8s/api/v1/certificates_refresh.go Outdated Show resolved Hide resolved
src/k8s/pkg/k8sd/api/certs_refresh.go Outdated Show resolved Hide resolved
src/k8s/pkg/k8sd/api/certs_refresh.go Outdated Show resolved Hide resolved
src/k8s/pkg/k8sd/api/certs_refresh.go Outdated Show resolved Hide resolved
src/k8s/pkg/k8sd/api/certs_refresh.go Outdated Show resolved Hide resolved
src/k8s/pkg/k8sd/api/certs_refresh.go Outdated Show resolved Hide resolved
src/k8s/pkg/k8sd/api/certs_refresh.go Outdated Show resolved Hide resolved
@mateoflorido mateoflorido requested a review from neoaggelos July 31, 2024 16:50
src/k8s/pkg/k8sd/api/certs_refresh.go Outdated Show resolved Hide resolved
src/k8s/pkg/k8sd/api/certs_refresh.go Outdated Show resolved Hide resolved
src/k8s/pkg/k8sd/api/certs_refresh.go Outdated Show resolved Hide resolved
@mateoflorido mateoflorido requested a review from neoaggelos August 1, 2024 14:01
Copy link
Contributor

@neoaggelos neoaggelos left a comment

Choose a reason for hiding this comment

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

Final comments, great work @mateoflorido !

src/k8s/pkg/k8sd/api/certs_refresh.go Outdated Show resolved Hide resolved
src/k8s/pkg/k8sd/api/certs_refresh.go Outdated Show resolved Hide resolved
src/k8s/pkg/k8sd/api/certs_refresh.go Outdated Show resolved Hide resolved
src/k8s/pkg/utils/pki/generate.go Outdated Show resolved Hide resolved
src/k8s/pkg/k8sd/api/certs_refresh.go Outdated Show resolved Hide resolved
src/k8s/pkg/k8sd/api/certs_refresh.go Outdated Show resolved Hide resolved
src/k8s/pkg/k8sd/api/certs_refresh.go Outdated Show resolved Hide resolved
src/k8s/pkg/k8sd/api/certs_refresh.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.

LGTM!

Copy link
Contributor

@neoaggelos neoaggelos left a comment

Choose a reason for hiding this comment

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

Final comments about the watcher restarts. It's OK to ignore the comment to start with and just a leave a NOTE if it becomes a problem in the future, rather than having huge functions, as they come with very big cyclomatic complexity

src/k8s/pkg/k8sd/api/certificates_refresh.go Outdated Show resolved Hide resolved
src/k8s/pkg/k8sd/api/certificates_refresh.go Outdated Show resolved Hide resolved
src/k8s/pkg/k8sd/api/certificates_refresh.go Outdated Show resolved Hide resolved
src/k8s/pkg/k8sd/api/certificates_refresh.go Outdated Show resolved Hide resolved
src/k8s/pkg/k8sd/api/certificates_refresh.go Outdated Show resolved Hide resolved
src/k8s/pkg/k8sd/api/certificates_refresh.go Outdated Show resolved Hide resolved
@mateoflorido mateoflorido requested a review from neoaggelos August 8, 2024 13:34
@mateoflorido mateoflorido requested a review from neoaggelos August 9, 2024 12:49
@mateoflorido mateoflorido force-pushed the KU-1130/worker-certs-plan branch from ec6d809 to b740591 Compare August 9, 2024 18:19
@mateoflorido mateoflorido merged commit 5031a2d into main Aug 12, 2024
19 checks passed
@mateoflorido mateoflorido deleted the KU-1130/worker-certs-plan branch August 12, 2024 14:27
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.

3 participants