Add httpHeaders to httpGet liveness probe #20116

therc · 2016-01-25T23:00:28Z

Also update existing documentation and try to steer users away from 'host'.

Fixes #14713

cc: @thockin

Release Note:

Allow HTTP readinessProbes and livenessProbes to include a list of arbitrary HTTP headers.

k8s-github-robot · 2016-01-25T23:06:30Z

Labelling this PR as size/L

lavalamp · 2016-01-25T23:53:54Z

Oh, I was just wanting exactly this, neat. We're in the middle of fixing flaky tests so it may be a few days before I get to an actual review.

k8s-bot · 2016-01-26T02:33:49Z

GCE e2e test build/test passed for commit 3b7edc4821d8dc0ad2e74cba74103152b4b10d0c.

k8s-github-robot · 2016-01-26T02:33:59Z

The author of this PR is not in the whitelist for merge, can one of the admins add the 'ok-to-merge' label?

k8s-bot · 2016-01-26T16:50:13Z

GCE e2e test build/test passed for commit 4cc5122ad62c49a37c83d2dc003b53aa05a9b527.

k8s-bot · 2016-01-26T18:54:33Z

GCE e2e test build/test passed for commit 46817ee9eb2c3b37789da96216e03459adefd8f6.

thockin · 2016-01-28T07:39:42Z

docs/user-guide/liveness/README.md

@@ -66,11 +66,13 @@ The [http-liveness.yaml](http-liveness.yaml) demonstrates the HTTP check.
      httpGet:
        path: /healthz
        port: 8080
+        httpHeaders:
+          X-Custom-Header: [ Awesome ]


what doe this being a list of values mean?

That's the actual type for http.Header. It results in the same header being output multiple times, with different values for each occurrence. I guess it might make sense for e.g. chains of X-Forwarded-For? Or for cookies, see http://stackoverflow.com/questions/3241326/set-more-than-one-http-header-with-the-same-name

HTTP lets you specify headers multiple times. Generally I find this feature very annoying, but we definitely should support it.

