From b52e57c589ac02b2439a530dc9042336e84b7769 Mon Sep 17 00:00:00 2001 From: Antoine Pelisse Date: Tue, 16 May 2023 11:16:54 -0700 Subject: [PATCH] managedfields: Improve/strengthen version checking for Apply --- .../util/managedfields/fieldmanager_test.go | 7 +-- .../managedfields/internal/fieldmanager.go | 25 +++++---- .../internal/testing/testfieldmanager.go | 22 ++++---- .../managedfields/internal/versioncheck.go | 52 +++++++++++++++++++ 4 files changed, 82 insertions(+), 24 deletions(-) create mode 100644 staging/src/k8s.io/apimachinery/pkg/util/managedfields/internal/versioncheck.go diff --git a/staging/src/k8s.io/apimachinery/pkg/util/managedfields/fieldmanager_test.go b/staging/src/k8s.io/apimachinery/pkg/util/managedfields/fieldmanager_test.go index 26a12ca010a..7110f2656c7 100644 --- a/staging/src/k8s.io/apimachinery/pkg/util/managedfields/fieldmanager_test.go +++ b/staging/src/k8s.io/apimachinery/pkg/util/managedfields/fieldmanager_test.go @@ -183,14 +183,14 @@ func TestVersionCheck(t *testing.T) { t.Fatalf("error decoding YAML: %v", err) } - // patch has 'apiVersion: apps/v2' but live version is apps/v1 -> error + // patch has 'apiVersion: apps/v1beta1' but live version is apps/v1 -> error err = f.Apply(appliedObj, "fieldmanager_test", false) if err == nil { t.Fatalf("expected an error from mismatched patch and live versions") } switch typ := err.(type) { default: - t.Fatalf("expected error to be of type %T was %T", apierrors.StatusError{}, typ) + t.Fatalf("expected error to be of type %T was %T (%v)", apierrors.StatusError{}, typ, err) case apierrors.APIStatus: if typ.Status().Code != http.StatusBadRequest { t.Fatalf("expected status code to be %d but was %d", @@ -198,6 +198,7 @@ func TestVersionCheck(t *testing.T) { } } } + func TestVersionCheckDoesNotPanic(t *testing.T) { f := managedfieldstest.NewTestFieldManager(fakeTypeConverter, schema.FromAPIVersionAndKind("apps/v1", "Deployment")) @@ -228,7 +229,7 @@ func TestVersionCheckDoesNotPanic(t *testing.T) { } switch typ := err.(type) { default: - t.Fatalf("expected error to be of type %T was %T", apierrors.StatusError{}, typ) + t.Fatalf("expected error to be of type %T was %T (%v)", apierrors.StatusError{}, typ, err) case apierrors.APIStatus: if typ.Status().Code != http.StatusBadRequest { t.Fatalf("expected status code to be %d but was %d", diff --git a/staging/src/k8s.io/apimachinery/pkg/util/managedfields/internal/fieldmanager.go b/staging/src/k8s.io/apimachinery/pkg/util/managedfields/internal/fieldmanager.go index f3111d4bc72..5e21cfa00b1 100644 --- a/staging/src/k8s.io/apimachinery/pkg/util/managedfields/internal/fieldmanager.go +++ b/staging/src/k8s.io/apimachinery/pkg/util/managedfields/internal/fieldmanager.go @@ -56,17 +56,20 @@ func NewFieldManager(f Manager, subresource string) *FieldManager { // newDefaultFieldManager is a helper function which wraps a Manager with certain default logic. func NewDefaultFieldManager(f Manager, typeConverter TypeConverter, objectConverter runtime.ObjectConvertor, objectCreater runtime.ObjectCreater, kind schema.GroupVersionKind, subresource string) *FieldManager { return NewFieldManager( - NewLastAppliedUpdater( - NewLastAppliedManager( - NewProbabilisticSkipNonAppliedManager( - NewCapManagersManager( - NewBuildManagerInfoManager( - NewManagedFieldsUpdater( - NewStripMetaManager(f), - ), kind.GroupVersion(), subresource, - ), DefaultMaxUpdateManagers, - ), objectCreater, kind, DefaultTrackOnCreateProbability, - ), typeConverter, objectConverter, kind.GroupVersion()), + NewVersionCheckManager( + NewLastAppliedUpdater( + NewLastAppliedManager( + NewProbabilisticSkipNonAppliedManager( + NewCapManagersManager( + NewBuildManagerInfoManager( + NewManagedFieldsUpdater( + NewStripMetaManager(f), + ), kind.GroupVersion(), subresource, + ), DefaultMaxUpdateManagers, + ), objectCreater, kind, DefaultTrackOnCreateProbability, + ), typeConverter, objectConverter, kind.GroupVersion(), + ), + ), kind, ), subresource, ) } diff --git a/staging/src/k8s.io/apimachinery/pkg/util/managedfields/internal/testing/testfieldmanager.go b/staging/src/k8s.io/apimachinery/pkg/util/managedfields/internal/testing/testfieldmanager.go index 805359e3147..83747242c0a 100644 --- a/staging/src/k8s.io/apimachinery/pkg/util/managedfields/internal/testing/testfieldmanager.go +++ b/staging/src/k8s.io/apimachinery/pkg/util/managedfields/internal/testing/testfieldmanager.go @@ -137,16 +137,18 @@ func NewTestFieldManagerImpl(typeConverter managedfields.TypeConverter, gvk sche // 1. We don't want to create a `internal.FieldManager` // 2. We don't want to use the CapManager that is tested separately with // a smaller than the default cap. - f = internal.NewLastAppliedUpdater( - internal.NewLastAppliedManager( - internal.NewProbabilisticSkipNonAppliedManager( - internal.NewBuildManagerInfoManager( - internal.NewManagedFieldsUpdater( - internal.NewStripMetaManager(f), - ), gvk.GroupVersion(), subresource, - ), &FakeObjectCreater{}, gvk, internal.DefaultTrackOnCreateProbability, - ), typeConverter, &FakeObjectConvertor{}, gvk.GroupVersion(), - ), + f = internal.NewVersionCheckManager( + internal.NewLastAppliedUpdater( + internal.NewLastAppliedManager( + internal.NewProbabilisticSkipNonAppliedManager( + internal.NewBuildManagerInfoManager( + internal.NewManagedFieldsUpdater( + internal.NewStripMetaManager(f), + ), gvk.GroupVersion(), subresource, + ), &FakeObjectCreater{}, gvk, internal.DefaultTrackOnCreateProbability, + ), typeConverter, &FakeObjectConvertor{}, gvk.GroupVersion(), + ), + ), gvk, ) if chainFieldManager != nil { f = chainFieldManager(f) diff --git a/staging/src/k8s.io/apimachinery/pkg/util/managedfields/internal/versioncheck.go b/staging/src/k8s.io/apimachinery/pkg/util/managedfields/internal/versioncheck.go new file mode 100644 index 00000000000..ee1e2bca701 --- /dev/null +++ b/staging/src/k8s.io/apimachinery/pkg/util/managedfields/internal/versioncheck.go @@ -0,0 +1,52 @@ +/* +Copyright 2023 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 internal + +import ( + "fmt" + + "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" +) + +type versionCheckManager struct { + fieldManager Manager + gvk schema.GroupVersionKind +} + +var _ Manager = &versionCheckManager{} + +// NewVersionCheckManager creates a manager that makes sure that the +// applied object is in the proper version. +func NewVersionCheckManager(fieldManager Manager, gvk schema.GroupVersionKind) Manager { + return &versionCheckManager{fieldManager: fieldManager, gvk: gvk} +} + +// Update implements Manager. +func (f *versionCheckManager) Update(liveObj, newObj runtime.Object, managed Managed, manager string) (runtime.Object, Managed, error) { + // Nothing to do for updates, this is checked in many other places. + return f.fieldManager.Update(liveObj, newObj, managed, manager) +} + +// Apply implements Manager. +func (f *versionCheckManager) Apply(liveObj, appliedObj runtime.Object, managed Managed, fieldManager string, force bool) (runtime.Object, Managed, error) { + if gvk := appliedObj.GetObjectKind().GroupVersionKind(); gvk != f.gvk { + return nil, nil, errors.NewBadRequest(fmt.Sprintf("invalid object type: %v", gvk)) + } + return f.fieldManager.Apply(liveObj, appliedObj, managed, fieldManager, force) +}