Produce errors when empty ids are passed into inspect calls. by emil2k · Pull Request #36144 · moby/moby

emil2k · 2018-01-30T12:42:16Z

- What I did

Made the client validate when empty IDs are passed into inspect methods to prevent calling the wrong endpoint and returning the wrong error.

Previously, if for example a blank nodeID was passed in to NodeInspectWithRaw the result was a node list request. The response would then fail to unmarshal into a Node type returning a JSON error.

- How I did it

Checked ids passed into inspect calls and returned an error if they were empty.

- How to verify it

Test added.

- Description for the changelog
Produce errors when empty ids are passed into inspect calls.

thaJeztah · 2018-01-30T19:56:27Z

client/node_inspect.go

I'm not sure this is needed. Other object types don't have this check (e.g.

moby/client/service_inspect.go

Lines 15 to 34 in 81bb997

// ServiceInspectWithRaw returns the service information and the raw data.

func (cli *Client) ServiceInspectWithRaw(ctx context.Context, serviceID string, opts types.ServiceInspectOptions) (swarm.Service, []byte, error) {

query := url.Values{}

query.Set("insertDefaults", fmt.Sprintf("%v", opts.InsertDefaults))

serverResp, err := cli.get(ctx, "/services/"+serviceID, query, nil)

if err != nil {

return swarm.Service{}, nil, wrapResponseError(err, serverResp, "service", serviceID)

}

defer ensureReaderClosed(serverResp)

body, err := ioutil.ReadAll(serverResp.body)

if err != nil {

return swarm.Service{}, nil, err

}

var response swarm.Service

rdr := bytes.NewReader(body)

err = json.NewDecoder(rdr).Decode(&response)

return response, body, err

}

)

There's already validation in place on the CLI;

$ docker node inspect "docker node inspect" requires at least 1 argument.

And the API server performs additional validation;

$ curl --unix-socket /var/run/docker.sock http://localhost/nodes/ {"message":"page not found"}

Not entirely against, but I want to guard against having validation in the client where it's not needed, and try to keep as much as possible on the daemon side

Unfortunately I think this is necessary. I added a similar check for volumes in #34770.

The problem is in building the url. If the arg is blank, the url path ends up being /nodes, which is a valid URL (https://docs.docker.com/engine/api/v1.30/#operation/NodeList). I'm not sure why that curl failed.

I wish there was a better way of handling this, but I haven't found anything yet.

Difference is in the trailing /; without a trailing /, it's "list nodes", and with a trailing /, it's "view/inspect node"

Ah, right! So I guess the line below this should be using path.Join() to construct the url

Difference is in the trailing /; without a trailing /, it's "list nodes", and with a trailing /, it's "view/inspect node"

@thaJeztah My client was making a request for nodes/ with the trailing /, but was receiving the results from the list nodes endpoint, so maybe there is an issue with the routing.

I found a go-swagger issue that seems to have removed any meaning associated with a trailing slash explaining this behavior:
go-swagger/go-swagger#906
go-swagger/go-swagger#899 (comment)

This could have affected your API server's routing. You might not be seeing this on your local setup because your Makefile is using an old version of go-swagger:

.PHONY: swagger-gen swagger-gen: docker run --rm -v $(PWD):/go/src/github.com/docker/docker \ -w /go/src/github.com/docker/docker \ --entrypoint hack/generate-swagger-api.sh \ -e GOPATH=/go \ quay.io/goswagger/swagger:0.7.4

While the commit that made these changes was added in version 9.0 of go-swagger:
go-swagger/go-swagger@5ef60f1

Meanwhile, the commit being used in the Dockerfile to build Docker is using a commit that points to version 0.13 of go-wrapper:

# Install go-swagger for validating swagger.yaml ENV GO_SWAGGER_COMMIT c28258affb0b6251755d92489ef685af8d4ff3eb RUN git clone https://github.com/go-swagger/go-swagger.git /go/src/github.com/go-swagger/go-swagger \ && (cd /go/src/github.com/go-swagger/go-swagger && git checkout -q $GO_SWAGGER_COMMIT) \ && go install -v github.com/go-swagger/go-swagger/cmd/swagger

go-swagger/go-swagger@c28258a

Here are my client/server versions:

Client: Version: 17.11.0-ce API version: 1.30 (downgraded from 1.34) Go version: go1.9.2 Git commit: 1caf76c Built: unknown-buildtime OS/Arch: darwin/amd64 Server: Version: 17.06.2-ce API version: 1.30 (minimum version 1.12) Go version: go1.8.4 Git commit: 402dd4a/17.06.2-ce Built: Fri Nov 10 00:51:08 2017 OS/Arch: linux/amd64 Experimental: false

dnephin · 2018-01-30T21:54:53Z

client/node_inspect.go

We should be consistent here, how about an error like this: https://github.com/moby/moby/pull/34770/files#diff-18fbe225ba23b481ee707e859ff03b0cR22

and a test case

I updated the error to be consistent with the volume inspect call and added a similar test case.

emil2k · 2018-02-05T15:44:48Z

I think maybe this comment got lost, because it was in a discussion on an old diff:
#36144 (comment)

I'm referring to it here because I think it is relevant to the issue at hand.

thaJeztah · 2018-02-06T02:36:12Z

Thanks @emil2k - regarding the trailing slash; this was the PR I had in mind; #24634. I recall a discussion somewhere where it was suggested that having a trailing slash (or not) should route to a different endpoint (but I wasn't really in favour of that). Perhaps it's worth to verify the behaviour of relevant endpoints.

For this change: I discussed this with @dnephin (I thought I left a comment, but apparently forgot 😅), and we agreed that having a validation for empty values in the client does make sense.

If we make this change, I'd prefer it to be done for all similar endpoints (i.e. all endpoints that expect an ID should produce an error indicating that the ID was empty - if it's not currently handled like that)