Sure we should support it, but should we support is as a multi-map (like this) or as a list of {key, value} (which feels more natural to me and less "because that is how Go does it).

Yeah, that's a good question. @thockin's way is probably more consistent with the rest of the API but the current way is going to be a lot more compact. I don't think there's much risk of us wanting to add other fields in here, so I guess I have an extremely minor preference for the current way. up to @thockin

I think the vast majority of cases will be one key, one value. Our API (and the documented conventions) eschew maps where the value is more than a simple primitive.

This can be a list of (key, value) or if you reallllly want to the map-like semantic a list of { key, []value}.

thockin · 2016-01-28T07:48:27Z

For defaulting you should init a nil map to be an empty map.

For validation, I don't know that there's much to validate. Does the HTTP spec say anything about what is allowed in headers, or max number of them or sizes?

therc · 2016-01-28T17:58:14Z

For validation, the RFC allows header field names to use alphanumeric plus a bunch of other ASCII characters (!#$%&+-, etc. but not :,; and others). The Go HTTP library, though, makes MIME and HTTP header canonical with a stricter subset: [A-Za-z0-9-]. I think I'll go with the latter, since it wouldn't make sense to specify characters that might result in erratic behaviour.

thockin · 2016-01-28T18:26:47Z

In general, we can always expand the supported character set if needed, but
we can never make it more restrictive.

On Thu, Jan 28, 2016 at 9:58 AM, Rudi C notifications@github.com wrote:

For validation, the RFC allows alphanumeric plus a bunch of other ASCII
characters (!#$%&+-, etc. but not :,; and others). The Go HTTP library,
though, makes MIME and HTTP header canonical with a stricter subset:
[A-Za-z0-9-]. I think I'll go with the latter, since it wouldn't make sense
to specify characters that might result in erratic behaviour.

—
Reply to this email directly or view it on GitHub
#20116 (comment)
.

therc · 2016-01-28T19:51:25Z

PTAL.
Verbose logging is gone. I added defaulting, validation (added to pkg/util/validation, too) and better comments about HTTP allowing repeated fields. I didn't squash the changes yet, as I think that will lose comment history.

k8s-bot · 2016-01-28T20:54:20Z

GCE e2e test build/test passed for commit 38ea010b8a14b6b1d0c88242e91bd7bcdd1cd3b2.

k8s-bot · 2016-01-28T21:20:26Z

GCE e2e test build/test passed for commit 8d880ad1fabca3481e9b43ce7bd1c9268602c1dc.

k8s-github-robot · 2016-01-28T22:10:03Z

PR needs rebase

k8s-bot · 2016-01-28T22:13:19Z

GCE e2e test build/test passed for commit ad88a00d9484f22312047af18c7cbae5588abb37.

lavalamp · 2016-01-28T23:14:21Z

I looked over this and have no objections. switching reviewer to @thockin

therc · 2016-01-28T23:17:49Z

Just fixing the testLabels test. It tripped over the defaulting. :-( That and rebasing generated code being painful.

thockin · 2016-02-04T04:50:20Z

Sorry, it's a busy project with a few heavily contended files...

k8s-github-robot · 2016-02-04T17:30:52Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-bot · 2016-02-04T18:15:12Z

GCE e2e build/test failed for commit e330c0bb5d5a2303682f62d88dfc0528e8e2cf85.

therc · 2016-02-04T19:28:02Z

"There is no hell, only the Kubernetes submit queue."

I'm assuming this is a flake and doesn't need a rebase.

k8s-github-robot · 2016-02-04T19:38:50Z

@therc
You must link to the test flake issue which caused you to request this manual re-test.
Re-test requests should be in the form of: k8s-bot test this issue: #<number>
Here is the list of open test flakes.

therc · 2016-02-04T19:44:09Z

k8s-bot test this issue: #20631

thockin · 2016-02-05T00:13:39Z

I'll keep my eye on this one

k8s-bot · 2016-02-05T14:43:39Z

GCE e2e test build/test passed for commit 2681f884b46c0cb9b3ef5ddecc8102c9abef3ae1.

k8s-github-robot · 2016-02-05T15:22:57Z

PR needs rebase

thockin · 2016-02-05T16:05:56Z

Sorry, rebase again. It's on my screen all-day in case of any delays :)

Also update existing documentation and try to steer users away from 'host'. Add validation.

therc · 2016-02-05T16:37:25Z

Even preemptive rebasing was pointless...

k8s-bot · 2016-02-05T16:54:41Z

GCE e2e test build/test passed for commit a2d1bb7.

thockin · 2016-02-05T17:19:05Z

@k8s-oncall this has been a long-suffering PR, if there's bandwidth to merge it, that would be very gracious :)

lavalamp · 2016-02-05T17:38:02Z

This is actually 2nd in the queue right now, and everything is green. I will just let the queue merge this unless I go on a manual merging spree--will keep this tab open.

k8s-github-robot · 2016-02-05T17:55:22Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-bot · 2016-02-05T18:28:08Z

GCE e2e test build/test passed for commit a2d1bb7.

k8s-github-robot · 2016-02-05T18:28:31Z

Automatic merge from submit-queue

Auto commit by PR queue bot

therc · 2016-02-05T18:31:14Z

Woo hoo!
http://cdn.meme.am/instances2/500x/4167846.jpg

googlebot added the cla: yes label Jan 25, 2016

k8s-github-robot assigned lavalamp Jan 25, 2016

k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 25, 2016

k8s-github-robot added the needs-ok-to-merge label Jan 26, 2016

therc force-pushed the http-headers branch from 3b7edc4 to 4cc5122 Compare January 26, 2016 16:10

therc force-pushed the http-headers branch from 4cc5122 to 46817ee Compare January 26, 2016 18:18

thockin reviewed Jan 28, 2016
View reviewed changes

therc force-pushed the http-headers branch from 46817ee to c8ebb96 Compare January 28, 2016 19:47

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 28, 2016

lavalamp assigned thockin and unassigned lavalamp Jan 28, 2016

thockin added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 4, 2016

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 4, 2016

therc force-pushed the http-headers branch from e330c0b to 2681f88 Compare February 5, 2016 14:13

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 5, 2016

Add httpHeaders to httpGet liveness probe

a2d1bb7

Also update existing documentation and try to steer users away from 'host'. Add validation.

therc force-pushed the http-headers branch from 2681f88 to a2d1bb7 Compare February 5, 2016 16:20

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 5, 2016

thockin added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 5, 2016

k8s-github-robot pushed a commit that referenced this pull request Feb 5, 2016

Merge pull request #20116 from Clarifai/http-headers

87fbfdc

Auto commit by PR queue bot

k8s-github-robot merged commit 87fbfdc into kubernetes:master Feb 5, 2016

therc deleted the http-headers branch February 5, 2016 18:37

errm mentioned this pull request Apr 14, 2016

httpHeaders in httpGet probe does not work for Host header. #24288

Closed

Add httpHeaders to httpGet liveness probe #20116

Add httpHeaders to httpGet liveness probe #20116

Conversation

therc commented Jan 25, 2016

k8s-github-robot commented Jan 25, 2016

lavalamp commented Jan 25, 2016

k8s-bot commented Jan 26, 2016

k8s-github-robot commented Jan 26, 2016

k8s-bot commented Jan 26, 2016

k8s-bot commented Jan 26, 2016

thockin Jan 28, 2016

Choose a reason for hiding this comment

therc Jan 28, 2016

Choose a reason for hiding this comment

lavalamp Jan 28, 2016

Choose a reason for hiding this comment

thockin Jan 28, 2016

Choose a reason for hiding this comment

lavalamp Jan 28, 2016

Choose a reason for hiding this comment

thockin Jan 29, 2016

Choose a reason for hiding this comment

thockin commented Jan 28, 2016

therc commented Jan 28, 2016

thockin commented Jan 28, 2016

therc commented Jan 28, 2016

k8s-bot commented Jan 28, 2016

k8s-bot commented Jan 28, 2016

k8s-github-robot commented Jan 28, 2016

k8s-bot commented Jan 28, 2016

lavalamp commented Jan 28, 2016

therc commented Jan 28, 2016

thockin commented Feb 4, 2016

k8s-github-robot commented Feb 4, 2016

k8s-bot commented Feb 4, 2016

therc commented Feb 4, 2016

k8s-github-robot commented Feb 4, 2016

therc commented Feb 4, 2016

thockin commented Feb 5, 2016

k8s-bot commented Feb 5, 2016

k8s-github-robot commented Feb 5, 2016

thockin commented Feb 5, 2016

therc commented Feb 5, 2016

k8s-bot commented Feb 5, 2016

thockin commented Feb 5, 2016

lavalamp commented Feb 5, 2016

k8s-github-robot commented Feb 5, 2016

k8s-bot commented Feb 5, 2016

k8s-github-robot commented Feb 5, 2016

therc commented Feb 5, 2016