audit: no RequestReceived events for read-only requests #46592

sttts · 2017-05-29T11:50:49Z

Follow-up of #46335

k8s-github-robot · 2017-05-29T11:51:01Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: sttts
We suggest the following additional approver: deads2k

Assign the PR to them by writing /assign @deads2k in a comment when ready.

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

plugin/pkg/auth/OWNERS
~~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

sttts · 2017-05-29T11:52:04Z

@soltysh @timstclair @cheftako @ericchiang do we want this?

timstclair · 2017-05-29T15:34:12Z

staging/src/k8s.io/apiserver/plugin/pkg/audit/log/backend.go

@@ -82,8 +82,8 @@ func (b *backend) logEvent(ev *auditinternal.Event) {
 		ip = ev.SourceIPs[0]
 	}

-	line := fmt.Sprintf("%s AUDIT: id=%q ip=%q method=%q user=%q groups=%q as=%q asgroups=%q namespace=%q uri=%q response=\"%s\"\n",
-		ev.Timestamp.Format(time.RFC3339Nano), ev.AuditID, ip, ev.Verb, username, groups, asuser, asgroups, namespace, ev.RequestURI, response)
+	line := fmt.Sprintf("%s AUDIT: id=%q stage=%q ip=%q method=%q user=%q groups=%q as=%q asgroups=%q namespace=%q uri=%q response=\"%s\"\n",


I think we should think a little about what the goal of this log format is before adding stage to it. Originally, I think it was trying to be consistent with basic auditing, but now that code was forked. Now, it's still missing a bunch of information from the audit event. Just thinking "out loud"...

BTW - I think we should add an option to write the log in JSON format, where each line is just a json-formatted audit event. (Not for this PR, just FYI)

--audit-log-format legacy/json/template:"{{foo}} {{bar}}"

This should be part of the proposal.

timstclair · 2017-05-29T15:34:51Z

staging/src/k8s.io/apiserver/pkg/apis/audit/types.go

@@ -61,6 +61,8 @@ const (
 	// The stage for events generated once the response body has been completed, and no more bytes
 	// will be sent.
 	StageResponseComplete = "ResponseComplete"
+	// The stage for events generated when a panic occured.
+	StagePanic = "Panic"


Add to the external types too

done in the other PR. this one has to be rebased.

timstclair · 2017-05-29T15:37:05Z

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

@@ -87,12 +87,18 @@ func WithAudit(handler http.Handler, requestContextMapper request.RequestContext
 			return
 		}

+		// send first event for non-read-only requests
+		ri, _ := request.RequestInfoFrom(ctx)
+		if !(ri != nil && ri.ReadOnly) {


My feeling is that we should make this decision in a single place, and I'd like an option to disable this stage. We talked about putting the option in the audit backend, but if we're already deciding whether to create a RequestReceived event here, perhaps this would be a good place to check that option as well?

if this is supposed to be configurable, I would make it a backend independent option: --audit-sparse-readonly-events.

Or into the policy.

For the option, I meant something for all requests, not just read-only requests. But in either case, we can discuss in a follow up.

This sound like the policy then. But I agree to discuss that as follow-up. Also this PR was meant as a base of discussion. We don't have to decide here.

timstclair · 2017-05-29T15:40:02Z

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

 				ev.ResponseStatus = &metav1.Status{
-					Code: 200,
+					Code:    http.StatusOK,
+					Reason:  metav1.StatusSuccess,


This should be on the Status.Status field.

timstclair · 2017-05-29T15:40:28Z

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

+			if ev.ResponseStatus == nil {
+				ev.ResponseStatus = &metav1.Status{
+					Code:    http.StatusOK,
+					Reason:  metav1.StatusSuccess,


timstclair · 2017-05-29T15:42:33Z

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

-					Code: 200,
+					Code:    http.StatusOK,
+					Reason:  metav1.StatusSuccess,
+					Message: "connection closed early",


nit: I think user-facing Messages usually start with a capital letter (same below)

timstclair · 2017-05-29T15:43:32Z

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

 			}

+			ev.Stage = auditinternal.StageResponseComplete
+			if ev.ResponseStatus == nil {


Move this above the previous if block, and you only need to build the fake status once.

timstclair · 2017-05-31T23:23:34Z

Please rebase (&squash while you're at it)

sttts · 2017-06-01T06:55:54Z

#46592 (comment) is still open.

k8s-ci-robot · 2017-06-01T07:32:42Z

@sttts: The following test(s) failed:

Test name	Commit	Details	Rerun command
pull-kubernetes-node-e2e	`56148e5`	link	`@k8s-bot pull-kubernetes-node-e2e test this`
pull-kubernetes-federation-e2e-gce	`56148e5`	link	`@k8s-bot pull-kubernetes-federation-e2e-gce test this`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

timstclair · 2017-06-01T07:34:51Z

Yeah, I don't see that being resolved by code freeze. I guess we don't want both an option in the policy and to drop them on read-only events... should we just leave this for now?

sttts · 2017-06-01T09:10:29Z

Let's leave it for now.

k8s-github-robot · 2017-06-05T08:13:11Z

@sttts PR needs rebase

sttts · 2017-07-17T12:04:56Z

@timstclair @ericchiang this PR is still pending. Is anything of this still relevant? Or do we have the same with the policy now?

tallclair · 2017-07-19T23:44:28Z

I think this needs more discussion...

One option is to incorporate the stage into the policy some how (which is documented in the beta changes cover bug: #48561)

My priority is just to reduce the number of events that are sent by the webhook, so an alternative would be to do some deduplication in the webhook. One way of doing so would be to put the early stages in a sort of "TTL Cache" which holds the latest stage of the event. If the ResponseComplete stage is logged, then that event is removed from the cache and added to the queue as with today. If an event is evicted from the cache (either due to size or TTL expiration), then the latest stage is added to the queue. The end result should be that most requests just send a ResponseComplete event, but requests that take an unusually long time to complete are guaranteed to be sent within X seconds (60?) of the request start time. (Obviously this would only be for the batching mode).

WDYT?

/cc @ericchiang @crassirostris

k8s-github-robot · 2017-08-19T08:07:43Z

This PR hasn't been active in 30 days. It will be closed in 59 days (Oct 17, 2017).

cc @sttts @tallclair

You can add 'keep-open' label to prevent this from happening, or add a comment to keep it open another 90 days

sttts · 2017-08-31T13:52:29Z

Closing as this is superseeded by a more powerful policy which can express this.

sttts added area/apiserver release-note-none Denotes a PR that doesn't merit a release note. labels May 29, 2017

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

sttts assigned timstclair May 29, 2017

k8s-github-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 29, 2017

sttts mentioned this pull request May 29, 2017

audit: implement stages #46335

Merged

sttts force-pushed the sttts-audit-stages-read-only-less-events branch from 9647872 to 5b1f3a5 Compare May 29, 2017 11:56

timstclair reviewed May 29, 2017

View reviewed changes

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

sttts added 2 commits June 1, 2017 08:56

apiserver: add endpoints/request.Read{,Write}Verbs

617fbb7

audit: no RequestReceived events for read-only requests

56148e5

sttts force-pushed the sttts-audit-stages-read-only-less-events branch from 5b1f3a5 to 56148e5 Compare June 1, 2017 06:58

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 1, 2017

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

k8s-ci-robot requested review from crassirostris and ericchiang July 19, 2017 23:44

tallclair assigned tallclair and unassigned timstclair Jul 20, 2017

This was referenced Jul 20, 2017

Provide a way to omit Event stages in audit policy #49280

Merged

Advanced Auditing Beta Changes #48561

Closed

sttts closed this Aug 31, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

audit: no RequestReceived events for read-only requests #46592

audit: no RequestReceived events for read-only requests #46592

sttts commented May 29, 2017

k8s-github-robot commented May 29, 2017

sttts commented May 29, 2017

timstclair May 29, 2017

timstclair May 29, 2017

sttts May 29, 2017

sttts May 29, 2017

timstclair May 29, 2017

sttts May 29, 2017

timstclair May 29, 2017

sttts May 29, 2017

sttts May 29, 2017

timstclair May 29, 2017

sttts May 29, 2017

timstclair May 29, 2017

sttts May 29, 2017

timstclair May 29, 2017

sttts May 29, 2017

timstclair May 29, 2017

sttts May 29, 2017

timstclair May 29, 2017

sttts May 29, 2017

timstclair commented May 31, 2017

sttts commented Jun 1, 2017

k8s-ci-robot commented Jun 1, 2017 •

edited

Loading

timstclair commented Jun 1, 2017

sttts commented Jun 1, 2017

k8s-github-robot commented Jun 5, 2017

sttts commented Jul 17, 2017

tallclair commented Jul 19, 2017

k8s-github-robot commented Aug 19, 2017

sttts commented Aug 31, 2017

audit: no RequestReceived events for read-only requests #46592

audit: no RequestReceived events for read-only requests #46592

Conversation

sttts commented May 29, 2017

k8s-github-robot commented May 29, 2017

sttts commented May 29, 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

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

Choose a reason for hiding this comment

timstclair commented May 31, 2017

sttts commented Jun 1, 2017

k8s-ci-robot commented Jun 1, 2017 • edited Loading

timstclair commented Jun 1, 2017

sttts commented Jun 1, 2017

k8s-github-robot commented Jun 5, 2017

sttts commented Jul 17, 2017

tallclair commented Jul 19, 2017

k8s-github-robot commented Aug 19, 2017

sttts commented Aug 31, 2017

k8s-ci-robot commented Jun 1, 2017 •

edited

Loading