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

Use case-insensitive header keys for --requestheader-group-headers. #49219

Merged
merged 1 commit into from
Jul 28, 2017
Merged

Use case-insensitive header keys for --requestheader-group-headers. #49219

merged 1 commit into from
Jul 28, 2017

Conversation

jmillikin-stripe
Copy link
Contributor

This flag is documented as being case-insensitive, but the code was
doing a case-sensitive map lookup.

Release note:

Bug fix: Parsing of `--requestheader-group-headers` in requests should be case-insensitive.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 19, 2017
@k8s-github-robot k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Jul 19, 2017
@jmillikin-stripe
Copy link
Contributor Author

/assign @wojtek-t

@wojtek-t
Copy link
Member

I don't have enough context to review it.

/assign @liggitt

@wojtek-t wojtek-t removed their assignment Jul 21, 2017
liggitt
liggitt reviewed Jul 21, 2017
View reviewed changes
staging/src/k8s.io/apiserver/pkg/authentication/request/headerrequest/requestheader.go Outdated
@@ -145,7 +146,8 @@ func headerValue(h http.Header, headerNames []string) string {
func allHeaderValues(h http.Header, headerNames []string) []string {
ret := []string{}
for _, headerName := range headerNames {
values, ok := h[headerName]
headerKey := textproto.CanonicalMIMEHeaderKey(headerName)
Copy link
Member

Choose a reason for hiding this comment

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

http.CanonicalHeaderKey() instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@jmillikin-stripe jmillikin-stripe force-pushed the case-insensitive-request-header-group branch from 671c305 to 2ca78a3 Compare July 21, 2017 16:14
@liggitt
Copy link
Member

liggitt commented Jul 21, 2017

/lgtm
thanks

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 21, 2017
@liggitt
Copy link
Member

liggitt commented Jul 21, 2017

/approve no-issue

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 21, 2017
@liggitt liggitt added release-note-none Denotes a PR that doesn't merit a release note. release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. release-note-none Denotes a PR that doesn't merit a release note. labels Jul 21, 2017
@jmillikin-stripe
Copy link
Contributor Author

Looks like tests are failing due to missing OAuth credentials in Jenkins.

@jmillikin-stripe
Copy link
Contributor Author

/retest

@k8s-ci-robot
Copy link
Contributor

@jmillikin-stripe: you can't request testing unless you are a kubernetes member.

In response to this:

/retest

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.

@jmillikin-stripe
Copy link
Contributor Author

Can someone with ci-robot privileges poke it to re-run those tests? I'm 99% confident that those failures are flakes.

@timothysc timothysc removed their assignment Jul 21, 2017
@liggitt
Copy link
Member

liggitt commented Jul 25, 2017 

I0721 16:39:51.502] diff ./staging/src/k8s.io/apiserver/pkg/authentication/request/headerrequest/requestheader_test.go gofmt/./staging/src/k8s.io/apiserver/pkg/authentication/request/headerrequest/requestheader_test.go
I0721 16:39:51.502] --- /tmp/gofmt228050102	2017-07-21 16:39:44.362522470 +0000
I0721 16:39:51.502] +++ /tmp/gofmt605347997	2017-07-21 16:39:44.362522470 +0000
I0721 16:39:51.502] @@ -115,8 +115,8 @@
I0721 16:39:51.503]  			nameHeaders:  []string{"X-REMOTE-User"},
I0721 16:39:51.503]  			groupHeaders: []string{"X-REMOTE-Group"},
I0721 16:39:51.503]  			requestHeaders: http.Header{
I0721 16:39:51.503] -				"X-Remote-User":    {"Bob"},
I0721 16:39:51.503] -				"X-Remote-Group":   {"Users"},
I0721 16:39:51.503] +				"X-Remote-User":  {"Bob"},
I0721 16:39:51.503] +				"X-Remote-Group": {"Users"},
I0721 16:39:51.503]  			},
I0721 16:39:51.504]  			expectedUser: &user.DefaultInfo{
I0721 16:39:51.504]  				Name:   "Bob",
I0721 16:39:51.504] FAILED   hack/make-rules/../../hack/verify-gofmt.sh	12s

@jmillikin-stripe
Use case-insensitive header keys for --requestheader-group-headers.
0acdc0c 
This flag is documented as being case-insensitive, but the code was
doing a case-sensitive map lookup.
@jmillikin-stripe jmillikin-stripe force-pushed the case-insensitive-request-header-group branch from 2ca78a3 to 0acdc0c Compare July 25, 2017 16:28
@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 25, 2017
@jmillikin-stripe
Copy link
Contributor Author

Good catch, thanks! I didn't see that error among all the OAuth failures. PR updated.

@liggitt
Copy link
Member

liggitt commented Jul 25, 2017

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 25, 2017
@liggitt
Copy link
Member

liggitt commented Jul 25, 2017

@k8s-bot ok to test

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jmillikin-stripe, liggitt

Associated issue requirement bypassed by: liggitt

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

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@fejta-bot
Copy link

/retest
Automatic retester of approved PRs with test failures, please send feedback to fejta

@fejta-bot
Copy link

/retest
Automated flake /retester experiment. Please send feedback to fejta

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 49081, 49318, 49219, 48989, 48486)

@k8s-github-robot k8s-github-robot merged commit e87f809 into kubernetes:master Jul 28, 2017
@jmillikin-stripe jmillikin-stripe deleted the case-insensitive-request-header-group branch July 28, 2017 16:37
@deads2k deads2k mentioned this pull request Sep 6, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liggitt liggitt liggitt left review comments

Assignees

@liggitt liggitt

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@jmillikin-stripe @wojtek-t @liggitt @k8s-ci-robot @k8s-github-robot @fejta-bot @timothysc