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 dockerfile parser failing silently on long tokens #35429

Merged
merged 2 commits into from Nov 15, 2017

Conversation

Projects
None yet
7 participants
@dnephin
Member

dnephin commented Nov 7, 2017

Fixes docker/for-linux#157
Fixes docker/for-mac#2208

Report errors when the scanner fails. The error message should include enough details for the user to fix the problem.

Fixing the first problem revealed another problem with handling remote urls that point at Dockerfiles, which is fixed in the second and third commits.

@tonistiigi

Could you squash 2nd and 3rd commit

@@ -110,7 +86,7 @@ func GetWithStatusError(address string) (resp *http.Response, err error) {
// - an io.Reader for the response body
// - an error value which will be non-nil either when something goes wrong while
// reading bytes from r or when the detected content-type is not acceptable.
func inspectResponse(ct string, r io.Reader, clen int64) (string, io.ReadCloser, error) {
func inspectResponse(ct string, r io.Reader, clen int64) (string, io.Reader, error) {

This comment has been minimized.

@tonistiigi

tonistiigi Nov 9, 2017

Member

Would it be cleaner for this function just to take a ReadCloser and return ReadCloser so caller doesn't need to do any wrapping?

@tonistiigi

tonistiigi Nov 9, 2017

Member

Would it be cleaner for this function just to take a ReadCloser and return ReadCloser so caller doesn't need to do any wrapping?

This comment has been minimized.

@dnephin

dnephin Nov 9, 2017

Member

I guess you mean move the ioutils.NewReadCloserWrapper() into inspectResponse() ? I think either way it needs to be wrapped, it's just about where it happens.

That could work. It would mean that the two four error returns in this function would have to call r.Close() instead of the one error case in the caller calling closer.

It also seems like it is more the responsibility of downloadRemote() since it is aware that it's a response body, where as inspectResponse only cares about a reader.

I have a very slight preference for the current implementation, but if you think it's cleaner to move it to inspectResponse I can do that.

I can also remove closer() that doesn't need to exist anymore, it can just be response.Body.Close

@dnephin

dnephin Nov 9, 2017

Member

I guess you mean move the ioutils.NewReadCloserWrapper() into inspectResponse() ? I think either way it needs to be wrapped, it's just about where it happens.

That could work. It would mean that the two four error returns in this function would have to call r.Close() instead of the one error case in the caller calling closer.

It also seems like it is more the responsibility of downloadRemote() since it is aware that it's a response body, where as inspectResponse only cares about a reader.

I have a very slight preference for the current implementation, but if you think it's cleaner to move it to inspectResponse I can do that.

I can also remove closer() that doesn't need to exist anymore, it can just be response.Body.Close

This comment has been minimized.

@tonistiigi

tonistiigi Nov 9, 2017

Member

ok, lets leave it like it is atm

@tonistiigi

tonistiigi Nov 9, 2017

Member

ok, lets leave it like it is atm

@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Nov 9, 2017

Member

Could you squash 2nd and 3rd commit

Yes, definitely. Just wanted to make sure you were happy with the final result before squashing. Will do that now.

Member

dnephin commented Nov 9, 2017

Could you squash 2nd and 3rd commit

Yes, definitely. Just wanted to make sure you were happy with the final result before squashing. Will do that now.

dnephin added some commits Nov 7, 2017

Fix dockerfile parser failing silently on long tokens
Signed-off-by: Daniel Nephin <dnephin@docker.com>
Fix remote build target as Dockerfile
The test was passing previously because the preamble was already buffered. After
the change to return Scanner.Err() the final read error on the buffer was no
longer being ignored.

Signed-off-by: Daniel Nephin <dnephin@docker.com>
@tonistiigi

LGTM

@tonistiigi tonistiigi removed their assignment Nov 9, 2017

@dnephin dnephin requested review from tiborvass and thaJeztah Nov 10, 2017

@thaJeztah

LGTM

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah
Member

thaJeztah commented Nov 11, 2017

ping @tiborvass

@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Nov 15, 2017

Member

Ok to merge this?

Member

dnephin commented Nov 15, 2017

Ok to merge this?

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Nov 15, 2017

Member

Ah, yes, I think so 👍

Member

thaJeztah commented Nov 15, 2017

Ah, yes, I think so 👍

@thaJeztah thaJeztah merged commit 7c53e73 into moby:master Nov 15, 2017

6 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 37780 has succeeded
Details
janky Jenkins build Docker-PRs 46491 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 6899 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 18050 has succeeded
Details
z Jenkins build Docker-PRs-s390x 6709 has succeeded
Details

@dnephin dnephin deleted the dnephin:build-fails-on-long-label branch Nov 15, 2017

@disarticulate

This comment has been minimized.

Show comment
Hide comment
@disarticulate

disarticulate Jan 2, 2018

Is this bug elsewhere?

I'm getting a null response on something like

"Entrypoint":[
	"/gnatsd",
	"-DV",
	"--tls",
	"--tlskey=/run/secrets/meleorkey",
	"--tlscert=/run/secrets/meleorcrt",
	"--tlscacert=/run/secrets/rootca",
	"--auth=SuperSecretAuthorizationIsForFoolsAndFillers"
],

disarticulate commented Jan 2, 2018

Is this bug elsewhere?

I'm getting a null response on something like

"Entrypoint":[
	"/gnatsd",
	"-DV",
	"--tls",
	"--tlskey=/run/secrets/meleorkey",
	"--tlscert=/run/secrets/meleorcrt",
	"--tlscacert=/run/secrets/rootca",
	"--auth=SuperSecretAuthorizationIsForFoolsAndFillers"
],
@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Jan 2, 2018

Member

@disarticulate this fix is included in Docker 17.12 and up

Member

thaJeztah commented Jan 2, 2018

@disarticulate this fix is included in Docker 17.12 and up

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment