fix: remove case sensitive checking of probe headers #114606

tuunit · 2022-12-20T13:30:12Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

This fixes a bug related to the default Accept header in probes introduced in release 1.20.

When I upgraded one of our older clusters from release 1.19 to one of the latest versions some of our readiness probes didn't work anymore. So I had a look into the code and realized in release 1.20 there was a change to include a default Accept header:

Original change: 0794bf6

Release notes: https://github.com/kubernetes/kubernetes/blob/master/CHANGELOG/CHANGELOG-1.20.md

Changed: default "Accept: /" header added to HTTP probes. See https://kubernetes.io/docs/tasks/configure-pod-container/configure-liveness-readiness-startup-probes/#http-probes (kubernetes/website#24756) (#95641, @fonsecas72) [SIG Network and Node]

After this investigation, I checked the master branch and realized, that the issue is still present. The http.Header type from the "net/http" package is not properly used and therefore handling of the casing / capitalization of headers is not correctly done. Which leads to failing probes when a custom "Accept" header is defined with lower casing like so "accept".

Location of the issue:

kubernetes/pkg/probe/http/request.go

Lines 113 to 119 in 05a0ce8

 func v1HeaderToHTTPHeader(headerList []v1.HTTPHeader) http.Header { 

 headers := make(http.Header) 

 for _, header := range headerList { 

 headers[header.Name] = append(headers[header.Name], header.Value) 

 } 

 return headers 

 }

The issue can be replicated with a simple go script, as it is related to how the http.Header type from the net/http package is used:

package main

import (
	"fmt"
	"net/http"
)

func main() {
	headers := http.Header{}

	// A deployment is done with a lower cased "accept" header
	headers["accept"] = append(headers["accept"], "application/json")

	// The check is not done via headers.Get() but instead via the array notation (which is case sensitive)
	if _, ok := headers["Accept"]; !ok {
		headers.Set("Accept", "*/*")
	}

	// Leading to the following misleading output.
	// In which case only the capital cased "Accept" header is then used with
	// the default value of "*/*" for the probing and because some applications
	// might enforce the specified header with a value of "application/json" the
	// probes will fail.
	fmt.Printf("headers.Get(\"Accept\"): %v\n", headers.Get("Accept"))   // Output: headers.Get("Accept"): */*
	fmt.Printf("headers.Get(\"accept\"): %v\n", headers.Get("accept"))   // Output: headers.Get("accept"): */*
	fmt.Printf("    headers[\"accept\"]: %v\n", headers["accept"])       // Output: headers["accept"]: [application/json]
}

Special notes for your reviewer:

The currently proposed solution will enforce canonical keys for all headers. For example if a deployment defines a probe header with lowercasing like so "accept-encoding" it will be converted to "Accept-Encoding" and will be send in this format to the underlying application unlike before it was stored and send as "accept-encoding". Only the special cases of "Accept" and "User-Agent" where enforced to be in the canonical standard.
/sig node
/sig network

Does this PR introduce a user-facing change?

Fixed a bug that unintentionally overrides your custom Accept headers in http (live-/readiness)-probes if the header is in lower casing

k8s-ci-robot · 2022-12-20T13:30:20Z

Welcome @tuunit!

It looks like this is your first PR to kubernetes/kubernetes 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/kubernetes has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2022-12-20T13:30:21Z

Hi @tuunit. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

linux-foundation-easycla · 2022-12-20T13:30:26Z

The committers listed above are authorized under a signed CLA.

✅ login: tuunit / name: Jan Larwig (6c0d29fdd380195c1d28478c9c47535e050d2c41, 1ba944ac548590b8be37aa4b490555a1c0cc4596, a096969459bd3ad3bfd0d8b94f9543bc204bf48d, 488f5aa1a6457fcb9709f85aad9aed907625ceee, a430d42ec6757740edf2cbb49f7598a2c302a897, 8dd6e4fc6228231a4d5eadba6d3b17fbecde387a)

tuunit · 2022-12-23T09:19:57Z

@mrunalp @bobbypage anything I can do to further assist you with this PR? Is my explanation clear enough? If necessary I can add an actual deployment example that would fail without the fix.

Happy christmas 🎄

bart0sh · 2022-12-27T18:47:25Z

/ok-to-test

bart0sh · 2022-12-27T18:47:59Z

@tuunit please, squash your commits. otherwise lgtm

tuunit · 2022-12-27T19:32:10Z

/test pull-kubernetes-unit

tuunit · 2023-01-03T13:14:33Z

@bobbypage @mrunalp ready for final review 😄

Happy New Year 🥳

tuunit · 2023-01-19T13:09:16Z

@bart0sh @bobbypage @mrunalp any updates on the when this will be reviewed / merged?

bart0sh · 2023-01-20T10:25:05Z

/assign @derekwaynecarr @mrunalp @dchen1107

tuunit · 2023-02-22T18:41:37Z

Any updates on the review?
@derekwaynecarr @mrunalp @dchen1107

tuunit · 2023-03-21T13:02:46Z

Any update on the review? @bobbypage @mrunalp @bart0sh @dchen1107 @derekwaynecarr

