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

CaoShuFeng · 2017-07-20T09:06:14Z

This provide a way to omit some stages for each audit policy rule.

For example:

```
  apiVersion: audit.k8s.io/v1beta1
  kind: Policy
  - level: Metadata
    resources:
       - group: "rbac.authorization.k8s.io"
         resources: ["roles"]
    omitStages:
      - "RequestReceived"
```

RequestReceived stage will not be emitted to audit backends with previous config.

Release note:

None

k8s-ci-robot · 2017-07-20T09:06:22Z

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-20T09:11:17Z

We want to omit the RequestReceived stage in GKE. The policy may be the right place to specify that.

@sttts @tallclair
I will add unit test for this change soon if my idea is "right".

tallclair · 2017-07-20T21:37:17Z

@CaoShuFeng - What is your motivation for omitting stages? Would the solution proposed in #46592 (comment) work for you? I'd like to explore that option before expanding the audit profile API.

CaoShuFeng · 2017-07-21T02:47:33Z

@tallclair Thanks for your review.

I write this pr because of this expression:

We want to omit the RequestReceived stage in GKE. The policy may be the right place to specify that.

It looks like I got it wrong.
What's the meaning behind The policy may be the right place to specify that. ?

tallclair · 2017-07-21T20:17:31Z

This isn't wrong, per say. I just wanted to discuss alternatives before putting the burden on the user (i.e. adding it to the policy). If we can accomplish the same goals automatically (the batching proposal) then I think that would be preferable.

CaoShuFeng · 2017-07-22T06:13:52Z

. I just wanted to discuss alternatives before putting the burden on the user (i.e. adding it to the policy)

Agree.

Thanks for teaching me.
I will leave this pr open for a while. Maybe this is useful.

caesarxuchao · 2017-07-24T20:08:12Z

/assign @tallclair
/unassign @caesarxuchao

CaoShuFeng · 2017-07-27T02:40:32Z

Closed according to commants from @tallclair and @ericchiang .
Ref:
#48561 (comment)
#48561 (comment)

CaoShuFeng · 2017-07-28T01:35:26Z

Repopened according to @crassirostris 's commants.
Ref:
#48561 (comment)

This is still WIP, Unit tests will come soon.

crassirostris · 2017-08-02T15:40:53Z

@tallclair suggested earlier that it might be useful to have omitStages field in the Policy object that will be "ored" with the omitStages field in the PolicyRule. That's a syntax sugar for cases where a stage has to be excluded in every rule

@sttts @CaoShuFeng WDYT?

crassirostris · 2017-08-02T15:41:58Z

Btw, it should probably should be rebased once #49115 is merged and the changes should go to the beta API

sttts · 2017-08-03T05:40:44Z

staging/src/k8s.io/apiserver/pkg/apis/audit/v1alpha1/generated.pb.go

@@ -528,6 +528,21 @@ func (m *PolicyRule) MarshalTo(dAtA []byte) (int, error) {
 			i += copy(dAtA[i:], s)
 		}
 	}
+	if len(m.OmitStages) > 0 {


no need for the if.

oops, that's generated proto :)

sttts · 2017-08-03T05:41:00Z

staging/src/k8s.io/apiserver/pkg/apis/audit/v1alpha1/generated.pb.go

@@ -723,6 +738,12 @@ func (m *PolicyRule) Size() (n int) {
 			n += 1 + l + sovGenerated(uint64(l))
 		}
 	}
+	if len(m.OmitStages) > 0 {


no need for the if

sttts · 2017-08-03T05:43:01Z

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

 }

 type policyChecker struct {
 	audit.Policy
 }

