Instrument advanced auditing #46732

timstclair · 2017-05-31T21:02:24Z

Add prometheus metrics for audit logging, including:

A total count of audit events generated and sent to the output backend
A count of audit events that failed to be audited due to an error (per backend)
A count of request audit levels (1 per request)

TODO: Call HandlePluginError from the webhook backend, once apiserver: add a webhook implementation of the audit backend #45919 merges (in this or a separate PR, depending on timing of the merge)

/cc @ihmccreery @sttts @soltysh @ericchiang

sttts · 2017-06-01T06:59:13Z

staging/src/k8s.io/apiserver/pkg/audit/metrics.go

+
+	"github.com/golang/glog"
+	"github.com/prometheus/client_golang/prometheus"
+	auditinternal "k8s.io/apiserver/pkg/apis/audit"


nit: empty line above

Not sure what you mean here?

By convention the k8s.io imports and third party libs are separated with an empty line.

sttts · 2017-06-01T07:01:51Z

staging/src/k8s.io/apiserver/pkg/audit/metrics.go

+		plugin, len(impacted), err)
+	for _, ev := range impacted {
+		// Simple formatting of audit event.
+		event := fmt.Sprintf("%s id=%q stage=%q ip=%v method=%q uri=%q user=%q groups=%q",


Can/Should we use the log backend format here?

sttts · 2017-06-01T07:02:57Z

staging/src/k8s.io/apiserver/pkg/audit/metrics.go

+		if ev.ResponseStatus != nil {
+			event = fmt.Sprintf("%s response=\"%d\"", event, ev.ResponseStatus.Code)
+		}
+		msg = msg + fmt.Sprintf("{%s}\n", event)


This pretty much looks like the log backend output. Let's share that code.

sttts · 2017-06-01T07:04:45Z

Looks good in general. Would like to see shared code for the glog error logging and the log backend.

sttts · 2017-06-01T07:05:48Z

^^ can be in a follow-up if the freeze becomes an issue. In that case, lgtm.

sttts · 2017-06-01T07:29:59Z

Squash, then lgtm

timstclair · 2017-06-01T07:33:19Z

Squashed. Thanks!

sttts · 2017-06-01T09:09:33Z

/approve
/lgtm

k8s-github-robot · 2017-06-01T09:09:50Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

timstclair · 2017-06-01T23:26:54Z

Rebased on #45919 and called HandlePluginError from the webhook backend.

@ericchiang Could you take a quick look?

timstclair · 2017-06-01T23:50:11Z

Reapplying LGTM after rebase.

ericchiang · 2017-06-01T23:53:08Z

lgtm

timstclair · 2017-06-02T00:49:40Z

@k8s-bot pull-kubernetes-federation-e2e-gce test this

timstclair · 2017-06-02T01:37:41Z

@k8s-bot pull-kubernetes-federation-e2e-gce test this

fejta · 2017-06-02T05:14:44Z

@k8s-bot pull-kubernetes-federation-e2e-gce test this
ref: #46827

k8s-github-robot · 2017-06-03T18:31:22Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2017-06-03T18:39:39Z

Automatic merge from submit-queue (batch tested with PRs 46620, 46732, 46773, 46772, 46725)

k8s-ci-robot · 2017-06-03T19:15:40Z

@timstclair: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-unit	`b77c819`	link	`@k8s-bot pull-kubernetes-unit 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.

piosz · 2017-06-26T15:23:57Z

cc @brancz @fabxc

brancz · 2017-06-26T16:33:16Z

staging/src/k8s.io/apiserver/pkg/audit/metrics.go

+	eventCounter = prometheus.NewCounter(
+		prometheus.CounterOpts{
+			Subsystem: subsystem,
+			Name:      "event_count",


According to metric naming convention these should have the suffix total instead of count.

@ericchiang

Automatic merge from submit-queue (batch tested with PRs 48106, 46761) s/count/total/ in audit prometheus metrics Responding to #46732 (comment), making the metric names conform with the [metric naming convention](https://prometheus.io/docs/practices/naming/#metric-names). kubernetes/enhancements#22 /cc @ericchiang @ihmccreery

piosz · 2017-06-30T19:02:13Z

staging/src/k8s.io/apiserver/pkg/audit/metrics.go

+)
+
+const (
+	subsystem = "apiserver_audit"


I'd say that the namespace should be "apiserver" and subsystem "audit". It doesn't have any impact on how the metric will eventually look like - just for sanity.

@brancz do you agree?

We should add whatever we decide to the instrumentation guide. (/cc @fabxc) Personally I think it's a good idea for easier consistency, but there have been previous discussions around this, but I don't remember the outcome. Maybe @fabxc remembers.

timstclair added the release-note-none Denotes a PR that doesn't merit a release note. label May 31, 2017

timstclair added this to the v1.7 milestone May 31, 2017

timstclair assigned sttts and ericchiang May 31, 2017

k8s-ci-robot requested review from ikehz, soltysh, sttts and ericchiang May 31, 2017 21:02

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

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

sttts reviewed Jun 1, 2017

View reviewed changes

timstclair force-pushed the audit-metrics branch from 208adde to bee2730 Compare June 1, 2017 07:33

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

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

Instrument advanced auditing

b77c819

timstclair force-pushed the audit-metrics branch from bee2730 to b77c819 Compare June 1, 2017 23:25

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 1, 2017

timstclair added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 1, 2017

k8s-github-robot merged commit 6b76c40 into kubernetes:master Jun 3, 2017

brancz reviewed Jun 26, 2017

View reviewed changes

timstclair mentioned this pull request Jun 26, 2017

s/count/total/ in audit prometheus metrics #48106

Merged

piosz reviewed Jun 30, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Instrument advanced auditing #46732

Instrument advanced auditing #46732

timstclair commented May 31, 2017 •

edited

sttts Jun 1, 2017

timstclair Jun 1, 2017

sttts Jun 1, 2017

sttts Jun 1, 2017

sttts Jun 1, 2017

timstclair Jun 1, 2017

sttts commented Jun 1, 2017

sttts commented Jun 1, 2017

sttts commented Jun 1, 2017

timstclair commented Jun 1, 2017

sttts commented Jun 1, 2017

k8s-github-robot commented Jun 1, 2017

timstclair commented Jun 1, 2017

timstclair commented Jun 1, 2017

ericchiang commented Jun 1, 2017

timstclair commented Jun 2, 2017

timstclair commented Jun 2, 2017

fejta commented Jun 2, 2017

k8s-github-robot commented Jun 3, 2017

k8s-github-robot commented Jun 3, 2017

k8s-ci-robot commented Jun 3, 2017 •

edited

piosz commented Jun 26, 2017

brancz Jun 26, 2017

piosz Jun 30, 2017

brancz Jun 30, 2017

Instrument advanced auditing #46732

Instrument advanced auditing #46732

Conversation

timstclair commented May 31, 2017 • edited

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

sttts commented Jun 1, 2017

sttts commented Jun 1, 2017

sttts commented Jun 1, 2017

timstclair commented Jun 1, 2017

sttts commented Jun 1, 2017

k8s-github-robot commented Jun 1, 2017

timstclair commented Jun 1, 2017

timstclair commented Jun 1, 2017

ericchiang commented Jun 1, 2017

timstclair commented Jun 2, 2017

timstclair commented Jun 2, 2017

fejta commented Jun 2, 2017

k8s-github-robot commented Jun 3, 2017

k8s-github-robot commented Jun 3, 2017

k8s-ci-robot commented Jun 3, 2017 • edited

piosz commented Jun 26, 2017

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 •

edited

k8s-ci-robot commented Jun 3, 2017 •

edited