✨ Support subresource modification #1922

g7r · 2022-06-02T16:00:17Z

I need to add finalizers without granting access to the entire resource. It certainly is possible in Kubernetes:

apiVersion: rbac.authorization.k8s.io/v1
kind: Role
metadata:
  name: myrole
rules:
  - apiGroups:
      - apps
    resources:
      - deployments
    verbs:
      - get
      - watch
      - list

  - apiGroups:
      - apps
    resources:
      - deployments/finalizers
    verbs:
      - update

But I can't update finalizers using client.Client. I don't have a permission to modify entire deployment and client.Client doesn't provide an API to modify generic subresource, it only has special support for "status" subresource. In this PR I add support to other subresources.

I'm open to suggestions of course. Consider the initial PR version also as an invitation to discussion even if the implementation is totally wrong.

Fixes #172

linux-foundation-easycla · 2022-06-02T16:00:25Z

The committers listed above are authorized under a signed CLA.

✅ login: g7r / name: Sergej Zagursky (03edfd3)

k8s-ci-robot · 2022-06-02T16:00:30Z

Welcome @g7r!

It looks like this is your first PR to kubernetes-sigs/controller-runtime 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/controller-runtime has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2022-06-02T16:00:31Z

Hi @g7r. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot · 2022-06-02T16:00:37Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: g7r
To complete the pull request process, please assign gerred after the PR has been reviewed.
You can assign the PR to them by writing /assign @gerred in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

alvaroaleman · 2022-06-02T16:50:29Z

/ok-to-test

alvaroaleman · 2022-06-02T16:50:56Z

/cc @FillZpp

joelanford

I actually ran into this exact problem recently and was scratching my head about why it wasn't working. Thanks for submitting this!

This seems to be the solution for #172

My only small concern with this is that it might imply it handles more than CRUD (e.g. pods/logs or an arbitrary subresource of an APIService. Perhaps GoDoc is enough to document the limitations?

joelanford · 2022-06-02T20:31:12Z

pkg/client/client.go

-func (c *client) Status() StatusWriter {
-	return &statusWriter{client: c}
+func (c *client) Status() SubResourceWriter {
+	return &subResourceWriter{client: c, subResource: "status"}


Does this work?

Suggested change

return &subResourceWriter{client: c, subResource: "status"}

return SubResource("status")

Absolutely! Thanks for the suggestion. I've applied it here and to other similar places as well.

alvaroaleman · 2022-06-03T15:18:35Z

pkg/client/interfaces.go

-type StatusWriter interface {
+// SubResourceClient knows how to create a client which can update subresource
+// for kubernetes objects.
+type SubResourceClient interface {


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

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

FillZpp

Some comments to this CRUD subresource client. It's nice that we can read/write more subresources in controller-runtime.

Also a non-CRUD subresource client will be great, but it is not easy to define a common interface for those subresources. It seems they are all defined for K8s built-in resources and don't support custom resources (I'm not sure). So maybe document that those could only be handled via client-go temporarily? WDYT @alvaroaleman @joelanford

FillZpp · 2022-06-06T08:40:46Z

pkg/client/interfaces.go

+type StatusWriter = SubResourceWriter
+
+// SubResourceWriter knows how to update subresource of a Kubernetes object.
+type SubResourceWriter interface {


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.

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

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.

FillZpp · 2022-06-06T09:30:19Z

pkg/client/typed_client.go

-// 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 {


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.

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?

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.

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))

alvaroaleman · 2022-07-20T15:26:53Z

@g7r do you have the cycles to address the feedback on this?

k8s-triage-robot · 2022-10-18T15:33:49Z

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle stale
Mark this issue or PR as rotten with /lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

Support subresource modification

03edfd3

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Jun 2, 2022

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 2, 2022

k8s-ci-robot requested review from droot and mengqiy June 2, 2022 16:00

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 2, 2022

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jun 2, 2022

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 2, 2022

k8s-ci-robot requested a review from FillZpp June 2, 2022 16:50

joelanford reviewed Jun 2, 2022

View reviewed changes

Use SubResource method to implement Status method

490c300

alvaroaleman reviewed Jun 3, 2022

View reviewed changes

alvaroaleman added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Jun 3, 2022

FillZpp reviewed Jun 6, 2022

View reviewed changes

jmcshane mentioned this pull request Oct 3, 2022

Support subresource modification #2015

Closed

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 18, 2022

alvaroaleman mentioned this pull request Nov 28, 2022

⚠️ Add client.SubResourceWriter #2072

Merged

k8s-ci-robot closed this in #2072 Nov 30, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Support subresource modification #1922

✨ Support subresource modification #1922

g7r commented Jun 2, 2022 •

edited by alvaroaleman

Loading

linux-foundation-easycla bot commented Jun 2, 2022 •

edited

Loading

k8s-ci-robot commented Jun 2, 2022

k8s-ci-robot commented Jun 2, 2022

k8s-ci-robot commented Jun 2, 2022

alvaroaleman commented Jun 2, 2022

alvaroaleman commented Jun 2, 2022

joelanford left a comment

joelanford Jun 2, 2022

g7r Jun 3, 2022

alvaroaleman Jun 3, 2022

alvaroaleman Nov 15, 2022

FillZpp left a comment •

edited

Loading

FillZpp Jun 6, 2022 •

edited

Loading

alvaroaleman Nov 15, 2022

FillZpp Nov 17, 2022

FillZpp Jun 6, 2022

alvaroaleman Nov 15, 2022

FillZpp Nov 16, 2022

alvaroaleman Nov 25, 2022

alvaroaleman commented Jul 20, 2022

k8s-triage-robot commented Oct 18, 2022

	return &subResourceWriter{client: c, subResource: "status"}
	return SubResource("status")

✨ Support subresource modification #1922

✨ Support subresource modification #1922

Conversation

g7r commented Jun 2, 2022 • edited by alvaroaleman Loading

linux-foundation-easycla bot commented Jun 2, 2022 • edited Loading

k8s-ci-robot commented Jun 2, 2022

k8s-ci-robot commented Jun 2, 2022

k8s-ci-robot commented Jun 2, 2022

alvaroaleman commented Jun 2, 2022

alvaroaleman commented Jun 2, 2022

joelanford left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FillZpp left a comment • edited Loading

Choose a reason for hiding this comment

FillZpp Jun 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alvaroaleman commented Jul 20, 2022

k8s-triage-robot commented Oct 18, 2022

g7r commented Jun 2, 2022 •

edited by alvaroaleman

Loading

linux-foundation-easycla bot commented Jun 2, 2022 •

edited

Loading

FillZpp left a comment •

edited

Loading

FillZpp Jun 6, 2022 •

edited

Loading