Return Audit-Id http response header for trouble shooting #49377

CaoShuFeng · 2017-07-21T13:11:48Z

Users can use Audit-Id http response header to grep the audit events in log.
This provides a fast way to find the events in audit.
Release note:

Audit-Id HTTP header is included in the apiserver responses for audited requests, except some cases when it's not possible, e.g. pods/exec.

@sttts @tallclair

k8s-ci-robot · 2017-07-21T13:11:56Z

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.

CaoShuFeng · 2017-07-21T13:14:50Z

staging/src/k8s.io/apiserver/pkg/endpoints/filters/audit_test.go

+		auditID    string
+		handler    func(http.ResponseWriter, *http.Request)
+		expected   []auditv1alpha1.Event
+		respHeader bool


Because some test cases don't call http.ResponseWriter.WriteHeader() function. So we can't check the response http header for them.

crassirostris · 2017-08-01T09:16:52Z

/ok-to-test
/assign

CaoShuFeng · 2017-08-01T09:47:48Z

@crassirostris
I thinks this pr does make sense for trouble shooting.
But there is a problem which I haven't solved yet.
I can't return http header for hijacked request.
That't means there is no audit-id http header for requests like:
"kubectl logs -f" "kubectl exec" "kubectl attach"

crassirostris · 2017-08-01T10:07:06Z

@CaoShuFeng Is it possible to set a header before hijacking?

sttts · 2017-08-03T06:17:25Z

staging/src/k8s.io/apiserver/pkg/endpoints/filters/audit.go

