lxc: Check the image really exists on the remote before exporting it

[gabrielmougard]

closes #12725

When doing a lxc image export .., dereferencing the provided image name either returns a fingerprint or a human readable alias. Both forms can be fine. However, since this dereferenceAlias operation is not really checking if the image associated with this alias exists on the remote, we were creating a local target based on the same string that a user entered. Most of the time, this targetMeta string was resolved to a fingerprint (e.g, /var/lib/snapd/hostfs/home/gab/d2a43fade9d0055e454a091b2e9bd819f7eb715a3d4d46667644a5784d40172) and this was fine because there is only the fingerprint file created and no intermediate directories.

Anyway, I think it is safer to systematically check for the image existence prior to creating any file resources. This way we avoid changing the filesystem write logic that could annoy exsting users and break their export image pipeline:

target := "."
targetMeta := fingerprint
if len(args) > 1 {
	target = args[1]
	if shared.IsDir(shared.HostPathFollow(args[1])) {
		targetMeta = filepath.Join(args[1], targetMeta)
	} else {
		targetMeta = args[1]
	}
}
targetMeta = shared.HostPathFollow(targetMeta)

[tomponline]

Which call was actually trying to access the local filesystem?

This seems like a sticking plaster rather than fixing the call that tried to access the filesystem incorrectly? I may be misunderstanding.

[gabrielmougard]

Which call was actually trying to access the local filesystem?

This seems like a sticking plaster rather than fixing the call that tried to access the filesystem incorrectly? I may be misunderstanding.

• Initially, when calling fingerprint := c.image.dereferenceAlias(remoteServer, imageType, name), we end up having fingerprint = name (let say that name = ubuntu/foo to take @simondeziel original example) because no fingerprint could be solved as this remote has an incorrect streams/v1/index.json

• Then, when targetMeta := shared.HostPathFollow(fingerprint) is called, we end up having targetMeta = /var/lib/snapd/hostfs/home/gab/ubuntu/foo (because ubuntu/foo does not exist on the remote, it has not been replaced by a fingerprint) instead of /var/lib/snapd/hostfs/home/gab/d2a43fade9d0055e454a091b2e9bd819f7eb715a3d4d46667644a5784d40172f. This, eventually makes the client fail at dest, err := os.Create(targetMeta) because we'd need to first create the folder ubuntu in order to create foo in it.

• Now, I realize that, indeed this PR is not complete and we need to deal with this intermediady folder situation (when the user do not provide an explicit output dir param in the args). Should we then create the missing directory path before calling dest, err := os.Create(targetMeta) ? But I was first asking myself why we weren't checking the remote for image existence first before doing all this (because c.image.dereferenceAlias won't return an explicit error), so that's why I added this remoteServer.GetImage(fingerprint) before.

I don't know if this explanation makes sense :)

[tomponline]

Yes this makes sense now thanks.

So my question now is whether we should change dereferenceAlias to return the same values as GetImage i.e (image *api.Image, ETag string, err error) and then do a call to GetImage inside dereferenceAlias and return an error if the image cannot be dereferenced. This would make all calls to dereferenceAlias safer, and avoid in several cases the caller having to do the GetImage call themselves next anyway.

I'm not too happy to see the errors being ignored in dereferenceAlias as this can have unexpected consequences, such as this.

What do you think?

[gabrielmougard]

Yes this makes sense now thanks.

So my question now is whether we should change dereferenceAlias to return the same values as GetImage i.e (image *api.Image, ETag string, err error) and then do a call to GetImage inside dereferenceAlias and return an error if the image cannot be dereferenced. This would make all calls to dereferenceAlias safer, and avoid in several cases the caller having to do the GetImage call themselves next anyway.

I'm not too happy to see the errors being ignored in dereferenceAlias as this can have unexpected consequences, such as this.

What do you think?

I like this solution. Let's do that.

[tomponline]

Hi

Thanks, but please see #13161 (comment) as we agreed that dereferenceAlias would return the equivalent of GetImage by calling it inside once it had resolved the alias.

Then we would remove extra unnecessary calls to GetImage that occur after it.

[gabrielmougard]

Hi

Thanks, but please see #13161 (comment) as we agreed that dereferenceAlias would return the equivalent of GetImage by calling it inside once it had resolved the alias.

Then we would remove extra unnecessary calls to GetImage that occur after it.

Should be updated

[tomponline]

This is looking a lot clearer thanks

[tomponline]

Thanks!

[simondeziel]

Awesome, thanks!