-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
⚠️ Fakeclient: Add apply support #2981
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -45,13 +45,17 @@ | |
"k8s.io/apimachinery/pkg/labels" | ||
"k8s.io/apimachinery/pkg/runtime" | ||
"k8s.io/apimachinery/pkg/runtime/schema" | ||
"k8s.io/apimachinery/pkg/runtime/serializer" | ||
"k8s.io/apimachinery/pkg/types" | ||
"k8s.io/apimachinery/pkg/util/managedfields" | ||
utilrand "k8s.io/apimachinery/pkg/util/rand" | ||
"k8s.io/apimachinery/pkg/util/sets" | ||
"k8s.io/apimachinery/pkg/util/strategicpatch" | ||
"k8s.io/apimachinery/pkg/util/validation/field" | ||
"k8s.io/apimachinery/pkg/watch" | ||
clientgoapplyconfigurations "k8s.io/client-go/applyconfigurations" | ||
"k8s.io/client-go/kubernetes/scheme" | ||
Check failure on line 57 in pkg/client/fake/client.go
|
||
clientgoscheme "k8s.io/client-go/kubernetes/scheme" | ||
Check failure on line 58 in pkg/client/fake/client.go
|
||
"k8s.io/client-go/testing" | ||
"k8s.io/utils/ptr" | ||
|
||
|
@@ -119,6 +123,7 @@ | |
withStatusSubresource []client.Object | ||
objectTracker testing.ObjectTracker | ||
interceptorFuncs *interceptor.Funcs | ||
typeConverters []managedfields.TypeConverter | ||
|
||
// indexes maps each GroupVersionKind (GVK) to the indexes registered for that GVK. | ||
// The inner map maps from index name to IndexerFunc. | ||
|
@@ -160,6 +165,8 @@ | |
} | ||
|
||
// WithObjectTracker can be optionally used to initialize this fake client with testing.ObjectTracker. | ||
// Setting this is incompatible with setting WithTypeConverters, as they are a setting on the | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Enforce this in code with an assertion? |
||
// tracker. | ||
func (f *ClientBuilder) WithObjectTracker(ot testing.ObjectTracker) *ClientBuilder { | ||
f.objectTracker = ot | ||
return f | ||
|
@@ -216,6 +223,18 @@ | |
return f | ||
} | ||
|
||
// WithTypeConverters sets the type converters for the fake client. The list is ordered and the first | ||
// non-erroring converter is used. | ||
// This setting is incompatible with WithObjectTracker, as the type converters are a setting on the tracker. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Enforce this in code with an assertion? |
||
// | ||
// If unset, this defaults to: | ||
// * clientgoapplyconfigurations.NewTypeConverter(clientgoscheme.Scheme), | ||
// * managedfields.NewDeducedTypeConverter(), | ||
func (f *ClientBuilder) WithTypeConverters(typeConverters ...managedfields.TypeConverter) *ClientBuilder { | ||
f.typeConverters = append(f.typeConverters, typeConverters...) | ||
return f | ||
} | ||
|
||
// Build builds and returns a new fake client. | ||
func (f *ClientBuilder) Build() client.WithWatch { | ||
if f.scheme == nil { | ||
|
@@ -236,11 +255,29 @@ | |
withStatusSubResource.Insert(gvk) | ||
} | ||
|
||
if f.objectTracker != nil && len(f.typeConverters) > 0 { | ||
panic(errors.New("WithObjectTracker and WithTypeConverters are incompatible")) | ||
} | ||
|
||
if f.objectTracker == nil { | ||
tracker = versionedTracker{ObjectTracker: testing.NewObjectTracker(f.scheme, scheme.Codecs.UniversalDecoder()), scheme: f.scheme, withStatusSubresource: withStatusSubResource} | ||
} else { | ||
tracker = versionedTracker{ObjectTracker: f.objectTracker, scheme: f.scheme, withStatusSubresource: withStatusSubResource} | ||
if len(f.typeConverters) == 0 { | ||
f.typeConverters = []managedfields.TypeConverter{ | ||
// Use corresponding scheme to ensure the converter error | ||
// for types it can't handle. | ||
clientgoapplyconfigurations.NewTypeConverter(clientgoscheme.Scheme), | ||
managedfields.NewDeducedTypeConverter(), | ||
} | ||
} | ||
f.objectTracker = testing.NewFieldManagedObjectTracker( | ||
f.scheme, | ||
serializer.NewCodecFactory(f.scheme).UniversalDecoder(), | ||
multiTypeConverter{upstream: f.typeConverters}, | ||
) | ||
} | ||
tracker = versionedTracker{ | ||
ObjectTracker: f.objectTracker, | ||
scheme: f.scheme, | ||
withStatusSubresource: withStatusSubResource} | ||
|
||
for _, obj := range f.initObject { | ||
if err := tracker.Add(obj); err != nil { | ||
|
@@ -901,6 +938,12 @@ | |
if err != nil { | ||
return err | ||
} | ||
|
||
// otherwise the merge logic in the tracker complains | ||
if patch.Type() == types.ApplyPatchType { | ||
obj.SetManagedFields(nil) | ||
} | ||
|
||
data, err := patch.Data(obj) | ||
if err != nil { | ||
return err | ||
|
@@ -915,7 +958,10 @@ | |
defer c.trackerWriteLock.Unlock() | ||
oldObj, err := c.tracker.Get(gvr, accessor.GetNamespace(), accessor.GetName()) | ||
if err != nil { | ||
return err | ||
if patch.Type() != types.ApplyPatchType { | ||
return err | ||
} | ||
oldObj = &unstructured.Unstructured{} | ||
} | ||
oldAccessor, err := meta.Accessor(oldObj) | ||
if err != nil { | ||
|
@@ -930,7 +976,7 @@ | |
// This ensures that the patch may be rejected if a deletionTimestamp is modified, prior | ||
// to updating the object. | ||
action := testing.NewPatchAction(gvr, accessor.GetNamespace(), accessor.GetName(), patch.Type(), data) | ||
o, err := dryPatch(action, c.tracker) | ||
o, err := dryPatch(action, c.tracker, obj) | ||
if err != nil { | ||
return err | ||
} | ||
|
@@ -989,12 +1035,15 @@ | |
// This results in some code duplication, but was found to be a cleaner alternative than unmarshalling and introspecting the patch data | ||
// and easier than refactoring the k8s client-go method upstream. | ||
// Duplicate of upstream: https://github.com/kubernetes/client-go/blob/783d0d33626e59d55d52bfd7696b775851f92107/testing/fixture.go#L146-L194 | ||
func dryPatch(action testing.PatchActionImpl, tracker testing.ObjectTracker) (runtime.Object, error) { | ||
func dryPatch(action testing.PatchActionImpl, tracker testing.ObjectTracker, newObj runtime.Object) (runtime.Object, error) { | ||
ns := action.GetNamespace() | ||
gvr := action.GetResource() | ||
|
||
obj, err := tracker.Get(gvr, ns, action.GetName()) | ||
if err != nil { | ||
if action.GetPatchType() == types.ApplyPatchType { | ||
return &unstructured.Unstructured{}, nil | ||
} | ||
return nil, err | ||
} | ||
|
||
|
@@ -1039,10 +1088,20 @@ | |
if err = json.Unmarshal(mergedByte, obj); err != nil { | ||
return nil, err | ||
} | ||
case types.ApplyPatchType: | ||
return nil, errors.New("apply patches are not supported in the fake client. Follow https://github.com/kubernetes/kubernetes/issues/115598 for the current status") | ||
case types.ApplyCBORPatchType: | ||
return nil, errors.New("apply CBOR patches are not supported in the fake client") | ||
case types.ApplyPatchType: | ||
// There doesn't seem to be a way to test this without actually applying it as apply is implemented in the tracker. | ||
// We have to make sure no reader sees this and we can not handle errors resetting the obj to the original state. | ||
defer func() { | ||
if err := tracker.Add(obj); err != nil { | ||
panic(err) | ||
} | ||
}() | ||
if err := tracker.Apply(gvr, newObj, ns, action.PatchOptions); err != nil { | ||
return nil, err | ||
} | ||
return tracker.Get(gvr, ns, action.GetName()) | ||
default: | ||
return nil, fmt.Errorf("%s PatchType is not supported", action.GetPatchType()) | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,60 @@ | ||
/* | ||
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 fake | ||
|
||
import ( | ||
"fmt" | ||
|
||
"k8s.io/apimachinery/pkg/runtime" | ||
utilerror "k8s.io/apimachinery/pkg/util/errors" | ||
Check failure on line 23 in pkg/client/fake/typeconverter.go
|
||
"k8s.io/apimachinery/pkg/util/managedfields" | ||
"sigs.k8s.io/structured-merge-diff/v4/typed" | ||
) | ||
|
||
type multiTypeConverter struct { | ||
upstream []managedfields.TypeConverter | ||
} | ||
Comment on lines
+28
to
+30
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Providing this composite type converter seems fine. We have a different use case for a composite type converter in the apiserver for admission: https://github.com/kubernetes/kubernetes/blob/25e11cd1c143ef136418c33bfbbbd4f24e32e529/staging/src/k8s.io/apiserver/pkg/admission/plugin/policy/mutating/patch/typeconverter.go#L37. But the use case is different, it takes a single static type converter and an openapi client, instead of multiple underlying type converters, so it is not useful here. I'd like to ensure we keep this implementation unexported. Maybe add some godoc to that effect? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep makes sense, will do that |
||
|
||
func (m multiTypeConverter) ObjectToTyped(r runtime.Object, o ...typed.ValidationOptions) (*typed.TypedValue, error) { | ||
var errs []error | ||
for _, u := range m.upstream { | ||
res, err := u.ObjectToTyped(r, o...) | ||
if err != nil { | ||
errs = append(errs, err) | ||
continue | ||
} | ||
|
||
return res, nil | ||
} | ||
|
||
return nil, fmt.Errorf("failed to convert Object to Typed: %w", utilerror.NewAggregate(errs)) | ||
} | ||
|
||
func (m multiTypeConverter) TypedToObject(v *typed.TypedValue) (runtime.Object, error) { | ||
var errs []error | ||
for _, u := range m.upstream { | ||
res, err := u.TypedToObject(v) | ||
if err != nil { | ||
errs = append(errs, err) | ||
continue | ||
} | ||
|
||
return res, nil | ||
} | ||
|
||
return nil, fmt.Errorf("failed to convert TypedValue to Object: %w", utilerror.NewAggregate(errs)) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Can this simply be
typeConverter managedfields.TypeConverter
? WhenmultiTypeConverter
is needed, it should implement themanagedfields.TypeConverter
interface and so can be initialized and used here?