Skip to content
This repository was archived by the owner on Jun 3, 2025. It is now read-only.

Upgrade go-containerregistry third-party library#957

Merged
cvgw merged 1 commit intoGoogleContainerTools:masterfrom
antechrestos:fix/scopes_asked_to_remote_registry
Jan 29, 2020
Merged

Upgrade go-containerregistry third-party library#957
cvgw merged 1 commit intoGoogleContainerTools:masterfrom
antechrestos:fix/scopes_asked_to_remote_registry

Conversation

@antechrestos
Copy link
Copy Markdown
Contributor

@antechrestos antechrestos commented Jan 8, 2020

Fixes #808

Description

In this change I updated go-containerregistry dependency to a more recent version. It was needed to get the fix brought by the following pull request on the third party project
By doing so, it forced me to upgrade to newer dependency and/or versions.

One of them is moby/buildkit. This late one is developed in go 1.11. Starting version 1.13, versioning does not allow invalid pseudo-version. The invalid pseudo versions used by moby/buildkit (or dependency using itself an invalid pseudo version) are the one that are declared in the replacesection.

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

  • Includes Not needed
  • Adds integration tests if needed Not needed

Reviewer Notes

  • The code flow looks good.
  • Unit tests and or integration tests added.

Release Notes

Describe any changes here so maintainer can include it in the release notes, or delete this block.

Fix issues when pushing images on some registry built from a base image hosted on a registry different than docker hub and target registry

@googlebot googlebot added the cla: yes CLA signed by all commit authors label Jan 8, 2020
@antechrestos antechrestos mentioned this pull request Jan 8, 2020
Closed
4 tasks
cvgw
cvgw reviewed Jan 10, 2020
View reviewed changes
Comment thread go.mod
go 1.13

