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

fix(k8s): issues with querying registries for image tags #1636

Merged
merged 4 commits into from
Feb 24, 2020

Conversation

edvald
Copy link
Collaborator

@edvald edvald commented Feb 22, 2020

What this PR does / why we need it:

This affected users using a non-default deploymentRegistry, as well as
microk8s users. The registry is now properly queried when checking
build status in all cases, both when building locally and remotely,
and whether an in-cluster registry is used or not.

Which issue(s) this PR fixes:

Fixes #1380

@edvald edvald force-pushed the fix-remote-registry-error branch from 60ad79c to db92e6a Compare February 22, 2020 16:54
@eysi09
Copy link
Collaborator

eysi09 commented Feb 23, 2020

Could you explain the PR a little better. I noticed we bumped the min Docker client version and that we're now downloading the bin if users don't have it. How and why are we using the new Docker version?

@edvald
Copy link
Collaborator Author

edvald commented Feb 23, 2020

Sure. The docker manifests inspect command is only available as an experimental command in the latest major docker release. That command provides a simple way to query registries for info, which we can use to check whether an image/tag exists in a registry. The reason we can't just use an API directly is because of auth. Docker supports a number of auth methods, at least locally (credential helpers and such) and we can't reliably replicate that or use libraries that do.

For the in-cluster building scenario I could have gotten around installing the new version because we exec from the in-cluster builder. The issue there had been that I left an assumption in the build status check that we were using the in-cluster registry. The new flow works for either scenario.

For local builds, this had been an open issue for a while. We'd check for the image in the local docker daemon, and if it existed we'd assume the same image was in the deployment registry. But if something had failed or the tag removed from the deployment registry, users would see issues.

I only realized yesterday that this was causing the problems for microk8s because we use the deployment registry flow there, due to how microk8s is set up.

Changing the minimum required docker version would have been a breaking change, so I needed to fetch docker automatically to address this. I added a bit of extra logic to avoid fetching it when a recent version is already installed, since that's a very common case, and this may otherwise come across as a bit unexpected.

This affected users using a non-default deploymentRegistry, as well as
microk8s users. The registry is now properly queried when checking
build status in all cases, both when building locally and remotely,
and whether an in-cluster registry is used or not.

Fixes #1380
@edvald edvald force-pushed the fix-remote-registry-error branch from db92e6a to 58fb78c Compare February 23, 2020 14:26

return { fresh: true, buildLog, details: { identifier } }
return { fresh: true, buildLog: res.output || "", details: { identifier } }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return { fresh: true, buildLog: res.output || "", details: { identifier } }
return { fresh: true, buildLog: res.output ?? "", details: { identifier } }

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, I keep forgetting that new operator. Makes no semantic difference in this particular case but it's worth keeping in mind.

Copy link
Collaborator

@eysi09 eysi09 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@edvald edvald merged commit 4bf8aab into master Feb 24, 2020
@edvald edvald deleted the fix-remote-registry-error branch February 24, 2020 15:59
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.

In-cluster builds fail to push to garden registry with microk8s clusters
3 participants