Skip to content

Commit

Permalink
⚠️ Fakeclient: Drop status changes in Update/Patch if configured
Browse files Browse the repository at this point in the history 
This changes makes the Update and Patch method drop the Status potion of
the object if it is an in-tree object that has a status subresouce or
was configured as having one. This replicas the behavior observed when
using an actual Kubernetes API.

This change might break tests due to the fact that it configures the
in-tree resources that have a status subresoruce as such. If that
happens however that likely means the test was passing incorrectly.
  • Loading branch information
@alvaroaleman
alvaroaleman committed Apr 1, 2023
1 parent 5db1738 commit ea6cd8c
Show file tree
Hide file tree
Showing 2 changed files with 201 additions and 23 deletions.
155 changes: 132 additions & 23 deletions pkg/client/fake/client.go
View file
Expand Up @@ -35,6 +35,7 @@ import (
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/runtime/schema"
utilrand "k8s.io/apimachinery/pkg/util/rand"
"k8s.io/apimachinery/pkg/util/sets"
"k8s.io/apimachinery/pkg/util/validation/field"
"k8s.io/apimachinery/pkg/watch"
"k8s.io/client-go/kubernetes/scheme"
Expand All @@ -48,13 +49,15 @@ import (

type versionedTracker struct {
testing.ObjectTracker
scheme *runtime.Scheme
scheme *runtime.Scheme
withStatusSubresource sets.Set[schema.GroupVersionKind]
}

type fakeClient struct {
tracker versionedTracker
scheme *runtime.Scheme
restMapper meta.RESTMapper
tracker versionedTracker
scheme *runtime.Scheme
restMapper meta.RESTMapper
withStatusSubresource sets.Set[schema.GroupVersionKind]

// indexes maps each GroupVersionKind (GVK) to the indexes registered for that GVK.
// The inner map maps from index name to IndexerFunc.
Expand Down Expand Up @@ -95,12 +98,13 @@ func NewClientBuilder() *ClientBuilder {

// ClientBuilder builds a fake client.
type ClientBuilder struct {
scheme *runtime.Scheme
restMapper meta.RESTMapper
initObject []client.Object
initLists []client.ObjectList
initRuntimeObjects []runtime.Object
objectTracker testing.ObjectTracker
scheme *runtime.Scheme
restMapper meta.RESTMapper
initObject []client.Object
initLists []client.ObjectList
initRuntimeObjects []runtime.Object
withStatusSubresource []client.Object
objectTracker testing.ObjectTracker

// indexes maps each GroupVersionKind (GVK) to the indexes registered for that GVK.
// The inner map maps from index name to IndexerFunc.
Expand Down Expand Up @@ -185,6 +189,13 @@ func (f *ClientBuilder) WithIndex(obj runtime.Object, field string, extractValue
return f
}

// WithStatusSubresource configures the passed object with a status subresource, which means
// calls to Update and Patch will not alters its status.
func (f *ClientBuilder) WithStatusSubresource(o ...client.Object) *ClientBuilder {
f.withStatusSubresource = append(f.withStatusSubresource, o...)
return f
}

// Build builds and returns a new fake client.
func (f *ClientBuilder) Build() client.WithWatch {
if f.scheme == nil {
Expand All @@ -196,10 +207,19 @@ func (f *ClientBuilder) Build() client.WithWatch {

var tracker versionedTracker

withStatusSubResource := sets.New(inTreeResourcesWithStatus()...)
for _, o := range f.withStatusSubresource {
gvk, err := apiutil.GVKForObject(o, f.scheme)
if err != nil {
panic(fmt.Errorf("failed to get gvk for object %T: %w", withStatusSubResource, err))
}
withStatusSubResource.Insert(gvk)
}

if f.objectTracker == nil {
tracker = versionedTracker{ObjectTracker: testing.NewObjectTracker(f.scheme, scheme.Codecs.UniversalDecoder()), scheme: f.scheme}
tracker = versionedTracker{ObjectTracker: testing.NewObjectTracker(f.scheme, scheme.Codecs.UniversalDecoder()), scheme: f.scheme, withStatusSubresource: withStatusSubResource}
} else {
tracker = versionedTracker{ObjectTracker: f.objectTracker, scheme: f.scheme}
tracker = versionedTracker{ObjectTracker: f.objectTracker, scheme: f.scheme, withStatusSubresource: withStatusSubResource}
}

for _, obj := range f.initObject {
Expand All @@ -217,11 +237,13 @@ func (f *ClientBuilder) Build() client.WithWatch {
panic(fmt.Errorf("failed to add runtime object %v to fake client: %w", obj, err))
}
}

return &fakeClient{
tracker: tracker,
scheme: f.scheme,
restMapper: f.restMapper,
indexes: f.indexes,
tracker: tracker,
scheme: f.scheme,
restMapper: f.restMapper,
indexes: f.indexes,
withStatusSubresource: withStatusSubResource,
}
}

Expand Down Expand Up @@ -318,6 +340,10 @@ func convertFromUnstructuredIfNecessary(s *runtime.Scheme, o runtime.Object) (ru
}

func (t versionedTracker) Update(gvr schema.GroupVersionResource, obj runtime.Object, ns string) error {
return t.update(gvr, obj, ns, false)
}

func (t versionedTracker) update(gvr schema.GroupVersionResource, obj runtime.Object, ns string, isStatus bool) error {
accessor, err := meta.Accessor(obj)
if err != nil {
return fmt.Errorf("failed to get accessor for object: %w", err)
Expand All @@ -337,6 +363,9 @@ func (t versionedTracker) Update(gvr schema.GroupVersionResource, obj runtime.Ob
return err
}
}
if !isStatus && t.withStatusSubresource.Has(gvk) {
clearObjectStatus(obj)
}

oldObject, err := t.ObjectTracker.Get(gvr, ns, accessor.GetName())
if err != nil {
Expand Down Expand Up @@ -689,6 +718,10 @@ func (c *fakeClient) DeleteAllOf(ctx context.Context, obj client.Object, opts ..
}

func (c *fakeClient) Update(ctx context.Context, obj client.Object, opts ...client.UpdateOption) error {
return c.update(ctx, obj, false, opts...)
}

func (c *fakeClient) update(ctx context.Context, obj client.Object, isStatus bool, opts ...client.UpdateOption) error {
updateOptions := &client.UpdateOptions{}
updateOptions.ApplyOptions(opts)

Check failure on line 726 in pkg/client/fake/client.go

View workflow job for this annotation

GitHub Actions / lint 

`(*fakeClient).update` - `ctx` is unused (unparam)

Expand All @@ -706,10 +739,14 @@ func (c *fakeClient) Update(ctx context.Context, obj client.Object, opts ...clie
if err != nil {
return err
}
return c.tracker.Update(gvr, obj, accessor.GetNamespace())
return c.tracker.update(gvr, obj, accessor.GetNamespace(), isStatus)
}

func (c *fakeClient) Patch(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.PatchOption) error {
return c.patch(ctx, obj, patch, false, opts...)
}

func (c *fakeClient) patch(ctx context.Context, obj client.Object, patch client.Patch, isStatus bool, opts ...client.PatchOption) error {
patchOptions := &client.PatchOptions{}
patchOptions.ApplyOptions(opts)

Check failure on line 751 in pkg/client/fake/client.go

View workflow job for this annotation

GitHub Actions / lint 

`(*fakeClient).patch` - `ctx` is unused (unparam)

Expand All @@ -732,6 +769,18 @@ func (c *fakeClient) Patch(ctx context.Context, obj client.Object, patch client.
return err
}

gvk, err := apiutil.GVKForObject(obj, c.scheme)
if err != nil {
return err
}

if !isStatus && c.withStatusSubresource.Has(gvk) {
data, err = clearStatus(data)
if err != nil {
return fmt.Errorf("failed to clear status: %w", err)
}
}

reaction := testing.ObjectReaction(c.tracker)
handled, o, err := reaction(testing.NewPatchAction(gvr, accessor.GetNamespace(), accessor.GetName(), patch.Type(), data))
if err != nil {
Expand All @@ -741,10 +790,6 @@ func (c *fakeClient) Patch(ctx context.Context, obj client.Object, patch client.
panic("tracker could not handle patch method")
}

gvk, err := apiutil.GVKForObject(obj, c.scheme)
if err != nil {
return err
}
ta, err := meta.TypeAccessor(o)
if err != nil {
return err
Expand All @@ -762,6 +807,34 @@ func (c *fakeClient) Patch(ctx context.Context, obj client.Object, patch client.
return err
}

func clearObjectStatus(o runtime.Object) {
if unstructured, isUnstructured := o.(*unstructured.Unstructured); isUnstructured {
delete(unstructured.Object, "status")
return
}
tp := reflect.TypeOf(o)
value := reflect.ValueOf(o)
if tp.Kind() == reflect.Pointer {
tp = tp.Elem()
value = value.Elem()
}
statusType, found := tp.FieldByName("Status")
if !found {
return
}
value.FieldByName("Status").Set(reflect.New(statusType.Type).Elem())
}

func clearStatus(in []byte) ([]byte, error) {
m := map[string]any{}
if err := json.Unmarshal(in, &m); err != nil {
return nil, err
}
delete(m, "status")

return json.Marshal(m)
}

func (c *fakeClient) Status() client.SubResourceWriter {
return c.SubResource("status")
}
Expand Down Expand Up @@ -818,7 +891,7 @@ func (sw *fakeSubResourceClient) Update(ctx context.Context, obj client.Object,
if updateOptions.SubResourceBody != nil {
body = updateOptions.SubResourceBody
}
return sw.client.Update(ctx, body, &updateOptions.UpdateOptions)
return sw.client.update(ctx, body, true, &updateOptions.UpdateOptions)
}

func (sw *fakeSubResourceClient) Patch(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.SubResourcePatchOption) error {
Expand All @@ -833,7 +906,7 @@ func (sw *fakeSubResourceClient) Patch(ctx context.Context, obj client.Object, p
body = patchOptions.SubResourceBody
}

return sw.client.Patch(ctx, body, patch, &patchOptions.PatchOptions)
return sw.client.patch(ctx, body, patch, true, &patchOptions.PatchOptions)
}

func allowsUnconditionalUpdate(gvk schema.GroupVersionKind) bool {
Expand Down Expand Up @@ -933,6 +1006,42 @@ func allowsCreateOnUpdate(gvk schema.GroupVersionKind) bool {
return false
}

func inTreeResourcesWithStatus() []schema.GroupVersionKind {
return []schema.GroupVersionKind{
{Version: "v1", Kind: "Namespace"},
{Version: "v1", Kind: "Node"},
{Version: "v1", Kind: "PersistentVolumeClaim"},
{Version: "v1", Kind: "PersistentVolume"},
{Version: "v1", Kind: "Pod"},
{Version: "v1", Kind: "ReplicationController"},
{Version: "v1", Kind: "Service"},

{Group: "apps", Version: "v1", Kind: "Deployment"},
{Group: "apps", Version: "v1", Kind: "DaemonSet"},
{Group: "apps", Version: "v1", Kind: "ReplicaSet"},
{Group: "apps", Version: "v1", Kind: "StatefulSet"},

{Group: "autoscaling", Version: "v1", Kind: "HorizontalPodAutoscaler"},

{Group: "batch", Version: "v1", Kind: "CronJob"},
{Group: "batch", Version: "v1", Kind: "Job"},

{Group: "certificates.k8s.io", Version: "v1", Kind: "CertificateSigningRequest"},

{Group: "networking.k8s.io", Version: "v1", Kind: "Ingress"},
{Group: "networking.k8s.io", Version: "v1", Kind: "NetworkPolicy"},

{Group: "policy", Version: "v1", Kind: "PodDisruptionBudget"},

{Group: "storage.k8s.io", Version: "v1", Kind: "VolumeAttachment"},

{Group: "apiextensions.k8s.io", Version: "v1", Kind: "CustomResourceDefinition"},

{Group: "flowcontrol.apiserver.k8s.io", Version: "v1beta2", Kind: "FlowSchema"},
{Group: "flowcontrol.apiserver.k8s.io", Version: "v1beta2", Kind: "PriorityLevelConfiguration"},
}
}

// zero zeros the value of a pointer.
func zero(x interface{}) {
if x == nil {
Expand Down
69 changes: 69 additions & 0 deletions pkg/client/fake/client_test.go
View file
Expand Up @@ -972,6 +972,38 @@ var _ = Describe("Fake client", func() {
Expect(err).To(BeNil())
Expect(len(newObj.Finalizers)).To(Equal(0))
})

It("should not change the status of objects that have a status subresource on update", func() {
obj := &corev1.Node{
ObjectMeta: metav1.ObjectMeta{
Name: "node",
},
}
Expect(cl.Create(context.Background(), obj)).To(BeNil())

obj.Status.Phase = corev1.NodeRunning
Expect(cl.Update(context.Background(), obj)).To(BeNil())

Expect(cl.Get(context.Background(), client.ObjectKeyFromObject(obj), obj)).To(BeNil())

Expect(obj.Status).To(BeEquivalentTo(corev1.NodeStatus{}))
})
It("should not change the status of objects that have a status subresource on patch", func() {
obj := &corev1.Node{
ObjectMeta: metav1.ObjectMeta{
Name: "node",
},
}
Expect(cl.Create(context.Background(), obj)).To(BeNil())
original := obj.DeepCopy()

obj.Status.Phase = corev1.NodeRunning
Expect(cl.Patch(context.Background(), obj, client.MergeFrom(original))).To(BeNil())

Expect(cl.Get(context.Background(), client.ObjectKeyFromObject(obj), obj)).To(BeNil())

Expect(obj.Status).To(BeEquivalentTo(corev1.NodeStatus{}))
})
}

Context("with default scheme.Scheme", func() {
Expand Down Expand Up @@ -1220,6 +1252,43 @@ var _ = Describe("Fake client", func() {
Expect(err).To(BeNil())
Expect(obj).To(Equal(dep3))
})

It("should not change the status of objects that are configured to have a status subresource on update", func() {
obj := &unstructured.Unstructured{}
obj.SetAPIVersion("foo/v1")
obj.SetKind("Foo")
obj.SetName("a-foo")
cl := NewClientBuilder().WithStatusSubresource(obj).Build()

Expect(cl.Create(context.Background(), obj)).To(BeNil())
err := unstructured.SetNestedField(obj.Object, map[string]interface{}{"count": int64(2)}, "status")
Expect(err).To(BeNil())

Expect(cl.Update(context.Background(), obj)).To(BeNil())

Expect(cl.Get(context.Background(), client.ObjectKeyFromObject(obj), obj)).To(BeNil())

Expect(obj.Object["status"]).To(BeNil())
})

It("should not change the status of objects that are configured to have a status subresource on patch", func() {
obj := &unstructured.Unstructured{}
obj.SetAPIVersion("foo/v1")
obj.SetKind("Foo")
obj.SetName("a-foo")
cl := NewClientBuilder().WithStatusSubresource(obj).Build()

Expect(cl.Create(context.Background(), obj)).To(BeNil())
original := obj.DeepCopy()

err := unstructured.SetNestedField(obj.Object, map[string]interface{}{"count": int64(2)}, "status")
Expect(err).To(BeNil())
Expect(cl.Patch(context.Background(), obj, client.MergeFrom(original))).To(BeNil())

Expect(cl.Get(context.Background(), client.ObjectKeyFromObject(obj), obj)).To(BeNil())

Expect(obj.Object["status"]).To(BeNil())
})
})

var _ = Describe("Fake client builder", func() {
Expand Down

0 comments on commit ea6cd8c

Please sign in to comment.