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

Add nil check for current docker context #68

Merged
merged 2 commits into from
Oct 27, 2024

Conversation

eharris128
Copy link
Contributor

What

  • Remove assumption that user system has a docker config file

Why

  • Prevent panic on invocation of mint images

Initial Bug Reproduction Steps

  • On latest master, invoke make build
  • Have a fresh install of docker on LTS Ubuntu 22.04 - resulting on no user configured options for docker.
    image

The panic:
image

How Tested

  • make build
  • mint images --runtime=docker
  • Confirm you get no panic

@eharris128 eharris128 changed the title Add nil check for current docker contxt Add nil check for current docker context Oct 26, 2024
@eharris128
Copy link
Contributor Author

Consequence of not performing the other work this PR leaves out:

image

Maybe the onus is on the user?

@kcq
Copy link
Contributor

kcq commented Oct 27, 2024

Consequence of not performing the other work this PR leaves out:

image

Maybe the onus is on the user?

Need to fail more gracefully either way... Don't need any credentials to pull the "busybox@sha256:05..." image. Also not quite sure where the "invalid reference format" API error is coming from.

@kcq kcq merged commit 4fd3971 into mintoolkit:master Oct 27, 2024
3 of 18 checks passed
@kcq
Copy link
Contributor

kcq commented Oct 27, 2024

Consequence of not performing the other work this PR leaves out:

image

Maybe the onus is on the user?

@eharris128 looks like we need a couple of enhancements here... First, the getDockerCredential - failed to acquire local docker config path warning login needs an extra check to see if the error is no docker credentials provider found (there's no error var defined for it in go-dockerclient unfortunately). The no docker credentials provider found is ok because it just means there's no credentials provider for the target registry, which happens all the time. If it's that error than we don't need to log the warning (pkg/crt/docker/dockercrtclient/dockercrtclient.go#L294) and we can do the same thing as if we don't find the auth config.

The image.inspector.Pull warning shows that repo is busybox@sha256 and tag is 05a79..., which explains that invalid reference format API error. The pull logic in the image inspector needs to be enhanced to parse image paths with digests properly. Right now it simply splits on : expecting name:tag, but it needs to check if we have a digest by checking for @ and then split on @ when we have name@algorithm:hashvalue.

@eharris128
Copy link
Contributor Author

For completeness - ran into the similar config enhancement opportunity with k8s in my testing last night.

image

Podman / containerd I did not test to see their behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants