From e418ee3a92ca6c670d26f775b0f669e8a5fe233c Mon Sep 17 00:00:00 2001 From: Davanum Srinivas Date: Mon, 12 May 2025 09:29:22 -0400 Subject: [PATCH 1/3] Avoid encoding in LogResponseObject when we are not going to use it Signed-off-by: Davanum Srinivas --- .../src/k8s.io/apiserver/pkg/audit/request.go | 2 +- .../apiserver/pkg/audit/request_log_test.go | 259 ++++++++++++++++++ 2 files changed, 260 insertions(+), 1 deletion(-) create mode 100644 staging/src/k8s.io/apiserver/pkg/audit/request_log_test.go diff --git a/staging/src/k8s.io/apiserver/pkg/audit/request.go b/staging/src/k8s.io/apiserver/pkg/audit/request.go index 60b69b0b29d..705d11e88fb 100644 --- a/staging/src/k8s.io/apiserver/pkg/audit/request.go +++ b/staging/src/k8s.io/apiserver/pkg/audit/request.go @@ -171,7 +171,7 @@ func LogRequestPatch(ctx context.Context, patch []byte) { // will be converted to the given gv. func LogResponseObject(ctx context.Context, obj runtime.Object, gv schema.GroupVersion, s runtime.NegotiatedSerializer) { ac := AuditContextFrom(WithAuditContext(ctx)) - if ac.GetEventLevel().Less(auditinternal.LevelMetadata) { + if ac.GetEventLevel().Less(auditinternal.LevelRequestResponse) { return } diff --git a/staging/src/k8s.io/apiserver/pkg/audit/request_log_test.go b/staging/src/k8s.io/apiserver/pkg/audit/request_log_test.go new file mode 100644 index 00000000000..b41450db208 --- /dev/null +++ b/staging/src/k8s.io/apiserver/pkg/audit/request_log_test.go @@ -0,0 +1,259 @@ +/* +Copyright 2025 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" + "io" + "strings" + "testing" + + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/runtime/serializer" + auditinternal "k8s.io/apiserver/pkg/apis/audit" +) + +func TestLogResponseObjectWithPod(t *testing.T) { + testPod := &corev1.Pod{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "v1", + Kind: "Pod", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "test-pod", + Namespace: "test-namespace", + }, + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: "test-container", + Image: "test-image", + }, + }, + }, + } + + scheme := runtime.NewScheme() + if err := corev1.AddToScheme(scheme); err != nil { + t.Fatalf("Failed to add core/v1 to scheme: %v", err) + } + codecs := serializer.NewCodecFactory(scheme) + negotiatedSerializer := codecs.WithoutConversion() + + // Create audit context with RequestResponse level + ctx := WithAuditContext(context.Background()) + ac := AuditContextFrom(ctx) + + captureSink := &capturingAuditSink{} + if err := ac.Init(RequestAuditConfig{Level: auditinternal.LevelRequestResponse}, captureSink); err != nil { + t.Fatalf("Failed to initialize audit context: %v", err) + } + + LogResponseObject(ctx, testPod, schema.GroupVersion{Group: "", Version: "v1"}, negotiatedSerializer) + ac.ProcessEventStage(ctx, auditinternal.StageResponseComplete) + + if len(captureSink.events) != 1 { + t.Fatalf("Expected one audit event to be captured, got %d", len(captureSink.events)) + } + event := captureSink.events[0] + if event.ResponseObject == nil { + t.Fatal("Expected ResponseObject to be set, but it was nil") + } + if event.ResponseObject.ContentType != runtime.ContentTypeJSON { + t.Errorf("Expected ContentType to be %q, got %q", runtime.ContentTypeJSON, event.ResponseObject.ContentType) + } + if len(event.ResponseObject.Raw) == 0 { + t.Error("Expected ResponseObject.Raw to contain data, but it was empty") + } + + responseJSON := string(event.ResponseObject.Raw) + expectedFields := []string{"test-pod", "test-namespace", "test-container", "test-image"} + for _, field := range expectedFields { + if !strings.Contains(responseJSON, field) { + t.Errorf("Response should contain %q but didn't. Response: %s", field, responseJSON) + } + } + + if event.ResponseStatus != nil { + t.Errorf("Expected ResponseStatus to be nil for regular object, got: %+v", event.ResponseStatus) + } +} + +func TestLogResponseObjectWithStatus(t *testing.T) { + // Create a status object to test ResponseStatus handling + testStatus := &metav1.Status{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "v1", + Kind: "Status", + }, + Status: "Success", + Message: "Test status message", + Reason: "TestReason", + Code: 200, + } + + scheme := runtime.NewScheme() + err := metav1.AddMetaToScheme(scheme) + if err != nil { + t.Fatalf("Failed to add meta to scheme: %v", err) + } + scheme.AddKnownTypes(schema.GroupVersion{Version: "v1"}, &metav1.Status{}) + codecs := serializer.NewCodecFactory(scheme) + negotiatedSerializer := codecs.WithoutConversion() + + ctx := WithAuditContext(context.Background()) + ac := AuditContextFrom(ctx) + + captureSink := &capturingAuditSink{} + if err := ac.Init(RequestAuditConfig{Level: auditinternal.LevelRequestResponse}, captureSink); err != nil { + t.Fatalf("Failed to initialize audit context: %v", err) + } + + LogResponseObject(ctx, testStatus, schema.GroupVersion{Group: "", Version: "v1"}, negotiatedSerializer) + ac.ProcessEventStage(ctx, auditinternal.StageResponseComplete) + + if len(captureSink.events) != 1 { + t.Fatalf("Expected one audit event to be captured, got %d", len(captureSink.events)) + } + event := captureSink.events[0] + + if event.ResponseObject == nil { + t.Fatal("Expected ResponseObject to be set, but it was nil") + } + if event.ResponseStatus == nil { + t.Fatal("Expected ResponseStatus to be set for Status object, but it was nil") + } + if event.ResponseStatus.Status != "Success" { + t.Errorf("Expected ResponseStatus.Status to be 'Success', got %q", event.ResponseStatus.Status) + } + if event.ResponseStatus.Message != "Test status message" { + t.Errorf("Expected ResponseStatus.Message to be 'Test status message', got %q", event.ResponseStatus.Message) + } + if event.ResponseStatus.Reason != "TestReason" { + t.Errorf("Expected ResponseStatus.Reason to be 'TestReason', got %q", event.ResponseStatus.Reason) + } + if event.ResponseStatus.Code != 200 { + t.Errorf("Expected ResponseStatus.Code to be 200, got %d", event.ResponseStatus.Code) + } +} + +func TestLogResponseObjectLevelCheck(t *testing.T) { + testCases := []struct { + name string + level auditinternal.Level + shouldEncode bool + }{ + { + name: "None level should not encode", + level: auditinternal.LevelNone, + shouldEncode: false, + }, + { + name: "Metadata level should not encode", + level: auditinternal.LevelMetadata, + shouldEncode: false, + }, + { + name: "Request level should not encode", + level: auditinternal.LevelRequest, + shouldEncode: false, + }, + { + name: "RequestResponse level should encode", + level: auditinternal.LevelRequestResponse, + shouldEncode: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + // Create a test object + testObj := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-pod", + Namespace: "test-namespace", + }, + } + + // Create audit context with the specified level + ctx := WithAuditContext(context.Background()) + ac := AuditContextFrom(ctx) + ac.Init(RequestAuditConfig{Level: tc.level}, nil) + + // Create a mock serializer that tracks if encoding was attempted + mockSerializer := &mockNegotiatedSerializer{} + + // Call the function under test + LogResponseObject(ctx, testObj, schema.GroupVersion{Group: "", Version: "v1"}, mockSerializer) + + // Check if encoding was attempted as expected + if mockSerializer.encodeCalled != tc.shouldEncode { + t.Errorf("Expected encoding to be called: %v, but got: %v", tc.shouldEncode, mockSerializer.encodeCalled) + } + }) + } +} + +type mockNegotiatedSerializer struct { + encodeCalled bool +} + +func (m *mockNegotiatedSerializer) SupportedMediaTypes() []runtime.SerializerInfo { + return []runtime.SerializerInfo{ + { + MediaType: runtime.ContentTypeJSON, + EncodesAsText: true, + Serializer: nil, + PrettySerializer: nil, + StreamSerializer: nil, + }, + } +} + +func (m *mockNegotiatedSerializer) EncoderForVersion(serializer runtime.Encoder, gv runtime.GroupVersioner) runtime.Encoder { + m.encodeCalled = true + return &mockEncoder{} +} + +func (m *mockNegotiatedSerializer) DecoderToVersion(serializer runtime.Decoder, gv runtime.GroupVersioner) runtime.Decoder { + return nil +} + +type mockEncoder struct{} + +func (e *mockEncoder) Encode(obj runtime.Object, w io.Writer) error { + return nil +} + +func (e *mockEncoder) Identifier() runtime.Identifier { + return runtime.Identifier("mock") +} + +type capturingAuditSink struct { + events []*auditinternal.Event +} + +func (s *capturingAuditSink) ProcessEvents(events ...*auditinternal.Event) bool { + for _, event := range events { + eventCopy := event.DeepCopy() + s.events = append(s.events, eventCopy) + } + return true +} From 153233c677d62c0254d54c1e7013645a081ac03d Mon Sep 17 00:00:00 2001 From: Davanum Srinivas Date: Mon, 12 May 2025 12:50:38 -0400 Subject: [PATCH 2/3] review from tallclair Signed-off-by: Davanum Srinivas --- .../src/k8s.io/apiserver/pkg/audit/request.go | 8 +- .../apiserver/pkg/audit/request_log_test.go | 128 ++++++++++++++---- 2 files changed, 103 insertions(+), 33 deletions(-) diff --git a/staging/src/k8s.io/apiserver/pkg/audit/request.go b/staging/src/k8s.io/apiserver/pkg/audit/request.go index 705d11e88fb..d5f9c730f51 100644 --- a/staging/src/k8s.io/apiserver/pkg/audit/request.go +++ b/staging/src/k8s.io/apiserver/pkg/audit/request.go @@ -171,12 +171,14 @@ func LogRequestPatch(ctx context.Context, patch []byte) { // will be converted to the given gv. func LogResponseObject(ctx context.Context, obj runtime.Object, gv schema.GroupVersion, s runtime.NegotiatedSerializer) { ac := AuditContextFrom(WithAuditContext(ctx)) - if ac.GetEventLevel().Less(auditinternal.LevelRequestResponse) { + status, _ := obj.(*metav1.Status) + if ac.GetEventLevel().Less(auditinternal.LevelMetadata) { + return + } else if ac.GetEventLevel().Less(auditinternal.LevelRequestResponse) { + ac.LogResponseObject(status, nil) return } - status, _ := obj.(*metav1.Status) - if shouldOmitManagedFields(ac) { copy, ok, err := copyWithoutManagedFields(obj) if err != nil { diff --git a/staging/src/k8s.io/apiserver/pkg/audit/request_log_test.go b/staging/src/k8s.io/apiserver/pkg/audit/request_log_test.go index b41450db208..236b9ebdffe 100644 --- a/staging/src/k8s.io/apiserver/pkg/audit/request_log_test.go +++ b/staging/src/k8s.io/apiserver/pkg/audit/request_log_test.go @@ -156,57 +156,125 @@ func TestLogResponseObjectWithStatus(t *testing.T) { func TestLogResponseObjectLevelCheck(t *testing.T) { testCases := []struct { - name string - level auditinternal.Level - shouldEncode bool + name string + level auditinternal.Level + obj runtime.Object + shouldEncode bool + expectResponseObj bool + expectStatusFields bool }{ { - name: "None level should not encode", - level: auditinternal.LevelNone, - shouldEncode: false, + name: "None level should not encode or log anything", + level: auditinternal.LevelNone, + obj: &corev1.Pod{}, + shouldEncode: false, + expectResponseObj: false, + expectStatusFields: false, }, { - name: "Metadata level should not encode", - level: auditinternal.LevelMetadata, - shouldEncode: false, + name: "Metadata level should not encode or log anything", + level: auditinternal.LevelMetadata, + obj: &corev1.Pod{}, + shouldEncode: false, + expectResponseObj: false, + expectStatusFields: false, }, { - name: "Request level should not encode", - level: auditinternal.LevelRequest, - shouldEncode: false, + name: "Request level with Pod should not encode or log", + level: auditinternal.LevelRequest, + obj: &corev1.Pod{}, + shouldEncode: false, + expectResponseObj: false, + expectStatusFields: false, }, { - name: "RequestResponse level should encode", - level: auditinternal.LevelRequestResponse, - shouldEncode: true, + name: "Request level with Status should log status fields without encoding", + level: auditinternal.LevelRequest, + obj: &metav1.Status{ + Status: "Success", + Message: "Test message", + Code: 200, + }, + shouldEncode: false, + expectResponseObj: false, + expectStatusFields: true, + }, + { + name: "RequestResponse level with Pod should encode", + level: auditinternal.LevelRequestResponse, + obj: &corev1.Pod{}, + shouldEncode: true, + expectResponseObj: true, + expectStatusFields: false, + }, + { + name: "RequestResponse level with Status should encode and log status fields", + level: auditinternal.LevelRequestResponse, + obj: &metav1.Status{ + Status: "Success", + Message: "Test message", + Code: 200, + }, + shouldEncode: true, + expectResponseObj: true, + expectStatusFields: true, }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - // Create a test object - testObj := &corev1.Pod{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-pod", - Namespace: "test-namespace", - }, - } - - // Create audit context with the specified level ctx := WithAuditContext(context.Background()) ac := AuditContextFrom(ctx) - ac.Init(RequestAuditConfig{Level: tc.level}, nil) - // Create a mock serializer that tracks if encoding was attempted + captureSink := &capturingAuditSink{} + if err := ac.Init(RequestAuditConfig{Level: tc.level}, captureSink); err != nil { + t.Fatalf("Failed to initialize audit context: %v", err) + } + mockSerializer := &mockNegotiatedSerializer{} + LogResponseObject(ctx, tc.obj, schema.GroupVersion{Group: "", Version: "v1"}, mockSerializer) + ac.ProcessEventStage(ctx, auditinternal.StageResponseComplete) - // Call the function under test - LogResponseObject(ctx, testObj, schema.GroupVersion{Group: "", Version: "v1"}, mockSerializer) - - // Check if encoding was attempted as expected if mockSerializer.encodeCalled != tc.shouldEncode { t.Errorf("Expected encoding to be called: %v, but got: %v", tc.shouldEncode, mockSerializer.encodeCalled) } + + if len(captureSink.events) != 1 { + t.Fatalf("Expected one audit event to be captured, got %d", len(captureSink.events)) + } + event := captureSink.events[0] + + if tc.expectResponseObj { + if event.ResponseObject == nil { + t.Error("Expected ResponseObject to be set, but it was nil") + } + } else { + if event.ResponseObject != nil { + t.Error("Expected ResponseObject to be nil") + } + } + + // Check ResponseStatus for Status objects + status, isStatus := tc.obj.(*metav1.Status) + if isStatus && tc.expectStatusFields { + if event.ResponseStatus == nil { + t.Error("Expected ResponseStatus to be set for Status object, but it was nil") + } else { + if event.ResponseStatus.Status != status.Status { + t.Errorf("Expected ResponseStatus.Status to be %q, got %q", status.Status, event.ResponseStatus.Status) + } + if event.ResponseStatus.Message != status.Message { + t.Errorf("Expected ResponseStatus.Message to be %q, got %q", status.Message, event.ResponseStatus.Message) + } + if event.ResponseStatus.Code != status.Code { + t.Errorf("Expected ResponseStatus.Code to be %d, got %d", status.Code, event.ResponseStatus.Code) + } + } + } else { + if event.ResponseStatus != nil { + t.Error("Expected ResponseStatus to be nil") + } + } }) } } From 721947a5703f2ffaafdc9bef983a6b60c6253ca8 Mon Sep 17 00:00:00 2001 From: Davanum Srinivas Date: Mon, 12 May 2025 13:29:57 -0400 Subject: [PATCH 3/3] Update staging/src/k8s.io/apiserver/pkg/audit/request_log_test.go Co-authored-by: Tim Allclair Signed-off-by: Davanum Srinivas --- .../apiserver/pkg/audit/request_log_test.go | 135 +++++++++++------- 1 file changed, 85 insertions(+), 50 deletions(-) diff --git a/staging/src/k8s.io/apiserver/pkg/audit/request_log_test.go b/staging/src/k8s.io/apiserver/pkg/audit/request_log_test.go index 236b9ebdffe..6ef3316d1df 100644 --- a/staging/src/k8s.io/apiserver/pkg/audit/request_log_test.go +++ b/staging/src/k8s.io/apiserver/pkg/audit/request_log_test.go @@ -97,60 +97,85 @@ func TestLogResponseObjectWithPod(t *testing.T) { } func TestLogResponseObjectWithStatus(t *testing.T) { - // Create a status object to test ResponseStatus handling - testStatus := &metav1.Status{ - TypeMeta: metav1.TypeMeta{ - APIVersion: "v1", - Kind: "Status", + testCases := []struct { + name string + level auditinternal.Level + status *metav1.Status + shouldEncode bool + expectResponseObj bool + expectStatusFields bool + }{ + { + name: "RequestResponse level should encode and log status fields", + level: auditinternal.LevelRequestResponse, + status: &metav1.Status{Status: "Success", Message: "Test message", Code: 200}, + shouldEncode: true, + expectResponseObj: true, + expectStatusFields: true, + }, + { + name: "Metadata level should log status fields without encoding", + level: auditinternal.LevelMetadata, + status: &metav1.Status{Status: "Success", Message: "Test message", Code: 200}, + shouldEncode: false, + expectResponseObj: false, + expectStatusFields: true, }, - Status: "Success", - Message: "Test status message", - Reason: "TestReason", - Code: 200, } - scheme := runtime.NewScheme() - err := metav1.AddMetaToScheme(scheme) - if err != nil { - t.Fatalf("Failed to add meta to scheme: %v", err) - } - scheme.AddKnownTypes(schema.GroupVersion{Version: "v1"}, &metav1.Status{}) - codecs := serializer.NewCodecFactory(scheme) - negotiatedSerializer := codecs.WithoutConversion() + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + scheme := runtime.NewScheme() + if err := metav1.AddMetaToScheme(scheme); err != nil { + t.Fatalf("Failed to add meta to scheme: %v", err) + } + scheme.AddKnownTypes(schema.GroupVersion{Version: "v1"}, &metav1.Status{}) + codecs := serializer.NewCodecFactory(scheme) + negotiatedSerializer := codecs.WithoutConversion() - ctx := WithAuditContext(context.Background()) - ac := AuditContextFrom(ctx) + ctx := WithAuditContext(context.Background()) + ac := AuditContextFrom(ctx) - captureSink := &capturingAuditSink{} - if err := ac.Init(RequestAuditConfig{Level: auditinternal.LevelRequestResponse}, captureSink); err != nil { - t.Fatalf("Failed to initialize audit context: %v", err) - } + captureSink := &capturingAuditSink{} + if err := ac.Init(RequestAuditConfig{Level: tc.level}, captureSink); err != nil { + t.Fatalf("Failed to initialize audit context: %v", err) + } - LogResponseObject(ctx, testStatus, schema.GroupVersion{Group: "", Version: "v1"}, negotiatedSerializer) - ac.ProcessEventStage(ctx, auditinternal.StageResponseComplete) + LogResponseObject(ctx, tc.status, schema.GroupVersion{Group: "", Version: "v1"}, negotiatedSerializer) + ac.ProcessEventStage(ctx, auditinternal.StageResponseComplete) - if len(captureSink.events) != 1 { - t.Fatalf("Expected one audit event to be captured, got %d", len(captureSink.events)) - } - event := captureSink.events[0] + if len(captureSink.events) != 1 { + t.Fatalf("Expected one audit event to be captured, got %d", len(captureSink.events)) + } + event := captureSink.events[0] - if event.ResponseObject == nil { - t.Fatal("Expected ResponseObject to be set, but it was nil") - } - if event.ResponseStatus == nil { - t.Fatal("Expected ResponseStatus to be set for Status object, but it was nil") - } - if event.ResponseStatus.Status != "Success" { - t.Errorf("Expected ResponseStatus.Status to be 'Success', got %q", event.ResponseStatus.Status) - } - if event.ResponseStatus.Message != "Test status message" { - t.Errorf("Expected ResponseStatus.Message to be 'Test status message', got %q", event.ResponseStatus.Message) - } - if event.ResponseStatus.Reason != "TestReason" { - t.Errorf("Expected ResponseStatus.Reason to be 'TestReason', got %q", event.ResponseStatus.Reason) - } - if event.ResponseStatus.Code != 200 { - t.Errorf("Expected ResponseStatus.Code to be 200, got %d", event.ResponseStatus.Code) + if tc.expectResponseObj { + if event.ResponseObject == nil { + t.Error("Expected ResponseObject to be set, but it was nil") + } + } else { + if event.ResponseObject != nil { + t.Error("Expected ResponseObject to be nil") + } + } + + if tc.expectStatusFields { + if event.ResponseStatus == nil { + t.Fatal("Expected ResponseStatus to be set, but it was nil") + } + if event.ResponseStatus.Status != tc.status.Status { + t.Errorf("Expected ResponseStatus.Status to be %q, got %q", tc.status.Status, event.ResponseStatus.Status) + } + if event.ResponseStatus.Message != tc.status.Message { + t.Errorf("Expected ResponseStatus.Message to be %q, got %q", tc.status.Message, event.ResponseStatus.Message) + } + if event.ResponseStatus.Code != tc.status.Code { + t.Errorf("Expected ResponseStatus.Code to be %d, got %d", tc.status.Code, event.ResponseStatus.Code) + } + } else if event.ResponseStatus != nil { + t.Error("Expected ResponseStatus to be nil") + } + }) } } @@ -179,6 +204,18 @@ func TestLogResponseObjectLevelCheck(t *testing.T) { expectResponseObj: false, expectStatusFields: false, }, + { + name: "Metadata level with Status should log status fields without encoding", + level: auditinternal.LevelMetadata, + obj: &metav1.Status{ + Status: "Success", + Message: "Test message", + Code: 200, + }, + shouldEncode: false, + expectResponseObj: false, + expectStatusFields: true, + }, { name: "Request level with Pod should not encode or log", level: auditinternal.LevelRequest, @@ -270,10 +307,8 @@ func TestLogResponseObjectLevelCheck(t *testing.T) { t.Errorf("Expected ResponseStatus.Code to be %d, got %d", status.Code, event.ResponseStatus.Code) } } - } else { - if event.ResponseStatus != nil { - t.Error("Expected ResponseStatus to be nil") - } + } else if event.ResponseStatus != nil { + t.Error("Expected ResponseStatus to be nil") } }) }