add support for impersonating dynamic client and configuring subresource #920

alaypatel07 · 2024-01-16T08:55:03Z

What type of PR is this?

/kind feature
/kind api-change

What this PR does / why we need it:

This PR adds API support to stage CRD for using impersonation to patch status and an option to patch entire object instead of subresource

Which issue(s) this PR fixes:

Fixes #903

Special notes for your reviewer:

The impersonate permission is not granted to the service account by default because this seems like an advance usecase. When user wants to use impersonation, the impersonate permissions can be patched as its done here https://github.com/kwok-ci/kubevirt-demo/blob/main/kustomize/rbac/role.yaml

Does this PR introduce a user-facing change?

Add support for impersonating client and configuring subresource to Stage api

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

In order to use the impersonating client an additional patch to rbac is required.
- apiGroups:
  - ""
  resources:
  - serviceaccounts
  verbs:
  - impersonate

linux-foundation-easycla · 2024-01-16T08:55:06Z

The committers listed above are authorized under a signed CLA.

✅ login: alaypatel07 / name: Alay Patel (7d18f43)

netlify · 2024-01-16T08:55:09Z

✅ Deploy Preview for k8s-kwok ready!

Name	Link
🔨 Latest commit	`7d18f43`
🔍 Latest deploy log	https://app.netlify.com/sites/k8s-kwok/deploys/65adf26c7d1b2500080280a0
😎 Deploy Preview	https://deploy-preview-920--k8s-kwok.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

k8s-ci-robot · 2024-01-16T08:55:11Z

Welcome @alaypatel07!

It looks like this is your first PR to kubernetes-sigs/kwok 🎉. 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/kwok 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 · 2024-01-16T08:55:12Z

Hi @alaypatel07. 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.

alaypatel07 · 2024-01-16T08:58:52Z

Tested the code changes with the following stage

$ k get stage vmi-ready -oyaml
apiVersion: kwok.x-k8s.io/v1alpha1
kind: Stage
metadata:
  creationTimestamp: "2024-01-16T08:44:09Z"
  generation: 3
  name: vmi-ready
  resourceVersion: "18495"
  uid: 2b9a141e-0a9f-4fb1-bb43-db581b95dace
spec:
  impersonationConfig:
    username: system:serviceaccount:kubevirt:kubevirt-controller
  isStatusSubresource: false
  next:
    statusTemplate: |
      {{ $now := Now }}
      phase: Running
      conditions:
      - type: Ready
        status: "True"
        lastTransitionTime: {{ $now }}
      phaseTransitionTimestamps:
      - phase: Running
        phaseTransitionTimestamp: {{ $now }}
  resourceRef:
    apiGroup: kubevirt.io/v1
    kind: VirtualMachineInstance
  selector:
    matchExpressions:                                                                                                                                                                                                                                              - key: .metadata.deletionTimestamp
      operator: DoesNotExist                                                                                                                                                                                                                                       - key: .status.phase
      operator: NotIn                                                                                                                                                                                                                                                values:
      - Running                                                                                                                                                                                                                                                    - key: .spec.nodeSelector.type
      operator: In                                                                                                                                                                                                                                                   values:
      - kwok                                                                                                                                                                                                                                                     weight: 0

Got the VMI to transition into running state, kwok logs


{"time":"2024-01-16T08:57:01.245931431Z","level":"INFO","source":{"function":"sigs.k8s.io/kwok/pkg/kwok/controllers.(*StageController).patchResource","file":"/home/alayp/go/src/sigs.k8s.io/kwok/pkg/kwok/controllers/stage_controller.go","line":362},"msg":"patchResource","id":"kind-control-plane_68c54e43-50ba-42a6-9ba7-a83aeb01ffff","resource":"default/test-vmi","client":"using impersonating client"}
{"time":"2024-01-16T08:57:01.245944716Z","level":"INFO","source":{"function":"sigs.k8s.io/kwok/pkg/kwok/controllers.(*StageController).patchResource","file":"/home/alayp/go/src/sigs.k8s.io/kwok/pkg/kwok/controllers/stage_controller.go","line":381},"msg":"isStatusSubresource false","id":"kind-control-plane_68c54e43-50ba-42a6-9ba7-a83aeb01ffff","resource":"default/test-vmi","patch":"{\"status\":{\"conditions\":[{\"lastTransitionTime\":\"2024-01-16T08:57:01.245788983Z\",\"status\":\"True\",\"type\":\"Ready\"}],\"phase\":\"Running\",\"phaseTransitionTimestamps\":[{\"phase\":\"Running\",\"phaseTransitionTimestamp\":\"2024-01-16T08:57:01.245788983Z\"}]}}"}

