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

auth: Add entitlements to LXD entities (part 2: Enrich LXD resources with entitlements) #14748

Open
wants to merge 30 commits into
base: main
Choose a base branch
from

Conversation

gabrielmougard
Copy link
Contributor

@gabrielmougard gabrielmougard commented Jan 6, 2025

This is the second part of a group of three stacked PRs.

test/suites/auth.sh Fixed Show fixed Hide fixed
test/suites/auth.sh Fixed Show fixed Hide fixed
test/suites/auth.sh Fixed Show fixed Hide fixed
test/suites/auth.sh Fixed Show fixed Hide fixed
@gabrielmougard gabrielmougard force-pushed the feat/auth-dry-run-check-part3 branch 2 times, most recently from e653ad9 to 7f5457a Compare January 8, 2025 21:09
@gabrielmougard gabrielmougard marked this pull request as ready for review January 8, 2025 21:09
@gabrielmougard gabrielmougard force-pushed the feat/auth-dry-run-check-part3 branch 2 times, most recently from 171b326 to 1a09849 Compare January 9, 2025 13:09
lxd/instance_get.go Outdated Show resolved Hide resolved
lxd/auth/types.go Outdated Show resolved Hide resolved
lxd/instances_get.go Outdated Show resolved Hide resolved
@gabrielmougard gabrielmougard force-pushed the feat/auth-dry-run-check-part3 branch from 1a09849 to e18b874 Compare January 10, 2025 08:44
@gabrielmougard
Copy link
Contributor Author

@minaelee thanks for the feedbacks. Fixing that right now

@gabrielmougard gabrielmougard force-pushed the feat/auth-dry-run-check-part3 branch 4 times, most recently from 6f763e7 to c793358 Compare January 10, 2025 14:28
Copy link
Contributor

@edlerd edlerd left a comment

Choose a reason for hiding this comment

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

Approach and format seems all right from my point of view. Left some comments below.

shared/api/image.go Outdated Show resolved Hide resolved
test/suites/auth.sh Show resolved Hide resolved
gabrielmougard and others added 12 commits January 24, 2025 14:09
This interface will need to be implemented for each API types that need to be returned from the API handlers
with `access_entitlements`

Signed-off-by: Gabriel Mougard <[email protected]>
Co-authored-by: Mark Laing <[email protected]>
…es that are eligible for entitlement enrichment

Signed-off-by: Gabriel Mougard <[email protected]>
Co-authored-by: Mark Laing <[email protected]>
Signed-off-by: Gabriel Mougard <[email protected]>
Signed-off-by: Gabriel Mougard <[email protected]>
Co-authored-by: Mark Laing <[email protected]>
Signed-off-by: Gabriel Mougard <[email protected]>
Co-authored-by: Mark Laing <[email protected]>
This function add entitlements to a map of entity URLs to EntitlementReporters, which
are in practice API types implementing the `ReportEntitlements` method.

Signed-off-by: Gabriel Mougard <[email protected]>
Co-authored-by: Mark Laing <[email protected]>
@gabrielmougard gabrielmougard force-pushed the feat/auth-dry-run-check-part3 branch from eebe53d to 53c6a8e Compare January 24, 2025 14:44
@gabrielmougard
Copy link
Contributor Author

@markylaing updated following the approach you suggested this morning. I also added the enrichment for all the other entities.

@gabrielmougard gabrielmougard force-pushed the feat/auth-dry-run-check-part3 branch from 53c6a8e to 01a6872 Compare January 24, 2025 14:53
Copy link
Contributor

@edlerd edlerd left a comment

Choose a reason for hiding this comment

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

New tests are great, thanks for adding. I skimmed through the code without detailed reading, just one non-blocking observation below. LGTM 👍

return response.SmartError(err)
}

if len(withEntitlements) > 0 && recursion != "1" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if len(withEntitlements) > 0 && recursion != "1" {
if len(withEntitlements) > 0 && !recursion {

Should be more future-proof this way in case higher recursion levels are introduced. wdyt?

return response.SmartError(err)
}

if len(withEntitlements) > 0 && recursion != "1" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to above, there are a couple of more occurrences like this below.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Changes to the REST API Documentation Documentation needs updating
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants