Skip to content
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.

Sign up for GitHub

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

Jump to bottom

✨ Support subresource modification #1922

Closed
wants to merge 2 commits into from
Closed

✨ Support subresource modification #1922

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
03edfd3
Support subresource modification
g7r Jun 2, 2022
490c300
Use SubResource method to implement Status method
g7r Jun 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 21 additions & 16 deletions pkg/client/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,40 +289,45 @@ func (c *client) List(ctx context.Context, obj ObjectList, opts ...ListOption) e
}

// Status implements client.StatusClient.
func (c *client) Status() StatusWriter {
return &statusWriter{client: c}
func (c *client) Status() SubResourceWriter {
return c.SubResource("status")
}

// statusWriter is client.StatusWriter that writes status subresource.
type statusWriter struct {
client *client
func (c *client) SubResource(subResource string) SubResourceWriter {
return &subResourceWriter{client: c, subResource: subResource}
}

// ensure statusWriter implements client.StatusWriter.
var _ StatusWriter = &statusWriter{}
// subResourceWriter is client.SubResourceWriter that writes to subresources.
type subResourceWriter struct {
client *client
subResource string
}

// ensure subResourceWriter implements client.SubResourceWriter.
var _ SubResourceWriter = &subResourceWriter{}

// Update implements client.StatusWriter.
func (sw *statusWriter) Update(ctx context.Context, obj Object, opts ...UpdateOption) error {
// Update implements client.SubResourceWriter.
func (sw *subResourceWriter) Update(ctx context.Context, obj Object, opts ...UpdateOption) error {
defer sw.client.resetGroupVersionKind(obj, obj.GetObjectKind().GroupVersionKind())
switch obj.(type) {
case *unstructured.Unstructured:
return sw.client.unstructuredClient.UpdateStatus(ctx, obj, opts...)
return sw.client.unstructuredClient.UpdateSubResource(ctx, obj, sw.subResource, opts...)
case *metav1.PartialObjectMetadata:
return fmt.Errorf("cannot update status using only metadata -- did you mean to patch?")
default:
return sw.client.typedClient.UpdateStatus(ctx, obj, opts...)
return sw.client.typedClient.UpdateSubResource(ctx, obj, sw.subResource, opts...)
}
}

// Patch implements client.Client.
func (sw *statusWriter) Patch(ctx context.Context, obj Object, patch Patch, opts ...PatchOption) error {
// Patch implements client.SubResourceWriter.
func (sw *subResourceWriter) Patch(ctx context.Context, obj Object, patch Patch, opts ...PatchOption) error {
defer sw.client.resetGroupVersionKind(obj, obj.GetObjectKind().GroupVersionKind())
switch obj.(type) {
case *unstructured.Unstructured:
return sw.client.unstructuredClient.PatchStatus(ctx, obj, patch, opts...)
return sw.client.unstructuredClient.PatchSubResource(ctx, obj, sw.subResource, patch, opts...)
case *metav1.PartialObjectMetadata:
return sw.client.metadataClient.PatchStatus(ctx, obj, patch, opts...)
return sw.client.metadataClient.PatchSubResource(ctx, obj, sw.subResource, patch, opts...)
default:
return sw.client.typedClient.PatchStatus(ctx, obj, patch, opts...)
return sw.client.typedClient.PatchSubResource(ctx, obj, sw.subResource, patch, opts...)
}
}
27 changes: 16 additions & 11 deletions pkg/client/dryrun.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,25 +82,30 @@ func (c *dryRunClient) List(ctx context.Context, obj ObjectList, opts ...ListOpt
}

// Status implements client.StatusClient.
func (c *dryRunClient) Status() StatusWriter {
return &dryRunStatusWriter{client: c.client.Status()}
func (c *dryRunClient) Status() SubResourceWriter {
return c.SubResource("status")
}

// ensure dryRunStatusWriter implements client.StatusWriter.
var _ StatusWriter = &dryRunStatusWriter{}
// SubResource implements client.SubResourceClient.
func (c *dryRunClient) SubResource(subResource string) SubResourceWriter {
return &dryRunSubResourceWriter{client: c.client.SubResource(subResource)}
}

// ensure dryRunSubResourceWriter implements client.SubResourceWriter.
var _ SubResourceWriter = &dryRunSubResourceWriter{}

// dryRunStatusWriter is client.StatusWriter that writes status subresource with dryRun mode
// dryRunSubResourceWriter is client.SubResourceWriter that writes status subresource with dryRun mode
// enforced.
type dryRunStatusWriter struct {
client StatusWriter
type dryRunSubResourceWriter struct {
client SubResourceWriter
}

// Update implements client.StatusWriter.
func (sw *dryRunStatusWriter) Update(ctx context.Context, obj Object, opts ...UpdateOption) error {
// Update implements client.SubResourceWriter.
func (sw *dryRunSubResourceWriter) Update(ctx context.Context, obj Object, opts ...UpdateOption) error {
return sw.client.Update(ctx, obj, append(opts, DryRunAll)...)
}

// Patch implements client.StatusWriter.
func (sw *dryRunStatusWriter) Patch(ctx context.Context, obj Object, patch Patch, opts ...PatchOption) error {
// Patch implements client.SubResourceWriter.
func (sw *dryRunSubResourceWriter) Patch(ctx context.Context, obj Object, patch Patch, opts ...PatchOption) error {
return sw.client.Patch(ctx, obj, patch, append(opts, DryRunAll)...)
}
22 changes: 13 additions & 9 deletions pkg/client/fake/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -634,8 +634,12 @@ func (c *fakeClient) Patch(ctx context.Context, obj client.Object, patch client.
return err
}

func (c *fakeClient) Status() client.StatusWriter {
return &fakeStatusWriter{client: c}
func (c *fakeClient) Status() client.SubResourceWriter {
return c.SubResource("status")
}

func (c *fakeClient) SubResource(subResource string) client.SubResourceWriter {
return &fakeSubResourceWriter{client: c}
}

func (c *fakeClient) deleteObject(gvr schema.GroupVersionResource, accessor metav1.Object) error {
Expand Down Expand Up @@ -664,19 +668,19 @@ func getGVRFromObject(obj runtime.Object, scheme *runtime.Scheme) (schema.GroupV
return gvr, nil
}

type fakeStatusWriter struct {
type fakeSubResourceWriter struct {
client *fakeClient
}

func (sw *fakeStatusWriter) Update(ctx context.Context, obj client.Object, opts ...client.UpdateOption) error {
// TODO(droot): This results in full update of the obj (spec + status). Need
// a way to update status field only.
func (sw *fakeSubResourceWriter) Update(ctx context.Context, obj client.Object, opts ...client.UpdateOption) error {
// TODO(droot): This results in full update of the obj (spec + subresources). Need
// a way to update subresource only.
return sw.client.Update(ctx, obj, opts...)
}

func (sw *fakeStatusWriter) Patch(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.PatchOption) error {
// TODO(droot): This results in full update of the obj (spec + status). Need
// a way to update status field only.
func (sw *fakeSubResourceWriter) Patch(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.PatchOption) error {
// TODO(droot): This results in full update of the obj (spec + subresources). Need
// a way to update subresource only.
return sw.client.Patch(ctx, obj, patch, opts...)
}

Expand Down
16 changes: 13 additions & 3 deletions pkg/client/interfaces.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,11 +81,20 @@ type Writer interface {
// StatusClient knows how to create a client which can update status subresource
// for kubernetes objects.
type StatusClient interface {
Status() StatusWriter
Status() SubResourceWriter
}

// StatusWriter knows how to update status subresource of a Kubernetes object.
type StatusWriter interface {
// SubResourceClient knows how to create a client which can update subresource
// for kubernetes objects.
type SubResourceClient interface {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing I am wondering is if this should maybe be called something like SubResourceCRUDClient or some such to clarify that this can not deal with other subresources like logs and differentiate it from a potential future non-crud subresource client?

I am not sure though if it is possible to come up with a sensible non-crud subresource client, since that could basically be anything? Maybe that one would just be a standard http client? Ref #452

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking about it it again, maybe we should instead just add more verb(s) once we get there?

SubResource(subResource string) SubResourceWriter
}

// StatusWriter is kept for backward compatibility.
type StatusWriter = SubResourceWriter

// SubResourceWriter knows how to update subresource of a Kubernetes object.
type SubResourceWriter interface {
Copy link
Contributor

@FillZpp FillZpp Jun 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may also have to define a SubResourceReader. Sometimes ppl want to get some subresources, for example getting a Scale subresource allows us to get the expected and current replicas number of any workloads (e.g., Deployment, StatefulSet), like what podautoscaler controller and disruption controller do.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could probably in the future replace the currrent SubResourceWriter interface with a SubResourceClient and then add read functionality in there?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or we could have both SubResourceReader and SubResourceWriter, like the Reader and Writer?

And also the SubResourceWriter needs more methods other than Update and Patch, for example the subresource pods/binding has to be POST, which is actually Create.

// Update updates the fields corresponding to the status subresource for the
// given obj. obj must be a struct pointer so that obj can be updated
// with the content returned by the Server.
Expand All @@ -102,6 +111,7 @@ type Client interface {
Reader
Writer
StatusClient
SubResourceClient

// Scheme returns the scheme this client is using.
Scheme() *runtime.Scheme
Expand Down
4 changes: 2 additions & 2 deletions pkg/client/metadata_client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ func (mc *metadataClient) List(ctx context.Context, obj ObjectList, opts ...List
return nil
}

func (mc *metadataClient) PatchStatus(ctx context.Context, obj Object, patch Patch, opts ...PatchOption) error {
func (mc *metadataClient) PatchSubResource(ctx context.Context, obj Object, subResource string, patch Patch, opts ...PatchOption) error {
metadata, ok := obj.(*metav1.PartialObjectMetadata)
if !ok {
return fmt.Errorf("metadata client did not understand object: %T", obj)
Expand All @@ -183,7 +183,7 @@ func (mc *metadataClient) PatchStatus(ctx context.Context, obj Object, patch Pat
}

patchOpts := &PatchOptions{}
res, err := resInt.Patch(ctx, metadata.Name, patch.Type(), data, *patchOpts.AsPatchOptions(), "status")
res, err := resInt.Patch(ctx, metadata.Name, patch.Type(), data, *patchOpts.AsPatchOptions(), subResource)
if err != nil {
return err
}
Expand Down
25 changes: 15 additions & 10 deletions pkg/client/namespaced_client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,21 +161,26 @@ func (n *namespacedClient) List(ctx context.Context, obj ObjectList, opts ...Lis
}

// Status implements client.StatusClient.
func (n *namespacedClient) Status() StatusWriter {
return &namespacedClientStatusWriter{StatusClient: n.client.Status(), namespace: n.namespace, namespacedclient: n}
func (n *namespacedClient) Status() SubResourceWriter {
return n.SubResource("status")
}

// ensure namespacedClientStatusWriter implements client.StatusWriter.
var _ StatusWriter = &namespacedClientStatusWriter{}
// SubResource implements client.SubResourceClient.
func (n *namespacedClient) SubResource(subResource string) SubResourceWriter {
return &namespacedClientSubResourceWriter{StatusClient: n.client.SubResource(subResource), namespace: n.namespace, namespacedclient: n}
}

// ensure namespacedClientSubResourceWriter implements client.SubResourceWriter.
var _ SubResourceWriter = &namespacedClientSubResourceWriter{}

type namespacedClientStatusWriter struct {
StatusClient StatusWriter
type namespacedClientSubResourceWriter struct {
StatusClient SubResourceWriter
namespace string
namespacedclient Client
}

// Update implements client.StatusWriter.
func (nsw *namespacedClientStatusWriter) Update(ctx context.Context, obj Object, opts ...UpdateOption) error {
// Update implements client.SubResourceWriter.
func (nsw *namespacedClientSubResourceWriter) Update(ctx context.Context, obj Object, opts ...UpdateOption) error {
isNamespaceScoped, err := objectutil.IsAPINamespaced(obj, nsw.namespacedclient.Scheme(), nsw.namespacedclient.RESTMapper())

if err != nil {
Expand All @@ -193,8 +198,8 @@ func (nsw *namespacedClientStatusWriter) Update(ctx context.Context, obj Object,
return nsw.StatusClient.Update(ctx, obj, opts...)
}

// Patch implements client.StatusWriter.
func (nsw *namespacedClientStatusWriter) Patch(ctx context.Context, obj Object, patch Patch, opts ...PatchOption) error {
// Patch implements client.SubResourceWriter.
func (nsw *namespacedClientSubResourceWriter) Patch(ctx context.Context, obj Object, patch Patch, opts ...PatchOption) error {
isNamespaceScoped, err := objectutil.IsAPINamespaced(obj, nsw.namespacedclient.Scheme(), nsw.namespacedclient.RESTMapper())

if err != nil {
Expand Down
10 changes: 5 additions & 5 deletions pkg/client/namespaced_client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -480,7 +480,7 @@ var _ = Describe("NamespacedClient", func() {
})
})

Describe("StatusWriter", func() {
Describe("SubResourceWriter", func() {
var err error
BeforeEach(func() {
dep, err = clientset.AppsV1().Deployments(ns).Create(ctx, dep, metav1.CreateOptions{})
Expand All @@ -495,7 +495,7 @@ var _ = Describe("NamespacedClient", func() {
changedDep := dep.DeepCopy()
changedDep.Status.Replicas = 99

Expect(getClient().Status().Update(ctx, changedDep)).NotTo(HaveOccurred())
Expect(getClient().SubResource("status").Update(ctx, changedDep)).NotTo(HaveOccurred())

actual, err := clientset.AppsV1().Deployments(ns).Get(ctx, dep.Name, metav1.GetOptions{})
Expect(err).NotTo(HaveOccurred())
Expand All @@ -509,14 +509,14 @@ var _ = Describe("NamespacedClient", func() {
changedDep.SetNamespace("test")
changedDep.Status.Replicas = 99

Expect(getClient().Status().Update(ctx, changedDep)).To(HaveOccurred())
Expect(getClient().SubResource("status").Update(ctx, changedDep)).To(HaveOccurred())
})

It("should change objects via status patch", func() {
changedDep := dep.DeepCopy()
changedDep.Status.Replicas = 99

Expect(getClient().Status().Patch(ctx, changedDep, client.MergeFrom(dep))).NotTo(HaveOccurred())
Expect(getClient().SubResource("status").Patch(ctx, changedDep, client.MergeFrom(dep))).NotTo(HaveOccurred())

actual, err := clientset.AppsV1().Deployments(ns).Get(ctx, dep.Name, metav1.GetOptions{})
Expect(err).NotTo(HaveOccurred())
Expand All @@ -530,7 +530,7 @@ var _ = Describe("NamespacedClient", func() {
changedDep.Status.Replicas = 99
changedDep.SetNamespace("test")

Expect(getClient().Status().Patch(ctx, changedDep, client.MergeFrom(dep))).To(HaveOccurred())
Expect(getClient().SubResource("status").Patch(ctx, changedDep, client.MergeFrom(dep))).To(HaveOccurred())
})
})

Expand Down
6 changes: 4 additions & 2 deletions pkg/client/split.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,15 +61,17 @@ func NewDelegatingClient(in NewDelegatingClientInput) (Client, error) {
uncachedGVKs: uncachedGVKs,
cacheUnstructured: in.CacheUnstructured,
},
Writer: in.Client,
StatusClient: in.Client,
Writer: in.Client,
StatusClient: in.Client,
SubResourceClient: in.Client,
}, nil
}

type delegatingClient struct {
Reader
Writer
StatusClient
SubResourceClient

scheme *runtime.Scheme
mapper meta.RESTMapper
Expand Down
14 changes: 7 additions & 7 deletions pkg/client/typed_client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import (

var _ Reader = &typedClient{}
var _ Writer = &typedClient{}
var _ StatusWriter = &typedClient{}
var _ SubResourceWriter = &typedClient{}

// client is a client.Client that reads and writes directly from/to an API server. It lazily initializes
// new clients at the time they are used, and caches the client.
Expand Down Expand Up @@ -159,8 +159,8 @@ func (c *typedClient) List(ctx context.Context, obj ObjectList, opts ...ListOpti
Into(obj)
}

// UpdateStatus used by StatusWriter to write status.
func (c *typedClient) UpdateStatus(ctx context.Context, obj Object, opts ...UpdateOption) error {
// UpdateSubResource used by SubResourceWriter to write status.
func (c *typedClient) UpdateSubResource(ctx context.Context, obj Object, subResource string, opts ...UpdateOption) error {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO, one obj Object in the parameters is not enough, because not all subresources (even CRUD subresources) take the original object as its request body. For subresources like Status and Finalize, yes. For subresources like Scale, no, which actually accept a autoscaling.Scale as the request body.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we definitely need that, apart from Scale there is also authenticationv1.TokenRequest in the case of serviceaccount tokens...Maybe we can have a StatusUpdateOption that has a Body Object field on top of the other UpdateOptions?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM, or we can have two objects in the UpdateSubResource parameters like
(ctx context.Context, obj Object, subObj Object, subResource string, opts ...UpdateOption)

If the subObj is not nil, we should take it as the update body, otherwise we will use the obj.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Due to the fact that it is not always mandatory I'd prefer to make it part of the opts and we can then provide a convenient wrapper, so that you can do something like:

c.UpdateSubresource(ctx, myObj, "my-subresource", client.WithSubResourceBody(mySubResource))

o, err := c.cache.getObjMeta(obj)
if err != nil {
return err
Expand All @@ -173,15 +173,15 @@ func (c *typedClient) UpdateStatus(ctx context.Context, obj Object, opts ...Upda
NamespaceIfScoped(o.GetNamespace(), o.isNamespaced()).
Resource(o.resource()).
Name(o.GetName()).
SubResource("status").
SubResource(subResource).
Body(obj).
VersionedParams((&UpdateOptions{}).ApplyOptions(opts).AsUpdateOptions(), c.paramCodec).
Do(ctx).
Into(obj)
}

// PatchStatus used by StatusWriter to write status.
func (c *typedClient) PatchStatus(ctx context.Context, obj Object, patch Patch, opts ...PatchOption) error {
// PatchSubResource used by SubResourceWriter to write subresource.
func (c *typedClient) PatchSubResource(ctx context.Context, obj Object, subResource string, patch Patch, opts ...PatchOption) error {
o, err := c.cache.getObjMeta(obj)
if err != nil {
return err
Expand All @@ -197,7 +197,7 @@ func (c *typedClient) PatchStatus(ctx context.Context, obj Object, patch Patch,
NamespaceIfScoped(o.GetNamespace(), o.isNamespaced()).
Resource(o.resource()).
Name(o.GetName()).
SubResource("status").
SubResource(subResource).
Body(data).
VersionedParams(patchOpts.ApplyOptions(opts).AsPatchOptions(), c.paramCodec).
Do(ctx).
Expand Down
10 changes: 5 additions & 5 deletions pkg/client/unstructured_client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ import (

var _ Reader = &unstructuredClient{}
var _ Writer = &unstructuredClient{}
var _ StatusWriter = &unstructuredClient{}
var _ SubResourceWriter = &unstructuredClient{}

// client is a client.Client that reads and writes directly from/to an API server. It lazily initializes
// new clients at the time they are used, and caches the client.
Expand Down Expand Up @@ -216,7 +216,7 @@ func (uc *unstructuredClient) List(ctx context.Context, obj ObjectList, opts ...
Into(obj)
}

func (uc *unstructuredClient) UpdateStatus(ctx context.Context, obj Object, opts ...UpdateOption) error {
func (uc *unstructuredClient) UpdateSubResource(ctx context.Context, obj Object, subResource string, opts ...UpdateOption) error {
if _, ok := obj.(*unstructured.Unstructured); !ok {
return fmt.Errorf("unstructured client did not understand object: %T", obj)
}
Expand All @@ -230,14 +230,14 @@ func (uc *unstructuredClient) UpdateStatus(ctx context.Context, obj Object, opts
NamespaceIfScoped(o.GetNamespace(), o.isNamespaced()).
Resource(o.resource()).
Name(o.GetName()).
SubResource("status").
SubResource(subResource).
Body(obj).
VersionedParams((&UpdateOptions{}).ApplyOptions(opts).AsUpdateOptions(), uc.paramCodec).
Do(ctx).
Into(obj)
}

func (uc *unstructuredClient) PatchStatus(ctx context.Context, obj Object, patch Patch, opts ...PatchOption) error {
func (uc *unstructuredClient) PatchSubResource(ctx context.Context, obj Object, subResource string, patch Patch, opts ...PatchOption) error {
u, ok := obj.(*unstructured.Unstructured)
if !ok {
return fmt.Errorf("unstructured client did not understand object: %T", obj)
Expand All @@ -260,7 +260,7 @@ func (uc *unstructuredClient) PatchStatus(ctx context.Context, obj Object, patch
NamespaceIfScoped(o.GetNamespace(), o.isNamespaced()).
Resource(o.resource()).
Name(o.GetName()).
SubResource("status").
SubResource(subResource).
Body(data).
VersionedParams(patchOpts.ApplyOptions(opts).AsPatchOptions(), uc.paramCodec).
Do(ctx).
Expand Down