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

Convert the certificates table into an identities table. #12807

Merged
merged 10 commits into from
Feb 6, 2024
Merged

Convert the certificates table into an identities table. #12807

merged 10 commits into from
Feb 6, 2024

Conversation

markylaing
Copy link
Contributor

In preparation for the addition of the identity and access managment APIs, this PR replaces the certificates table with an identities table. There are no changes to the /1.0/certificates API itself.

@markylaing markylaing self-assigned this Feb 1, 2024
@github-actions github-actions bot added the API Changes to the REST API label Feb 1, 2024
@markylaing
Copy link
Contributor Author

Test failures immediately 😢

I will ping everyone again when I've got the tests to pass.

@markylaing
Copy link
Contributor Author

@tomponline @roosterfish @MusicDin @gabrielmougard Tests passing so ready for review :)

PS: I did run the tests not long before submitting the PR. Then I made a couple of changes thinking they were just cosmetic and everything blew up!

@markylaing markylaing force-pushed the identities-table branch 4 times, most recently from 443703f to 553727d Compare February 2, 2024 12:53
roosterfish
roosterfish previously requested changes Feb 2, 2024
View reviewed changes
Copy link
Contributor

@roosterfish roosterfish left a comment

Choose a reason for hiding this comment

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

Looking good, just one suggestion.

lxd/db/cluster/identities.go Show resolved Hide resolved
lxd/db/cluster/identities.go Outdated Show resolved Hide resolved
@markylaing markylaing force-pushed the identities-table branch 2 times, most recently from a3c320d to 5f202de Compare February 4, 2024 13:23
This was referenced Feb 4, 2024
Closed
Merged
tomponline
tomponline reviewed Feb 6, 2024
View reviewed changes
shared/api/auth.go Outdated Show resolved Hide resolved
tomponline
tomponline reviewed Feb 6, 2024
View reviewed changes
lxd/db/cluster/update.go Outdated Show resolved Hide resolved
tomponline
tomponline reviewed Feb 6, 2024
View reviewed changes
Copy link
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 for the schema update test!

tomponline
tomponline reviewed Feb 6, 2024
View reviewed changes
Copy link
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.

Overall this looks great, my main concern is the auth_type column as it seems to be redundant in place of filtering by (in some cases combinations) of identity type.

@markylaing
shared/api: Adds identity type constants.
0661597 
Signed-off-by: Mark Laing <mark.laing@canonical.com>
@markylaing
lxd/db/cluster: Add schema update.
9e61ed5 
Signed-off-by: Mark Laing <mark.laing@canonical.com>
@markylaing
lxd/db/cluster: Add types for identities and identities_projects tables.
1ae8d43 
Signed-off-by: Mark Laing <mark.laing@canonical.com>
@markylaing
lxd/db/cluster: Runs make update-schema.
d9b286f 
Signed-off-by: Mark Laing <mark.laing@canonical.com>
@markylaing
lxd/db/cluster: Removes generated certificate methods.
efd92d1 
Signed-off-by: Mark Laing <mark.laing@canonical.com>
@markylaing
lxd/db/cluster: Updates certificate methods to call identity methods.
82d0453 
It is useful to refactor certificate methods rather than deleting and
using identity methods for the following reasons:
- The node database still contains a 'certificates' table and the
  Certificate type is used to represent those certificates.
- It leaves handlers for the certificate API unadulterated and easier to
  reason about.

Signed-off-by: Mark Laing <mark.laing@canonical.com>
@markylaing
lxd/db/cluster: Adds test for schema update.
dae9373 
Signed-off-by: Mark Laing <mark.laing@canonical.com>
@markylaing
lxd/db/cluster: Fix lint error (revive: var-naming).
55337ae 
Signed-off-by: Mark Laing <mark.laing@canonical.com>
@markylaing
lxd/db/cluster: Update clustering test to select from identities table.
f35271e 
Signed-off-by: Mark Laing <mark.laing@canonical.com>
@markylaing
lxd/db: Update certificates test.
8e16476 
It is no longer possible to create a certificate without a type.

Signed-off-by: Mark Laing <mark.laing@canonical.com>
@tomponline
Copy link
Member

@markylaing just want to check you've tested this upgrade procedure on a real cluster, because TLS certs are used for trusted intra-cluster comms and want to ensure this isn't going to cause an issue when upgrading clusters.

@markylaing
Copy link
Contributor Author

@markylaing just want to check you've tested this upgrade procedure on a real cluster, because TLS certs are used for trusted intra-cluster comms and want to ensure this isn't going to cause an issue when upgrading clusters.

I admit that I have not... I will do so now.

It would be ideal if we could write a test for this somehow. Maybe the clustering tests could accept a different LXD binary to use for other members (or one could be compiled from the main branch).

@tomponline
Copy link
Member

@markylaing just want to check you've tested this upgrade procedure on a real cluster, because TLS certs are used for trusted intra-cluster comms and want to ensure this isn't going to cause an issue when upgrading clusters.

I admit that I have not... I will do so now.

It would be ideal if we could write a test for this somehow. Maybe the clustering tests could accept a different LXD binary to use for other members (or one could be compiled from the main branch).

We have https://github.com/canonical/lxd-ci/blob/main/tests/cluster which could be extended to check auth.

@markylaing
Copy link
Contributor Author

@markylaing just want to check you've tested this upgrade procedure on a real cluster, because TLS certs are used for trusted intra-cluster comms and want to ensure this isn't going to cause an issue when upgrading clusters.

I admit that I have not... I will do so now.
It would be ideal if we could write a test for this somehow. Maybe the clustering tests could accept a different LXD binary to use for other members (or one could be compiled from the main branch).

We have https://github.com/canonical/lxd-ci/blob/main/tests/cluster which could be extended to check auth.

Ah nice, thanks

@markylaing
Copy link
Contributor Author

@markylaing just want to check you've tested this upgrade procedure on a real cluster, because TLS certs are used for trusted intra-cluster comms and want to ensure this isn't going to cause an issue when upgrading clusters.

@tomponline upgrades working fine in a cluster. They of course need to be upgraded at the same time as the schema is different. After the upgrade I am able to move instances around and target particular members.

@tomponline tomponline merged commit f3383ec into canonical:main Feb 6, 2024
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomponline tomponline tomponline approved these changes

@gabrielmougard gabrielmougard Awaiting requested review from gabrielmougard

@MusicDin MusicDin Awaiting requested review from MusicDin

@masnax masnax Awaiting requested review from masnax

@roosterfish roosterfish Awaiting requested review from roosterfish

Assignees

@markylaing markylaing

Labels
API Changes to the REST API
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@markylaing @tomponline @roosterfish @masnax