Image registries client-side changes - #18203
Conversation
1590c30 to
c4f78d2
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds the client-side and shared-API plumbing needed for LXD's upcoming image registries feature, so the CLI and Go client can describe image-registry sources before the server-side API lands.
Changes:
- Adds new shared API types/fields for image registries and image-registry-based image sources.
- Extends the Go client with image registry CRUD/list methods and image-copy support for server-side registry resolution.
- Updates
lxc init,launch,rebuild, andimage copyto route image download operations through image registries when the target server advertises support.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
shared/api/instance.go |
Adds image_registry to instance image sources. |
shared/api/image.go |
Adds registry-related image source fields, including copy_aliases. |
shared/api/image_registry.go |
Introduces image registry API structs and transitional SimpleStreams helpers. |
shared/api/image_registry_test.go |
Adds unit tests for transitional registry helper functions. |
lxc/utils.go |
Adds helper logic for resolving registry-backed image sources. |
lxc/rebuild.go |
Updates rebuild to use registry-aware image resolution. |
lxc/launch.go |
Updates launch help text for registry-aware image sourcing. |
lxc/init.go |
Updates init/create flow to support server-side registry resolution and fallback logic. |
lxc/image.go |
Updates image copy/info flows for image registries. |
lxc/config/remote.go |
Adds unchecked remote parsing to allow registry names. |
doc/rest-api.yaml |
Adds OpenAPI schema entries for image registry types and fields. |
client/lxd.go |
Adjusts source image connection handling for server-side registry resolution. |
client/lxd_instances.go |
Marks legacy source fields deprecated in instance copy/rebuild paths. |
client/lxd_images.go |
Adds server-side registry-aware image copy handling. |
client/lxd_image_registries.go |
Adds new ProtocolLXD image registry client methods. |
client/interfaces.go |
Extends the client interfaces and copy args for image registries. |
c4f78d2 to
8255e18
Compare
8255e18 to
03b99b8
Compare
03b99b8 to
72ceda1
Compare
| GetImageRegistryNames() (names []string, err error) | ||
| GetImageRegistryImages(name string) (images []api.Image, err error) | ||
| GetImageRegistries() (imageRegistries []api.ImageRegistry, err error) | ||
| CreateImageRegistry(imageRegistry api.ImageRegistriesPost) (err error) |
There was a problem hiding this comment.
Lets future proof these by making all mutate actions return an Operation
| // server's own local image store (e.g., when copying between projects). | ||
|
|
||
| // Enforce the pull mode for image download. | ||
| if args.Mode != "pull" { |
There was a problem hiding this comment.
Is args.Mode required for local images (i.e when args.ImageRegistry is empty?)
There was a problem hiding this comment.
Is
args.Moderequired for local images (i.e whenargs.ImageRegistryis empty?)
No, it's not required for local images, good catch.
There was a problem hiding this comment.
I've fixed the condition.
| // resolveRegistryImageSource determines the image source when the target server supports image registries. | ||
| // For local copies (same server), it resolves the source project and returns an empty registry name. | ||
| // For remote copies, it returns the image remote as the registry name. | ||
| func resolveRegistryImageSource(conf *config.Config, imgRemote string, imgRef string, instRemote string, projectOverride string) (imgInfo *api.Image, registryName string) { |
There was a problem hiding this comment.
If we dont modify config.Config does it need to be a pointer?
There was a problem hiding this comment.
It doesn't need to be a pointer, true. Now that I'm thinking, perhaps we shouldn't even be passing the whole config, and instead we can pass only the map conf.Remotes.
There was a problem hiding this comment.
agreed, reduce the size of the API scope where possible.
| // resolveRegistryImageSource determines the image source when the target server supports image registries. | ||
| // For local copies (same server), it resolves the source project and returns an empty registry name. | ||
| // For remote copies, it returns the image remote as the registry name. | ||
| func resolveRegistryImageSource(conf *config.Config, imgRemote string, imgRef string, instRemote string, projectOverride string) (imgInfo *api.Image, registryName string) { |
There was a problem hiding this comment.
Please can we have a unit test for this function.
| return err | ||
| } | ||
| // Parse source remote without validating its existence in the local config. | ||
| remoteName, name := c.global.conf.ParseRemoteUnchecked(args[0]) |
There was a problem hiding this comment.
Can we call this only inside the if destinationServer.HasExtension("image_registries") block and then use c.global.conf.ParseRemote for the fallback block for that the remote name is still validated in that case?
| return err | ||
| if sourceServer != nil { | ||
| // Resolve any alias and then grab the image information from the source | ||
| imgInfo, _, err = c.image.dereferenceAlias(sourceServer, imageType, name) |
There was a problem hiding this comment.
this section seems to override the imgInfo.Type = imageType line you added above?
There was a problem hiding this comment.
maybe the imgInfo.Type = imageType should be in an else after this block?
| progress.Done("Image copied successfully!") | ||
|
|
||
| // If using an image registry, the aliases are handled server-side. | ||
| if sourceServer == nil { |
There was a problem hiding this comment.
what happens if the sourceServer is nil and the target server doesn't support image_registries extension, is this validated earlier on?
There was a problem hiding this comment.
sourceServer is only set to nil inside the if destinationServer.HasExtension("image_registries") block. The else branch always populates sourceServer via GetImageServer() function, which either succeeds or returns an error.
So yes, this is essentially validated earlier on.
| } | ||
| } | ||
|
|
||
| // Store the fingerprint for use when creating aliases later (as imgInfo.Fingerprint may be overridden) |
There was a problem hiding this comment.
can this alias section be moved down after the the function has returned if using a target with image_registry support?
There was a problem hiding this comment.
This section is needed for both paths. It builds the copyArgs variable which is used in both cases (including its Aliases field). There's an early return already on lines 316-319 that skips the legacy only part:
// If using an image registry, the aliases are handled server-side.
if sourceServer == nil {
return nil
}| with --project and the instance remote with --target-project (if different from --project). | ||
|
|
||
| If the destination LXD server supports image registries, the source image | ||
| must be from an image registry or a local store.`) |
There was a problem hiding this comment.
"or a local store" suggests there can be multiple local stores.
Also its not clear what is "local" in this case.
Please can you clarify, something like:
If the destination LXD remote supports image registries, the source image must be from an image registry or available locally on the destination remote.
There was a problem hiding this comment.
Also can you update other places you've updated with similar messages. Thanks
Finally we should avoid saying "LXD server" in this case, and use "LXD remote", because the remote may be a LXD cluster and not a single server.
| } | ||
| } else { | ||
| // Fetch image info from the given remote (legacy client-side resolution path). | ||
| // Normalize empty remote to the default remote, since ParseRemoteUnchecked |
There was a problem hiding this comment.
Like my earlier comment, I wonder if we should only be using ParseRemoteUnchecked inside the if d.HasExtension("image_registries") { block and in the fallback section using conf.ParseRemote?
There was a problem hiding this comment.
I wonder if we should only be using
ParseRemoteUncheckedinside theif d.HasExtension("image_registries") {block and in the fallback section usingconf.ParseRemote?
Hm, yes, I think that's a better approach
|
|
||
| if conf.Remotes[iremote].Protocol != "simplestreams" { | ||
| // Only perform legacy type and protocol checks if we are NOT using an image registry. | ||
| if imgRemoteServer != nil && conf.Remotes[legacyRemote].Protocol != "simplestreams" { |
There was a problem hiding this comment.
As you've defined constants for "simplestreams" now, we should use them where possible.
cddf9c4 to
7ee55a1
Compare
Signed-off-by: Nikita Mezhenskyi <nikita.mezhenskyi@canonical.com>
…e registries This commit introduces a new field `ImageRegistry` to the `ImageSource` struct. Also, it introduces a new field `CopyAliases` to the `ImagesPostSource` struct. These changes are needed to allow LXD to use image registries for image related operations. Signed-off-by: Nikita Mezhenskyi <nikita.mezhenskyi@canonical.com>
…ries This commit introduces a new field `ImageRegistry` to the `InstanceSource` struct. This change is needed to allow LXD to use image registries when creating instances. Signed-off-by: Nikita Mezhenskyi <nikita.mezhenskyi@canonical.com>
Signed-off-by: Nikita Mezhenskyi <nikita.mezhenskyi@canonical.com>
Signed-off-by: Nikita Mezhenskyi <nikita.mezhenskyi@canonical.com>
…age registries Signed-off-by: Nikita Mezhenskyi <nikita.mezhenskyi@canonical.com>
…ificate fields Signed-off-by: Nikita Mezhenskyi <nikita.mezhenskyi@canonical.com>
…and Certificate fields Signed-off-by: Nikita Mezhenskyi <nikita.mezhenskyi@canonical.com>
Signed-off-by: Nikita Mezhenskyi <nikita.mezhenskyi@canonical.com>
Signed-off-by: Nikita Mezhenskyi <nikita.mezhenskyi@canonical.com>
Signed-off-by: Nikita Mezhenskyi <nikita.mezhenskyi@canonical.com>
Signed-off-by: Nikita Mezhenskyi <nikita.mezhenskyi@canonical.com>
Signed-off-by: Nikita Mezhenskyi <nikita.mezhenskyi@canonical.com>
Signed-off-by: Nikita Mezhenskyi <nikita.mezhenskyi@canonical.com>
…l fields Signed-off-by: Nikita Mezhenskyi <nikita.mezhenskyi@canonical.com>
…ailable Signed-off-by: Nikita Mezhenskyi <nikita.mezhenskyi@canonical.com>
Signed-off-by: Nikita Mezhenskyi <nikita.mezhenskyi@canonical.com>
Signed-off-by: Nikita Mezhenskyi <nikita.mezhenskyi@canonical.com>
Signed-off-by: Nikita Mezhenskyi <nikita.mezhenskyi@canonical.com>
Signed-off-by: Nikita Mezhenskyi <nikita.mezhenskyi@canonical.com>
Signed-off-by: Nikita Mezhenskyi <nikita.mezhenskyi@canonical.com>
Signed-off-by: Nikita Mezhenskyi <nikita.mezhenskyi@canonical.com>
Signed-off-by: Nikita Mezhenskyi <nikita.mezhenskyi@canonical.com>
…ring This is needed because image source for `lxc init`, `lxc launch`, and `lxc rebuild` commands is now parsed using `ParseRemoteUnchecked` function, which returns an empty string as image remote if it's not explicitly specified by user. Signed-off-by: Nikita Mezhenskyi <nikita.mezhenskyi@canonical.com>
Signed-off-by: Nikita Mezhenskyi <nikita.mezhenskyi@canonical.com>
7ee55a1 to
379930f
Compare
This PR introduces the client-side changes needed to make use of the new image registries feature for image download operations. It also defines new API structs for image registries and new
ProtocolLXDclient methods for interacting with image registry API.This is the first PR in a series to introduce image registries and update LXD's image sourcing functionality.
The actual image registry API for LXD is introduced in the following PRs.