From 75dea6b8bc2b7fb889cd41957bc5d061ab614ff4 Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Wed, 7 Jul 2021 10:25:27 -0700 Subject: [PATCH 1/3] Service REST: Use DeepCopy() on Create() and fix tests --- pkg/registry/core/service/storage/rest.go | 3 +++ pkg/registry/core/service/storage/rest_test.go | 6 ++++-- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/pkg/registry/core/service/storage/rest.go b/pkg/registry/core/service/storage/rest.go index 313a41417eb..5167c7bc8bc 100644 --- a/pkg/registry/core/service/storage/rest.go +++ b/pkg/registry/core/service/storage/rest.go @@ -189,6 +189,9 @@ func (rs *REST) Watch(ctx context.Context, options *metainternalversion.ListOpti } func (rs *REST) Create(ctx context.Context, obj runtime.Object, createValidation rest.ValidateObjectFunc, options *metav1.CreateOptions) (runtime.Object, error) { + // DeepCopy to prevent writes here propagating back to tests. + obj = obj.DeepCopyObject() + service := obj.(*api.Service) // bag of clusterIPs allocated in the process of creation diff --git a/pkg/registry/core/service/storage/rest_test.go b/pkg/registry/core/service/storage/rest_test.go index d317444fcbc..c05c2bfa213 100644 --- a/pkg/registry/core/service/storage/rest_test.go +++ b/pkg/registry/core/service/storage/rest_test.go @@ -993,7 +993,7 @@ func TestServiceRegistryDeleteDryRun(t *testing.T) { t.Errorf("expected NodePort to be allocated") } - isValidClusterIPFields(t, storage, svc, svc) + isValidClusterIPFields(t, storage, svc, createdSvc) _, _, err = storage.Delete(ctx, svc.Name, rest.ValidateAllObjectFunc, &metav1.DeleteOptions{DryRun: []string{metav1.DryRunAll}}) if err != nil { @@ -1371,7 +1371,7 @@ func TestServiceRegistryIPReallocation(t *testing.T) { t.Errorf("Unexpected error deleting service: %v", err) } - svc2 := svctest.MakeService("bar", svctest.SetClusterIPs(svc1.Spec.ClusterIP)) + svc2 := svctest.MakeService("bar", svctest.SetClusterIPs(createdSvc1.Spec.ClusterIP)) ctx = genericapirequest.NewDefaultContext() obj, err = storage.Create(ctx, svc2, rest.ValidateAllObjectFunc, &metav1.CreateOptions{}) if err != nil { @@ -3585,6 +3585,8 @@ func TestDefaultingValidation(t *testing.T) { // validates that the service created, updated by REST // has correct ClusterIPs related fields func isValidClusterIPFields(t *testing.T, storage *REST, pre *api.Service, post *api.Service) { + t.Helper() + // valid for gate off/on scenarios // ClusterIP if len(post.Spec.ClusterIP) == 0 { From 42c7e62180446910f41d3a064016c0d7cdebb407 Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Fri, 13 Aug 2021 11:54:21 -0700 Subject: [PATCH 2/3] Fix registry tests to look at result objects --- .../apiserver/pkg/registry/rest/resttest/resttest.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/staging/src/k8s.io/apiserver/pkg/registry/rest/resttest/resttest.go b/staging/src/k8s.io/apiserver/pkg/registry/rest/resttest/resttest.go index b394da620fa..c271c66edf6 100644 --- a/staging/src/k8s.io/apiserver/pkg/registry/rest/resttest/resttest.go +++ b/staging/src/k8s.io/apiserver/pkg/registry/rest/resttest/resttest.go @@ -381,7 +381,8 @@ func (t *Tester) testCreateGeneratesName(valid runtime.Object) { t.Fatalf("Unexpected error: %v", err) } defer t.delete(t.TestContext(), created) - if objectMeta.GetName() == "test-" || !strings.HasPrefix(objectMeta.GetName(), "test-") { + createdMeta := t.getObjectMetaOrFail(created) + if createdMeta.GetName() == "test-" || !strings.HasPrefix(createdMeta.GetName(), "test-") { t.Errorf("unexpected name: %#v", valid) } } @@ -399,7 +400,8 @@ func (t *Tester) testCreateHasMetadata(valid runtime.Object) { t.Fatalf("Unexpected object from result: %#v", obj) } defer t.delete(t.TestContext(), obj) - if !metav1.HasObjectMetaSystemFieldValues(objectMeta) { + createdMeta := t.getObjectMetaOrFail(obj) + if !metav1.HasObjectMetaSystemFieldValues(createdMeta) { t.Errorf("storage did not populate object meta field values") } } @@ -501,7 +503,8 @@ func (t *Tester) testCreateResetsUserData(valid runtime.Object, opts metav1.Crea t.Fatalf("Unexpected object from result: %#v", obj) } defer t.delete(t.TestContext(), obj) - if objectMeta.GetUID() == "bad-uid" || objectMeta.GetCreationTimestamp() == now { + createdMeta := t.getObjectMetaOrFail(obj) + if createdMeta.GetUID() == "bad-uid" || createdMeta.GetCreationTimestamp() == now { t.Errorf("ObjectMeta did not reset basic fields: %#v", objectMeta) } } From 6dfae64d9bebb2c40680bbd6e8270f69839ab013 Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Fri, 13 Aug 2021 11:54:00 -0700 Subject: [PATCH 3/3] REST: Document mutable inputs on Create() If one doesn't DeepCopy() on the way into Create, we can end up writing into the original object. This is by design, and should not be a problem EXCEPT for tests. If a test compares the input to this function with the result, but the input was mutated in-situ, it may hide errors, resulting in tests that pass, but shouldn't. --- .../k8s.io/apiserver/pkg/registry/generic/registry/store.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store.go b/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store.go index ec9870a035a..d94627834da 100644 --- a/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store.go +++ b/staging/src/k8s.io/apiserver/pkg/registry/generic/registry/store.go @@ -360,6 +360,9 @@ func (e *Store) ListPredicate(ctx context.Context, p storage.SelectionPredicate, func finishNothing(context.Context, bool) {} // Create inserts a new item according to the unique key from the object. +// Note that registries may mutate the input object (e.g. in the strategy +// hooks). Tests which call this might want to call DeepCopy if they expect to +// be able to examine the input and output objects for differences. func (e *Store) Create(ctx context.Context, obj runtime.Object, createValidation rest.ValidateObjectFunc, options *metav1.CreateOptions) (runtime.Object, error) { var finishCreate FinishFunc = finishNothing