Skip to content

Do not perform build context switch when content trust is not enabled.#19099

Merged
tiborvass merged 1 commit intomoby:masterfrom
calavera:replace_docker_only_trust_enabled
Jan 6, 2016
Merged

Do not perform build context switch when content trust is not enabled.#19099
tiborvass merged 1 commit intomoby:masterfrom
calavera:replace_docker_only_trust_enabled

Conversation

@calavera
Copy link
Copy Markdown
Contributor

@calavera calavera commented Jan 5, 2016

This PR does not fix #15785 but it mitigates its impact for 1.10.
There is a great discussion going on on that issue about better ways to do the dockerfile context switch that we should consider.

Given that we're slightly time constrained, I think we should at least try to mitigate the issue as much as possible.
We don't need that context switch when TRUST is not enabled, therefore we should not do it.

Signed-off-by: David Calavera david.calavera@gmail.com

@calavera calavera force-pushed the replace_docker_only_trust_enabled branch 2 times, most recently from 8a49035 to 4a66433 Compare January 5, 2016 18:45
@calavera calavera added this to the 1.10.0 milestone Jan 5, 2016
@calavera calavera mentioned this pull request Jan 5, 2016
Closed
@cpuguy83
Copy link
Copy Markdown
Member

cpuguy83 commented Jan 5, 2016

SGTM

@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Jan 5, 2016

SGTM as a workaround for now

diogomonica
diogomonica reviewed Jan 5, 2016
View reviewed changes
api/client/build.go Outdated
Copy link
Copy Markdown
Contributor

@diogomonica diogomonica Jan 5, 2016

Choose a reason for hiding this comment

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

There is another way of enabling DCT, by providing --disable-content-trust=false. I'm pretty sure there is a helper method to decide if DCT is on somewhere.

Copy link
Copy Markdown
Contributor Author

@calavera calavera Jan 6, 2016

Choose a reason for hiding this comment

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

fixed, thanks.

@calavera
Do not perform build context switch when content trust is not enabled.
18d15ba 
Signed-off-by: David Calavera <david.calavera@gmail.com>
@calavera calavera force-pushed the replace_docker_only_trust_enabled branch from 4a66433 to 18d15ba Compare January 6, 2016 00:23
@diogomonica
Copy link
Copy Markdown
Contributor

diogomonica commented Jan 6, 2016

LGTM

@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Jan 6, 2016

got a failed test on experimental, not sure if it's related here, or just a hiccup 😄;

01:07:46 ----------------------------------------------------------------------
01:07:46 FAIL: docker_cli_pull_test.go:40: DockerHubPullSuite.TestPullNonExistingImage
01:07:46 
01:07:46 docker_cli_pull_test.go:58:
01:07:46     // Hub returns 401 rather than 404 for nonexistent repos over
01:07:46     // the v2 protocol - but we should end up falling back to v1,
01:07:46     // which does return a 404.
01:07:46     c.Assert(out, checker.Contains, fmt.Sprintf("Error: image %s not found", e.Repo), check.Commentf("expected image not found error messages"))
01:07:46 ... obtained string = "Error response from daemon: Get https://registry-1.docker.io/v2/library/asdfasdf/manifests/foobar: Get https://auth.docker.io/token?scope=repository%!A(MISSING)library%!F(MISSING)asdfasdf%!A(MISSING)pull&service=registry.docker.io: EOF\n"
01:07:46 ... substring string = "Error: image library/asdfasdf not found"
01:07:46 ... expected image not found error messages
01:07:46 
01:08:02 
01:08:02 ----------------------------------------------------------------------
01:08:02 PASS: docker_cli_pull_test.go:99: DockerHubPullSuite.TestPullScratchNotAllowed 0.027s
01:08:02 OOPS: 1126 passed, 12 skipped, 1 FAILED
01:08:02 --- FAIL: Test (2037.79s)
01:08:02 FAIL
01:08:02 coverage: 75.0% of statements
01:08:02 exit status 1
01:08:02 FAIL   github.com/docker/docker/integration-cli    2037.845s
01:08:02 ---> Making bundle: .integration-daemon-stop (in bundles/1.10.0-dev/test-integration-cli)
01:08:02 +++++ cat bundles/1.10.0-dev/test-integration-cli/docker.pid
01:08:02 ++++ kill 13359
01:08:02 +++++ cat bundles/1.10.0-dev/test-integration-cli/d59516763/docker.pid
01:08:02 ++++ kill 13424
01:08:02 /go/src/github.com/docker/docker/hack/make/.integration-daemon-stop: line 9: wait: pid 13424 is not a child of this shell
01:08:02 warning: PID 13424 from bundles/1.10.0-dev/test-integration-cli/d59516763/docker.pid had a nonzero exit code

@calavera
Copy link
Copy Markdown
Contributor Author

calavera commented Jan 6, 2016

@thaJeztah it looks like it was a temporary issue with the registry. It's all 💚 now.

@jessfraz
Copy link
Copy Markdown
Contributor

jessfraz commented Jan 6, 2016

LGTM

1 similar comment
@tiborvass
Copy link
Copy Markdown
Contributor

tiborvass commented Jan 6, 2016

LGTM

tiborvass added a commit that referenced this pull request Jan 6, 2016
@tiborvass
Merge pull request #19099 from calavera/replace_docker_only_trust_ena…
7fab931 
…bled

Do not perform build context switch when content trust is not enabled.
@tiborvass tiborvass merged commit 7fab931 into moby:master Jan 6, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

status/2-code-review

Projects

None yet

Milestone

1.10.0

Development

Successfully merging this pull request may close these issues.

Permissions problem on Docker 1.8 when reading from stdin

7 participants

@calavera @cpuguy83 @thaJeztah @diogomonica @jessfraz @tiborvass @GordonTheTurtle