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

API: Add support for fetching images across all projects (from Incus) #14260

Conversation

kadinsayani
Copy link
Contributor

@kadinsayani kadinsayani commented Oct 10, 2024

Resolves #14126.

This PR adds support for fetching images across all projects by adding a query parameter all-projects.

@github-actions github-actions bot added Documentation Documentation needs updating API Changes to the REST API labels Oct 10, 2024
Copy link

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

@kadinsayani kadinsayani force-pushed the 14126-add-all-projects-support-to-fetch-images branch from 80348bf to 9c8ceab Compare October 11, 2024 00:07
lxd/images.go Outdated Show resolved Hide resolved
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.

The all-projects parameter should only change which resources are returned, it should not change the output format.

@kadinsayani kadinsayani force-pushed the 14126-add-all-projects-support-to-fetch-images branch from 9c8ceab to a949a09 Compare October 28, 2024 16:26
@kadinsayani kadinsayani marked this pull request as draft October 28, 2024 16:27
@kadinsayani kadinsayani changed the title Add support for fetching images across all projects WIP: Add support for fetching images across all projects (from Incus) Oct 28, 2024
@kadinsayani kadinsayani force-pushed the 14126-add-all-projects-support-to-fetch-images branch from a949a09 to 5308878 Compare October 29, 2024 09:45
@kadinsayani kadinsayani changed the title WIP: Add support for fetching images across all projects (from Incus) Add support for fetching images across all projects (from Incus) Oct 29, 2024
@kadinsayani kadinsayani marked this pull request as ready for review October 29, 2024 09:49
@kadinsayani
Copy link
Contributor Author

This PR should be good to go. It adds support for the --all-projects flag when running lxc image list. I'm planning to open separate PR's for --all-projects support for network zones, profiles, networks, network-acls, and storage buckets (Incus cherry picks).

Copy link
Contributor

@markylaing markylaing left a comment

Choose a reason for hiding this comment

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

Can you please add something to reduce the number of images returned by the database when the caller is not authenticated?

Other than that I just have a few questions :)

lxd/images.go Outdated Show resolved Hide resolved
client/interfaces.go Outdated Show resolved Hide resolved
lxc/image.go Outdated Show resolved Hide resolved
@kadinsayani kadinsayani force-pushed the 14126-add-all-projects-support-to-fetch-images branch 4 times, most recently from 53c78dc to 4b4ae6b Compare November 8, 2024 16:23
Copy link
Contributor

@markylaing markylaing left a comment

Choose a reason for hiding this comment

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

Just a few nits that should be easy to fix :)

lxd/images.go Outdated Show resolved Hide resolved
lxd/images.go Outdated Show resolved Hide resolved
client/lxd_images.go Outdated Show resolved Hide resolved
client/lxd_images.go Outdated Show resolved Hide resolved
@kadinsayani kadinsayani force-pushed the 14126-add-all-projects-support-to-fetch-images branch 3 times, most recently from 79816ce to 3722085 Compare November 8, 2024 18:53
@kadinsayani
Copy link
Contributor Author

Just a few nits that should be easy to fix :)

Thanks for the review @markylaing :) I think I've addressed everything now.

Copy link
Contributor

@markylaing markylaing left a comment

Choose a reason for hiding this comment

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

Just a couple of last bits. Thanks :)

lxd/images.go Show resolved Hide resolved
lxd/images.go Outdated Show resolved Hide resolved
@kadinsayani kadinsayani force-pushed the 14126-add-all-projects-support-to-fetch-images branch from 3722085 to 3a1f0a2 Compare November 11, 2024 18:38
@kadinsayani
Copy link
Contributor Author

Rebased and good to go 👍

doc/api-extensions.md Outdated Show resolved Hide resolved
Copy link
Contributor

@markylaing markylaing left a comment

Choose a reason for hiding this comment

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

Please double check the API extension commit and cherry-pick if it was added to Incus (splitting the doc change if necessary). Other than that, LGTM. cc @tomponline.

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.

There are no tests, we especially need tests to check access control of images.

