Initialize the AuditEvent with the AuditContext #113611

tallclair · 2022-11-04T00:15:21Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Always initialize the audit event at the beginning of the request chain, and store audit data directly on the event.

Which issue(s) this PR fixes:

Does this PR introduce a user-facing change?

NONE

/sig auth api-machinery
/milestone v1.26

fedebongio · 2022-11-08T21:23:49Z

/triage accepted

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

sttts · 2022-12-01T08:58:03Z

Lgtm.

As this is pretty deep in the handler chain, maybe another set of eyes would be helpful. @tkashem @benluddy does one of you have a moment to double check?

tkashem · 2022-12-02T14:26:00Z

Lgtm.

As this is pretty deep in the handler chain, maybe another set of eyes would be helpful. @tkashem @benluddy does one of you have a moment to double check?

Yes, I will take a look at it next week, thanks!
/assign

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

ibihim · 2022-12-02T18:04:58Z

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

 }

 // AuditIDFrom returns the value of the audit ID from the request context.
 func AuditIDFrom(ctx context.Context) (types.UID, bool) {
 	if ac := AuditContextFrom(ctx); ac != nil {
-		return ac.auditID, ac.auditID != ""
+		return ac.Event.AuditID, true


Nit: Is it always initialized with an ID, so that we don't need to check for empty string as it initialized by WithAuditInit. Isn't it making it easier to make a mistake if the audit pkg is used outside of k8s.io/apiserver? I could imagine that it takes some debugging to figure out that it can be true on an empty string.

Yes, the audit ID is always initialized at the same time as the audit context: https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiserver/pkg/endpoints/filters/audit_init.go#L41-L50

But you bring up a good point. Looking at everywhere this is used, I think the bool return value is really meant to indicate whether auditing is enabled. This method isn't used in very many places, so I think we should probably just inline it with an explicit auditContext.Enabled() check instead.

Actually, since the eventual goal is to make the AuditContext.Event non-exported, I think it makes sense to keep this function as-is for now.

According to the updated description, ac.Enabled() should be returned instead, in case the policy level is "None".
I am not sure whether that would still align with the original purpose of the bool.

Is this comment still open?

ibihim · 2022-12-02T18:05:23Z

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

-		RequestURI:               req.URL.RequestURI(),
-		UserAgent:                maybeTruncateUserAgent(req),
-		Level:                    level,
+func LogRequestMetadata(ctx context.Context, req *http.Request, requestReceivedTimestamp time.Time, level auditinternal.Level, attribs authorizer.Attributes) {


We use the audit package for or openshift/oauth-server and the removal of NewEventFromRequest would break our tests. So I rewrote them against your PR and the usage of the audit package is now really nice. All the changes around the removal of *[]annotations are amazing! 🎉

ibihim · 2022-12-05T06:40:59Z

lgtm

tkashem

this LGTM, I have a few minor comments, thanks!

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

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

tkashem · 2022-12-06T19:13:46Z

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

-	key, value string
+// Enabled checks whether auditing is enabled for this audit context.
+func (ac *AuditContext) Enabled() bool {
+	return ac != nil && ac.Event.Level != auditinternal.LevelNone


I think the initial default should return false, right?

func TestEnabled(t *testing.T) { ac := &AuditContext{} if ac.Enabled() { t.Errorf("expected the default to return false") } }

maybe add len(ac.Event.Level) > 0

No, this is intentional. If annotations (or any other request metadata) are recorded before the policy is evaluated, we want to store them. Once the policy is evaluated to None we can stop doing the work in that case. I'll add a comment & test to this affect.

tkashem · 2022-12-06T19:23:14Z

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

@@ -167,8 +131,8 @@ func WithAuditContext(parent context.Context) context.Context {



I think it's safer for the the initial context to return a LevelNone for the Level field

func WithAuditContext(parent context.Context) context.Context { if AuditContextFrom(parent) != nil { return parent // Avoid double registering. } ac := &AuditContext{ Event: { Level: LevelNone } } return genericapirequest.WithValue(parent, auditKey, ac) }

kubernetes/staging/src/k8s.io/apiserver/pkg/audit/context.go

Lines 159 to 166 in 17bf864

// WithAuditContext returns a new context that stores the AuditContext.

func WithAuditContext(parent context.Context) context.Context {

if AuditContextFrom(parent) != nil {

return parent // Avoid double registering.

}

return genericapirequest.WithValue(parent, auditKey, &AuditContext{})

}

See above comment about capturing data before the policy is evaluated.

yes, with the above approach, we are good here.

tkashem · 2022-12-06T19:26:52Z

staging/src/k8s.io/apiserver/pkg/server/config_test.go

@@ -283,11 +283,6 @@ func TestAuthenticationAuditAnnotationsDefaultChain(t *testing.T) {
 		// confirm that we can set an audit annotation in a handler before WithAudit
 		audit.AddAuditAnnotation(req.Context(), "pandas", "are awesome")

-		// confirm that trying to use the audit event directly would never work


maybe we should confirm here that the level is set to LevelNone, assert that ac.Enabled() == false, in keeping with the current test assertion?

(See above comments)

tkashem · 2022-12-07T13:44:52Z

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

@@ -188,19 +152,16 @@ func WithAuditID(ctx context.Context, auditID types.UID) {
 		return
 	}
 	ac := AuditContextFrom(ctx)
-	if ac == nil {
+	if !ac.Enabled() {


I think storing the auditID in the request context is orthogonal to audit logging. A cluster operator can disable audit logging, but we would still want to store the auditID in the request context. The auditID is also propagated to the aggregated APIServer(s).

We have some logs that emit the auditID, for example:

kubernetes/staging/src/k8s.io/apiserver/pkg/server/filters/wrap.go

Line 58 in 4db6bde

klog.ErrorS(nil, "apiserver panic'd", "method", req.Method, "URI", req.RequestURI, "audit-ID", audit.GetAuditIDTruncated(req.Context()))

I think we would want to keep the log correlation ^, even though audit logging is disabled, thoughts?

Yeah, that makes sense. You might also be interested in this issue, which has come up a couple of times in sig-apimachinery: #101597

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

tkashem · 2022-12-07T14:24:16Z

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

@@ -81,8 +75,7 @@ func AddAuditAnnotation(ctx context.Context, key, value string) {
 // keysAndValues are the key-value pairs to add, and must have an even number of items.
 func AddAuditAnnotations(ctx context.Context, keysAndValues ...string) {
 	ac := AuditContextFrom(ctx)
-	if ac == nil {
-		// auditing is not enabled
+	if !ac.Enabled() {


same as the above comment

tkashem · 2023-03-02T22:23:35Z

/lgtm

Thanks!

tallclair · 2023-03-09T19:59:26Z

/assign @lavalamp

dims · 2023-03-10T17:43:56Z

/assign @liggitt

liggitt · 2023-03-13T13:58:10Z

/unassign
unfortunately I don't have bandwidth to pick up this review before freeze

tallclair · 2023-03-14T20:23:07Z

/milestone v1.28

Punting this to the next release. Hopefully we can get it merged early in the cycle.

furkatgofurov7 · 2023-06-15T08:58:30Z

Hey! v1.28 Bug Triage Lead here.

I am reaching out to know if this PR is on track for the 1.28 release and if we could set a priority? Thanks in advance!

sttts · 2023-06-15T13:24:26Z

/approve
/hold

Please unhold when https://github.com/kubernetes/kubernetes/pull/113611/files#r1173750961 is clarified.

k8s-ci-robot · 2023-06-15T13:24:53Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

~~staging/src/k8s.io/apiserver/OWNERS~~ [sttts]

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

furkatgofurov7 · 2023-06-30T06:57:48Z

@tallclair hey! Can we please label this with priority?

Also, just a reminder that the code freeze is starting 01:00 UTC Wednesday 19th July 2023 / 18:00 PDT Tuesday 18th July 2023 (about 3 weeks from now) and while there is still plenty of time, we want to ensure that each PR has a chance to be merged on time.

stlaz

/lgtm cancel
https://github.com/kubernetes/kubernetes/pull/113611/files#r1231007227 has not been addressed yet

stlaz · 2023-06-30T08:26:40Z

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

@@ -65,8 +62,7 @@ type annotation struct {
 // prefer AddAuditAnnotation over LogAnnotation to avoid dropping annotations.


This godoc mentions functions that no longer exist, please fix that. Perhaps the changes from #117406 might be applicable here?

stlaz · 2023-07-03T14:19:27Z

/lgtm
/hold cancel
the comments are non-blocking, can be addressed as follow-up

k8s-ci-robot · 2023-07-03T14:19:34Z

LGTM label has been added.

Git tree hash: 0c3c22de7f9e5557e679b50aeb4888c2409d9e81

tallclair · 2023-07-09T22:17:54Z

Thanks for getting this merged. I'm on leave right now, but I'll make sure to pick this back up in the next release cycle.

k8s-ci-robot added this to the v1.26 milestone Nov 4, 2022

tallclair assigned lavalamp and sttts Nov 4, 2022

k8s-ci-robot requested review from hzxuzhonghu and liggitt November 4, 2022 00:15

k8s-ci-robot added the area/apiserver label Nov 4, 2022

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Nov 8, 2022

leonardpahlke removed this from the v1.26 milestone Nov 9, 2022

enj added this to Needs Triage in SIG Auth Old Nov 14, 2022

sttts reviewed Dec 1, 2022

View reviewed changes

staging/src/k8s.io/apiserver/pkg/audit/context.go Show resolved Hide resolved

k8s-ci-robot assigned tkashem Dec 2, 2022

ibihim reviewed Dec 2, 2022

View reviewed changes

tallclair added this to the v1.27 milestone Dec 5, 2022

tkashem reviewed Dec 6, 2022

View reviewed changes

tkashem reviewed Dec 7, 2022

View reviewed changes

staging/src/k8s.io/apiserver/pkg/audit/context.go Show resolved Hide resolved

tkashem reviewed Dec 7, 2022

View reviewed changes

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

k8s-ci-robot assigned liggitt Mar 10, 2023

k8s-ci-robot unassigned liggitt Mar 13, 2023

k8s-ci-robot modified the milestones: v1.27, v1.28 Mar 14, 2023

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 15, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 15, 2023

sttts added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Jun 30, 2023

k8s-ci-robot removed the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Jun 30, 2023

stlaz suggested changes Jun 30, 2023

View reviewed changes

k8s-ci-robot assigned stlaz Jun 30, 2023

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 30, 2023

k8s-ci-robot requested review from lavalamp, sttts and tkashem June 30, 2023 08:27

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jul 3, 2023

k8s-ci-robot merged commit 2b03f04 into kubernetes:master Jul 3, 2023
11 checks passed

		@@ -167,8 +131,8 @@ func WithAuditContext(parent context.Context) context.Context {

	// WithAuditContext returns a new context that stores the AuditContext.
	func WithAuditContext(parent context.Context) context.Context {
	if AuditContextFrom(parent) != nil {
	return parent // Avoid double registering.
	}

	return genericapirequest.WithValue(parent, auditKey, &AuditContext{})
	}

		@@ -65,8 +62,7 @@ type annotation struct {
		// prefer AddAuditAnnotation over LogAnnotation to avoid dropping annotations.

Initialize the AuditEvent with the AuditContext #113611

Initialize the AuditEvent with the AuditContext #113611

Conversation

tallclair commented Nov 4, 2022 • edited

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Does this PR introduce a user-facing change?

fedebongio commented Nov 8, 2022

sttts commented Dec 1, 2022

tkashem commented Dec 2, 2022

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

ibihim Dec 2, 2022 • edited

Choose a reason for hiding this comment

ibihim commented Dec 5, 2022

tkashem left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tallclair Dec 13, 2022 • 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

tkashem Dec 6, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkashem Dec 7, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkashem commented Mar 2, 2023

tallclair commented Mar 9, 2023

dims commented Mar 10, 2023

liggitt commented Mar 13, 2023

tallclair commented Mar 14, 2023

furkatgofurov7 commented Jun 15, 2023 • edited

sttts commented Jun 15, 2023

k8s-ci-robot commented Jun 15, 2023

furkatgofurov7 commented Jun 30, 2023

stlaz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stlaz commented Jul 3, 2023

k8s-ci-robot commented Jul 3, 2023

tallclair commented Jul 9, 2023

tallclair commented Nov 4, 2022 •

edited

ibihim Dec 2, 2022 •

edited

tallclair Dec 13, 2022 •

edited

tkashem Dec 6, 2022 •

edited

tkashem Dec 7, 2022 •

edited

furkatgofurov7 commented Jun 15, 2023 •

edited