Allow absolute URI specification for action images

[ptrivedi]

These changes allow for action image paths in workflow template
to be specified as complete URIs based on use-absolute-action-image-uri
parameter. This will skip the prepending of docker-registry to the
action image paths.

Signed-off-by: Pooja Trivedi tripooja@amazon.com

Description

Why is this needed

Fixes: #

How Has This Been Tested?

How are existing users impacted? What migration steps/scripts do we need?

Checklist:

I have:

• updated the documentation and/or roadmap (if required)
• added unit or e2e tests
• provided instructions on how to upgrade

[sonatype-lift]

gofmt: File is not gofmt-ed with -s

(at-me in a reply with help or ignore)

---

Was this a good recommendation?
[ 🙁 Not relevant ] - [ 😕 Won't fix ] - [ 😑 Not critical, will fix ] - [ 🙂 Critical, will fix ] - [ 😊 Critical, fixing now ]

[micahhausler]

Thanks for adding this! Just a few minor changes

[codecov]

Codecov Report

Merging #616 (8fb902b) into main (720195f) will increase coverage by 0.00%.
The diff coverage is 54.54%.

❗ Current head 8fb902b differs from pull request most recent head d5a4ac0. Consider uploading reports for the commit d5a4ac0 to get more accurate results

@@           Coverage Diff           @@
##             main     #616   +/-   ##
=======================================
  Coverage   44.37%   44.37%           
=======================================
  Files          61       61           
  Lines        3491     3500    +9     
=======================================
+ Hits         1549     1553    +4     
- Misses       1858     1860    +2     
- Partials       84       87    +3     

Impacted Files	Coverage Δ
cmd/tink-worker/worker/registry.go	75.86% <50.00%> (-4.91%)	⬇️
cmd/tink-worker/worker/container_manager.go	94.73% <57.14%> (-3.84%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 720195f...d5a4ac0. Read the comment docs.

[micahhausler]

LGTM

[jacobweinstock]

currently, if the flag/env var docker-registry is not set then the m.cli.ImagePull behavior is the same as the way docker client does its image pulls, link. This PR would create a different UX than that.

Sounds like you possibly want to improve the UX with an explicit flag/env for allowing absolute URIs. Is that an accurate assumption?

If this is the case, I'd be hesitant to move away from the UX docker provides as many, possibly most, people already use and understand this UX.

[ptrivedi]

It allows for the case where different action images could potentially come from different registries, docker_registry could be specified and applied for tink-worker pull but not to action image pulls, docker_registry could be specified even when there's full paths for images and there would be no mysterious prepending. Allows for a cleaner specification of "just use the URI I give you" instead of having to know about implicit assumptions of specifying or not specifying the docker_registry etc. Also, the failure in this case is a more significant consequence as you'd have to be on the remote console and have to run look at containers and logs to figure out what exactly went wrong. This makes things simpler, bypasses assumptions, provides more flexibility.

Consider this scenario: user has tens of actions in the template and wants all but one of them to come from a specified docker_registry -- the user needs that one exception to have a full path due to different registry being used. In this case, they would have to specify full path for each image, would have to ensure they empty out the docker_registry. Then they would also have to specify the full path for tink-worker image because registry wasn't specified. Not saying there is one right answer, but this makes it much simpler.

[jacobweinstock]

It allows for the case where different action images could potentially come from different registries, docker_registry could be specified and applied for tink-worker pull but not to action image pulls, docker_registry could be specified even when there's full paths for images and there would be no mysterious prepending. Allows for a cleaner specification of "just use the URI I give you" instead of having to know about implicit assumptions of specifying or not specifying the docker_registry etc. Also, the failure in this case is a more significant consequence as you'd have to be on the remote console and have to run look at containers and logs to figure out what exactly went wrong. This makes things simpler, bypasses assumptions, provides more flexibility.

Consider this scenario: user has tens of actions in the template and wants all but one of them to come from a specified docker_registry -- the user needs that one exception to have a full path due to different registry being used. In this case, they would have to specify full path for each image, would have to ensure they empty out the docker_registry. Then they would also have to specify the full path for tink-worker image because registry wasn't specified. Not saying there is one right answer, but this makes it much simpler.

Tink worker pulling actually has no bearing here in this code base. That is a Hook concern. Also, the top of tree Hook now sends workflow container logs to Boots so console access for troubleshooting is not needed.

The only difference I can tell is the use of docker_registry versus use-absolute-image-uri. As i see it they both provide basically the same functionality. The trade offs of this PR adding more logic and code makes me hesitant. It almost feels like both of these gates docker_registry and use-absolute-image-uri could just be removed entirely.

[mmlb]

I'll echo all of @jacobweinstock concerns and also 👍 to:

It almost feels like both of these gates docker_registry and use-absolute-image-uri could just be removed entirely.

and add the registry username&passwords too! Only because the simple case can be managed with extra-kernel-args and the hard case can't be managed with just one set of url/auth. What if someone wants to use quay and docker hub and needs to log in to docker hub because of rate limits. Or any other set of oci repo. In the end its a hook/osie issue and boots should not care and doesn't need to have code for it.

[ptrivedi]

tink-worker pulling was affected by docker_registry being specified at Boots level and being passed over to Hook and then onto tink-worker, as far as the sandbox was concerned. With the extraArgs added to Boots for kernel arguments, tink-worker is not affected, you are right, and this becomes a Hook-only concern.

I do agree that use-absolute-image-uri is just another flag to accommodate for something that truly should be simplified and fixed by better debugging logs redirected from Hook to Boots which is already being done now. And that both docker_registry and use-absolute-image-uri could be removed.

These changes that I dusted off yesterday here were put in a month or two ago, when a lot of this refactoring effort was not in place. Almost seems like it would be better to hold off on these set of PRs, given how much refactoring is still in flight.

As a side node, if/when we get rid of docker_registry gate, we have to ensure that the hardcoded template (which shouldn't be hardcoded in the first place), must be given full URIs for action images.

[micahhausler]

It almost feels like both of these gates docker_registry and use-absolute-image-uri could just be removed entirely

Taking a second look, this now makes sense.

As a side node, if/when we get rid of docker_registry gate, we have to ensure that the hardcoded template (which shouldn't be hardcoded in the first place), must be given full URIs for action images.

The conundrum we have is that some users have used the --docker-registry field to prepend a fully qualified domain name to their incomplete template images, instead of a local registry. CAPT did this in the default template.

To me, that use case is an invariant we don't want to encourage, and should clearly document that template images MUST use a FQDN to the registry, and the --docker-registry flag is only used for a local registry

[mmlb]

wow, I thought this was boots not tink while reading the comments... :concern:. Anyways I still agree with jacob :)

[jacobweinstock]

just to clarify terms we're using. Is this accurate to everyone?

registry:

a repository—or collection of repositories—used to store and access container images. -- ref

local registry:

a registry available as part of a container runtime. Local to the container runtime machine. for example, docker images will show the images in the local registry.

public registry:

a registry generally available via the internet. for example, docker hub (docker.io), quay.io, public.ecr.aws

private registry:

a registry that is not generally available via the internet. for example, an internal registry only available on your network, or the registry in the sandbox as it is generally not available via the internet.
auth enable registry: a public or private registry that requires authentication/authorization in order to push, pull, etc images.