MaheshPunjabi and others added 7 commits November 22, 2024 11:14
Signed-off-by: Mahesh Punjabi <mahesh.punjabi86@gmail.com>
(cherry picked from commit 09576c289f57dc657ad99ef4a9a2fc5630d78007)
Signed-off-by: Kadin Sayani <kadin.sayani@canonical.com>
License: Apache-2.0
Signed-off-by: Mahesh Punjabi <mahesh.punjabi86@gmail.com>
(cherry picked from commit 087e0b3701787e9ab237d57321aff329b706295b)
Signed-off-by: Kadin Sayani <kadin.sayani@canonical.com>
License: Apache-2.0
Signed-off-by: Mahesh Punjabi <mahesh.punjabi86@gmail.com>
(cherry picked from commit a3544074d69dd2438986311d2996a408deb5ad2e)
Signed-off-by: Kadin Sayani <kadin.sayani@canonical.com>
License: Apache-2.0
Signed-off-by: Kadin Sayani <kadin.sayani@canonical.com>
Signed-off-by: Mahesh Punjabi <mahesh.punjabi86@gmail.com>
(cherry picked from commit 34ae1bd35a1ea79fb78b06dd0de7eb5f70ccb4a6)
Signed-off-by: Kadin Sayani <kadin.sayani@canonical.com>
License: Apache-2.0
Signed-off-by: Kadin Sayani <kadin.sayani@canonical.com>
Signed-off-by: Mahesh Punjabi <mahesh.punjabi86@gmail.com>
(cherry picked from commit f5c5a8170106e94ed314f5761b4b91d9621acc65)
Signed-off-by: Kadin Sayani <kadin.sayani@canonical.com>
License: Apache-2.0
@kadinsayani kadinsayani force-pushed the 14126-add-all-projects-support-to-fetch-images branch 4 times, most recently from c202961 to 7ad4209 Compare November 22, 2024 18:51
…ojects`

This commit adds tests to `test/suites/auth.sh` for fetching images
across all projects.

Signed-off-by: Kadin Sayani <kadin.sayani@canonical.com>
Signed-off-by: Kadin Sayani <kadin.sayani@canonical.com>
Signed-off-by: Kadin Sayani <kadin.sayani@canonical.com>
@kadinsayani kadinsayani force-pushed the 14126-add-all-projects-support-to-fetch-images branch from 7ad4209 to 0a73c3e Compare November 22, 2024 19:10
@kadinsayani
Copy link
Contributor Author

There are no tests, we especially need tests to check access control of images.

Tests added! 😄

@kadinsayani kadinsayani changed the title Add support for fetching images across all projects (from Incus) API: Add support for fetching images across all projects (from Incus) Nov 26, 2024
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!

@tomponline tomponline merged commit f8ea6e0 into canonical:main Nov 29, 2024
25 of 26 checks passed
@kadinsayani kadinsayani deleted the 14126-add-all-projects-support-to-fetch-images branch November 29, 2024 15:39
tomponline added a commit that referenced this pull request Nov 30, 2024
Fixes #14561 introduced with
#14260.

This PR fixes a regression introduced with
#14260 by adding
`GetImagesAllProjects` and
`GetImagesAllProjectsWithFilter` functions to
`client/simplestreams_images.go`, and moving their method signatures to
the `ImageServer` interface. While adding support for fetching images
across all projects, we changed the logic in the `lxc image list`
command to call different functions based on whether the
`--all-projects` flag is passed in or not:


1fae6e6

This is fine, but we needed to add `simplestreams` protocol support as
well to facilitate listing public images from remotes such as `images`
and `ubuntu-minimal-daily`.

The actual bug comes from the following LOC: 


https://github.com/canonical/lxd/blob/4a3c4a343e4a9490105bd3da893f9892b86e01d8/lxc/image.go#L1356-L1359

Fetching from an instance server isn't applicable for `simplestreams`
servers, so `GetImagesAllProjects` is never reached due to the error
raised by `c.global.conf.GetInstanceServer`. Instead, we should fetch an
image server before fetching images, hence why moving the new client
functions method signatures to the `ImageServer` interface fixes the
regression.
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.

Add all projects support to fetch images
4 participants