vmi status:

status:
 activePods:
   023580f8-06bd-4c0d-90fc-698663c12fdb: kwok-node-0
 conditions:
 - lastTransitionTime: "2024-01-16T08:57:01.245788983Z"
   status: "True"
   type: Ready
 guestOSInfo: {}
 launcherContainerImageVersion: quay.io/kubevirt/virt-launcher:v1.1.1
 memory:
   guestAtBoot: 50M
   guestCurrent: 50M
   guestRequested: 50M
 phase: Running
 phaseTransitionTimestamps:
 - phase: Running
   phaseTransitionTimestamp: "2024-01-16T08:57:01.245788983Z"
 runtimeUser: 107
 volumeStatus:
 - name: containerdisk
   target: ""

alaypatel07 · 2024-01-16T09:00:10Z

Please note this is just a draft for sharing the POC, will clean up the code if this is acceptable API change. @wzshiming PTAL

pkg/kwok/controllers/stage_controller.go

pkg/apis/internalversion/stage_types.go

pkg/apis/v1alpha1/stage_types.go

wzshiming · 2024-01-16T11:51:25Z

❌ The email address for the commit (51744a9) is not linked to the GitHub account, preventing the EasyCLA check. Consult this Help Article and GitHub Help to resolve. (To view the commit's email address, add .patch at the end of this PR page's URL.) For further assistance with EasyCLA, please submit a support request ticket.

Please take a look and sign the CLA

pkg/apis/v1alpha1/stage_types.go

alaypatel07 · 2024-01-16T15:06:03Z

Thanks for the review, I will address all the comments soon

alaypatel07 · 2024-01-17T18:04:29Z

can I please get an ok-to-test to trigger the prow tests?

pkg/kwok/controllers/stage_controller.go

pkg/kwok/controllers/controller.go

pkg/kwok/cmd/root.go

wzshiming · 2024-01-18T01:59:17Z

pkg/apis/internalversion/stage_types.go

