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

Fix Audit-ID header key #48492

Merged
merged 1 commit into from
Jul 6, 2017
Merged

Fix Audit-ID header key #48492

merged 1 commit into from
Jul 6, 2017

Conversation

CaoShuFeng
Copy link
Contributor

@CaoShuFeng CaoShuFeng commented Jul 5, 2017 •
edited
Loading

Now http header key "Audit-ID" doesn't have effect, because golang
automaticly transforms "Audit-ID" into "Audit-Id". This change use
http.Header.Get() function to canonicalize "Audit-ID" to "Audit-Id".

Release note:

NONE

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 5, 2017
@k8s-ci-robot
Copy link
Contributor

Hi @CaoShuFeng. 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.

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. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 5, 2017
@CaoShuFeng
Copy link
Contributor Author

@sttts
@timstclair

@k8s-github-robot k8s-github-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Jul 5, 2017
@CaoShuFeng
Copy link
Contributor Author

Before this change the following Audit-ID will not work:
curl -H "Audit-ID: this-is-my-invalid-id" http://localhost:8080/version

sttts
sttts reviewed Jul 5, 2017
View reviewed changes
staging/src/k8s.io/apiserver/pkg/audit/request.go Outdated

// golang canonicalizes the header name when adding values to the header map
// see https://golang.org/src/net/http/request.go#L150 for reference
ids = append(ids, req.Header[http.CanonicalHeaderKey(auditinternal.HeaderAuditID)]...)
Copy link
Contributor

Choose a reason for hiding this comment

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

ouch, I hate magic. Great finding!

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you link with a commit sha1? The referenced line will be invalid soon otherwise.

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder, shouldn't we switch auditinternal.HeaderAuditID to the canonical form?

@sttts sttts assigned sttts and timstclair and unassigned ncdc and liggitt Jul 5, 2017
@CaoShuFeng CaoShuFeng force-pushed the CanonicalHeaderKey branch from 7894ade to 9a63af2 Compare July 5, 2017 11:31
@CaoShuFeng
Copy link
Contributor Author

CaoShuFeng commented Jul 5, 2017 •
edited
Loading

I wonder, shouldn't we switch auditinternal.HeaderAuditID to the canonical form?

Done.

@sttts
Copy link
Contributor

sttts commented Jul 5, 2017

/ok-to-test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 5, 2017
@CaoShuFeng
Copy link
Contributor Author

/test pull-kubernetes-e2e-gce-etcd3

@timstclair
Copy link

I think a better approach would be to change the usage:

ids := req.Header[auditinternal.HeaderAuditID]

to

ids := req.Header.Get(auditinternal.HeaderAuditID)

I think this is more idiomatic, and makes the constant definition independent of the internal go implementation.

@CaoShuFeng
Fix Audit-ID header key
f21bc7b 
Now http header key "Audit-ID" doesn't have effect, because golang
automaticly transforms "Audit-ID" into "Audit-Id". This change use
http.Header.Get() function to canonicalize "Audit-ID" to "Audit-Id".
@CaoShuFeng CaoShuFeng force-pushed the CanonicalHeaderKey branch from 9a63af2 to f21bc7b Compare July 6, 2017 08:12
@CaoShuFeng
Copy link
Contributor Author

@timstclair
Done.
Thanks for your suggestion.

@sttts
Copy link
Contributor

sttts commented Jul 6, 2017

/approve no-issue
/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 6, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CaoShuFeng, sttts

Associated issue requirement bypassed by: sttts

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

@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 6, 2017
@CaoShuFeng
Copy link
Contributor Author

/test pull-kubernetes-unit

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 40825b2 into kubernetes:master Jul 6, 2017
@CaoShuFeng CaoShuFeng mentioned this pull request Jul 10, 2017
Merged
@CaoShuFeng
Copy link
Contributor Author

I wrote a test for this pr:
#48693

CaoShuFeng added a commit to CaoShuFeng/kubernetes that referenced this pull request Jul 10, 2017
@CaoShuFeng
add a regression test for Audit-ID http header
a5df09b 
This change add a test for: kubernetes#48492
k8s-github-robot pushed a commit that referenced this pull request Jul 12, 2017
Merge pull request #48693 from CaoShuFeng/audit_id_header
7b650c9 
Automatic merge from submit-queue (batch tested with PRs 47948, 48631, 48693, 48549, 47593)

add a regression test for Audit-ID http header

This change add a test for: #48492



**What this PR does / why we need it**:

**Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes #

**Special notes for your reviewer**:

**Release note**:

```
NONE
```
caesarxuchao pushed a commit to caesarxuchao/apiserver that referenced this pull request Jul 13, 2017
@CaoShuFeng @k8s-publish-robot
add a regression test for Audit-ID http header
f7c6e65 
This change add a test for: kubernetes/kubernetes#48492

Kubernetes-commit: a5df09ba89f4c010eed76ffd985895aa80de9845
k8s-publish-robot pushed a commit to kubernetes/apiserver that referenced this pull request Jul 16, 2017
@CaoShuFeng @k8s-publish-robot
add a regression test for Audit-ID http header
aeff5f2 
This change add a test for: kubernetes/kubernetes#48492

Kubernetes-commit: a5df09ba89f4c010eed76ffd985895aa80de9845
@CaoShuFeng CaoShuFeng deleted the CanonicalHeaderKey branch November 6, 2017 05:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sttts sttts sttts left review comments

Assignees

@sttts sttts

@timstclair timstclair

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/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@CaoShuFeng @k8s-ci-robot @sttts @timstclair @k8s-github-robot @ncdc @liggitt