replace (
github.com/containerd/containerd v1.4.0-0.20191014053712-acdcf13d5eaf => github.com/containerd/containerd v0.0.0-20191014053712-acdcf13d5eaf
Copy link
Copy Markdown
Contributor

@cvgw cvgw Jan 10, 2020

Choose a reason for hiding this comment

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

@antechrestos do you have a link to some info or would you mind giving me a quick explanation on why this is the correct fix for the problem? AFAIK this is correct, but I can't really find much info about this problem.

Copy link
Copy Markdown
Contributor Author

@antechrestos antechrestos Jan 10, 2020

Choose a reason for hiding this comment

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

@cvgw I crawled around the web to find the way to fix this problem. I though that using a simple commit id would be better, yet go tidy replaced it with worse version.
Aside from that I don't understand why go 1.13 does not allow the minus zero, as minus zero means do nothing.

Truly I am very confused with that: it allows using github.com/docker/docker v1.14.0-0.20190319215453-e7b5f7dbe98c in require section, but if one of the dependency needs it with the very same version, it cries.

I would have preferred something like removing every -0.from go.mod file :

@@ -4,7 +4,7 @@ go 1.13
 
 replace (
        github.com/containerd/containerd v1.4.0-0.20191014053712-acdcf13d5eaf => github.com/containerd/containerd v0.0.0-20191014053712-acdcf13d5eaf
-       github.com/docker/docker v1.14.0-0.20190319215453-e7b5f7dbe98c => github.com/docker/docker v0.0.0-20190319215453-e7b5f7dbe98c
+       github.com/docker/docker v1.14.0-0.20190319215453-e7b5f7dbe98c => github.com/docker/docker v1.14.0-20190319215453-e7b5f7dbe98c
        github.com/tonistiigi/fsutil v0.0.0-20190819224149-3d2716dd0a4d => github.com/tonistiigi/fsutil v0.0.0-20191018213012-0f039a052ca1
 )
 
@@ -12,14 +12,14 @@ require (
        cloud.google.com/go v0.26.0
        github.com/Azure/azure-pipeline-go v0.2.2 // indirect
        github.com/Azure/azure-storage-blob-go v0.8.0
-       github.com/Microsoft/go-winio v0.4.15-0.20190919025122-fc70bd9a86b5 // indirect
+       github.com/Microsoft/go-winio v0.4.15.20190919025122-fc70bd9a86b5 // indirect
        github.com/alcortesm/tgz v0.0.0-20161220082320-9c5fe88206d7 // indirect
        github.com/anmitsu/go-shlex v0.0.0-20161002113705-648efa622239 // indirect
        github.com/aws/aws-sdk-go v1.25.19
        github.com/beorn7/perks v0.0.0-20180321164747-3a771d992973 // indirect
-       github.com/docker/docker v1.14.0-0.20190319215453-e7b5f7dbe98c
+       github.com/docker/docker v1.14.0.20190319215453-e7b5f7dbe98c
        github.com/docker/go-metrics v0.0.0-20180209012529-399ea8c73916 // indirect
-       github.com/docker/swarmkit v1.12.1-0.20180726190244-7567d47988d8 // indirect
+       github.com/docker/swarmkit v1.12.1.20180726190244-7567d47988d8 // indirect
        github.com/emirpasic/gods v1.9.0 // indirect
        github.com/flynn/go-shlex v0.0.0-20150515145356-3f9db97f8568 // indirect
        github.com/genuinetools/amicontained v0.4.3

Yet it does not find dependencies after that.:

$> go mod vendor                                                                                                                                    fix/scopes_asked_to_remote_registry ✖ ✱
go: finding github.com/Microsoft/go-winio v0.4.15.20190919025122-fc70bd9a86b5
go: finding github.com/Microsoft/go-winio v0.4.15.20190919025122-fc70bd9a86b5
go: finding github.com/docker/docker v1.14.0.20190319215453-e7b5f7dbe98c
go: finding github.com/docker/docker v1.14.0.20190319215453-e7b5f7dbe98c
go: finding github.com/docker/swarmkit v1.12.1.20180726190244-7567d47988d8
go: finding github.com/docker/swarmkit v1.12.1.20180726190244-7567d47988d8
go: errors parsing go.mod:
kaniko/go.mod:15: require github.com/Microsoft/go-winio: version "v0.4.15.20190919025122-fc70bd9a86b5" invalid: unknown revision v0.4.15.20190919025122-fc70bd9a86b5
kaniko/go.mod:20: require github.com/docker/docker: version "v1.14.0.20190319215453-e7b5f7dbe98c" invalid: unknown revision v1.14.0.20190319215453-e7b5f7dbe98c
kaniko/go.mod:22: require github.com/docker/swarmkit: version "v1.12.1.20180726190244-7567d47988d8" invalid: unknown revision v1.12.1.20180726190244-7567d47988d8

I am opened to any suggestion on this point as I am not an expert on go modules.

Copy link
Copy Markdown
Contributor

@cvgw cvgw Jan 11, 2020

Choose a reason for hiding this comment

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

Thanks for that additional information @antechrestos

I found this documentation which I think is particularly relevant https://golang.org/cmd/go/#hdr-Pseudo_versions

This section caught my eye

Pseudo-versions never need to be typed by hand: the go command will accept the plain commit hash and translate it into a pseudo-version (or a tagged version if available) automatically. This conversion is an example of a module query.

I think that explains why go tidy updated the replace statements when you used the commit id.

I know there was some additional pseudo version validation added in golang 1.13 which probably has something to do with this.

Copy link
Copy Markdown
Contributor

@cvgw cvgw Jan 11, 2020

Choose a reason for hiding this comment

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

@antechrestos did you use go mod edit -replace for these?

Copy link
Copy Markdown
Contributor Author

@antechrestos antechrestos Jan 11, 2020

Choose a reason for hiding this comment

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

@cvgw
I did used the go mod edit -replacecommand at first but did not like the result as I think it is less nice.

I've just used it and here is the diff, please let me know if you prefer this way:

 go 1.13
 
-replace (
-       github.com/containerd/containerd v1.4.0-0.20191014053712-acdcf13d5eaf => github.com/containerd/containerd v0.0.0-20191014053712-acdcf13d5eaf
-       github.com/docker/docker v1.14.0-0.20190319215453-e7b5f7dbe98c => github.com/docker/docker v0.0.0-20190319215453-e7b5f7dbe98c
-       github.com/tonistiigi/fsutil v0.0.0-20190819224149-3d2716dd0a4d => github.com/tonistiigi/fsutil v0.0.0-20191018213012-0f039a052ca1
-)
-
 require (
        cloud.google.com/go v0.26.0
        github.com/Azure/azure-pipeline-go v0.2.2 // indirect
@@ -72,3 +66,9 @@ require (
        gopkg.in/warnings.v0 v0.1.2 // indirect
        k8s.io/kubernetes v1.13.0 // indirect
 )
+
+replace github.com/docker/docker v1.14.0-0.20190319215453-e7b5f7dbe98c => github.com/docker/docker v1.4.2-0.20190319215453-e7b5f7dbe98c
+
+replace github.com/containerd/containerd v1.4.0-0.20191014053712-acdcf13d5eaf => github.com/containerd/containerd v1.3.1-0.20191014053712-acdcf13d5eaf
+
+replace github.com/tonistiigi/fsutil v0.0.0-20190819224149-3d2716dd0a4d => github.com/tonistiigi/fsutil v0.0.0-20191018213012-0f039a052ca1

Shall I group the replace in a single replace section?

BTW, would you share the output of kaniko tests?

I launched them from master branch and I have random results.

First launch on master

--- FAIL: TestRun (0.00s)
    --- FAIL: TestRun/test_Dockerfile_test_cmd (32.40s)
    --- FAIL: TestRun/test_Dockerfile_test_metadata (32.41s)
    --- FAIL: TestRun/test_Dockerfile_test_volume_4 (50.78s)
--- FAIL: TestLayers (0.00s)
    --- FAIL: TestLayers/test_layer_Dockerfile_test_arg_multi (4.25s)
    --- FAIL: TestLayers/test_layer_Dockerfile_test_arg_multi_with_quotes (2.53s)
    --- FAIL: TestLayers/test_layer_Dockerfile_test_meta_arg (13.72s)
    --- FAIL: TestLayers/test_layer_Dockerfile_test_copy_same_file_many_times (17.26s)
FAIL
FAIL	github.com/GoogleContainerTools/kaniko/integration	1020.141s

I cleaned images, containers, and gcloud and retried (still on master):

--- FAIL: TestRun (0.00s)
    --- FAIL: TestRun/test_Dockerfile_test_volume_4 (26.22s)
    --- FAIL: TestRun/test_Dockerfile_test_cmd (22.37s)
    --- FAIL: TestRun/test_Dockerfile_test_metadata (29.66s)
    --- FAIL: TestRun/test_Dockerfile_test_onbuild (40.64s)
    --- FAIL: TestRun/test_Dockerfile_test_hardlink (83.07s)
--- FAIL: TestLayers (0.00s)
    --- FAIL: TestLayers/test_layer_Dockerfile_test_onbuild (10.27s)
    --- FAIL: TestLayers/test_layer_Dockerfile_test_cache_copy (11.03s)
    --- FAIL: TestLayers/test_layer_Dockerfile_test_hardlink (31.00s)
    --- FAIL: TestLayers/test_layer_Dockerfile_test_volume_4 (15.86s)
--- FAIL: TestCache (1.33s)
    --- FAIL: TestCache/test_cache_Dockerfile_test_cache_perm (27.73s)
    --- FAIL: TestCache/test_cache_Dockerfile_test_cache_install (70.29s)
    --- FAIL: TestCache/test_cache_Dockerfile_test_cache (71.23s)
    --- FAIL: TestCache/test_cache_Dockerfile_test_cache_copy (73.13s)
FAIL
FAIL	github.com/GoogleContainerTools/kaniko/integration	610.926s

Do you also observe random results on migration tests? 🤔

I also launched on my branch:

--- FAIL: TestRun (0.00s)
    --- FAIL: TestRun/test_Dockerfile_test_cmd (21.51s)
    --- FAIL: TestRun/test_Dockerfile_test_add_dest_symlink_dir (17.46s)
    --- FAIL: TestRun/test_Dockerfile_test_volume_4 (60.68s)
    --- FAIL: TestRun/test_Dockerfile_test_scratch (13.83s)
    --- FAIL: TestRun/test_Dockerfile_test_replaced_symlinks (28.89s)
    --- FAIL: TestRun/test_Dockerfile_test_metadata (19.60s)
    --- FAIL: TestRun/test_Dockerfile_test_hardlink (94.27s)
--- FAIL: TestLayers (0.00s)
    --- FAIL: TestLayers/test_layer_Dockerfile_test_arg_multi (4.12s)
    --- FAIL: TestLayers/test_layer_Dockerfile_test_arg_multi_with_quotes (2.43s)
    --- FAIL: TestLayers/test_layer_Dockerfile_test_scratch (5.34s)
    --- FAIL: TestLayers/test_layer_Dockerfile_test_meta_arg (18.65s)
    --- FAIL: TestLayers/test_layer_Dockerfile_test_replaced_symlinks (6.44s)
    --- FAIL: TestLayers/test_layer_Dockerfile_test_add_dest_symlink_dir (10.40s)
    --- FAIL: TestLayers/test_layer_Dockerfile_test_arg_blank_with_quotes (2.07s)
    --- FAIL: TestLayers/test_layer_Dockerfile_test_copy_same_file_many_times (18.39s)
    --- FAIL: TestLayers/test_layer_Dockerfile_test_hardlink (10.63s)
FAIL
FAIL	github.com/GoogleContainerTools/kaniko/integration	740.007s

Copy link
Copy Markdown
Contributor

@cvgw cvgw Jan 17, 2020

Choose a reason for hiding this comment

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

@antechrestos I don't have a strong preference between go mod edit version and yours. I would say keep what you have.

The following tests failed

FAIL: TestRun/test_Dockerfile_test_hardlink (31.88s)
error building image: file with no instructions.
FAIL: TestRun/test_Dockerfile_test_replaced_symlinks (8.67s)
            INFO[0000] Resolved base name tenstartups/alpine@sha256:31dc8b12e0f73a1de899146c3663644b7668f8fd198cfe9b266886c9abfa944b to tenstartups/alpine@sha256:31dc8b12e0f73a1de899146c3663644b7668f8fd198cfe9b266886c9abfa944b 
            INFO[0000] Using dockerignore file: /workspace/.dockerignore 
            INFO[0000] Resolved base name tenstartups/alpine@sha256:31dc8b12e0f73a1de899146c3663644b7668f8fd198cfe9b266886c9abfa944b to tenstartups/alpine@sha256:31dc8b12e0f73a1de899146c3663644b7668f8fd198cfe9b266886c9abfa944b 
            INFO[0000] Image tenstartups/alpine@sha256:31dc8b12e0f73a1de899146c3663644b7668f8fd198cfe9b266886c9abfa944b not found in cache 
            INFO[0000] Retrieving image manifest tenstartups/alpine@sha256:31dc8b12e0f73a1de899146c3663644b7668f8fd198cfe9b266886c9abfa944b 
            INFO[0000] Built cross stage deps: map[]                
            INFO[0000] Image tenstartups/alpine@sha256:31dc8b12e0f73a1de899146c3663644b7668f8fd198cfe9b266886c9abfa944b not found in cache 
            INFO[0000] Retrieving image manifest tenstartups/alpine@sha256:31dc8b12e0f73a1de899146c3663644b7668f8fd198cfe9b266886c9abfa944b 
            error building image: file with no instructions.
FAIL: TestRun/test_Dockerfile_test_scratch (7.11s)
            INFO[0000] Resolved base name ${image} to scratch       
            INFO[0000] Using dockerignore file: /workspace/.dockerignore 
            INFO[0000] Resolved base name ${image} to scratch       
            INFO[0000] Built cross stage deps: map[]                
            ERRO[0000] Error while retrieving image from cache:  could not parse reference:  
            INFO[0000] Image  not found in cache                    
            INFO[0000] Retrieving image manifest    
FAIL: TestRun/test_Dockerfile_test_add_dest_symlink_dir (10.78s)
INFO[0000] Resolved base name phusion/baseimage:0.11 to phusion/baseimage:0.11 
            INFO[0000] Using dockerignore file: /workspace/.dockerignore 
            INFO[0000] Resolved base name phusion/baseimage:0.11 to phusion/baseimage:0.11 
            INFO[0000] Retrieving image manifest phusion/baseimage:0.11 
            INFO[0000] Image phusion/baseimage:0.11 not found in cache 
            INFO[0000] Retrieving image manifest phusion/baseimage:0.11 
            INFO[0000] Built cross stage deps: map[]                
            INFO[0000] Retrieving image manifest phusion/baseimage:0.11 
            INFO[0000] Image phusion/baseimage:0.11 not found in cache 
            INFO[0000] Retrieving image manifest phusion/baseimage:0.11 
            error building image: file with no instructions.

Three of them seem to be the same error error building image: file with no instructions which seems to be coming from buildkit https://github.com/moby/buildkit/blob/f53dcc830b03ccc1719fb9472852dcb960131e24/frontend/dockerfile/parser/parser.go#L280

But it certainly doesn't look like those dockerfiles are empty https://github.com/GoogleContainerTools/kaniko/blob/master/integration/dockerfiles/Dockerfile_test_add_dest_symlink_dir

Looking at the integration code I don't see any obvious reasons for the failure. Need to investigate more

Copy link
Copy Markdown
Contributor Author

@antechrestos antechrestos Jan 18, 2020

Choose a reason for hiding this comment

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

@cvgw Thanks for the info. I will take a look.

Copy link
Copy Markdown
Contributor Author

@antechrestos antechrestos Jan 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@cvgw I think I solved one of the two problems.

The first one is linked to evolution in parser. This method is called by pkg/executor/build.go:resolveOnBuild with an empty array as config.OnBuild. It used to be accepted by buildkit but no longer. No more Mr nice guy they may have though.
I changed the if config.OnBuild == nil to if config.OnBuild == nil || len(config.OnBuild) == 0 in pkg/executor/build.go:resolveOnBuild and it solved tests with Dockerfile_test_replaced_symlinks, Dockerfile_test_add_dest_symlink_dir and Dockerfile_test_hardlink.

The second issue is linked to Dockerfile_test_scratch. It has an ARG named image. Don't know why but, it also has a meta argument (don't know what it is...yet) also named imageand valued to empty string; this late one overrides the build arg and the image resolution made by pkg/util/image_util.go:RetrieveSourceImage results in empty value. (ERRO[0000] Error while retrieving image from cache: could not parse reference: )

I will go deeper for this problem.

Copy link
Copy Markdown
Contributor Author

@antechrestos antechrestos Jan 26, 2020

Choose a reason for hiding this comment

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

I found it: now in moby BuildEnvs function they override value while before they kept the previous one. I will try to solve it by putting meta args first and then build args (which is logic)

Copy link
Copy Markdown
Contributor Author

@antechrestos antechrestos Jan 26, 2020

Choose a reason for hiding this comment

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

👌

@cvgw cvgw self-assigned this Jan 17, 2020
@antechrestos antechrestos reopened this Jan 18, 2020
@tejal29
Copy link
Copy Markdown
Contributor

tejal29 commented Jan 22, 2020

@antechrestos We now run integration tests on Travis. If you rebase on master, you will pick up the change.
Thanks
Tejal

@antechrestos
Copy link
Copy Markdown
Contributor Author

antechrestos commented Jan 22, 2020

@tejal29 thank you I will take a look tomorrow.

@antechrestos
Upgrade dependency to go-containerregistry third-party library
03b8922 
By doing so it will fix issues met when mixing source/remote registry

Close #808
@antechrestos
Copy link
Copy Markdown
Contributor Author

antechrestos commented Jan 26, 2020

@cvgw @tejal29 unit tests and integration tests are passing.

@antechrestos
Copy link
Copy Markdown
Contributor Author

antechrestos commented Jan 27, 2020

BTW I am stuck with need rebase label despite the rebase 😏

@antechrestos
Copy link
Copy Markdown
Contributor Author

antechrestos commented Jan 28, 2020

@cvgw hurray kokoro succeeded here 😊

@cvgw cvgw merged commit 65cd912 into GoogleContainerTools:master Jan 29, 2020
@cvgw
Copy link
Copy Markdown
Contributor

cvgw commented Jan 29, 2020

Awesome, thanks for making this one happen @antechrestos 👍

@tejal29 tejal29 mentioned this pull request Jan 29, 2020
Closed
4 tasks
@antechrestos antechrestos deleted the fix/scopes_asked_to_remote_registry branch January 29, 2020 08:02
@antechrestos antechrestos mentioned this pull request Feb 5, 2020
Closed
@Forst Forst mentioned this pull request May 5, 2020
Open
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@cvgw cvgw cvgw approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@cvgw cvgw

Labels

cla: yes CLA signed by all commit authors

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Docker: unable to push on a some registry when FROM clause contains a private registry different that target one or a proxy url

5 participants

@antechrestos @tejal29 @cvgw @googlebot @kokoro-team