From bbc59348318c29199e23b27981fb56436ac68705 Mon Sep 17 00:00:00 2001 From: Abu Kashem Date: Mon, 12 Oct 2020 13:18:59 -0400 Subject: [PATCH] drop managed fields from audit entries drop the managed fields of the objects from the audit entries when we are logging request and response bodies. --- .../src/k8s.io/apiserver/pkg/audit/request.go | 100 ++++++++- .../apiserver/pkg/audit/request_test.go | 191 +++++++++++++++++- .../pkg/endpoints/handlers/create.go | 2 +- .../pkg/endpoints/handlers/delete.go | 6 +- .../apiserver/pkg/endpoints/handlers/patch.go | 2 +- .../handlers/responsewriters/writers.go | 4 +- .../pkg/endpoints/handlers/update.go | 2 +- 7 files changed, 292 insertions(+), 15 deletions(-) diff --git a/staging/src/k8s.io/apiserver/pkg/audit/request.go b/staging/src/k8s.io/apiserver/pkg/audit/request.go index 29ad4b72b428..cdfd535f7e86 100644 --- a/staging/src/k8s.io/apiserver/pkg/audit/request.go +++ b/staging/src/k8s.io/apiserver/pkg/audit/request.go @@ -18,6 +18,7 @@ package audit import ( "bytes" + "context" "fmt" "net/http" "reflect" @@ -111,7 +112,8 @@ func LogImpersonatedUser(ae *auditinternal.Event, user user.Info) { // LogRequestObject fills in the request object into an audit event. The passed runtime.Object // will be converted to the given gv. -func LogRequestObject(ae *auditinternal.Event, obj runtime.Object, objGV schema.GroupVersion, gvr schema.GroupVersionResource, subresource string, s runtime.NegotiatedSerializer) { +func LogRequestObject(ctx context.Context, obj runtime.Object, objGV schema.GroupVersion, gvr schema.GroupVersionResource, subresource string, s runtime.NegotiatedSerializer) { + ae := AuditEventFrom(ctx) if ae == nil || ae.Level.Less(auditinternal.LevelMetadata) { return } @@ -151,6 +153,16 @@ func LogRequestObject(ae *auditinternal.Event, obj runtime.Object, objGV schema. return } + if shouldOmitManagedFields(ctx) { + copy, ok, err := copyWithoutManagedFields(obj) + if err != nil { + klog.Warningf("error while dropping managed fields from the request for %q error: %v", reflect.TypeOf(obj).Name(), err) + } + if ok { + obj = copy + } + } + // TODO(audit): hook into the serializer to avoid double conversion var err error ae.RequestObject, err = encodeObject(obj, objGV, s) @@ -162,7 +174,8 @@ func LogRequestObject(ae *auditinternal.Event, obj runtime.Object, objGV schema. } // LogRequestPatch fills in the given patch as the request object into an audit event. -func LogRequestPatch(ae *auditinternal.Event, patch []byte) { +func LogRequestPatch(ctx context.Context, patch []byte) { + ae := AuditEventFrom(ctx) if ae == nil || ae.Level.Less(auditinternal.LevelRequest) { return } @@ -175,7 +188,8 @@ func LogRequestPatch(ae *auditinternal.Event, patch []byte) { // LogResponseObject fills in the response object into an audit event. The passed runtime.Object // will be converted to the given gv. -func LogResponseObject(ae *auditinternal.Event, obj runtime.Object, gv schema.GroupVersion, s runtime.NegotiatedSerializer) { +func LogResponseObject(ctx context.Context, obj runtime.Object, gv schema.GroupVersion, s runtime.NegotiatedSerializer) { + ae := AuditEventFrom(ctx) if ae == nil || ae.Level.Less(auditinternal.LevelMetadata) { return } @@ -191,6 +205,17 @@ func LogResponseObject(ae *auditinternal.Event, obj runtime.Object, gv schema.Gr if ae.Level.Less(auditinternal.LevelRequestResponse) { return } + + if shouldOmitManagedFields(ctx) { + copy, ok, err := copyWithoutManagedFields(obj) + if err != nil { + klog.Warningf("error while dropping managed fields from the response for %q error: %v", reflect.TypeOf(obj).Name(), err) + } + if ok { + obj = copy + } + } + // TODO(audit): hook into the serializer to avoid double conversion var err error ae.ResponseObject, err = encodeObject(obj, gv, s) @@ -242,3 +267,72 @@ func maybeTruncateUserAgent(req *http.Request) string { return ua } + +// copyWithoutManagedFields will make a deep copy of the specified object and +// will discard the managed fields from the copy. +// The specified object is expected to be a meta.Object or a "list". +// The specified object obj is treated as readonly and hence not mutated. +// On return, an error is set if the function runs into any error while +// removing the managed fields, the boolean value is true if the copy has +// been made successfully, otherwise false. +func copyWithoutManagedFields(obj runtime.Object) (runtime.Object, bool, error) { + isAccessor := true + if _, err := meta.Accessor(obj); err != nil { + isAccessor = false + } + isList := meta.IsListType(obj) + _, isTable := obj.(*metav1.Table) + if !isAccessor && !isList && !isTable { + return nil, false, nil + } + + // TODO a deep copy isn't really needed here, figure out how we can reliably + // use shallow copy here to omit the manageFields. + copy := obj.DeepCopyObject() + + if isAccessor { + if err := removeManagedFields(copy); err != nil { + return nil, false, err + } + } + + if isList { + if err := meta.EachListItem(copy, removeManagedFields); err != nil { + return nil, false, err + } + } + + if isTable { + table := copy.(*metav1.Table) + for i := range table.Rows { + rowObj := table.Rows[i].Object + if err := removeManagedFields(rowObj.Object); err != nil { + return nil, false, err + } + } + } + + return copy, true, nil +} + +func removeManagedFields(obj runtime.Object) error { + if obj == nil { + return nil + } + accessor, err := meta.Accessor(obj) + if err != nil { + return err + } + accessor.SetManagedFields(nil) + return nil +} + +func shouldOmitManagedFields(ctx context.Context) bool { + if auditContext := AuditContextFrom(ctx); auditContext != nil { + return auditContext.RequestAuditConfig.OmitManagedFields + } + + // If we can't decide, return false to maintain current behavior which is + // to retain the manage fields in the audit. + return false +} 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 12c36ccd6690..ee1701ba0bd0 100644 --- a/staging/src/k8s.io/apiserver/pkg/audit/request_test.go +++ b/staging/src/k8s.io/apiserver/pkg/audit/request_test.go @@ -18,11 +18,17 @@ package audit import ( "net/http" + "reflect" "testing" + "time" - "github.com/stretchr/testify/assert" - + 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) { @@ -54,3 +60,184 @@ func TestMaybeTruncateUserAgent(t *testing.T) { req.Header.Set("User-Agent", ua) assert.NotEqual(t, ua, maybeTruncateUserAgent(req)) } + +func TestCopyWithoutManagedFields(t *testing.T) { + tests := []struct { + name string + object runtime.Object + expected runtime.Object + ok bool + err error + }{ + { + name: "object specified is not a meta.Accessor or a list or a table", + object: &metav1.Status{}, + }, + { + name: "object specified is a meta.Accessor and has managed fields", + object: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foo", + Namespace: "bar", + ManagedFields: []metav1.ManagedFieldsEntry{ + {Manager: "a", Operation: metav1.ManagedFieldsOperationUpdate, Time: &metav1.Time{Time: time.Now()}}, + {Manager: "b", Operation: metav1.ManagedFieldsOperationUpdate, Time: &metav1.Time{Time: time.Now()}}, + }, + }, + }, + expected: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foo", + Namespace: "bar", + }, + }, + ok: true, + }, + { + name: "object specified is a list and its items have managed fields", + object: &corev1.PodList{ + Items: []corev1.Pod{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "foo", + Namespace: "ns1", + ManagedFields: []metav1.ManagedFieldsEntry{ + {Manager: "a", Operation: metav1.ManagedFieldsOperationUpdate, Time: &metav1.Time{Time: time.Now()}}, + {Manager: "b", Operation: metav1.ManagedFieldsOperationUpdate, Time: &metav1.Time{Time: time.Now()}}, + }, + }, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "bar", + Namespace: "ns2", + ManagedFields: []metav1.ManagedFieldsEntry{ + {Manager: "c", Operation: metav1.ManagedFieldsOperationUpdate, Time: &metav1.Time{Time: time.Now()}}, + {Manager: "d", Operation: metav1.ManagedFieldsOperationUpdate, Time: &metav1.Time{Time: time.Now()}}, + }, + }, + }, + }, + }, + expected: &corev1.PodList{ + Items: []corev1.Pod{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "foo", + Namespace: "ns1", + }, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "bar", + Namespace: "ns2", + }, + }, + }, + }, + ok: true, + }, + { + name: "object specified is a Table and objects in its rows have managed fields", + object: &metav1.Table{ + Rows: []metav1.TableRow{ + { + Object: runtime.RawExtension{ + Object: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foo", + Namespace: "ns1", + ManagedFields: []metav1.ManagedFieldsEntry{ + {Manager: "a", Operation: metav1.ManagedFieldsOperationUpdate, Time: &metav1.Time{Time: time.Now()}}, + {Manager: "b", Operation: metav1.ManagedFieldsOperationUpdate, Time: &metav1.Time{Time: time.Now()}}, + }, + }, + }, + }, + }, + { + Object: runtime.RawExtension{ + Object: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "bar", + Namespace: "ns2", + ManagedFields: []metav1.ManagedFieldsEntry{ + {Manager: "c", Operation: metav1.ManagedFieldsOperationUpdate, Time: &metav1.Time{Time: time.Now()}}, + {Manager: "d", Operation: metav1.ManagedFieldsOperationUpdate, Time: &metav1.Time{Time: time.Now()}}, + }, + }, + }, + }, + }, + // add an empty row to make sure we don't panic + { + Object: runtime.RawExtension{}, + }, + }, + }, + expected: &metav1.Table{ + Rows: []metav1.TableRow{ + { + Object: runtime.RawExtension{ + Object: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foo", + Namespace: "ns1", + }, + }, + }, + }, + { + Object: runtime.RawExtension{ + Object: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "bar", + Namespace: "ns2", + }, + }, + }, + }, + { + Object: runtime.RawExtension{}, + }, + }, + }, + ok: true, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + original := test.object.DeepCopyObject() + objectGot, ok, err := copyWithoutManagedFields(test.object) + + if test.err != err { + t.Errorf("expected error: %v, but got: %v", test.err, err) + } + if test.ok != ok { + t.Errorf("expected ok: %t, but got: %t", test.ok, ok) + } + + switch { + case test.expected == nil: + if objectGot != nil { + t.Errorf("expected the returned object to be nil, but got %#v", objectGot) + } + default: + // verify that a deep copy of the specified object is made before mutating it. + if expected, actual := reflect.ValueOf(test.object), reflect.ValueOf(objectGot); expected.Pointer() == actual.Pointer() { + t.Error("expected the returned object to be a deep copy of the input object") + } + + if !cmp.Equal(test.expected, objectGot) { + t.Errorf("expected and actual do not match, diff: %s", cmp.Diff(test.expected, objectGot)) + } + } + + // we always expect the original object to be unchanged. + if !cmp.Equal(original, test.object) { + t.Errorf("the original object has mutated, diff: %s", cmp.Diff(original, test.object)) + } + }) + } +} 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 51ad05bf16ba..f2e9e50135b3 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/create.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/create.go @@ -143,7 +143,7 @@ func createHandler(r rest.NamedCreater, scope *RequestScope, admit admission.Int ae := audit.AuditEventFrom(ctx) admit = admission.WithAudit(admit, ae) - audit.LogRequestObject(ae, obj, objGV, scope.Resource, scope.Subresource, scope.Serializer) + 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 5b9c36ecab23..e58495bed275 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/delete.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/delete.go @@ -103,9 +103,8 @@ func DeleteResource(r rest.GracefulDeleter, allowsOptions bool, scope *RequestSc } trace.Step("Decoded delete options") - ae := audit.AuditEventFrom(ctx) objGV := gvk.GroupVersion() - audit.LogRequestObject(ae, obj, objGV, scope.Resource, scope.Subresource, scope.Serializer) + audit.LogRequestObject(req.Context(), obj, objGV, scope.Resource, scope.Subresource, scope.Serializer) trace.Step("Recorded the audit event") } else { if err := metainternalversionscheme.ParameterCodec.DecodeParameters(req.URL.Query(), scope.MetaGroupVersion, options); err != nil { @@ -250,9 +249,8 @@ func DeleteCollection(r rest.CollectionDeleter, checkBody bool, scope *RequestSc return } - ae := audit.AuditEventFrom(ctx) objGV := gvk.GroupVersion() - audit.LogRequestObject(ae, obj, objGV, scope.Resource, scope.Subresource, scope.Serializer) + audit.LogRequestObject(req.Context(), obj, objGV, scope.Resource, scope.Subresource, scope.Serializer) } else { if err := metainternalversionscheme.ParameterCodec.DecodeParameters(req.URL.Query(), scope.MetaGroupVersion, options); err != nil { err = errors.NewBadRequest(err.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 1138bc0aee3b..89c834a4ca57 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/patch.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/patch.go @@ -126,7 +126,7 @@ func PatchResource(r rest.Patcher, scope *RequestScope, admit admission.Interfac ae := audit.AuditEventFrom(ctx) admit = admission.WithAudit(admit, ae) - audit.LogRequestPatch(ae, patchBytes) + audit.LogRequestPatch(req.Context(), patchBytes) trace.Step("Recorded the audit event") baseContentType := runtime.ContentTypeJSON diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/responsewriters/writers.go b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/responsewriters/writers.go index 676b32fc8dd8..d6d1250b0e8d 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/responsewriters/writers.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/responsewriters/writers.go @@ -267,9 +267,7 @@ func WriteObjectNegotiated(s runtime.NegotiatedSerializer, restrictions negotiat return } - if ae := audit.AuditEventFrom(req.Context()); ae != nil { - audit.LogResponseObject(ae, object, gv, s) - } + audit.LogResponseObject(req.Context(), object, gv, s) encoder := s.EncoderForVersion(serializer.Serializer, gv) SerializeObject(serializer.MediaType, encoder, w, req, statusCode, object) 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 bfd8cb4b5147..f771fa80e00d 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/update.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/handlers/update.go @@ -119,7 +119,7 @@ func UpdateResource(r rest.Updater, scope *RequestScope, admit admission.Interfa trace.Step("Conversion done") ae := audit.AuditEventFrom(ctx) - audit.LogRequestObject(ae, obj, objGV, scope.Resource, scope.Subresource, scope.Serializer) + audit.LogRequestObject(req.Context(), obj, objGV, scope.Resource, scope.Subresource, scope.Serializer) admit = admission.WithAudit(admit, ae) if err := checkName(obj, name, namespace, scope.Namer); err != nil {