@@ -83,6 +83,19 @@ type StageNext struct {
 	Delete bool
 	// StatusTemplate indicates the template for modifying the status of the resource in the next.
 	StatusTemplate string
+	// StatusSubresource indicates the name of the subresource that will be patched. A nil value means
+	// entire object will be patches
+	StatusSubresource *string


Suggested change

StatusSubresource *string

StatusSubresource string

A null pointer is assigned to a value during the conversion, so thie don't have to use a pointer here

I have updated this.

Just out of curiosity, whats the point of have *string in api, when we are automatically converting nil string to ""?

My understanding is that a *string in api is recommended to differentiate nil and "" as different states

pkg/apis/v1alpha1/stage_types.go

wzshiming · 2024-01-18T02:06:28Z

/ok-to-test

alaypatel07 · 2024-01-18T03:25:20Z

Unsure why this failed https://github.com/kubernetes-sigs/kwok/actions/runs/7564581117/job/20598936655?pr=920. @wzshiming is the failure related to the PR?

wzshiming · 2024-01-18T05:56:57Z

Unsure why this failed https://github.com/kubernetes-sigs/kwok/actions/runs/7564581117/job/20598936655?pr=920. @wzshiming is the failure related to the PR?

Yes, you can ignore the failures in mac, the recent Github Action MacOS Runner is very unstable with docker.

alaypatel07 · 2024-01-18T06:06:42Z

Okay thanks @wzshiming. I addressed all your comments. Can I have another round of reviews? TIA

wzshiming

I have a try, and seems that the merge did not work properly, but overwrote the original content.

status:
  activePods:
    08451704-809e-450c-a45c-862995158c57: kwok-node-0
  conditions:
  - lastTransitionTime: "2024-01-18T14:59:32.084011845Z"
    status: "True"
    type: Ready
  guestOSInfo: {}
  launcherContainerImageVersion: quay.io/kubevirt/virt-launcher:v1.1.1
  memory:
    guestAtBoot: 50M
    guestCurrent: 50M
    guestRequested: 50M
  phase: Running
  phaseTransitionTimestamps:
  - phase: Running
    phaseTransitionTimestamp: "2024-01-18T14:59:32.084011845Z"
  runtimeUser: 107
  volumeStatus:
  - name: containerdisk
    target: ""

status:
  activePods:
    4de00779-ba67-438f-8357-213ac7500f22: kwok-node-0
  conditions:
  - lastProbeTime: "2024-01-18T14:56:22Z"
    lastTransitionTime: "2024-01-18T14:56:22Z"
    message: Guest VM is not reported as running
    reason: GuestNotRunning
    status: "False"
    type: Ready
  guestOSInfo: {}
  launcherContainerImageVersion: quay.io/kubevirt/virt-launcher:v1.1.1
  memory:
    guestAtBoot: 50M
    guestCurrent: 50M
    guestRequested: 50M
  migrationTransport: Unix
  nodeName: kwok-node-0
  phase: Scheduled
  phaseTransitionTimestamps:
  - phase: Pending
    phaseTransitionTimestamp: "2024-01-18T14:56:22Z"
  - phase: Scheduling
    phaseTransitionTimestamp: "2024-01-18T14:56:22Z"
  - phase: Scheduled
    phaseTransitionTimestamp: "2024-01-18T14:56:22Z"
  qosClass: Burstable
  runtimeUser: 107
  volumeStatus:
  - name: containerdisk
    target: ""

pkg/utils/client/clientset.go

pkg/kwok/controllers/stage_controller.go

pkg/kwok/controllers/controller.go

pkg/apis/internalversion/stage_types.go

pkg/apis/v1alpha1/stage_types.go

alaypatel07 · 2024-01-18T15:39:58Z

@wzshiming, yes I had this problem too

I have a try, and seems that the merge did not work properly, but overwrote the original content

Although I modified the stage vmi-ready with following:

apiVersion: kwok.x-k8s.io/v1alpha1
kind: Stage
metadata:
  name: vmi-ready
spec
  resourceRef:
    apiGroup: kubevirt.io/v1
    kind: VirtualMachineInstance
  selector:
    matchExpressions:
    - key: '.metadata.deletionTimestamp'
      operator: 'DoesNotExist'
    - key: '.status.phase'
      operator: 'In'
      values:
      - 'Scheduled'
    - key: '.spec.nodeSelector.type'
      operator: 'In'
      values:
      - 'kwok'
  next:
    statusTemplate: |
      {{ $now := Now }}
      phase: Running
      conditions:
      - type: Ready
        status: "True"
        lastTransitionTime: {{ $now }}
        {{ with .status.phaseTransitionTimestamps }}
      phaseTransitionTimestamps:
        {{ range . }}
      - phase: {{ .phase }}
        phaseTransitionTimestamp: {{ .phaseTransitionTimestamp }}
        {{ end }}
        {{ end }}
      - phase: Running
        phaseTransitionTimestamp: {{ $now }}
    statusPatchAs:
      username: 'system:serviceaccount:kubevirt:kubevirt-controller'
    statusSubresource: ''

With this change to vmi-ready, the merge worked properly for me
Before:

status:
  activePods:
    ddf44ad9-5437-4dc3-bf9f-86884364079d: kwok-node-0
  conditions:
    - lastProbeTime: "2024-01-18T15:34:08Z"
      lastTransitionTime: "2024-01-18T15:34:08Z"
      message: Guest VM is not reported as running
      reason: GuestNotRunning
      status: "False"
      type: Ready
  guestOSInfo: {}
  launcherContainerImageVersion: quay.io/kubevirt/virt-launcher:v1.1.1
  memory:
    guestAtBoot: 50M
    guestCurrent: 50M
    guestRequested: 50M
  migrationTransport: Unix
  nodeName: kwok-node-0
  phase: Scheduled
  phaseTransitionTimestamps:
    - phase: Pending
      phaseTransitionTimestamp: "2024-01-18T15:34:08Z"
    - phase: Scheduling
      phaseTransitionTimestamp: "2024-01-18T15:34:08Z"
    - phase: Scheduled
      phaseTransitionTimestamp: "2024-01-18T15:34:08Z"
  qosClass: Burstable
  runtimeUser: 107
  volumeStatus:
    - name: containerdisk
      target: ""

After:

activePods:
 ddf44ad9-5437-4dc3-bf9f-86884364079d: kwok-node-0
conditions:
 - lastTransitionTime: "2024-01-18T15:34:08.608218243Z"
   status: "True"
   type: Ready
guestOSInfo: {}
launcherContainerImageVersion: quay.io/kubevirt/virt-launcher:v1.1.1
memory:
 guestAtBoot: 50M
 guestCurrent: 50M
 guestRequested: 50M
migrationTransport: Unix
nodeName: kwok-node-0
phase: Running
phaseTransitionTimestamps:
 - phase: Pending
   phaseTransitionTimestamp: "2024-01-18T15:34:08Z"
 - phase: Scheduling
   phaseTransitionTimestamp: "2024-01-18T15:34:08Z"
 - phase: Scheduled
   phaseTransitionTimestamp: "2024-01-18T15:34:08Z"
 - phase: Running
   phaseTransitionTimestamp: "2024-01-18T15:34:08.608218243Z"
qosClass: Burstable
runtimeUser: 107
volumeStatus:
 - name: containerdisk
   target: ""

alaypatel07 · 2024-01-18T15:40:31Z

@wzshiming thanks for another round of reviews, I pushed a change addressing all the comments

pkg/apis/v1alpha1/stage_types.go

Signed-off-by: Alay Patel <alay1431@gmail.com>

wzshiming · 2024-01-22T10:12:54Z

/approve
/lgtm

Thank you for your contribution!

k8s-ci-robot · 2024-01-22T10:13:01Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alaypatel07, wzshiming

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [wzshiming]

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

caozhuozi · 2024-01-22T10:57:44Z

Once this is merged, I will rebase the #911

alaypatel07 · 2024-01-22T13:57:41Z

@wzshiming is there a way to manually re-trigger the failing github actions test Test / test-kwokctl (macos-latest, kind)?

alaypatel07 · 2024-01-22T14:06:35Z

/retest

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 16, 2024

k8s-ci-robot added the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Jan 16, 2024

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API labels Jan 16, 2024

k8s-ci-robot requested review from wzshiming and yibozhuang January 16, 2024 08:55

k8s-ci-robot added the kind/documentation Categorizes issue or PR as related to documentation. label Jan 16, 2024

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 16, 2024

alaypatel07 force-pushed the impersonating-client branch from 044f3a9 to 51744a9 Compare January 16, 2024 09:01

wzshiming reviewed Jan 16, 2024

View reviewed changes

pkg/apis/v1alpha1/stage_types.go Outdated Show resolved Hide resolved

alaypatel07 force-pushed the impersonating-client branch from 51744a9 to 4b7f01d Compare January 17, 2024 15:22

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 Jan 17, 2024

alaypatel07 force-pushed the impersonating-client branch 2 times, most recently from ebcc587 to 16f5f92 Compare January 17, 2024 15:57

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jan 17, 2024

alaypatel07 changed the title ~~wip: app support for impersonating dynamic client~~ app support for impersonating dynamic client and configuring subresource Jan 17, 2024

alaypatel07 marked this pull request as ready for review January 17, 2024 16:17

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 17, 2024

wzshiming reviewed Jan 18, 2024

View reviewed changes

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 Jan 18, 2024

alaypatel07 force-pushed the impersonating-client branch 2 times, most recently from 5682ebf to d975050 Compare January 18, 2024 02:36

wzshiming reviewed Jan 18, 2024

View reviewed changes

pkg/apis/internalversion/stage_types.go Outdated Show resolved Hide resolved

pkg/apis/v1alpha1/stage_types.go Outdated Show resolved Hide resolved

pkg/apis/v1alpha1/stage_types.go Outdated Show resolved Hide resolved

alaypatel07 force-pushed the impersonating-client branch from d975050 to 2fab875 Compare January 18, 2024 15:17

alaypatel07 changed the title ~~app support for impersonating dynamic client and configuring subresource~~ add support for impersonating dynamic client and configuring subresource Jan 19, 2024

alaypatel07 force-pushed the impersonating-client branch from 2fab875 to 8511434 Compare January 19, 2024 04:24

wzshiming reviewed Jan 19, 2024

View reviewed changes

pkg/apis/v1alpha1/stage_types.go Outdated Show resolved Hide resolved

add support for impersonating dynamic client

7d18f43

Signed-off-by: Alay Patel <alay1431@gmail.com>

alaypatel07 force-pushed the impersonating-client branch from 8511434 to 7d18f43 Compare January 22, 2024 04:43

k8s-ci-robot assigned wzshiming Jan 22, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 22, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 22, 2024

wzshiming mentioned this pull request Jan 22, 2024

[KWOK] retry when applying Stages fails #911

Merged

k8s-ci-robot merged commit 2e0e8bf into kubernetes-sigs:main Jan 23, 2024
29 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add support for impersonating dynamic client and configuring subresource #920

add support for impersonating dynamic client and configuring subresource #920

alaypatel07 commented Jan 16, 2024 •

edited

linux-foundation-easycla bot commented Jan 16, 2024 •

edited

netlify bot commented Jan 16, 2024 •

edited

k8s-ci-robot commented Jan 16, 2024

k8s-ci-robot commented Jan 16, 2024

alaypatel07 commented Jan 16, 2024 •

edited

alaypatel07 commented Jan 16, 2024

wzshiming commented Jan 16, 2024

alaypatel07 commented Jan 16, 2024

alaypatel07 commented Jan 17, 2024

wzshiming Jan 18, 2024

alaypatel07 Jan 18, 2024

wzshiming commented Jan 18, 2024

alaypatel07 commented Jan 18, 2024

wzshiming commented Jan 18, 2024

alaypatel07 commented Jan 18, 2024 •

edited

wzshiming left a comment

alaypatel07 commented Jan 18, 2024

alaypatel07 commented Jan 18, 2024

wzshiming commented Jan 22, 2024

k8s-ci-robot commented Jan 22, 2024

caozhuozi commented Jan 22, 2024

alaypatel07 commented Jan 22, 2024

alaypatel07 commented Jan 22, 2024

add support for impersonating dynamic client and configuring subresource #920

add support for impersonating dynamic client and configuring subresource #920

Conversation

alaypatel07 commented Jan 16, 2024 • edited

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

linux-foundation-easycla bot commented Jan 16, 2024 • edited

netlify bot commented Jan 16, 2024 • edited

✅ Deploy Preview for k8s-kwok ready!

k8s-ci-robot commented Jan 16, 2024

k8s-ci-robot commented Jan 16, 2024

alaypatel07 commented Jan 16, 2024 • edited

alaypatel07 commented Jan 16, 2024

wzshiming commented Jan 16, 2024

alaypatel07 commented Jan 16, 2024

alaypatel07 commented Jan 17, 2024

wzshiming Jan 18, 2024

Choose a reason for hiding this comment

alaypatel07 Jan 18, 2024

Choose a reason for hiding this comment

wzshiming commented Jan 18, 2024

alaypatel07 commented Jan 18, 2024

wzshiming commented Jan 18, 2024

alaypatel07 commented Jan 18, 2024 • edited

wzshiming left a comment

Choose a reason for hiding this comment

alaypatel07 commented Jan 18, 2024

alaypatel07 commented Jan 18, 2024

wzshiming commented Jan 22, 2024

k8s-ci-robot commented Jan 22, 2024

caozhuozi commented Jan 22, 2024

alaypatel07 commented Jan 22, 2024

alaypatel07 commented Jan 22, 2024

alaypatel07 commented Jan 16, 2024 •

edited

linux-foundation-easycla bot commented Jan 16, 2024 •

edited

netlify bot commented Jan 16, 2024 •

edited

alaypatel07 commented Jan 16, 2024 •

edited

alaypatel07 commented Jan 18, 2024 •

edited