From c3a68d5de83116289799571de57ace47cd950364 Mon Sep 17 00:00:00 2001 From: Tim Allclair Date: Mon, 28 Mar 2022 17:21:14 +0000 Subject: [PATCH 1/2] Audit annotations mutex --- .../src/k8s.io/apiserver/pkg/audit/context.go | 50 ++++++++- .../apiserver/pkg/audit/context_test.go | 106 ++++++++++++++++++ .../src/k8s.io/apiserver/pkg/audit/request.go | 4 +- 3 files changed, 153 insertions(+), 7 deletions(-) create mode 100644 staging/src/k8s.io/apiserver/pkg/audit/context_test.go diff --git a/staging/src/k8s.io/apiserver/pkg/audit/context.go b/staging/src/k8s.io/apiserver/pkg/audit/context.go index eada7add9a8..1167f67f210 100644 --- a/staging/src/k8s.io/apiserver/pkg/audit/context.go +++ b/staging/src/k8s.io/apiserver/pkg/audit/context.go @@ -18,9 +18,11 @@ package audit import ( "context" + "sync" auditinternal "k8s.io/apiserver/pkg/apis/audit" genericapirequest "k8s.io/apiserver/pkg/endpoints/request" + "k8s.io/klog/v2" ) // The key type is unexported to prevent collisions @@ -37,6 +39,9 @@ const ( // auditKey is the context key for storing the audit event that is being // captured and the evaluated policy that applies to the given request. auditKey + + // auditAnnotationsMutexKey is the context key for the audit annotations mutex. + auditAnnotationsMutexKey ) // annotations = *[]annotation instead of a map to preserve order of insertions @@ -54,6 +59,7 @@ func WithAuditAnnotations(parent context.Context) context.Context { if _, ok := parent.Value(auditAnnotationsKey).(*[]annotation); ok { return parent } + parent = withAuditAnnotationsMutex(parent) var annotations []annotation // avoid allocations until we actually need it return genericapirequest.WithValue(parent, auditAnnotationsKey, &annotations) @@ -67,6 +73,15 @@ func WithAuditAnnotations(parent context.Context) context.Context { // Handlers that are unaware of their position in the overall request flow should // prefer AddAuditAnnotation over LogAnnotation to avoid dropping annotations. func AddAuditAnnotation(ctx context.Context, key, value string) { + mutex, ok := auditAnnotationsMutex(ctx) + if !ok { + klog.Errorf("Attempted to add audit annotation from unsupported request chain: %q=%q", key, value) + return + } + + mutex.Lock() + defer mutex.Unlock() + // use the audit event directly if we have it if ae := AuditEventFrom(ctx); ae != nil { LogAnnotation(ae, key, value) @@ -83,19 +98,31 @@ func AddAuditAnnotation(ctx context.Context, key, value string) { // This is private to prevent reads/write to the slice from outside of this package. // The audit event should be directly read to get access to the annotations. -func auditAnnotationsFrom(ctx context.Context) []annotation { - annotations, ok := ctx.Value(auditAnnotationsKey).(*[]annotation) +func addAuditAnnotationsFrom(ctx context.Context, ev *auditinternal.Event) { + mutex, ok := auditAnnotationsMutex(ctx) if !ok { - return nil // adding audit annotation is not supported at this call site + klog.Errorf("Attempted to copy audit annotations from unsupported request chain") + return } - return *annotations + mutex.Lock() + defer mutex.Unlock() + + annotations, ok := ctx.Value(auditAnnotationsKey).(*[]annotation) + if !ok { + return // no annotations to copy + } + + for _, kv := range *annotations { + LogAnnotation(ev, kv.key, kv.value) + } } // WithAuditContext returns a new context that stores the pair of the audit // configuration object that applies to the given request and // the audit event that is going to be written to the API audit log. func WithAuditContext(parent context.Context, ev *AuditContext) context.Context { + parent = withAuditAnnotationsMutex(parent) return genericapirequest.WithValue(parent, auditKey, ev) } @@ -114,3 +141,18 @@ func AuditContextFrom(ctx context.Context) *AuditContext { ev, _ := ctx.Value(auditKey).(*AuditContext) return ev } + +// WithAuditAnnotationMutex adds a mutex for guarding context.AddAuditAnnotation. +func withAuditAnnotationsMutex(parent context.Context) context.Context { + if _, ok := parent.Value(auditAnnotationsMutexKey).(*sync.Mutex); ok { + return parent + } + var mutex sync.Mutex + return genericapirequest.WithValue(parent, auditAnnotationsMutexKey, &mutex) +} + +// AuditAnnotationsMutex returns the audit annotations mutex from the context. +func auditAnnotationsMutex(ctx context.Context) (*sync.Mutex, bool) { + mutex, ok := ctx.Value(auditAnnotationsMutexKey).(*sync.Mutex) + return mutex, ok +} diff --git a/staging/src/k8s.io/apiserver/pkg/audit/context_test.go b/staging/src/k8s.io/apiserver/pkg/audit/context_test.go new file mode 100644 index 00000000000..a593f90fe48 --- /dev/null +++ b/staging/src/k8s.io/apiserver/pkg/audit/context_test.go @@ -0,0 +1,106 @@ +/* +Copyright 2021 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package audit + +import ( + "context" + "fmt" + "sync" + "testing" + + auditinternal "k8s.io/apiserver/pkg/apis/audit" + + "github.com/stretchr/testify/assert" +) + +func TestAddAuditAnnotation(t *testing.T) { + const ( + annotationKeyTemplate = "test-annotation-%d" + annotationValue = "test-annotation-value" + numAnnotations = 10 + ) + + expectAnnotations := func(t *testing.T, annotations map[string]string) { + assert.Len(t, annotations, numAnnotations) + } + noopValidator := func(_ *testing.T, _ context.Context) {} + preEventValidator := func(t *testing.T, ctx context.Context) { + ev := auditinternal.Event{ + Level: auditinternal.LevelMetadata, + } + addAuditAnnotationsFrom(ctx, &ev) + expectAnnotations(t, ev.Annotations) + } + postEventValidator := func(t *testing.T, ctx context.Context) { + ev := AuditEventFrom(ctx) + expectAnnotations(t, ev.Annotations) + } + postEventEmptyValidator := func(t *testing.T, ctx context.Context) { + ev := AuditEventFrom(ctx) + assert.Empty(t, ev.Annotations) + } + + tests := []struct { + description string + ctx context.Context + validator func(t *testing.T, ctx context.Context) + }{{ + description: "no audit", + ctx: context.Background(), + validator: noopValidator, + }, { + description: "no annotations context", + ctx: WithAuditContext(context.Background(), newAuditContext(auditinternal.LevelMetadata)), + validator: postEventValidator, + }, { + description: "no audit context", + ctx: WithAuditAnnotations(context.Background()), + validator: preEventValidator, + }, { + description: "both contexts metadata level", + ctx: WithAuditContext(WithAuditAnnotations(context.Background()), newAuditContext(auditinternal.LevelMetadata)), + validator: postEventValidator, + }, { + description: "both contexts none level", + ctx: WithAuditContext(WithAuditAnnotations(context.Background()), newAuditContext(auditinternal.LevelNone)), + validator: postEventEmptyValidator, + }} + + for _, test := range tests { + t.Run(test.description, func(t *testing.T) { + var wg sync.WaitGroup + wg.Add(numAnnotations) + for i := 0; i < numAnnotations; i++ { + go func(i int) { + AddAuditAnnotation(test.ctx, fmt.Sprintf(annotationKeyTemplate, i), annotationValue) + wg.Done() + }(i) + } + wg.Wait() + + test.validator(t, test.ctx) + }) + } +} + +func newAuditContext(l auditinternal.Level) *AuditContext { + return &AuditContext{ + Event: &auditinternal.Event{ + Level: l, + }, + } +} diff --git a/staging/src/k8s.io/apiserver/pkg/audit/request.go b/staging/src/k8s.io/apiserver/pkg/audit/request.go index f48566576bc..19bb9993a39 100644 --- a/staging/src/k8s.io/apiserver/pkg/audit/request.go +++ b/staging/src/k8s.io/apiserver/pkg/audit/request.go @@ -87,9 +87,7 @@ func NewEventFromRequest(req *http.Request, requestReceivedTimestamp time.Time, } } - for _, kv := range auditAnnotationsFrom(req.Context()) { - LogAnnotation(ev, kv.key, kv.value) - } + addAuditAnnotationsFrom(req.Context(), ev) return ev, nil } From bdebc62d49293a0fbbd7e0d95bfd94b1ce21015c Mon Sep 17 00:00:00 2001 From: Tim Allclair Date: Mon, 28 Mar 2022 11:38:38 -0700 Subject: [PATCH 2/2] Don't add audit annotations directly to the audit event --- .../security/podsecurity/admission.go | 10 +- .../k8s.io/apiserver/pkg/admission/audit.go | 34 +++---- .../apiserver/pkg/admission/audit_test.go | 13 ++- .../src/k8s.io/apiserver/pkg/audit/context.go | 94 ++++++++++++++++--- .../apiserver/pkg/audit/context_test.go | 14 +++ .../src/k8s.io/apiserver/pkg/audit/request.go | 15 --- .../apiserver/pkg/audit/request_test.go | 15 --- .../apiserver/pkg/endpoints/filters/audit.go | 6 +- .../pkg/endpoints/filters/authorization.go | 13 +-- .../pkg/endpoints/handlers/create.go | 3 +- .../pkg/endpoints/handlers/delete.go | 6 +- .../apiserver/pkg/endpoints/handlers/patch.go | 3 +- .../apiserver/pkg/endpoints/handlers/rest.go | 4 +- .../pkg/endpoints/handlers/update.go | 3 +- .../apiserver/pkg/server/config_test.go | 5 +- .../plugin/pkg/audit/truncate/truncate.go | 7 +- 16 files changed, 146 insertions(+), 99 deletions(-) diff --git a/plugin/pkg/admission/security/podsecurity/admission.go b/plugin/pkg/admission/security/podsecurity/admission.go index f4b4859e34f..498c0eae00c 100644 --- a/plugin/pkg/admission/security/podsecurity/admission.go +++ b/plugin/pkg/admission/security/podsecurity/admission.go @@ -190,8 +190,14 @@ func (p *Plugin) Validate(ctx context.Context, a admission.Attributes, o admissi for _, w := range result.Warnings { warning.AddWarning(ctx, "", w) } - for k, v := range result.AuditAnnotations { - audit.AddAuditAnnotation(ctx, podsecurityadmissionapi.AuditAnnotationPrefix+k, v) + if len(result.AuditAnnotations) > 0 { + annotations := make([]string, len(result.AuditAnnotations)*2) + i := 0 + for k, v := range result.AuditAnnotations { + annotations[i], annotations[i+1] = podsecurityadmissionapi.AuditAnnotationPrefix+k, v + i += 2 + } + audit.AddAuditAnnotations(ctx, annotations...) } if !result.Allowed { // start with a generic forbidden error diff --git a/staging/src/k8s.io/apiserver/pkg/admission/audit.go b/staging/src/k8s.io/apiserver/pkg/admission/audit.go index 03f84236210..7c0993f0908 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/audit.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/audit.go @@ -19,19 +19,13 @@ package admission import ( "context" "fmt" - "sync" - auditinternal "k8s.io/apiserver/pkg/apis/audit" "k8s.io/apiserver/pkg/audit" ) // auditHandler logs annotations set by other admission handlers type auditHandler struct { Interface - // TODO: move the lock near the Annotations field of the audit event so it is always protected from concurrent access. - // to protect the 'Annotations' map of the audit event from concurrent writes - mutex sync.Mutex - ae *auditinternal.Event } var _ Interface = &auditHandler{} @@ -42,11 +36,11 @@ var _ ValidationInterface = &auditHandler{} // of attribute into the audit event. Attributes passed to the Admit and // Validate function must be instance of privateAnnotationsGetter or // AnnotationsGetter, otherwise an error is returned. -func WithAudit(i Interface, ae *auditinternal.Event) Interface { - if i == nil || ae == nil { +func WithAudit(i Interface) Interface { + if i == nil { return i } - return &auditHandler{Interface: i, ae: ae} + return &auditHandler{Interface: i} } func (handler *auditHandler) Admit(ctx context.Context, a Attributes, o ObjectInterfaces) error { @@ -59,7 +53,7 @@ func (handler *auditHandler) Admit(ctx context.Context, a Attributes, o ObjectIn var err error if mutator, ok := handler.Interface.(MutationInterface); ok { err = mutator.Admit(ctx, a, o) - handler.logAnnotations(a) + handler.logAnnotations(ctx, a) } return err } @@ -74,7 +68,7 @@ func (handler *auditHandler) Validate(ctx context.Context, a Attributes, o Objec var err error if validator, ok := handler.Interface.(ValidationInterface); ok { err = validator.Validate(ctx, a, o) - handler.logAnnotations(a) + handler.logAnnotations(ctx, a) } return err } @@ -88,23 +82,21 @@ func ensureAnnotationGetter(a Attributes) error { return fmt.Errorf("attributes must be an instance of privateAnnotationsGetter or AnnotationsGetter") } -func (handler *auditHandler) logAnnotations(a Attributes) { - if handler.ae == nil { +func (handler *auditHandler) logAnnotations(ctx context.Context, a Attributes) { + ae := audit.AuditEventFrom(ctx) + if ae == nil { return } - handler.mutex.Lock() - defer handler.mutex.Unlock() + var annotations map[string]string switch a := a.(type) { case privateAnnotationsGetter: - for key, value := range a.getAnnotations(handler.ae.Level) { - audit.LogAnnotation(handler.ae, key, value) - } + annotations = a.getAnnotations(ae.Level) case AnnotationsGetter: - for key, value := range a.GetAnnotations(handler.ae.Level) { - audit.LogAnnotation(handler.ae, key, value) - } + annotations = a.GetAnnotations(ae.Level) default: // this will never happen, because we have already checked it in ensureAnnotationGetter } + + audit.AddAuditAnnotationsMap(ctx, annotations) } diff --git a/staging/src/k8s.io/apiserver/pkg/admission/audit_test.go b/staging/src/k8s.io/apiserver/pkg/admission/audit_test.go index 32b7497e949..e319fc56369 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/audit_test.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/audit_test.go @@ -24,6 +24,7 @@ import ( "k8s.io/apimachinery/pkg/runtime/schema" auditinternal "k8s.io/apiserver/pkg/apis/audit" + "k8s.io/apiserver/pkg/audit" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -142,7 +143,8 @@ func TestWithAudit(t *testing.T) { for tcName, tc := range testCases { var handler Interface = fakeHandler{tc.admit, tc.admitAnnotations, tc.validate, tc.validateAnnotations, tc.handles} ae := &auditinternal.Event{Level: auditinternal.LevelMetadata} - auditHandler := WithAudit(handler, ae) + ctx := audit.WithAuditContext(context.Background(), &audit.AuditContext{Event: ae}) + auditHandler := WithAudit(handler) a := attributes() assert.Equal(t, handler.Handles(Create), auditHandler.Handles(Create), tcName+": WithAudit decorator should not effect the return value") @@ -151,13 +153,13 @@ func TestWithAudit(t *testing.T) { require.True(t, ok) auditMutator, ok := auditHandler.(MutationInterface) require.True(t, ok) - assert.Equal(t, mutator.Admit(context.TODO(), a, nil), auditMutator.Admit(context.TODO(), a, nil), tcName+": WithAudit decorator should not effect the return value") + assert.Equal(t, mutator.Admit(ctx, a, nil), auditMutator.Admit(ctx, a, nil), tcName+": WithAudit decorator should not effect the return value") validator, ok := handler.(ValidationInterface) require.True(t, ok) auditValidator, ok := auditHandler.(ValidationInterface) require.True(t, ok) - assert.Equal(t, validator.Validate(context.TODO(), a, nil), auditValidator.Validate(context.TODO(), a, nil), tcName+": WithAudit decorator should not effect the return value") + assert.Equal(t, validator.Validate(ctx, a, nil), auditValidator.Validate(ctx, a, nil), tcName+": WithAudit decorator should not effect the return value") annotations := make(map[string]string, len(tc.admitAnnotations)+len(tc.validateAnnotations)) for k, v := range tc.admitAnnotations { @@ -183,7 +185,8 @@ func TestWithAuditConcurrency(t *testing.T) { } var handler Interface = fakeHandler{admitAnnotations: admitAnnotations, handles: true} ae := &auditinternal.Event{Level: auditinternal.LevelMetadata} - auditHandler := WithAudit(handler, ae) + ctx := audit.WithAuditContext(context.Background(), &audit.AuditContext{Event: ae}) + auditHandler := WithAudit(handler) a := attributes() // Simulate the scenario store.DeleteCollection @@ -197,7 +200,7 @@ func TestWithAuditConcurrency(t *testing.T) { require.True(t, ok) auditMutator, ok := auditHandler.(MutationInterface) require.True(t, ok) - assert.Equal(t, mutator.Admit(context.TODO(), a, nil), auditMutator.Admit(context.TODO(), a, nil), "WithAudit decorator should not effect the return value") + assert.Equal(t, mutator.Admit(ctx, a, nil), auditMutator.Admit(ctx, a, nil), "WithAudit decorator should not effect the return value") }() } wg.Wait() diff --git a/staging/src/k8s.io/apiserver/pkg/audit/context.go b/staging/src/k8s.io/apiserver/pkg/audit/context.go index 1167f67f210..122666546fc 100644 --- a/staging/src/k8s.io/apiserver/pkg/audit/context.go +++ b/staging/src/k8s.io/apiserver/pkg/audit/context.go @@ -18,6 +18,7 @@ package audit import ( "context" + "fmt" "sync" auditinternal "k8s.io/apiserver/pkg/apis/audit" @@ -30,10 +31,7 @@ type key int const ( // auditAnnotationsKey is the context key for the audit annotations. - // TODO: it's wasteful to store the audit annotations under a separate key, we - // copy the request context twice for audit purposes. We should move the audit - // annotations under AuditContext so we can get rid of the additional request - // context copy. + // TODO: consolidate all audit info under the AuditContext, rather than storing 3 separate keys. auditAnnotationsKey key = iota // auditKey is the context key for storing the audit event that is being @@ -75,25 +73,80 @@ func WithAuditAnnotations(parent context.Context) context.Context { func AddAuditAnnotation(ctx context.Context, key, value string) { mutex, ok := auditAnnotationsMutex(ctx) if !ok { - klog.Errorf("Attempted to add audit annotation from unsupported request chain: %q=%q", key, value) + klog.ErrorS(nil, "Attempted to add audit annotations from unsupported request chain", "annotation", fmt.Sprintf("%s=%s", key, value)) return } mutex.Lock() defer mutex.Unlock() - // use the audit event directly if we have it - if ae := AuditEventFrom(ctx); ae != nil { - LogAnnotation(ae, key, value) + ae := AuditEventFrom(ctx) + var ctxAnnotations *[]annotation + if ae == nil { + ctxAnnotations, _ = ctx.Value(auditAnnotationsKey).(*[]annotation) + } + + addAuditAnnotationLocked(ae, ctxAnnotations, key, value) +} + +// AddAuditAnnotations is a bulk version of AddAuditAnnotation. Refer to AddAuditAnnotation for +// restrictions on when this can be called. +// keysAndValues are the key-value pairs to add, and must have an even number of items. +func AddAuditAnnotations(ctx context.Context, keysAndValues ...string) { + mutex, ok := auditAnnotationsMutex(ctx) + if !ok { + klog.ErrorS(nil, "Attempted to add audit annotations from unsupported request chain", "annotations", keysAndValues) return } - annotations, ok := ctx.Value(auditAnnotationsKey).(*[]annotation) - if !ok { - return // adding audit annotation is not supported at this call site + mutex.Lock() + defer mutex.Unlock() + + ae := AuditEventFrom(ctx) + var ctxAnnotations *[]annotation + if ae == nil { + ctxAnnotations, _ = ctx.Value(auditAnnotationsKey).(*[]annotation) } - *annotations = append(*annotations, annotation{key: key, value: value}) + if len(keysAndValues)%2 != 0 { + klog.Errorf("Dropping mismatched audit annotation %q", keysAndValues[len(keysAndValues)-1]) + } + for i := 0; i < len(keysAndValues); i += 2 { + addAuditAnnotationLocked(ae, ctxAnnotations, keysAndValues[i], keysAndValues[i+1]) + } +} + +// AddAuditAnnotationsMap is a bulk version of AddAuditAnnotation. Refer to AddAuditAnnotation for +// restrictions on when this can be called. +func AddAuditAnnotationsMap(ctx context.Context, annotations map[string]string) { + mutex, ok := auditAnnotationsMutex(ctx) + if !ok { + klog.ErrorS(nil, "Attempted to add audit annotations from unsupported request chain", "annotations", annotations) + return + } + + mutex.Lock() + defer mutex.Unlock() + + ae := AuditEventFrom(ctx) + var ctxAnnotations *[]annotation + if ae == nil { + ctxAnnotations, _ = ctx.Value(auditAnnotationsKey).(*[]annotation) + } + + for k, v := range annotations { + addAuditAnnotationLocked(ae, ctxAnnotations, k, v) + } +} + +// addAuditAnnotationLocked is the shared code for recording an audit annotation. This method should +// only be called while the auditAnnotationsMutex is locked. +func addAuditAnnotationLocked(ae *auditinternal.Event, annotations *[]annotation, key, value string) { + if ae != nil { + logAnnotation(ae, key, value) + } else if annotations != nil { + *annotations = append(*annotations, annotation{key: key, value: value}) + } } // This is private to prevent reads/write to the slice from outside of this package. @@ -114,10 +167,25 @@ func addAuditAnnotationsFrom(ctx context.Context, ev *auditinternal.Event) { } for _, kv := range *annotations { - LogAnnotation(ev, kv.key, kv.value) + logAnnotation(ev, kv.key, kv.value) } } +// LogAnnotation fills in the Annotations according to the key value pair. +func logAnnotation(ae *auditinternal.Event, key, value string) { + if ae == nil || ae.Level.Less(auditinternal.LevelMetadata) { + return + } + if ae.Annotations == nil { + ae.Annotations = make(map[string]string) + } + if v, ok := ae.Annotations[key]; ok && v != value { + klog.Warningf("Failed to set annotations[%q] to %q for audit:%q, it has already been set to %q", key, value, ae.AuditID, ae.Annotations[key]) + return + } + ae.Annotations[key] = value +} + // WithAuditContext returns a new context that stores the pair of the audit // configuration object that applies to the given request and // the audit event that is going to be written to the API audit log. diff --git a/staging/src/k8s.io/apiserver/pkg/audit/context_test.go b/staging/src/k8s.io/apiserver/pkg/audit/context_test.go index a593f90fe48..0dbca762d9f 100644 --- a/staging/src/k8s.io/apiserver/pkg/audit/context_test.go +++ b/staging/src/k8s.io/apiserver/pkg/audit/context_test.go @@ -97,6 +97,20 @@ func TestAddAuditAnnotation(t *testing.T) { } } +func TestLogAnnotation(t *testing.T) { + ev := &auditinternal.Event{ + Level: auditinternal.LevelMetadata, + AuditID: "fake id", + } + logAnnotation(ev, "foo", "bar") + logAnnotation(ev, "foo", "baz") + assert.Equal(t, "bar", ev.Annotations["foo"], "audit annotation should not be overwritten.") + + logAnnotation(ev, "qux", "") + logAnnotation(ev, "qux", "baz") + assert.Equal(t, "", ev.Annotations["qux"], "audit annotation should not be overwritten.") +} + func newAuditContext(l auditinternal.Level) *AuditContext { return &AuditContext{ Event: &auditinternal.Event{ diff --git a/staging/src/k8s.io/apiserver/pkg/audit/request.go b/staging/src/k8s.io/apiserver/pkg/audit/request.go index 19bb9993a39..57a10e62b02 100644 --- a/staging/src/k8s.io/apiserver/pkg/audit/request.go +++ b/staging/src/k8s.io/apiserver/pkg/audit/request.go @@ -243,21 +243,6 @@ func encodeObject(obj runtime.Object, gv schema.GroupVersion, serializer runtime }, nil } -// LogAnnotation fills in the Annotations according to the key value pair. -func LogAnnotation(ae *auditinternal.Event, key, value string) { - if ae == nil || ae.Level.Less(auditinternal.LevelMetadata) { - return - } - if ae.Annotations == nil { - ae.Annotations = make(map[string]string) - } - if v, ok := ae.Annotations[key]; ok && v != value { - klog.Warningf("Failed to set annotations[%q] to %q for audit:%q, it has already been set to %q", key, value, ae.AuditID, ae.Annotations[key]) - return - } - ae.Annotations[key] = value -} - // truncate User-Agent if too long, otherwise return it directly. func maybeTruncateUserAgent(req *http.Request) string { ua := req.UserAgent() diff --git a/staging/src/k8s.io/apiserver/pkg/audit/request_test.go b/staging/src/k8s.io/apiserver/pkg/audit/request_test.go index ee1701ba0bd..a8905f52a81 100644 --- a/staging/src/k8s.io/apiserver/pkg/audit/request_test.go +++ b/staging/src/k8s.io/apiserver/pkg/audit/request_test.go @@ -25,26 +25,11 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" - auditinternal "k8s.io/apiserver/pkg/apis/audit" "github.com/google/go-cmp/cmp" "github.com/stretchr/testify/assert" ) -func TestLogAnnotation(t *testing.T) { - ev := &auditinternal.Event{ - Level: auditinternal.LevelMetadata, - AuditID: "fake id", - } - LogAnnotation(ev, "foo", "bar") - LogAnnotation(ev, "foo", "baz") - assert.Equal(t, "bar", ev.Annotations["foo"], "audit annotation should not be overwritten.") - - LogAnnotation(ev, "qux", "") - LogAnnotation(ev, "qux", "baz") - assert.Equal(t, "", ev.Annotations["qux"], "audit annotation should not be overwritten.") -} - func TestMaybeTruncateUserAgent(t *testing.T) { req := &http.Request{} req.Header = http.Header{} diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/filters/audit.go b/staging/src/k8s.io/apiserver/pkg/endpoints/filters/audit.go index 2f3197b2c01..b539072e601 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/filters/audit.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/filters/audit.go @@ -177,10 +177,8 @@ func writeLatencyToAnnotation(ctx context.Context, ev *auditinternal.Event) { } // record the total latency for this request, for convenience. - audit.LogAnnotation(ev, "apiserver.latency.k8s.io/total", latency.String()) - for k, v := range layerLatencies { - audit.LogAnnotation(ev, k, v) - } + layerLatencies["apiserver.latency.k8s.io/total"] = latency.String() + audit.AddAuditAnnotationsMap(ctx, layerLatencies) } func processAuditEvent(ctx context.Context, sink audit.Sink, ev *auditinternal.Event, omitStages []auditinternal.Stage) bool { diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/filters/authorization.go b/staging/src/k8s.io/apiserver/pkg/endpoints/filters/authorization.go index 395d9332a1f..fba58828390 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/filters/authorization.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/filters/authorization.go @@ -49,7 +49,6 @@ func WithAuthorization(handler http.Handler, a authorizer.Authorizer, s runtime. } return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { ctx := req.Context() - ae := audit.AuditEventFrom(ctx) attributes, err := GetAuthorizerAttributes(ctx) if err != nil { @@ -59,20 +58,22 @@ func WithAuthorization(handler http.Handler, a authorizer.Authorizer, s runtime. authorized, reason, err := a.Authorize(ctx, attributes) // an authorizer like RBAC could encounter evaluation errors and still allow the request, so authorizer decision is checked before error here. if authorized == authorizer.DecisionAllow { - audit.LogAnnotation(ae, decisionAnnotationKey, decisionAllow) - audit.LogAnnotation(ae, reasonAnnotationKey, reason) + audit.AddAuditAnnotations(ctx, + decisionAnnotationKey, decisionAllow, + reasonAnnotationKey, reason) handler.ServeHTTP(w, req) return } if err != nil { - audit.LogAnnotation(ae, reasonAnnotationKey, reasonError) + audit.AddAuditAnnotation(ctx, reasonAnnotationKey, reasonError) responsewriters.InternalError(w, req, err) return } klog.V(4).InfoS("Forbidden", "URI", req.RequestURI, "Reason", reason) - audit.LogAnnotation(ae, decisionAnnotationKey, decisionForbid) - audit.LogAnnotation(ae, reasonAnnotationKey, reason) + audit.AddAuditAnnotations(ctx, + decisionAnnotationKey, decisionForbid, + reasonAnnotationKey, reason) responsewriters.Forbidden(ctx, attributes, w, req, reason, s) }) } diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/create.go b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/create.go index 878c2cfa42a..19ca0dae9f5 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/create.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/create.go @@ -157,8 +157,7 @@ func createHandler(r rest.NamedCreater, scope *RequestScope, admit admission.Int } ctx = request.WithNamespace(ctx, namespace) - ae := audit.AuditEventFrom(ctx) - admit = admission.WithAudit(admit, ae) + admit = admission.WithAudit(admit) audit.LogRequestObject(req.Context(), obj, objGV, scope.Resource, scope.Subresource, scope.Serializer) userInfo, _ := request.UserFrom(ctx) diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/delete.go b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/delete.go index 5f3d2c9b91d..99979fc14b5 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/delete.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/delete.go @@ -67,8 +67,7 @@ func DeleteResource(r rest.GracefulDeleter, allowsOptions bool, scope *RequestSc defer cancel() ctx = request.WithNamespace(ctx, namespace) - ae := audit.AuditEventFrom(ctx) - admit = admission.WithAudit(admit, ae) + admit = admission.WithAudit(admit) outputMediaType, _, err := negotiation.NegotiateOutputMediaType(req, scope.Serializer, scope) if err != nil { @@ -190,7 +189,6 @@ func DeleteCollection(r rest.CollectionDeleter, checkBody bool, scope *RequestSc defer cancel() ctx = request.WithNamespace(ctx, namespace) - ae := audit.AuditEventFrom(ctx) outputMediaType, _, err := negotiation.NegotiateOutputMediaType(req, scope.Serializer, scope) if err != nil { @@ -268,7 +266,7 @@ func DeleteCollection(r rest.CollectionDeleter, checkBody bool, scope *RequestSc } options.TypeMeta.SetGroupVersionKind(metav1.SchemeGroupVersion.WithKind("DeleteOptions")) - admit = admission.WithAudit(admit, ae) + admit = admission.WithAudit(admit) userInfo, _ := request.UserFrom(ctx) staticAdmissionAttrs := admission.NewAttributesRecord(nil, nil, scope.Kind, namespace, "", scope.Resource, scope.Subresource, admission.Delete, options, dryrun.IsDryRun(options.DryRun), userInfo) result, err := finisher.FinishRequest(ctx, func() (runtime.Object, error) { diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/patch.go b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/patch.go index a951c1de7a8..5a2af674cd8 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/patch.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/patch.go @@ -124,8 +124,7 @@ func PatchResource(r rest.Patcher, scope *RequestScope, admit admission.Interfac } options.TypeMeta.SetGroupVersionKind(metav1.SchemeGroupVersion.WithKind("PatchOptions")) - ae := audit.AuditEventFrom(ctx) - admit = admission.WithAudit(admit, ae) + admit = admission.WithAudit(admit) audit.LogRequestPatch(req.Context(), patchBytes) trace.Step("Recorded the audit event") diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/rest.go b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/rest.go index 15e3d9f7f1e..e413661e760 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/rest.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/rest.go @@ -39,7 +39,6 @@ import ( "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" "k8s.io/apiserver/pkg/admission" - "k8s.io/apiserver/pkg/audit" "k8s.io/apiserver/pkg/authorization/authorizer" "k8s.io/apiserver/pkg/endpoints/handlers/fieldmanager" "k8s.io/apiserver/pkg/endpoints/handlers/responsewriters" @@ -191,8 +190,7 @@ func ConnectResource(connecter rest.Connecter, scope *RequestScope, admit admiss } ctx := req.Context() ctx = request.WithNamespace(ctx, namespace) - ae := audit.AuditEventFrom(ctx) - admit = admission.WithAudit(admit, ae) + admit = admission.WithAudit(admit) opts, subpath, subpathKey := connecter.NewConnectOptions() if err := getRequestOptions(req, scope, opts, subpath, subpathKey, isSubresource); err != nil { diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/update.go b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/update.go index dced59404d4..36f3d31ea23 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/update.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/update.go @@ -135,9 +135,8 @@ func UpdateResource(r rest.Updater, scope *RequestScope, admit admission.Interfa } trace.Step("Conversion done") - ae := audit.AuditEventFrom(ctx) audit.LogRequestObject(req.Context(), obj, objGV, scope.Resource, scope.Subresource, scope.Serializer) - admit = admission.WithAudit(admit, ae) + admit = admission.WithAudit(admit) // if this object supports namespace info if objectMeta, err := meta.Accessor(obj); err == nil { diff --git a/staging/src/k8s.io/apiserver/pkg/server/config_test.go b/staging/src/k8s.io/apiserver/pkg/server/config_test.go index 0616088c7ff..b2a106e48f5 100644 --- a/staging/src/k8s.io/apiserver/pkg/server/config_test.go +++ b/staging/src/k8s.io/apiserver/pkg/server/config_test.go @@ -312,9 +312,6 @@ func TestAuthenticationAuditAnnotationsDefaultChain(t *testing.T) { t.Error("unexpected nil audit event") } - // confirm that the direct way of setting audit annotations later in the chain works as expected - audit.LogAnnotation(ae, "snorlax", "is cool too") - // confirm that the indirect way of setting audit annotations later in the chain also works audit.AddAuditAnnotation(r.Context(), "dogs", "are okay") @@ -334,7 +331,7 @@ func TestAuthenticationAuditAnnotationsDefaultChain(t *testing.T) { t.Error("expected audit events, got none") } // these should all be the same because the handler chain mutates the event in place - want := map[string]string{"pandas": "are awesome", "snorlax": "is cool too", "dogs": "are okay"} + want := map[string]string{"pandas": "are awesome", "dogs": "are okay"} for _, event := range backend.events { if event.Stage != auditinternal.StageResponseComplete { t.Errorf("expected event stage to be complete, got: %s", event.Stage) diff --git a/staging/src/k8s.io/apiserver/plugin/pkg/audit/truncate/truncate.go b/staging/src/k8s.io/apiserver/plugin/pkg/audit/truncate/truncate.go index de1c2d9f74b..42a40e3ebea 100644 --- a/staging/src/k8s.io/apiserver/plugin/pkg/audit/truncate/truncate.go +++ b/staging/src/k8s.io/apiserver/plugin/pkg/audit/truncate/truncate.go @@ -126,7 +126,12 @@ func truncate(e *auditinternal.Event) *auditinternal.Event { newEvent.RequestObject = nil newEvent.ResponseObject = nil - audit.LogAnnotation(newEvent, annotationKey, annotationValue) + + if newEvent.Annotations == nil { + newEvent.Annotations = make(map[string]string) + } + newEvent.Annotations[annotationKey] = annotationValue + return newEvent }