If a blank nodeID was previously passed in it resulted in a node list request. The response would then fail to umarshal into a `Node` type returning a JSON error. This adds an extra validation to all inspect calls to check that the ID that is required is provided and if not return an error. Signed-off-by: Emil Davtyan <emil2k@gmail.com>

emil2k · 2018-02-07T13:13:48Z

@thaJeztah I added this check and tests to all the inspect calls.

thaJeztah

LGTM, thanks!

@dnephin PTAL

dnephin · 2018-02-15T19:45:36Z

client/volume_inspect.go


 	var volume types.Volume
-	resp, err := cli.get(ctx, path.Join("/volumes", volumeID), nil, nil)
+	resp, err := cli.get(ctx, "/volumes/"+volumeID, nil, nil)


Why this change? I think using path.Join() is correct

Looks like we use concatenation in all the other endpoints

I changed it to be consistent with the other similar methods. Also for simplicity, I could not see a case where it would be necessary to use path.Join over the simple concat, but I maybe overlooking something.

I think the only case where it would be different is the empty string value. I guess it's fine, we can fix them all at once at some later time.

dnephin

LGTM

emil2k requested a review from dnephin as a code owner January 30, 2018 12:42

GordonTheTurtle added the status/0-triage label Jan 30, 2018

vdemeester added status/1-design-review and removed status/0-triage labels Jan 30, 2018

vdemeester requested a review from thaJeztah January 30, 2018 19:24

thaJeztah requested changes Jan 30, 2018

View reviewed changes

dnephin reviewed Jan 30, 2018

View reviewed changes

emil2k force-pushed the node-id-required branch from 98943d8 to 0eabcc7 Compare January 31, 2018 10:15

emil2k force-pushed the node-id-required branch from 0eabcc7 to e4185dd Compare February 7, 2018 12:43

emil2k force-pushed the node-id-required branch from e4185dd to 3e6bbef Compare February 7, 2018 13:05

emil2k changed the title ~~Produce "node id is required" error.~~ Produce errors when empty ids are passed into inspect calls. Feb 7, 2018

GordonTheTurtle assigned tonistiigi Feb 14, 2018

thaJeztah added status/2-code-review and removed status/1-design-review labels Feb 15, 2018

thaJeztah approved these changes Feb 15, 2018

View reviewed changes

dnephin reviewed Feb 15, 2018

View reviewed changes

dnephin approved these changes Feb 16, 2018

View reviewed changes

tonistiigi approved these changes Feb 16, 2018

View reviewed changes

tonistiigi merged commit db36099 into moby:master Feb 16, 2018

emil2k deleted the node-id-required branch February 16, 2018 20:28

thaJeztah mentioned this pull request Feb 20, 2018

Bump moby vendor and dependencies docker/cli#892

Merged

	// ServiceInspectWithRaw returns the service information and the raw data.
	func (cli *Client) ServiceInspectWithRaw(ctx context.Context, serviceID string, opts types.ServiceInspectOptions) (swarm.Service, []byte, error) {
	query := url.Values{}
	query.Set("insertDefaults", fmt.Sprintf("%v", opts.InsertDefaults))
	serverResp, err := cli.get(ctx, "/services/"+serviceID, query, nil)
	if err != nil {
	return swarm.Service{}, nil, wrapResponseError(err, serverResp, "service", serviceID)
	}
	defer ensureReaderClosed(serverResp)

	body, err := ioutil.ReadAll(serverResp.body)
	if err != nil {
	return swarm.Service{}, nil, err
	}

	var response swarm.Service
	rdr := bytes.NewReader(body)
	err = json.NewDecoder(rdr).Decode(&response)
	return response, body, err
	}

Conversation

emil2k commented Jan 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emil2k commented Feb 5, 2018

Uh oh!

thaJeztah commented Feb 6, 2018

Uh oh!

emil2k commented Feb 7, 2018

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnephin left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

emil2k commented Jan 30, 2018 •

edited

Loading