tuunit · 2023-03-21T13:03:33Z

New changes are detected. LGTM label has been removed.

@dchen1107 @bart0sh no new changes. Only a rebase of the latest changes on the master branch.

bart0sh · 2023-03-31T21:09:44Z

/lgtm

k8s-ci-robot · 2023-03-31T21:09:50Z

LGTM label has been added.

Git tree hash: 2802f2178cf056d5043ec2f8129f49933d986ab3

tuunit · 2023-05-12T09:10:56Z

Hey all,
@bobbypage @mrunalp @bart0sh @dchen1107 @derekwaynecarr

this PR has been open since December. Are there any concerns regarding the change or other reasons for that?

k8s-ci-robot · 2023-05-16T04:08:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mrunalp, tuunit

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/probe/OWNERS~~ [mrunalp]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Dec 20, 2022

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. do-not-merge/contains-merge-commits Indicates a PR which contains merge commits. labels Dec 20, 2022

k8s-ci-robot requested review from bobbypage and mrunalp December 20, 2022 13:31

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Dec 20, 2022

tuunit force-pushed the bugfix-case-sensitive-headers branch from 8dd6e4f to 4b73463 Compare December 20, 2022 13:40

k8s-ci-robot removed the do-not-merge/contains-merge-commits Indicates a PR which contains merge commits. label Dec 20, 2022

tuunit force-pushed the bugfix-case-sensitive-headers branch 2 times, most recently from bef61c2 to 798b1e6 Compare December 20, 2022 18:36

tuunit changed the title ~~[WIP] fix: remove case sensitive checking of probe headers~~ fix: remove case sensitive checking of probe headers Dec 20, 2022

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 20, 2022

bart0sh added this to Triage in SIG Node PR Triage Dec 27, 2022

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 27, 2022

bart0sh moved this from Triage to Needs Reviewer in SIG Node PR Triage Dec 27, 2022

bart0sh moved this from Needs Reviewer to Needs Approver in SIG Node PR Triage Dec 27, 2022

k8s-ci-robot assigned dchen1107, derekwaynecarr and mrunalp Jan 20, 2023

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 11, 2023

fix: remove case sensitive checking of probe headers

4a667a1

tuunit force-pushed the bugfix-case-sensitive-headers branch from 9f5d1c2 to 4a667a1 Compare March 21, 2023 11:39

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 21, 2023

k8s-ci-robot requested review from bart0sh, dchen1107 and derekwaynecarr March 21, 2023 11:39

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 21, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 31, 2023

mrunalp approved these changes May 16, 2023

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 16, 2023

mrunalp moved this from Needs Approver to Done in SIG Node PR Triage May 16, 2023

k8s-ci-robot merged commit 70033bf into kubernetes:master May 16, 2023
11 checks passed

k8s-ci-robot added this to the v1.28 milestone May 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: remove case sensitive checking of probe headers #114606

fix: remove case sensitive checking of probe headers #114606

tuunit commented Dec 20, 2022 •

edited

k8s-ci-robot commented Dec 20, 2022

k8s-ci-robot commented Dec 20, 2022

linux-foundation-easycla bot commented Dec 20, 2022 •

edited

tuunit commented Dec 23, 2022 •

edited

bart0sh commented Dec 27, 2022

bart0sh commented Dec 27, 2022

tuunit commented Dec 27, 2022

tuunit commented Jan 3, 2023

tuunit commented Jan 19, 2023

bart0sh commented Jan 20, 2023

tuunit commented Feb 22, 2023

tuunit commented Mar 21, 2023

tuunit commented Mar 21, 2023

bart0sh commented Mar 31, 2023

k8s-ci-robot commented Mar 31, 2023

tuunit commented May 12, 2023

k8s-ci-robot commented May 16, 2023

	func v1HeaderToHTTPHeader(headerList []v1.HTTPHeader) http.Header {
	headers := make(http.Header)
	for _, header := range headerList {
	headers[header.Name] = append(headers[header.Name], header.Value)
	}
	return headers
	}

fix: remove case sensitive checking of probe headers #114606

fix: remove case sensitive checking of probe headers #114606

Conversation

tuunit commented Dec 20, 2022 • edited

What type of PR is this?

What this PR does / why we need it:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

k8s-ci-robot commented Dec 20, 2022

k8s-ci-robot commented Dec 20, 2022

linux-foundation-easycla bot commented Dec 20, 2022 • edited

tuunit commented Dec 23, 2022 • edited

bart0sh commented Dec 27, 2022

bart0sh commented Dec 27, 2022

tuunit commented Dec 27, 2022

tuunit commented Jan 3, 2023

tuunit commented Jan 19, 2023

bart0sh commented Jan 20, 2023

tuunit commented Feb 22, 2023

tuunit commented Mar 21, 2023

tuunit commented Mar 21, 2023

bart0sh commented Mar 31, 2023

k8s-ci-robot commented Mar 31, 2023

tuunit commented May 12, 2023

k8s-ci-robot commented May 16, 2023

tuunit commented Dec 20, 2022 •

edited

linux-foundation-easycla bot commented Dec 20, 2022 •

edited

tuunit commented Dec 23, 2022 •

edited