Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

validate healthcheck params in daemon side#30203

Merged
cpuguy83 merged 1 commit intomoby:mastermoby/moby:masterfrom
allencloud:validate-healthcheck-params-in-daemon-sideallencloud/docker:validate-healthcheck-params-in-daemon-sideCopy head branch name to clipboard
Feb 2, 2017
Merged

validate healthcheck params in daemon side#30203
cpuguy83 merged 1 commit intomoby:mastermoby/moby:masterfrom
allencloud:validate-healthcheck-params-in-daemon-sideallencloud/docker:validate-healthcheck-params-in-daemon-sideCopy head branch name to clipboard

Conversation

@allencloud
Copy link
Copy Markdown
Contributor

@allencloud allencloud commented Jan 17, 2017

Signed-off-by: allencloud allen.sun@daocloud.io

fixes #30202

validate user input healthcheck parameters via API in the daemon side.

- What I did

  1. validate healthcheck in daemon side
  2. add a test for api testing invalid healthcheck parameters
  3. add missing --health-retries validation in client side

- How I did it

- How to verify it

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

@allencloud allencloud force-pushed the validate-healthcheck-params-in-daemon-side branch 8 times, most recently from 19f76c9 to 131d5ef Compare January 17, 2017 10:54
@allencloud allencloud force-pushed the validate-healthcheck-params-in-daemon-side branch 2 times, most recently from 6b6f784 to eee33a9 Compare January 18, 2017 04:11
Comment thread daemon/container.go Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this should validate less than 1s, wdyt as I don't think we can guarantee sub-second checking.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If that, maybe we need to use a consistent validation in the whole procedure. I think there are three places to validate:

  1. client side;
  2. daemon received parameters from API;
  3. parameters from dockerfile when building

I think consistency is the first thing(no matter less than 0 or less than 1s), the second thing to add more specific explanation in docs.

But currently, I am on less than 0. 😄 Open for input.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sure, consistent is good, but the daemon side is the "important" piece.
I just think if we are going to add a validation, let's make sure it's not letting non-sensical values.

@LK4D4
Copy link
Copy Markdown
Contributor

LK4D4 commented Jan 27, 2017

@cpuguy83 there is question for you here :)

@allencloud allencloud force-pushed the validate-healthcheck-params-in-daemon-side branch 5 times, most recently from dc8c202 to 6ce0fed Compare January 28, 2017 15:35
@allencloud
Copy link
Copy Markdown
Contributor Author

Hi @cpuguy83 @LK4D4
Thanks for the review. Currently I updated this PR with modifying the validation from 0 to 1s. And in additional, I added more specific details in docs swagger.yml and update api 1.24, since healthcheck is introduced in docker 1.12.x and api 1.24. PTAL, thanks 😄

@allencloud allencloud force-pushed the validate-healthcheck-params-in-daemon-side branch 2 times, most recently from 49df201 to b88b9a8 Compare January 29, 2017 03:23
Signed-off-by: allencloud <allen.sun@daocloud.io>
@allencloud allencloud force-pushed the validate-healthcheck-params-in-daemon-side branch from b88b9a8 to e399c55 Compare January 29, 2017 05:35
@LK4D4
Copy link
Copy Markdown
Contributor

LK4D4 commented Jan 30, 2017

LGTM

Copy link
Copy Markdown
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

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

LGTM

@cpuguy83 cpuguy83 merged commit bb0a532 into moby:master Feb 2, 2017
@GordonTheTurtle GordonTheTurtle added this to the 1.14.0 milestone Feb 2, 2017
@allencloud allencloud deleted the validate-healthcheck-params-in-daemon-side branch February 2, 2017 04:27
dnephin pushed a commit to dnephin/docker that referenced this pull request Apr 17, 2017
…rams-in-daemon-side

validate healthcheck params in daemon side
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Docker Daemon does not validate healthcheck param

5 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.