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

Auth: Untangle auth entity types and functions from the driver implementations #13551

Merged
merged 26 commits into from
Jun 6, 2024
Merged

Auth: Untangle auth entity types and functions from the driver implementations #13551

merged 26 commits into from
Jun 6, 2024

Conversation

tomponline
Copy link
Member

@tomponline tomponline commented Jun 5, 2024 •
edited
Loading

This avoids the driver implementation dependencies (such as openfga) being built where they are not needed.

This stops openfga packages being used by the lxd-agent binary (which we previously avoided using build tags workaround).

Also bumps Go to 1.22.3 and updates go mod dependencies.

@tomponline tomponline self-assigned this Jun 5, 2024
@github-actions github-actions bot added the Documentation Documentation needs updating label Jun 5, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Jun 5, 2024

Heads up @ru-fu - the "Documentation" label was applied to this issue.

simondeziel
simondeziel previously approved these changes Jun 5, 2024
View reviewed changes
@simondeziel
Copy link
Member

LGTM in principle. The lxd-agent dep list needs a slight update for the new deps. The mini-oidc seems to need to be switched from "github.com/go-jose/go-jose/v3" to "github.com/go-jose/go-jose/v4".

@tomponline tomponline force-pushed the main branch 2 times, most recently from 77a2e82 to a4f8db0 Compare June 6, 2024 10:38
@tomponline tomponline changed the title Go: Bump to 1.22.3 as min version and update dependencies Auth: Untangles auth entity types and functions from the driver implementations Jun 6, 2024
@tomponline tomponline force-pushed the main branch 6 times, most recently from dc092fb to 27989a5 Compare June 6, 2024 13:52
@tomponline
lxd/auth/entity: Moved generated entitlements and some helper types/f…
356a6dd 
…unctions into own package

To avoid unnecessary dependencies elsewhere.

Signed-off-by: Thomas Parrott <thomas.parrott@canonical.com>
@tomponline
lxd/dev/openfga: Move openfga datastore to own package to avoid unnec…
8d59b53 
…essary dependencies elsewhere

Signed-off-by: Thomas Parrott <thomas.parrott@canonical.com>
@tomponline
lxd/daemon: Use github.com/canonical/lxd/lxd/db/openfga
00cd18f 
Signed-off-by: Thomas Parrott <thomas.parrott@canonical.com>
@tomponline
lxd-agent/devlxd: Removes unused reference to daemon.Debug
31cd1ba 
Signed-off-by: Thomas Parrott <thomas.parrott@canonical.com>
@tomponline
lxd: Use github.com/canonical/lxd/lxd/auth/entity
f1e39d2 
Changes references of github.com/canonical/lxd/lxd/auth to github.com/canonical/lxd/lxd/auth/entity

Signed-off-by: Thomas Parrott <thomas.parrott@canonical.com>
@tomponline
go: Update minimum version to 1.22.3
17f57dc 
Signed-off-by: Thomas Parrott <thomas.parrott@canonical.com>
@tomponline
gomod: Update dependencies and switch to go 1.22.3
1b655d1 
Signed-off-by: Thomas Parrott <thomas.parrott@canonical.com>
@tomponline
test/mini-oidc: Merges into main Go module
b7d3d24 
Signed-off-by: Thomas Parrott <thomas.parrott@canonical.com>
@tomponline
test/mini-oidc: Standardise on github.com/go-jose/go-jose/v4
c990938 
Signed-off-by: Thomas Parrott <thomas.parrott@canonical.com>
@tomponline
gomod: Update depedencies after test/mini-oidic is merged into main m…
3057ad1 
…odule

Signed-off-by: Thomas Parrott <thomas.parrott@canonical.com>
@tomponline
lxd/auth/generate: Update entity generator to use new location and na…
281c4a4 
…me changes

Signed-off-by: Thomas Parrott <thomas.parrott@canonical.com>
@tomponline
test/godeps: Removes openfga from expected deps for lxd-agent
0f91e16 
And updates for other change.

Signed-off-by: Thomas Parrott <thomas.parrott@canonical.com>
@tomponline
github: Updated expected size of lxd-agent
af3ca06 
Signed-off-by: Thomas Parrott <thomas.parrott@canonical.com>
@tomponline
lxd/storage/volumes/snapshot: Fix duplicate import
51a7dee 
Signed-off-by: Thomas Parrott <thomas.parrott@canonical.com>
@tomponline
lxd/events: Fix go linter error
09187e9 
Signed-off-by: Thomas Parrott <thomas.parrott@canonical.com>
@tomponline
test/mini-oidc/storage: Ignore exported func NewMultiStorage returns …
a29a27f 
…unexported type *storage.multiStorage, which can be annoying to use linter warning

