Audit use buffered backend #60237

crassirostris · 2018-02-22T18:58:05Z

This is the next step after #60076

This PR fixes #53020, to address #53006 later

In this PR buffered backend, introduced in #60076, is used to replace ad-hoc solution for webhook and add an ability to enable buffering for the log audit backend.

Log audit backend can now be configured to perform batching before writing events to disk.

/cc @sttts @tallclair @ericchiang @CaoShuFeng

lavalamp · 2018-02-22T21:07:20Z

/assign @yliaog

sttts · 2018-02-24T08:22:27Z

staging/src/k8s.io/apiserver/plugin/pkg/audit/buffered/buffered.go

+	<-b.shutdownCh
+
+	// Wait until all sending routines exit.
+	b.wg.Wait()


I believe the new wg shutdown logic is correct:

when b.shutdownCh is closed, we know that the go routine in Run has terminated.

this means that processIncomingEvents has terminated

which means that b.buffer is closed and cannot accept any new events anymore.

because processEvents is called synchronously from the Run go routine, the waitgroup has its final value.

Hence, wg.Wait will not miss any more outgoing batches.

Should we put this proof into a comment (follow-up is fine)?

Sounds good, added a comment

sttts · 2018-02-24T08:27:10Z

staging/src/k8s.io/apiserver/plugin/pkg/audit/buffered/buffered.go

