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 jib tagging #1475

Merged
merged 1 commit into from
Jan 16, 2019
Merged

Fix jib tagging #1475

merged 1 commit into from
Jan 16, 2019

Conversation

nkubala
Copy link
Contributor

@nkubala nkubala commented Jan 15, 2019

Fixes #1460

This fixes an issue when tagging a remote image built by jib directly to the registry.

Previously, the digest of the remote image was being passed to docker.AddTag(), which uses go-containerregistry's name.ParseReference() to retrieve a remote image reference to add a new tag.

The issue was that Skaffold was using "WeakValidation", which tells the go-containerregistry to try and infer the remote registry in the event that we don't pass one in: so in trying to retrieve the remote image reference via image digest (e.g. sha256:deadbeef...), we were receiving back a "valid" remote reference to index.docker.io/sha256:deadbeef.....

This change prepends the remote registry specified in the artifact to the provided remote digest (where we retrieved it from anyway) before attempting to add the tag, ensuring that the remote image reference comes from the place we expect it to. This also changes the call to name.ParseReference() to use "StrictValidation", which requires that we pass in a fully-qualified remote, so we're not making any misleading assumptions about the remote registry target.

@nkubala
Copy link
Contributor Author

nkubala commented Jan 15, 2019

cc @jonjohnsonjr @dgageot

@codecov-io
Copy link

codecov-io commented Jan 15, 2019

Codecov Report

Merging #1475 into master will decrease coverage by <.01%.
The diff coverage is 14.28%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1475      +/-   ##
==========================================
- Coverage   44.43%   44.42%   -0.01%     
==========================================
  Files         112      112              
  Lines        4641     4642       +1     
==========================================
  Hits         2062     2062              
- Misses       2372     2373       +1     
  Partials      207      207
Impacted Files Coverage Δ
pkg/skaffold/docker/remote.go 0% <0%> (ø) ⬆️
pkg/skaffold/build/local/local.go 54.34% <33.33%> (ø) ⬆️

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 da2090d...227c4a9. Read the comment docs.

Copy link
Contributor

@jonjohnsonjr jonjohnsonjr left a comment

Choose a reason for hiding this comment

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

We should probably expose a simpler remote.AddTag implementation in go-containerregistry for this, since we can optimize that a bit better than the whole remote.Write flow.

@@ -84,7 +86,7 @@ func retrieveRemoteConfig(identifier string) (*v1.ConfigFile, error) {
}

func remoteImage(identifier string) (v1.Image, error) {
ref, err := name.ParseReference(identifier, name.WeakValidation)
ref, err := name.ParseReference(identifier, name.StrictValidation)
Copy link
Contributor

Choose a reason for hiding this comment

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

This might have unintended consequences since it's used in a few places. If you're confident in your test coverage, it's probably fine 😃

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah it's slightly risky, but it's the "right" thing to do since we should never be passing an invalid reference anyway. I'll take responsibility if it breaks something 👀

@nkubala
Copy link
Contributor Author

nkubala commented Jan 15, 2019

We should probably expose a simpler remote.AddTag implementation in go-containerregistry for this, since we can optimize that a bit better than the whole remote.Write flow.

That would be nice 😄

@dgageot dgageot merged commit 0c8435e into GoogleContainerTools:master Jan 16, 2019
@NicklasWallgren
Copy link
Contributor

Great, when is the next release expected? (tag v0.2x)?

@NicklasWallgren
Copy link
Contributor

@nkubala
Copy link
Contributor Author

nkubala commented Jan 16, 2019

@NicklasWallgren we've been on a short hiatus because of the holidays, but we normally release on a 2 week cycle every other Thursday. this should go out with the v0.21.0 release tomorrow.

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.

tagging image: UNAUTHORIZED
5 participants