Adding a nop-op when trying to check access for run-image against the daemon

[jjbustamante]

Summary

In a previous PR we added a new logic to calculate the run-image according to the spec, but we also introduced an error when trying to check read access for run-images that are saved only in the daemon. In those cases, the checker was returning false and panic was thrown in this method because the runImageList was empty.

Following a similar behavior we already have in the lifecycle, this PR uses the publish value to check if we are running against the daemon or the registry, when running against the daemon, we don't need to check for read access

Output

Before

Panic was thrown

After

The correct run-image is resolved

Documentation

• Should this change be documented?
  • Yes, see #___
  • No

Related

Resolves #2078

[jjbustamante]

if we remove these lines:

if !publish {
	return true
}

From the FakeAccessChecker and remove this fix from Aidan this test reproduces the panic error

panic: runtime error: index out of range [0] with length 0

[codecov]

Codecov Report

Attention: Patch coverage is 89.65517% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 70.29%. Comparing base (12b7d24) to head (e85ddee).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2092      +/-   ##
==========================================
+ Coverage   70.21%   70.29%   +0.09%     
==========================================
  Files         233      231       -2     
  Lines       17097    17104       +7     
==========================================
+ Hits        12003    12022      +19     
+ Misses       4325     4317       -8     
+ Partials      769      765       -4     

Flag	Coverage Δ
os_linux	69.44% <89.66%> (+0.09%)	⬆️
os_macos	65.83% <46.56%> (-0.07%)	⬇️
os_windows	69.81% <89.66%> (+0.09%)	⬆️
unit	70.29% <89.66%> (+0.09%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

[c0d1ngm0nk3y]

this PR uses the publish value to check if we are running against the daemon or the registry

Are you sure that using the publish flag for this is the right approach? The problem is that the stack is local and not that the result should not be published, right? It would be valid to use a remote stack that should be checked.

[jjbustamante]

this PR uses the publish value to check if we are running against the daemon or the registry

Are you sure that using the publish flag for this is the right approach? The problem is that the stack is local and not that the result should not be published, right? It would be valid to use a remote stack that should be checked.

The problem was: when running pack build without --publish which is the flow to run against the daemon, pack was throwing the error:

 CheckReadAccess failed for the run image stack-run-845695c9-5147-42a2-8cb3-c0ea0500c6a4:latest, error: HEAD https://index.docker.io/v2/library/stack-run-845695c9-5147-42a2-8cb3-c0ea0500c6a4/manifests/latest: unexpected status code 401 Unauthorized (HEAD responses have no body, use GET for details)
        panic: runtime error: index out of range [0] with length 0

The reason is we are trying to check the read access in a remote location, pack was converting the image reference and by default it tries to hit dockerhub. When we use the publish flag, yes it means we want the final image to be saved into a registry, but it also defines the place from where pack is going to read the images, it means which image from imgutil is going to be instantiated (remote, local, layout) in this case, we want pack to instantiate a imgutil.local image and that's the actual problem

[c0d1ngm0nk3y]

I see the problem and actually found this because I have the very same error. But there are 2 things I wondered while I had a first look at this change

1. Having publish as a criteria how to interpret the run image (docker or daemon) sounds wrong to me. I is also a use case to use a run image from docker, but not publish the result, right? Would it be possible to identify daemon via daemon:// for example?

2. Passing the publish flag to the ReadChecker to then decide not to check anything defeats the purpose I guess. So maybe we can find a way to inject a NoOpReadChecker like in the unit tests. But I think we should have a better criteria to distinguish references to docker.io from references to the local daemon.

[jjbustamante]

Having publish as a criteria how to interpret the run image (docker or daemon) sounds wrong to me. I is also a use case to use a run image from docker, but not publish the result, right? Would it be possible to identify daemon via daemon:// for example?

We've been using the publish flag to configure the Fetcher from where it should get the image, maybe as Natalie said the best way to solve it could be to move the AccessChecker to be a method exposed by the Fetcher, but even if we do that, well be implementing a logic that checks if !options.Daemon do nothing.

By adding daemon:// you mean adding it to the run-image metadata? I do not see why we should request end-users to add a extra information for something that was working (from their perspective) in the previous release?

Passing the publish flag to the ReadChecker to then decide not to check anything defeats the purpose I guess. So maybe we can find a way to inject a NoOpReadChecker like in the unit tests. But I think we should have a better criteria to distinguish references to docker.io from references to the local daemon.

Yeah, I think ideally the no-op should be the best way, that's how I think it is working in the lifecycle but again, at end lifecycle is also checking if it hitting the daemon or the registry to decide if it needs to do the check or not.

[c0d1ngm0nk3y]

But adding publish to the signature of AccessChecker.Check just to return true feels wrong. Especially since we have to pass publish down the the actual call. Should/could we maybe just exchange the AccessChecker in this case? Similar like it is done in the tests.

I still think that using publish as indicator if the run image is daemon only or default the location to docker.io is strange.

[natalieparellano]

This will work. An alternative way to solve this would be to delegate checking the access to the image fetcher (as noted here: #2078 (comment)); this will fix the edge case where we expect the run image to be in the daemon but can't actually pull it.

[jjbustamante]

@natalieparellano @c0d1ngm0nk3y

I just did a little refactoring and converted the check read access into a function and the Fetcher is responsible for returning this function. The logic behind is actually the same, if you try to fetch an image from the Daemon, then we skip the validation.

[jjbustamante]

@natalieparellano @c0d1ngm0nk3y could you give me one more review on this one?

[c0d1ngm0nk3y]

lgtm

[natalieparellano]

LGTM. Nice work @jjbustamante