-func (p *policyChecker) Level(attrs authorizer.Attributes) audit.Level {
+func (p *policyChecker) Level(attrs authorizer.Attributes) (audit.Level, []audit.Stage) {


don't the level and stages overlap?

Renamed to LevelAndStages

LevelAndOmitStaged

sttts · 2017-08-03T05:47:33Z

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

+
+	// OmitStages specify events generated in which stages will not be emitted to backend
+	// Allowed stages are RequestReceived,ResponseStarted,ResponseComplete,Panic
+	// empty list means every events will be emitted.


nit: An empty....

sttts · 2017-08-03T05:47:51Z

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

@@ -200,6 +200,11 @@ type PolicyRule struct {
 	//  "/healthz*" - Log all health checks
 	// +optional
 	NonResourceURLs []string
+
+	// OmitStages specify events generated in which stages will not be emitted to backend
+	// Allowed stages are RequestReceived,ResponseStarted,ResponseComplete,Panic


nit: fullstop and spaces after ,

Is it common to relist the constants in those comments?

Is it common to relist the constants in those comments?

+1, I don't think it's a great idea

sttts · 2017-08-03T05:50:54Z

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

+	event      *auditinternal.Event
+	once       sync.Once
+	sink       audit.Sink
+	omitStages []auditinternal.Stage


please add a comment: omitStages are skipped, i.e. not sent to the backend. But their information is still recorded in the context Event for later stages.

sttts · 2017-08-03T05:52:13Z

Some nit. Overall lgtm.

sttts · 2017-08-03T05:53:11Z

@tallclair suggested earlier that it might be useful to have omitStages field in the Policy object that will be "ored" with the omitStages

Sounds good.

sttts · 2017-09-03T10:47:04Z

Needs rebase.

CaoShuFeng · 2017-09-03T14:27:09Z

Needs rebase.

Partially done. This will need another rebase after #51797.
#51797 has gone into the submit queue.

CaoShuFeng · 2017-09-04T03:40:26Z

/test pull-kubernetes-e2e-gce-bazel

sttts · 2017-09-04T06:50:30Z

/retest

sttts · 2017-09-04T06:55:10Z

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

@@ -42,19 +42,43 @@ func WithAudit(handler http.Handler, requestContextMapper request.RequestContext
 		return handler
 	}
 	return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
-		ctx, ev, err := createAuditEventAndAttachToContext(requestContextMapper, req, policy)


Can't we return omitStages from createAuditEventAndAttachToContext? Am not a fan of this code duplication.

I'm not sure why this function is removed at all. omitStages are later obtained from the policy and not used until the code that was previously outside of createAuditEventAndAttachToContext. @CaoShuFeng Could you please elaborate more?

I have took createAuditEventAndAttachToContext back.
And the codes look cleaner for me, thanks.

much better now

lgtm. @crassirostris can you take another look and /lgtm if it is ok?

crassirostris

Some nits regarding the comments, otherwise LGTM

crassirostris · 2017-09-04T08:24:40Z

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

@@ -207,6 +207,10 @@ type PolicyRule struct {
 	//  "/healthz*" - Log all health checks
 	// +optional
 	NonResourceURLs []string
+
+	// OmitStages specify events generated in which stages will not be emitted to backend.
+	// An empty list means every events will be emitted.


I think that might be confusing. What about An empty list means no restrictions will apply.?

crassirostris · 2017-09-04T08:33:16Z

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

@@ -173,6 +179,8 @@ type auditResponseWriter struct {
 	event *auditinternal.Event
 	once  sync.Once
 	sink  audit.Sink
+	// omitStages are skipped, i.e. not sent to the backend. But their information is still recorded in the context Event for later stages.


omitStages are skipped

Is this comment necessary? once and sink are not set to the backend also :) Basically, the only explanation why omitStages field is there is b/c it's used later and this description is not very informative. Maybe remove the comment altogether?

Good for me.
Deleted.

crassirostris

LGTM

Updates kubernetes#48561 This provide a way to omit some stages for each audit policy rule. For example: apiVersion: audit.k8s.io/v1beta1 kind: Policy - level: Metadata resources: - group: "rbac.authorization.k8s.io" resources: ["roles"] omitStages: - "RequestReceived" RequestReceived stage will not be emitted to audit backends with previous config.

./hack/update-codegen.sh ./hack/update-generated-protobuf.sh

crassirostris · 2017-09-04T11:04:02Z

/lgtm

k8s-github-robot · 2017-09-04T11:04:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CaoShuFeng, crassirostris, 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-09-04T12:07:10Z

Automatic merge from submit-queue

k8s-ci-robot · 2017-09-04T12:07:24Z

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

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-kops-aws	`b50acbd`	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.

tengqm · 2017-11-06T04:35:31Z

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

@@ -207,6 +207,10 @@ type PolicyRule struct {
 	//  "/healthz*" - Log all health checks
 	// +optional
 	NonResourceURLs []string
+
+	// OmitStages specify events generated in which stages will not be emitted to backend.
+	// An empty list means no restrictions will apply.


should we mark this as optional?

Yes, we should.

Added here together:
#54634

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 20, 2017

k8s-github-robot assigned caesarxuchao and liggitt Jul 20, 2017

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

k8s-ci-robot assigned tallclair and unassigned caesarxuchao Jul 24, 2017

CaoShuFeng closed this Jul 27, 2017

CaoShuFeng reopened this Jul 28, 2017

sttts reviewed Aug 3, 2017

View reviewed changes

CaoShuFeng force-pushed the RequestReceived branch from 337730a to e9a9f6c Compare August 4, 2017 11:37

sttts changed the title ~~[Do not merge]Provide a way to omit Event stages in audit policy~~ Provide a way to omit Event stages in audit policy Sep 3, 2017

CaoShuFeng force-pushed the RequestReceived branch from c28fdac to db3e05b Compare September 3, 2017 14:16

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 3, 2017

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

CaoShuFeng force-pushed the RequestReceived branch from db3e05b to 41eebcd Compare September 4, 2017 02:19

sttts added this to the v1.8 milestone Sep 4, 2017

sttts added the kind/feature Categorizes issue or PR as related to a new feature. label Sep 4, 2017

sttts reviewed Sep 4, 2017

View reviewed changes

CaoShuFeng force-pushed the RequestReceived branch from 41eebcd to aba616e Compare September 4, 2017 07:46

crassirostris reviewed Sep 4, 2017

View reviewed changes

CaoShuFeng force-pushed the RequestReceived branch from aba616e to 9b1e0a9 Compare September 4, 2017 08:55

crassirostris approved these changes Sep 4, 2017

View reviewed changes

CaoShuFeng added 2 commits September 4, 2017 17:51

generated: update API resources

b50acbd

./hack/update-codegen.sh ./hack/update-generated-protobuf.sh

CaoShuFeng force-pushed the RequestReceived branch from cd004f4 to b50acbd Compare September 4, 2017 09:52

k8s-ci-robot assigned crassirostris Sep 4, 2017

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

k8s-github-robot merged commit 9d29ce1 into kubernetes:master Sep 4, 2017

CaoShuFeng mentioned this pull request Oct 25, 2017

Advanced Auditing 1.9 umbrella bug #54551

Closed

14 tasks

tengqm reviewed Nov 6, 2017

View reviewed changes

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

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

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

Conversation

CaoShuFeng commented Jul 20, 2017 • edited

k8s-ci-robot commented Jul 20, 2017

CaoShuFeng commented Jul 20, 2017 • edited

tallclair commented Jul 20, 2017

CaoShuFeng commented Jul 21, 2017

tallclair commented Jul 21, 2017

CaoShuFeng commented Jul 22, 2017 • edited

caesarxuchao commented Jul 24, 2017

CaoShuFeng commented Jul 27, 2017

CaoShuFeng commented Jul 28, 2017

crassirostris commented Aug 2, 2017

crassirostris commented Aug 2, 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

sttts commented Aug 3, 2017

sttts commented Aug 3, 2017

sttts commented Sep 3, 2017

CaoShuFeng commented Sep 3, 2017

CaoShuFeng commented Sep 4, 2017

sttts commented Sep 4, 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

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

crassirostris left a comment

Choose a reason for hiding this comment

crassirostris commented Sep 4, 2017

k8s-github-robot commented Sep 4, 2017

k8s-github-robot commented Sep 4, 2017

k8s-ci-robot commented Sep 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CaoShuFeng commented Jul 20, 2017 •

edited

CaoShuFeng commented Jul 20, 2017 •

edited

CaoShuFeng commented Jul 22, 2017 •

edited