Signed-off-by: Thomas Parrott <thomas.parrott@canonical.com>
@tomponline
test/mini-oidc/storage: Return standard error rather than pointer to …
7732924 
…oidc.Error

Signed-off-by: Thomas Parrott <thomas.parrott@canonical.com>
@tomponline
lxd/api/internal/recover: Ignore defer: prefer not to defer inside lo…
9cbd77e 
…ops (revive) as this is intended

As per comment.

Signed-off-by: Thomas Parrott <thomas.parrott@canonical.com>
@tomponline
lxd/api/internal/recover: Fix early return from linter suggestion
c7e6d04 
Signed-off-by: Thomas Parrott <thomas.parrott@canonical.com>
@tomponline
lxd/storage/volumes: Remove empty new line
b0cd7fd 
Signed-off-by: Thomas Parrott <thomas.parrott@canonical.com>
@tomponline tomponline force-pushed the main branch 4 times, most recently from 10bc181 to f4ffb34 Compare June 6, 2024 15:15
@tomponline
test/mini-oidc/storage/storage: linter fixes
cf34f13 
Signed-off-by: Thomas Parrott <thomas.parrott@canonical.com>
@tomponline
lxd/api/internal/recover: Fix early-return from linter
a24233b 
Signed-off-by: Thomas Parrott <thomas.parrott@canonical.com>
@tomponline
test/mini-oidc/storage: Fix import spacing
d581e43 
Signed-off-by: Thomas Parrott <thomas.parrott@canonical.com>
@tomponline tomponline marked this pull request as ready for review June 6, 2024 15:29
simondeziel
simondeziel approved these changes Jun 6, 2024
View reviewed changes
Copy link
Member

@simondeziel simondeziel left a comment

Choose a reason for hiding this comment

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

I reviewed the non-Go bits and it LGTM, thanks!

.github/workflows/tests.yml Show resolved Hide resolved
@tomponline
Copy link
Member Author

Ill merge for now and can change if mark sees some issues

@tomponline tomponline merged commit bb4b554 into canonical:main Jun 6, 2024
29 checks passed
@tomponline tomponline changed the title Auth: Untangles auth entity types and functions from the driver implementations Auth: Untangle auth entity types and functions from the driver implementations Jun 6, 2024
@markylaing
Copy link
Contributor

@tomponline what was the import path that pulled the lxd/auth package into lxd-agent? The change looks good in general (e.g. moving openfga stuff into separate packages) but I'm sure about adding another package called entity.

For the sake of import naming I would consider a pattern more like we have in lxd/firewall. That would keep the definitions and generated entitlements under auth and the openfga stuff under a drivers package.

@tomponline
Copy link
Member Author

For the sake of import naming I would consider a pattern more like we have in lxd/firewall. That would keep the definitions and generated entitlements under auth and the openfga stuff under a drivers package.

I agree, but didn't really have time to start a wide ranging refactor of how auth was implemented - but not against your proposal at all.

IIRC correctly lxd-agent was pulling in auth package by way of events and operations, and possibly indirectly via state.

@markylaing
Copy link
Contributor

I agree, but didn't really have time to start a wide ranging refactor of how auth was implemented - but not against your proposal at all.

Yeah fair enough. I'm happy to make an attempt. It would lead to fewer merge conflicts when rebasing #13262

@tomponline
Copy link
Member Author

tomponline commented Jun 10, 2024 •
edited
Loading

I agree, but didn't really have time to start a wide ranging refactor of how auth was implemented - but not against your proposal at all.

Yeah fair enough. I'm happy to make an attempt. It would lead to fewer merge conflicts when rebasing #13262

Thanks. As long as the lxd-agent import list remains as it is, or gets smaller :)

markylaing added a commit to markylaing/lxd that referenced this pull request Jun 12, 2024
@markylaing
lxd/auth: Move Authorizer implementations into drivers package.
3160035 
Additionally, move entitlement definitions out of `auth/entity` and
back into the `auth` package. This performs the same function as canonical#13551
but has more intuitive package naming.

Signed-off-by: Mark Laing <mark.laing@canonical.com>
@markylaing markylaing mentioned this pull request Jun 12, 2024
Merged
tomponline pushed a commit to tomponline/lxd that referenced this pull request Jun 13, 2024
@markylaing @tomponline
lxd/auth: Move Authorizer implementations into drivers package.
3b22fd4 
Additionally, move entitlement definitions out of `auth/entity` and
back into the `auth` package. This performs the same function as canonical#13551
but has more intuitive package naming.

Signed-off-by: Mark Laing <mark.laing@canonical.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simondeziel simondeziel simondeziel approved these changes

@markylaing markylaing Awaiting requested review from markylaing

Assignees

@tomponline tomponline

Labels
Documentation Documentation needs updating
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tomponline @simondeziel @markylaing