[advanced audit]add a policy wide omitStage #54634

CaoShuFeng · 2017-10-26T13:53:52Z

Related to: #54551
For example:

only log panic events

apiVersion: audit.k8s.io/v1beta1
kind: Policy
omitStages:
  - "RequestReceived"
  - "ResponseStarted"
  - "ResponseComplete"
rules:
  - level: Request

only log events inRequestReceived stage:

apiVersion: audit.k8s.io/v1beta1
kind: Policy
omitStages:
  - "ResponseStarted"
  - "ResponseComplete"
  - "Panic"
rules:
  - level: Request

Release note:

support a policy wide omitStage for advanced audit

CaoShuFeng · 2017-10-26T13:55:30Z

/assign @crassirostris
/assign @sttts
cc @hzxuzhonghu

crassirostris · 2017-10-26T13:56:19Z

Wow, cool! I'll take a look later this week

ericchiang · 2017-10-26T22:33:37Z

staging/src/k8s.io/apiserver/pkg/audit/policy/checker_test.go

@@ -213,3 +228,19 @@ func TestChecker(t *testing.T) {
 	test("subresource", audit.LevelRequest, nil, "getPodLogs", "getPods")
 	test("subresource", audit.LevelRequest, nil, "getPods", "getPodLogs")
 }
+
+func stageEqual(s1 []audit.Stage, s2 []audit.Stage) bool {


The following call returns true:

stageEqual([]audit.Stage{"foo", "bar", "baz"}, []audit.Stage{"foo", "foo", "foo"})

ericchiang · 2017-10-26T22:36:40Z

staging/src/k8s.io/apiserver/pkg/audit/policy/checker.go

 	return &policyChecker{*policy}
 }

+func unionStages(s1 []audit.Stage, s2 []audit.Stage) []audit.Stage {


Add fast path?

if len(s1) == 0 { return s2 } if len(s2) == 0 { return s1 }

This will be helpful if we have duplicated elements in s1 or s2.

For example:
s1 = nil
s2 = []audit.Stage{"foo", "foo", "bar"}

hzxuzhonghu · 2017-10-27T01:36:51Z

staging/src/k8s.io/apiserver/pkg/audit/policy/checker.go

 	return &policyChecker{*policy}
 }

+func unionStages(s1 []audit.Stage, s2 []audit.Stage) []audit.Stage {
+	m := make(map[audit.Stage]struct{})


why not reuse "k8s.io/apimachinery/pkg/util/sets"

I tried, but "k8s.io/apimachinery/pkg/util/sets" only supports int64, string, int, string, etc.
It doesn't work well with our audit.Stage.

Do I miss something?
If we can, I will be happy to use it.

How about type convert ?

package main import ( "fmt" ) type Stage string func main() { a := Stage("aaaaa") b := string(a) fmt.Printf(b) /* s := make([]Stage, 3) ss := []string(s) fmt.Print(s) */ }

Doesn't work as expected.

can not convert slice, one element by one

Yeah, I don't want to convert them one by one.
So I wrote this function.

CaoShuFeng · 2017-10-27T04:02:22Z

/test pull-kubernetes-unit

sttts · 2017-10-30T09:16:12Z

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

@@ -156,6 +156,11 @@ type Policy struct {
 	// The default audit level is None, but can be overridden by a catch-all rule at the end of the list.
 	// PolicyRules are strictly ordered.
 	Rules []PolicyRule `json:"rules" protobuf:"bytes,2,rep,name=rules"`
+
+	// OmitStages specify events generated in which stages will not be emitted to backend.


Having difficulties to parse this sentence (same for the one in the rules). Isn't this simpler:

OmitStages are a list of stages for which no events are created. Note that this can also
be specified per rule in which case the union of stages is skipped.

sttts · 2017-10-30T09:18:17Z

staging/src/k8s.io/apiserver/pkg/audit/policy/checker.go

+	result := make([]audit.Stage, len(m))
+	i := 0
+	for key := range m {
+		result[i] = key


append, no index i, and make([]audit.Stage, 0, len(m))

crassirostris · 2017-10-31T13:49:20Z

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

@@ -153,6 +153,10 @@ type Policy struct {
 	// The default audit level is None, but can be overridden by a catch-all rule at the end of the list.
 	// PolicyRules are strictly ordered.
 	Rules []PolicyRule
+
+	// OmitStages are a list of stages for which no events are created. Note that this can also
+	// be specified per rule in which case the union of stages is skipped.


are a list

is a list?

crassirostris · 2017-10-31T13:53:18Z

staging/src/k8s.io/apiserver/pkg/apis/audit/validation/validation_test.go

@@ -131,4 +134,20 @@ func TestValidatePolicy(t *testing.T) {
 			t.Errorf("[%d] Expected policy %#v to be invalid!", i, policy)
 		}
 	}
+
+	// test invalid omitStages in policy


Why not just add to the list of errorCases?

crassirostris · 2017-10-31T13:56:46Z

staging/src/k8s.io/apiserver/pkg/audit/policy/checker.go

 	return &policyChecker{*policy}
 }

+func unionStages(s1 []audit.Stage, s2 []audit.Stage) []audit.Stage {
+	m := make(map[audit.Stage]struct{})


Why not map[T]bool? I think bool looks little bit cleaner than struct{}{}

This is copied from here:

kubernetes/staging/src/k8s.io/apimachinery/pkg/util/sets/string.go

Line 27 in 7f9f847

type String map[string]Empty

crassirostris · 2017-10-31T13:57:23Z

staging/src/k8s.io/apiserver/pkg/audit/policy/checker.go

 	return &policyChecker{*policy}
 }

+func unionStages(s1 []audit.Stage, s2 []audit.Stage) []audit.Stage {
+	m := make(map[audit.Stage]struct{})
+	for _, s := range s1 {


for _, s := range append(s1, s2...)?

Or you can make one parameter of type ...[]audit.Stage, still looks cleaner IMO

for _, s := range append(s1, s2...)?

append will cause an extra memory copy, I guess.

Or you can make one parameter of type ...[]audit.Stage, still looks cleaner IMO

Done.

crassirostris · 2017-10-31T14:11:40Z

staging/src/k8s.io/apiserver/pkg/audit/policy/checker_test.go

+		assert.True(t, stageEqual(expOmitStages, actualOmitStages), "request:%s rules:%s, expected stages: %v, actual stages: %v",
+			req, strings.Join(ruleNames, ","), expOmitStages, actualOmitStages)
+
+		// test policy.OmitStages


Please don't do this (hacking around existing code structure and adding a comment)

Change the test signature and move test cases out of the method

crassirostris · 2017-10-31T14:12:59Z

staging/src/k8s.io/apiserver/pkg/audit/policy/checker.go

 	return &policyChecker{*policy}
 }

+func unionStages(s1 []audit.Stage, s2 []audit.Stage) []audit.Stage {


Please test this method separately

CaoShuFeng · 2017-11-01T09:25:41Z

/test pull-kubernetes-unit

CaoShuFeng · 2017-11-01T10:12:03Z

/test pull-kubernetes-unit

crassirostris

SGTM, several nits

crassirostris · 2017-11-02T14:37:00Z

staging/src/k8s.io/apiserver/pkg/audit/policy/checker.go

 	return &policyChecker{*policy}
 }

+func unionStages(s1, s2 []audit.Stage) []audit.Stage {


Why not

func unionStages(stageLists ...[]audit.Stage) []audit.Stage { m := make(map[audit.Stage]bool) for _, sl := range stageLists { for _, s := range sl { m[s] = true } } result := make([]audit.Stage, 0, len(m)) for key := range m { result = append(result, key) } return result }

Why do you need exactly two parameters in this case?

map[T]bool is more frequently used approach than map[T]struct{}

crassirostris · 2017-11-02T14:40:12Z

staging/src/k8s.io/apiserver/pkg/audit/policy/checker_test.go

-			Path:            "/api/v1/namespaces/default/pods/busybox",
-		},
-	}
+var tim = &user.DefaultInfo{


Replace with one var ( ... ) block

crassirostris · 2017-11-02T14:44:09Z

staging/src/k8s.io/apiserver/pkg/audit/policy/checker_test.go

+
+// stageEqual returns true if s1 and s2 are super set of each other
+func stageEqual(s1, s2 []audit.Stage) bool {
+	s1 = unionStages(s1, nil)


If you convert them both to map[T]bool and then compare maps, this it not needed

crassirostris · 2017-11-02T14:47:46Z

staging/src/k8s.io/apiserver/pkg/audit/policy/checker_test.go

+			req, strings.Join(ruleNames, ","), expOmitStages, actualOmitStages)
+	}
+
+	test("namespaced", audit.LevelMetadata, globalStages, "default")


You can extract the method signature to a named type and then extract this copypasta to a separate method that takes the test function

Then you don't even need the separate method, just call this new method twice with a different set of global stages

CaoShuFeng · 2017-11-08T02:39:56Z

/test pull-kubernetes-node-e2e

tallclair · 2017-11-17T23:59:44Z

/lgtm

crassirostris

LGTM

@sttts PTAL

crassirostris · 2017-11-18T16:27:44Z

staging/src/k8s.io/apiserver/pkg/audit/policy/checker_test.go

+}
+
+func TestChecker(t *testing.T) {
+	// test audit level


Nit: extra comment

crassirostris · 2017-11-18T16:28:27Z

staging/src/k8s.io/apiserver/pkg/audit/policy/checker_test.go

+func TestChecker(t *testing.T) {
+	// test audit level
+	testAuditLevel(t, nil)
+	// test omitStages pre rule


Nit: make consistent with the next test function by adding an extra line and changing the comment description

sttts · 2017-11-20T08:38:09Z

/approve no-issue

crassirostris · 2017-11-20T16:05:53Z

/lgtm

k8s-github-robot · 2017-11-20T16:06:47Z

[APPROVALNOTIFIER] This PR is APPROVED

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

Associated issue: 54551

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

CaoShuFeng · 2017-11-21T02:42:44Z

This needs a milestone.

k8s-github-robot · 2017-11-21T08:36:32Z

[MILESTONENOTIFIER] Milestone Pull Request Labels Incomplete

@CaoShuFeng @crassirostris @sttts @tallclair

Action required: This pull request requires label changes. If the required changes are not made within 2 days, the pull request will be moved out of the v1.9 milestone.

kind: Must specify exactly one of kind/bug, kind/cleanup or kind/feature.
priority: Must specify exactly one of priority/critical-urgent, priority/important-longterm or priority/important-soon.
sig owner: Must specify at least one label prefixed with sig/.

Help

k8s-github-robot · 2017-11-21T09:35:11Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2017-11-21T09:48:29Z

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

k8s-ci-robot · 2017-11-21T10:12:02Z

@CaoShuFeng: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-gce	`4a20d72`	link	`/test pull-kubernetes-e2e-gce`
pull-kubernetes-e2e-kops-aws	`4a20d72`	link	`/test pull-kubernetes-e2e-kops-aws`

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.

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 26, 2017

k8s-github-robot assigned tallclair and ncdc Oct 26, 2017

k8s-ci-robot assigned crassirostris and sttts Oct 26, 2017

CaoShuFeng force-pushed the omit_stage branch from 67fe2bc to 960b068 Compare October 26, 2017 14:03

ericchiang reviewed Oct 26, 2017

View reviewed changes

hzxuzhonghu reviewed Oct 27, 2017

View reviewed changes

CaoShuFeng force-pushed the omit_stage branch from 960b068 to ed4dc23 Compare October 27, 2017 02:01

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 27, 2017

CaoShuFeng force-pushed the omit_stage branch from ed4dc23 to f5b26a7 Compare October 27, 2017 03:30

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 27, 2017

sttts reviewed Oct 30, 2017

View reviewed changes

CaoShuFeng force-pushed the omit_stage branch from f5b26a7 to d70015f Compare October 30, 2017 11:11

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 30, 2017

crassirostris reviewed Oct 31, 2017

View reviewed changes

CaoShuFeng force-pushed the omit_stage branch from d70015f to e99e283 Compare November 1, 2017 08:49

crassirostris reviewed Nov 2, 2017

View reviewed changes

CaoShuFeng force-pushed the omit_stage branch 2 times, most recently from 668a071 to 3864e41 Compare November 6, 2017 05:57

CaoShuFeng mentioned this pull request Nov 6, 2017

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

Merged

CaoShuFeng force-pushed the omit_stage branch 2 times, most recently from 46e4d37 to c0b5b43 Compare November 8, 2017 02:05

ncdc removed their assignment Nov 14, 2017

crassirostris added the area/audit label Nov 17, 2017

crassirostris mentioned this pull request Nov 17, 2017

Advanced Auditing 1.9 umbrella bug #54551

Closed

14 tasks

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

crassirostris reviewed Nov 18, 2017

View reviewed changes

CaoShuFeng added 2 commits November 19, 2017 12:45

[advanced audit]add a policy wide omitStage

d75c0f0

run hack/update-all.sh

4a20d72

CaoShuFeng force-pushed the omit_stage branch from c0b5b43 to 4a20d72 Compare November 19, 2017 04:45

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

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

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

crassirostris added this to the v1.9 milestone Nov 21, 2017

crassirostris added the status/approved-for-milestone label Nov 21, 2017

k8s-github-robot added the milestone/incomplete-labels label Nov 21, 2017

k8s-github-robot merged commit 7b9affa into kubernetes:master Nov 21, 2017

[advanced audit]add a policy wide omitStage #54634

[advanced audit]add a policy wide omitStage #54634

Conversation

CaoShuFeng commented Oct 26, 2017 • edited Loading

CaoShuFeng commented Oct 26, 2017

crassirostris commented Oct 26, 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

CaoShuFeng commented Oct 27, 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

CaoShuFeng commented Nov 1, 2017

CaoShuFeng commented Nov 1, 2017

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

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 commented Nov 8, 2017

tallclair commented Nov 17, 2017

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

Choose a reason for hiding this comment

sttts commented Nov 20, 2017

crassirostris commented Nov 20, 2017

k8s-github-robot commented Nov 20, 2017

CaoShuFeng commented Nov 21, 2017

k8s-github-robot commented Nov 21, 2017

k8s-github-robot commented Nov 21, 2017

k8s-github-robot commented Nov 21, 2017

k8s-ci-robot commented Nov 21, 2017 • edited Loading

CaoShuFeng commented Oct 26, 2017 •

edited

Loading

k8s-ci-robot commented Nov 21, 2017 •

edited

Loading