+	// recover from.
+	defer func() {
+		if err := recover(); err != nil {
+			sendErr = fmt.Errorf("panic when processing events: %v", err)


nit: I liked the old error message more. The new one sounds like something is broken.

Agree, done

crassirostris

@sttts Thanks for the review, comments addressed

crassirostris · 2018-02-26T16:16:20Z

staging/src/k8s.io/apiserver/plugin/pkg/audit/buffered/buffered.go

+	<-b.shutdownCh
+
+	// Wait until all sending routines exit.
+	b.wg.Wait()


Sounds good, added a comment

crassirostris · 2018-02-26T16:16:48Z

staging/src/k8s.io/apiserver/plugin/pkg/audit/buffered/buffered.go

+	// recover from.
+	defer func() {
+		if err := recover(); err != nil {
+			sendErr = fmt.Errorf("panic when processing events: %v", err)


Agree, done

sttts · 2018-02-26T18:19:58Z

/approve
Lgtm, but would like to have some more eyes one it, especially a detailed review of the changes to the old backends. @tallclair are you available?

crassirostris · 2018-02-26T18:49:04Z

/assign @thockin

Could you please approve this PR for the new dependency?

thockin · 2018-02-26T20:44:02Z

/approve

tallclair

lgtm, except the flag issue

tallclair · 2018-02-27T19:26:35Z

staging/src/k8s.io/apiserver/pkg/server/options/audit.go

-	fs.DurationVar(&o.BatchConfig.InitialBackoff, "audit-webhook-batch-initial-backoff",
-		o.BatchConfig.InitialBackoff, "The amount of time to wait before retrying the "+
-			"first failed requests. Only used in batch mode.")
+	fs.DurationVar(&o.InitialBackoff, "audit-webhook-initial-backoff",


Unfortunately you can't just change flags (see deprecation policy, #5b). I suggest marking the old one as deprecated, and making it an alias for the new one.

Good point, thanks, done

crassirostris

@tallclair Thanks! PTAL

crassirostris · 2018-02-28T21:30:20Z

staging/src/k8s.io/apiserver/pkg/server/options/audit.go

-	fs.DurationVar(&o.BatchConfig.InitialBackoff, "audit-webhook-batch-initial-backoff",
-		o.BatchConfig.InitialBackoff, "The amount of time to wait before retrying the "+
-			"first failed requests. Only used in batch mode.")
+	fs.DurationVar(&o.InitialBackoff, "audit-webhook-initial-backoff",


Good point, thanks, done

tallclair · 2018-03-01T03:53:02Z

staging/src/k8s.io/apiserver/pkg/server/options/audit.go

+			"first failed requests.")
+	fs.DurationVar(&o.InitialBackoff, "audit-webhook-batch-initial-backoff",
+		o.InitialBackoff, "The amount of time to wait before retrying the first failed "+
+			"requests. Deprecated, use --audit-webhook-initial-backoff instead.")


Use fs.MarkDeprecated(...)

Thanks! Was deceived by an example in another file

Done

Signed-off-by: Mik Vyatskov <vmik@google.com>

tallclair · 2018-03-01T17:49:52Z

/lgtm

k8s-ci-robot · 2018-03-01T17:49:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: crassirostris, sttts, tallclair, thockin

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:

~~cluster/gce/OWNERS~~ [thockin]
~~cmd/kube-apiserver/OWNERS~~ [sttts,thockin]
~~staging/src/k8s.io/apiextensions-apiserver/Godeps/OWNERS~~ [sttts,thockin]
~~staging/src/k8s.io/apiserver/OWNERS~~ [sttts,thockin]
~~staging/src/k8s.io/kube-aggregator/Godeps/OWNERS~~ [sttts,thockin]
~~staging/src/k8s.io/sample-apiserver/Godeps/OWNERS~~ [sttts,thockin]

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

k8s-github-robot · 2018-03-01T19:42:47Z

Automatic merge from submit-queue (batch tested with PRs 60542, 60237). If you want to cherry-pick this change to another branch, please follow the instructions here.

liggitt · 2018-03-02T17:38:52Z

audit e2e tests started flaking as soon as this merged. tracking in #60719

CaoShuFeng · 2018-03-05T06:21:52Z

staging/src/k8s.io/apiserver/pkg/server/options/audit.go

+		LogOptions: AuditLogOptions{
+			Format: pluginlog.FormatJson,
+			BatchOptions: AuditBatchOptions{
+				Mode:        ModeBatch,


This changes default behavior of log backend.

#60773

There is already a pull request: #60739

I know, this is a conscious decision. I believe batching is always better by default than blocking

crassirostris added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/auth Categorizes an issue or PR as relevant to SIG Auth. area/audit approved-for-milestone labels Feb 22, 2018

crassirostris added this to the v1.10 milestone Feb 22, 2018

crassirostris assigned sttts Feb 22, 2018

k8s-ci-robot requested a review from CaoShuFeng February 22, 2018 18:58

k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Feb 22, 2018

k8s-ci-robot requested review from ericchiang, sttts and tallclair February 22, 2018 18:58

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Feb 22, 2018

crassirostris mentioned this pull request Feb 22, 2018

Introduce buffered audit backend #60076

Merged

crassirostris force-pushed the audit-use-buffered-backend branch 2 times, most recently from 5d8bb64 to 502a2b1 Compare February 22, 2018 19:12

k8s-ci-robot assigned yliaog Feb 22, 2018

sttts reviewed Feb 24, 2018

View reviewed changes

k8s-ci-robot added status/approved-for-milestone and removed approved-for-milestone labels Feb 24, 2018

crassirostris force-pushed the audit-use-buffered-backend branch 2 times, most recently from d27e4d4 to 6900f27 Compare February 26, 2018 16:38

crassirostris commented Feb 26, 2018

View reviewed changes

crassirostris changed the title ~~[WIP] Audit use buffered backend~~ Audit use buffered backend Feb 26, 2018

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 Feb 26, 2018

crassirostris force-pushed the audit-use-buffered-backend branch 2 times, most recently from 161d1a3 to 4a42acb Compare February 26, 2018 18:13

crassirostris force-pushed the audit-use-buffered-backend branch from 4a42acb to 503c0a7 Compare February 26, 2018 18:47

k8s-ci-robot assigned thockin Feb 26, 2018

crassirostris mentioned this pull request Feb 26, 2018

Re-enable audit logging in scale tests kubernetes/test-infra#7000

Merged

crassirostris force-pushed the audit-use-buffered-backend branch from 503c0a7 to 68cfc48 Compare February 26, 2018 19:24

crassirostris closed this Feb 26, 2018

crassirostris reopened this Feb 26, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 26, 2018

thockin removed their assignment Feb 26, 2018

crassirostris force-pushed the audit-use-buffered-backend branch from 68cfc48 to 2a54b3b Compare February 27, 2018 11:40

sttts mentioned this pull request Feb 27, 2018

Increase in apiserver/c-m mem usage by 100-200 MB in 100-node cluster #60500

Closed

tallclair reviewed Feb 27, 2018

View reviewed changes

crassirostris mentioned this pull request Feb 28, 2018

Advanced Auditing 1.10 umbrella bug #58083

Closed

11 tasks

crassirostris force-pushed the audit-use-buffered-backend branch from 2a54b3b to c870cab Compare February 28, 2018 21:29

crassirostris commented Feb 28, 2018

View reviewed changes

tallclair reviewed Mar 1, 2018

View reviewed changes

Add buffering to the log audit backend

881e6d4

Signed-off-by: Mik Vyatskov <vmik@google.com>

crassirostris force-pushed the audit-use-buffered-backend branch from c870cab to 881e6d4 Compare March 1, 2018 13:31

k8s-ci-robot assigned tallclair Mar 1, 2018

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

k8s-github-robot merged commit 209cdd9 into kubernetes:master Mar 1, 2018

liggitt mentioned this pull request Mar 2, 2018

Advanced Audit tests flaking #60719

Closed

tallclair mentioned this pull request Mar 2, 2018

Revert "Audit use buffered backend" #60727

Closed

CaoShuFeng reviewed Mar 5, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Audit use buffered backend #60237

Audit use buffered backend #60237

crassirostris commented Feb 22, 2018

lavalamp commented Feb 22, 2018

sttts Feb 24, 2018

crassirostris Feb 26, 2018

sttts Feb 24, 2018

crassirostris Feb 26, 2018

crassirostris left a comment

crassirostris Feb 26, 2018

crassirostris Feb 26, 2018

sttts commented Feb 26, 2018

crassirostris commented Feb 26, 2018

thockin commented Feb 26, 2018

tallclair left a comment

tallclair Feb 27, 2018

crassirostris Feb 28, 2018

crassirostris left a comment

crassirostris Feb 28, 2018

tallclair Mar 1, 2018

crassirostris Mar 1, 2018

tallclair commented Mar 1, 2018

k8s-ci-robot commented Mar 1, 2018

k8s-github-robot commented Mar 1, 2018

liggitt commented Mar 2, 2018

CaoShuFeng Mar 5, 2018

CaoShuFeng Mar 5, 2018

crassirostris Mar 5, 2018

Audit use buffered backend #60237

Audit use buffered backend #60237

Conversation

crassirostris commented Feb 22, 2018

lavalamp commented Feb 22, 2018

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

crassirostris left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sttts commented Feb 26, 2018

crassirostris commented Feb 26, 2018

thockin commented Feb 26, 2018

tallclair left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crassirostris left a 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

tallclair commented Mar 1, 2018

k8s-ci-robot commented Mar 1, 2018

k8s-github-robot commented Mar 1, 2018

liggitt commented Mar 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment