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.

Sign up for GitHub

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

Jump to bottom

Validate hostname starting from 1.24 API. #23740

Merged
merged 1 commit into from
Jul 6, 2016
Merged

Validate hostname starting from 1.24 API. #23740

merged 1 commit into from
Jul 6, 2016

Conversation

vdemeester
Copy link
Member

@vdemeester vdemeester commented Jun 19, 2016 •
edited

In order to keep a little bit of "sanity" on the API side, validate hostname only starting from v1.24 API version 🐯.

Fixes #23729.

/cc @thaJeztah @tiborvass @icecrime

🐸

Signed-off-by: Vincent Demeester vincent@sbr.pm

@vdemeester vdemeester added this to the 1.12.0 milestone Jun 19, 2016
@vdemeester vdemeester force-pushed the 23729-validate-hostname-conditionnaly branch from 4e441e4 to 1e23537 Compare June 19, 2016 17:09
@thaJeztah
Copy link
Member

❤️ thank you!

/cc @ibuildthecloud

@icecrime
Copy link
Contributor

Cc @alena1108: works for you?

@alena1108
Copy link
Contributor

@icecrime yup, thank you!

@thaJeztah
Copy link
Member

LGTM

@tiborvass
Copy link
Contributor

I'm not fond of passing version around in the API :S not sure what the alternative is...

@runcom
Copy link
Member

runcom commented Jul 4, 2016 •
edited

can't we have a bool argument instead?

version := httputils.VersionFromContext(ctx)
validateHostname := versions.GreaterThanOrEqualTo(version, "1.24")

and then pass around validateHostname so version remains in the http layer and what we have inside it's just an argument (maybe some day we'll chose to validate the hostname based on something else)

@vdemeester
Copy link
Member Author

vdemeester commented Jul 4, 2016 •
edited

@runcom That's a good idea 😉

@vdemeester vdemeester force-pushed the 23729-validate-hostname-conditionnaly branch from 1e23537 to 46dd0ea Compare July 4, 2016 13:40
@vdemeester
Copy link
Member Author

Rebased and update 👼
@runcom @tiborvass wdyt ?

@runcom
Copy link
Member

runcom commented Jul 4, 2016

👍 LGTM

@tiborvass
Copy link
Contributor

@vdemeester TestParseWords is failing

@vdemeester
Copy link
Member Author

@tiborvass hum… I'll look into it 👼

@vdemeester vdemeester force-pushed the 23729-validate-hostname-conditionnaly branch from 46dd0ea to 795ed7d Compare July 5, 2016 19:10
@vdemeester
Copy link
Member Author

@tiborvass rebase because I don't reproduce the failure of this test 😓 (which is worrying me a bit 😭 ).

@vdemeester vdemeester force-pushed the 23729-validate-hostname-conditionnaly branch from 795ed7d to 17fc51c Compare July 5, 2016 19:13
@tiborvass
Copy link
Contributor

Much better! LGTM

@thaJeztah
Copy link
Member

relates to #21641, #21595

@thaJeztah
Copy link
Member

@vdemeester can you add a mention of this change to https://github.com/docker/docker/blob/master/docs/reference/api/docker_remote_api.md#v124-api-changes ?

@thaJeztah
Copy link
Member

Perhaps we should mention it in the "HostName" JSON option in https://github.com/docker/docker/blob/master/docs/reference/api/docker_remote_api_v1.24.md#create-a-container as well (that it must be a valid RFC 1123 hostname, per #20566)

@vdemeester
Validate hostname starting from 1.24 API.
6daf3d2 
In order to keep a little bit of "sanity" on the API side, validate
hostname only starting from v1.24 API version.

Signed-off-by: Vincent Demeester <vincent@sbr.pm>
@vdemeester vdemeester force-pushed the 23729-validate-hostname-conditionnaly branch from 17fc51c to 6daf3d2 Compare July 6, 2016 07:14
@vdemeester
Copy link
Member Author

@thaJeztah updated 👼

thaJeztah
thaJeztah reviewed Jul 6, 2016
View reviewed changes
docs/reference/api/docker_remote_api.md
@@ -133,9 +133,10 @@ This section lists each version from latest to oldest. Each listing includes a
* `POST /containers/{name:.*}/copy` is now removed and errors out starting from this API version.
* API errors are now returned as JSON instead of plain text.
* `POST /containers/create` and `POST /containers/(id)/start` allow you to configure kernel parameters (sysctls) for use in the container.
* `POST /v1.23/containers/<container ID>/exec` and `POST /v1.23/exec/<exec ID>/start`
* `POST /containers/<container ID>/exec` and `POST /exec/<exec ID>/start`
Copy link
Member

Choose a reason for hiding this comment

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

Thx 😊

Copy link
Member Author

Choose a reason for hiding this comment

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

😝

@thaJeztah
Copy link
Member

LGTM, thanks!

@vdemeester vdemeester merged commit 2cc5383 into moby:master Jul 6, 2016
@vdemeester vdemeester deleted the 23729-validate-hostname-conditionnaly branch July 6, 2016 09:38
@thaJeztah thaJeztah mentioned this pull request Jul 7, 2016
Closed
@tiborvass tiborvass mentioned this pull request Jul 12, 2016
Merged
@vdemeester vdemeester mentioned this pull request Nov 30, 2016
Closed
@deniseschannon deniseschannon mentioned this pull request Dec 7, 2016
Closed
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
area/distribution process/cherry-picked status/4-merge
Projects
None yet
Milestone
1.12.0
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@vdemeester @thaJeztah @icecrime @alena1108 @tiborvass @runcom @GordonTheTurtle