+
+func (a *auditResponseWriter) setHttpHeader() {
+	a.headerOnce.Do(func() {
+		a.ResponseWriter.Header().Set(auditinternal.HeaderAuditID, string(a.event.AuditID))


isn't there a way to override the header if it already exists? a sync.Once seems to be overkill.

Set has this behavior already, but it's not thread-safe, if it's applicable here

In the handler code we assume that only one thread/go-routine accesses the writer. So thread-safety should not matter here.

crassirostris · 2017-08-03T12:04:19Z

staging/src/k8s.io/apiserver/pkg/endpoints/filters/audit.go

@@ -204,6 +213,10 @@ func (f *fancyResponseWriterDelegator) Flush() {
 func (f *fancyResponseWriterDelegator) Hijack() (net.Conn, *bufio.ReadWriter, error) {
 	// fake a response status before protocol switch happens
 	f.processCode(http.StatusSwitchingProtocols)
+
+	// TODO(caoshufeng): we can't call WriteHeader before Hijack(), because WriteHeader may be called later:


@sttts What do you think about that? Should it be a TODO or is it working as intended and we shouldn't send audit-id for hijack'd requests?

Do we want to call WriteHeader (which is writing the code) or do we want only to add a header? The later is possible as long as WriteHeader was not called.

The latter, but @CaoShuFeng says it doesn't work, b/c in case of proxying request the headers are sent by the kubelet and not the apiserver

But then we don't have a chance in that case. But setting the header won't harm, it's just ignored. @CaoShuFeng is that correct?

Agree, a header should be set and then a comment should be added that it's ignored in certain cases

Also, I think it's worth mentioning in the release notes that Audit-Id http header is not set for some requests (and describe in short for which)

But setting the header won't harm, it's just ignored. @CaoShuFeng is that correct?

Yes I tested that.

Agree, a header should be set and then a comment should be added that it's ignored in certain cases
Also, I think it's worth mentioning in the release notes that Audit-Id http header is not set for some requests (and describe in short for which)

+1

ericchiang · 2017-08-03T16:24:06Z

This is great.

Probably want to update the HeaderAuditID docstring too.

const (
	// Header to hold the audit ID as the request is propagated through the serving hierarchy. The
	// Audit-ID header should be set by the first server to receive the request (e.g. the federation
	// server or kube-aggregator).
	HeaderAuditID = "Audit-ID"
)

CaoShuFeng · 2017-08-04T02:51:27Z

Probably want to update the HeaderAuditID docstring too.

Done

crassirostris · 2017-08-04T09:17:13Z

@CaoShuFeng Thanks! Though I think it's better to keep the release notes short + one line. Usually separate lines in the release notes mean several independent changes

I suggest smth like

Audit-Id HTTP header is included in the apiserver responses for audited requests, except some cases when it's not possible, e.g. pods/exec.

CaoShuFeng · 2017-08-04T09:22:02Z

I suggest smth like

Done

crassirostris · 2017-08-04T09:28:16Z

SGTM, thanks!

@sttts Needs your approval now

sttts · 2017-08-07T08:20:38Z

/approve no-issue

sttts · 2017-08-07T08:20:52Z

/lgtm

k8s-github-robot · 2017-08-07T08:22:32Z

[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:

~~staging/src/k8s.io/apiserver/OWNERS~~ [sttts]

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 · 2017-08-07T11:04:56Z

Automatic merge from submit-queue (batch tested with PRs 49868, 50143, 49377, 50141, 50145)

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

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 21, 2017

k8s-github-robot assigned soltysh and deads2k Jul 21, 2017

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Jul 21, 2017

CaoShuFeng commented Jul 21, 2017

View reviewed changes

deads2k removed their assignment Jul 24, 2017

CaoShuFeng mentioned this pull request Aug 1, 2017

Advanced Auditing Beta Changes #48561

Closed

7 tasks

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 Aug 1, 2017

k8s-ci-robot assigned crassirostris Aug 1, 2017

sttts reviewed Aug 3, 2017

View reviewed changes

CaoShuFeng force-pushed the audit_id_http_response_header branch from 87d7d72 to b62aa09 Compare August 3, 2017 10:51

crassirostris reviewed Aug 3, 2017

View reviewed changes

CaoShuFeng force-pushed the audit_id_http_response_header branch from b62aa09 to 0e2aca0 Compare August 3, 2017 13:10

Return Audit-Id http header for trouble shooting

4a1e7dd

CaoShuFeng force-pushed the audit_id_http_response_header branch from 0e2aca0 to 4a1e7dd Compare August 4, 2017 02:47

k8s-ci-robot assigned sttts Aug 7, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 7, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 7, 2017

k8s-github-robot merged commit a0826e1 into kubernetes:master Aug 7, 2017

CaoShuFeng deleted the audit_id_http_response_header branch November 6, 2017 05:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Return Audit-Id http response header for trouble shooting #49377

Return Audit-Id http response header for trouble shooting #49377

CaoShuFeng commented Jul 21, 2017 •

edited

Loading

k8s-ci-robot commented Jul 21, 2017

CaoShuFeng Jul 21, 2017 •

edited

Loading

crassirostris commented Aug 1, 2017

CaoShuFeng commented Aug 1, 2017

crassirostris commented Aug 1, 2017

sttts Aug 3, 2017

crassirostris Aug 3, 2017

CaoShuFeng Aug 3, 2017

sttts Aug 3, 2017

crassirostris Aug 3, 2017

sttts Aug 3, 2017

crassirostris Aug 3, 2017

sttts Aug 3, 2017

crassirostris Aug 3, 2017

CaoShuFeng Aug 3, 2017 •

edited

Loading

CaoShuFeng Aug 3, 2017

CaoShuFeng Aug 4, 2017

ericchiang commented Aug 3, 2017

CaoShuFeng commented Aug 4, 2017

crassirostris commented Aug 4, 2017

CaoShuFeng commented Aug 4, 2017

crassirostris commented Aug 4, 2017 •

edited

Loading

sttts commented Aug 7, 2017

sttts commented Aug 7, 2017

k8s-github-robot commented Aug 7, 2017

k8s-github-robot commented Aug 7, 2017

Return Audit-Id http response header for trouble shooting #49377

Return Audit-Id http response header for trouble shooting #49377

Conversation

CaoShuFeng commented Jul 21, 2017 • edited Loading

k8s-ci-robot commented Jul 21, 2017

CaoShuFeng Jul 21, 2017 • edited Loading

Choose a reason for hiding this comment

crassirostris commented Aug 1, 2017

CaoShuFeng commented Aug 1, 2017

crassirostris commented Aug 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CaoShuFeng Aug 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericchiang commented Aug 3, 2017

CaoShuFeng commented Aug 4, 2017

crassirostris commented Aug 4, 2017

CaoShuFeng commented Aug 4, 2017

crassirostris commented Aug 4, 2017 • edited Loading

sttts commented Aug 7, 2017

sttts commented Aug 7, 2017

k8s-github-robot commented Aug 7, 2017

k8s-github-robot commented Aug 7, 2017

CaoShuFeng commented Jul 21, 2017 •

edited

Loading

CaoShuFeng Jul 21, 2017 •

edited

Loading

CaoShuFeng Aug 3, 2017 •

edited

Loading

crassirostris commented Aug 4, 2017 •

edited

Loading