8000
Skip to content

Extract certificate add token metadata from remote member#13733

Merged
tomponline merged 2 commits intocanonical:mainfrom
markylaing:certificate-token-remote-operation
Jul 11, 2024
Merged

Extract certificate add token metadata from remote member#13733
tomponline merged 2 commits intocanonical:mainfrom
markylaing:certificate-token-remote-operation

Conversation

@markylaing
Copy link
Copy Markdown
Contributor

When a certificate add operation is returned by another member, the operation metadata is unmarshalled into a map[string]any. Since there is no hint that the value of Metadata["request"] should be an api.CertificatesPost, the contents will be another map[string]any following json unmarshalling defaults.

So that we do not encounter issues with diverging field names if the api.CertificatesPost type changes, I have opted to marshal the any that is in Metadata["request"], and subsequently unmarshal it into the correct type.

Closes #13732

@markylaing markylaing added the Bug label Jul 10, 2024
@markylaing markylaing self-assigned this Jul 10, 2024
@markylaing markylaing requested a review from tomponline as a code owner July 10, 2024 15:51
@markylaing markylaing marked this pull request as draft July 10, 2024 15:57
Copy link
Copy Markdown
Member
@tomponline tomponline left a comment

Choose a reason for hiding this comment

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

Thanks!

Could we do this wrangling inside certificateTokenValid and have it return a *api.CertificatesPost to avoid the caller having to care anything about how the operation is decoded.

@markylaing markylaing force-pushed the certificate-token-remote-operation branch from 9e3f120 to f300e7f Compare July 10, 2024 16:27
@markylaing markylaing marked this pull request as ready for review July 10, 2024 16:27
@markylaing markylaing requested a review from tomponline July 10, 2024 16:27
@markylaing
Copy link
Copy Markdown
Contributor Author

Thanks!

Could we do this wrangling inside certificateTokenValid and have it return a *api.CertificatesPost to avoid the caller having to care anything about how the operation is decoded.

Done.

I've tested this locally by creating a two node cluster, getting a join token from one member, then using the URL of the other member when adding the remote. Should I also add a test for this somewhere in the clustering suite? Not sure how easy it'll be.

@tomponline
Copy link
Copy Markdown
Member

Should I also add a test for this somewhere in the clustering suite

That would be great if not too involved

Comment thread lxd/certificates.go Outdated
When a certificate add operation is returned by another member, the
operation metadata is unmarshalled into a map[string]any. Since there is
no hint that the value of `Metadata["request"]` should be an
`api.CertificatesPost`, the contents will be another `map[string]any`
following json unmarshalling defaults.

So that we do not encounter issues with diverging field names if the
`api.CertificatesPost` type changes, I have opted to marshal the `any`
that is in `Metadata["request"]`, and subsequently unmarshal it into the
correct type.

Signed-off-by: Mark Laing <mark.laing@canonical.com>
Signed-off-by: Mark Laing <mark.laing@canonical.com>
@markylaing markylaing force-pushed the certificate-token-remote-operation branch from f300e7f to af70982 Compare July 10, 2024 17:27
@markylaing
Copy link
Copy Markdown
Contributor Author

Should I also add a test for this somewhere in the clustering suite

That would be great if not too involved

Ok I'll give it a go tomorrow morning.

Copy link
Copy Markdown
Member
@tomponline tomponline left a comment

Choose a reason for hiding this comment

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

Thanks!

@tomponline tomponline merged commit 5a1ca7d into canonical:main Jul 11, 2024
@markylaing
Copy link
Copy Markdown
Contributor Author

Tests added in #13740

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.

Using a client join token only works when talking to the cluster member that generated it

2 participants

0