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

Add capi-auth-token header to /dqlite/remove request #68

Merged
merged 4 commits into from
Oct 11, 2024
Merged

Add capi-auth-token header to /dqlite/remove request #68

merged 4 commits into from
Oct 11, 2024

Conversation

HomayoonAlimohammadi
Copy link

@HomayoonAlimohammadi HomayoonAlimohammadi commented Oct 9, 2024 •
edited
Loading

Summary

This PR adds the capi-auth-token header to the dqlite/remove request.
The <cluster>-capi-auth-token secret is supposed to be created by the Microk8s bootstrap provider. This coupling is (AFAIK) inevitable because we're having them in separate repos and the bootstrap provider is responsible for populating the /capi/etc/token file on the control plane machines.

  • Also the removeEndpoint request field is changed to remove_endpoint to comply with the recent MAAS API guidelines.
  • Minor other refactorings to improve the code structure.

PR series

  1. Cluster agent token validation --> validate capi-auth-token on dqlite/remove microk8s-cluster-agent#56
  2. Microk8s bootstrap provider add token file and secret --> Add capi-auth-token file to control plane machines cluster-api-bootstrap-provider-microk8s#115
  3. Microk8s control plane provider use token for request --> This one

@HomayoonAlimohammadi HomayoonAlimohammadi requested a review from a team October 9, 2024 13:53
This was referenced Oct 9, 2024
Merged
Merged
bschimke95
bschimke95 approved these changes Oct 10, 2024
View reviewed changes
Copy link

@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.

Minor nits, LGTM in general

pkg/token/token.go Outdated Show resolved Hide resolved
pkg/token/token_test.go
@@ -0,0 +1 @@
package token_test
Copy link

Choose a reason for hiding this comment

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

left over?

Copy link
Author

Choose a reason for hiding this comment

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

Nah thanks for pointing this out. Forgot to add the tests. Add the tests both here and in the bootstrap provider.

berkayoz
berkayoz approved these changes Oct 11, 2024
View reviewed changes
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 overall, should we update len(machines) > 1 check to len(machines) > 2 since we shouldn't do this on a 2 node cluster and we are not handling this situation in the cluster agent?

@HomayoonAlimohammadi HomayoonAlimohammadi merged commit 2258ddc into dqlite-remove-endpoint Oct 11, 2024
3 checks passed
@HomayoonAlimohammadi HomayoonAlimohammadi deleted the KU-1719/use-auth-token-in-request-header branch October 11, 2024 10:47
@HomayoonAlimohammadi HomayoonAlimohammadi mentioned this pull request Oct 11, 2024
Merged
HomayoonAlimohammadi added a commit that referenced this pull request Oct 14, 2024
@HomayoonAlimohammadi
Call Dqlite remove endpoint for cleanup (KU-1719) (#69)
ac3d9e3 
* Call node removal endpoint on machine deletion (#66)
* Add pkg to Dockerfile and Fix TLS issue (#67)
* Add capi-auth-token header to /dqlite/remove request (#68)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bschimke95 bschimke95 bschimke95 approved these changes

@berkayoz berkayoz berkayoz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@HomayoonAlimohammadi @berkayoz @bschimke95