KEP-3895: kubectl delete: Add interactive(-i) flag #3896

ardaguclu · 2023-03-02T07:52:23Z

One-line PR description: Add interactive(-i) flag to kubectl delete command that will ask confirmation to user to proceed
Issue link: kubectl delete: Add interactive(-i) flag #3895
Other comments: kubectl delete: Introduce new interactive flag for interactive deletion kubernetes#114530

ardaguclu · 2023-03-07T13:30:07Z

/cc @KnVerey @eddiezane

keps/sig-cli/3895-kubectl-delete-interactivity/README.md

ardaguclu · 2023-05-22T07:48:02Z

/label tide/merge-method-squash

ardaguclu · 2023-05-29T13:31:54Z

@soltysh @atiratree could you PTAL?. Thanks.

soltysh

Minor nits
/lgtm
/approve

soltysh · 2023-05-30T09:09:43Z

keps/sig-cli/3895-kubectl-delete-interactivity/README.md

+In order to overcome this limitation, this feature will use separate builder to show resources as preview. Current builder
+will continue being used as deletion traverse, other builder will be used for preview. Builders must match in terms of 
+configurations to prevent such discrepancies and required tests will be added to assure that configurations are same or not.
+Moreover, deletion will only be performed to the resources whose `uidMap` match the ones in the preview list.


Suggested change

Moreover, deletion will only be performed to the resources whose `uidMap` match the ones in the preview list.

Moreover, deletion will only be performed on the resources whose `.metadata.uid` match the ones gathered in the preview list, to ensure we remove only the resources a user explicitly agreed to.

Does this need a stronger guarantee like RV based matching?

Thanks for review @enj.
We need to not delete the resources whose are not in the preview list(especially the cases where user types y after a couple of hours and preview list becomes stale most likely). And uid based check allows us to do it and this looks sufficient for our aim(Since kubectl delete does not know resource versions of resources, that might complicate the issue).

soltysh · 2023-05-30T09:22:55Z

keps/sig-cli/3895-kubectl-delete-interactivity/README.md

+-->
+Only standard input via terminal will be supported because main motivation of this flag
+is to provide a way to users. That would be a risk if there is exceptional inputs whose are available
+for users that we can not handle.


I'd propose stating this something like:

This mechanism will apply only to client-side operation (kubectl delete) and as mentioned in non-goals section is not meant to solve server-side protection, which can be addressed using custom ValidatingAdmissionWebhook, for example.

keps/sig-cli/3895-kubectl-delete-interactivity/README.md

soltysh · 2023-06-01T09:32:10Z

/assign @johnbelamaric
for PRR review

enj

Initial review as PRR shadow.

enj · 2023-06-12T19:31:39Z

keps/sig-cli/3895-kubectl-delete-interactivity/README.md

+In order to overcome this limitation, this feature will use separate builder to show resources as preview. Current builder
+will continue being used as deletion traverse, other builder will be used for preview. Builders must match in terms of 
+configurations to prevent such discrepancies and required tests will be added to assure that configurations are same or not.
+Moreover, deletion will only be performed to the resources whose `uidMap` match the ones in the preview list.


Does this need a stronger guarantee like RV based matching?

enj · 2023-06-12T19:36:00Z

keps/sig-cli/3895-kubectl-delete-interactivity/README.md

+-->
+- In terms of backwards compatibility, this flag's default will always be false.
+- If user cancels deletion, exit code will be 0.
+- This flag will not be used with `--raw` flag. 


How does this change behave in regards to label based deletion, especially if the set of objects matched by the label changes?

This problem is also true for default delete behavior(although agree that this is much more visible with this interactivity flag). We'll delete the resources whose are listed as preview and user confirms. If set of objects belonging to the some label is changed, user should execute kubectl delete again.

Include the intended behavior in the KEP.

Updated. Does it make sense?. Thanks.

enj · 2023-06-12T19:37:57Z

keps/sig-cli/3895-kubectl-delete-interactivity/README.md

+You can take a look at one potential example of such test in:
+https://github.com/kubernetes/kubernetes/pull/97058/files#diff-7826f7adbc1996a05ab52e3f5f02429e94b68ce6bce0dc534d1be636154fded3R246-R282
+-->
+No, users can only use this feature via enabling environment variable.


You need a test (likely an e2e) that shows that you cannot use the feature when the env var is not set.

That is a good point and I'll add a test case for this. Do you want me to update also the KEP by mentioning this?

keps/sig-cli/3895-kubectl-delete-interactivity/README.md

johnbelamaric · 2023-06-15T00:16:39Z

/approve

k8s-ci-robot · 2023-06-15T00:16:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ardaguclu, johnbelamaric, soltysh

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:

~~keps/prod-readiness/OWNERS~~ [johnbelamaric]
~~keps/sig-cli/OWNERS~~ [johnbelamaric,soltysh]

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

wojtek-t · 2023-06-15T05:59:41Z

/lgtm

KEP-3895: kubectl delete: Add interactive(-i) flag

ee14307

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 2, 2023

k8s-ci-robot requested a review from seans3 March 2, 2023 07:52

k8s-ci-robot added the kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory label Mar 2, 2023

k8s-ci-robot requested a review from soltysh March 2, 2023 07:52

k8s-ci-robot added sig/cli Categorizes an issue or PR as relevant to SIG CLI. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 2, 2023

This was referenced Mar 2, 2023

kubectl delete: Add interactive(-i) flag #3895

Closed

KEP 2775: kubectl delete interactivity and delay #2777

Closed

k8s-ci-robot requested review from eddiezane and KnVerey March 7, 2023 13:30

atiratree reviewed Apr 3, 2023

View reviewed changes

keps/sig-cli/3895-kubectl-delete-interactivity/README.md Show resolved Hide resolved

keps/sig-cli/3895-kubectl-delete-interactivity/README.md Show resolved Hide resolved

soltysh reviewed May 16, 2023

View reviewed changes

ardaguclu added 2 commits May 22, 2023 09:34

first round of reviews

07484c0

update toc

e28f8ab

k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label May 22, 2023

soltysh approved these changes May 30, 2023

View reviewed changes

k8s-ci-robot assigned soltysh May 30, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 30, 2023

varshaprasad96 reviewed May 31, 2023

View reviewed changes

keps/sig-cli/3895-kubectl-delete-interactivity/README.md Show resolved Hide resolved

k8s-ci-robot assigned johnbelamaric Jun 1, 2023

enj requested changes Jun 12, 2023

View reviewed changes

Mention tests cases and possible caveats

b2f1d5f

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 13, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 15, 2023

k8s-ci-robot assigned wojtek-t Jun 15, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 15, 2023

k8s-ci-robot merged commit dc5d562 into kubernetes:master Jun 15, 2023

k8s-ci-robot added this to the v1.28 milestone Jun 15, 2023

ardaguclu deleted the kep-3895 branch June 15, 2023 06:16

enj mentioned this pull request Oct 4, 2023

KEP-3895: Promote interactive flag in delete command to beta #4178

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KEP-3895: kubectl delete: Add interactive(-i) flag #3896

KEP-3895: kubectl delete: Add interactive(-i) flag #3896

ardaguclu commented Mar 2, 2023

ardaguclu commented Mar 7, 2023

ardaguclu commented May 22, 2023

ardaguclu commented May 29, 2023

soltysh left a comment

soltysh May 30, 2023

enj Jun 12, 2023

ardaguclu Jun 13, 2023

soltysh May 30, 2023

soltysh commented Jun 1, 2023

enj left a comment

enj Jun 12, 2023

enj Jun 12, 2023

ardaguclu Jun 13, 2023

enj Jun 13, 2023

ardaguclu Jun 13, 2023

enj Jun 12, 2023

ardaguclu Jun 13, 2023

enj Jun 13, 2023

johnbelamaric commented Jun 15, 2023

k8s-ci-robot commented Jun 15, 2023

wojtek-t commented Jun 15, 2023

	Moreover, deletion will only be performed to the resources whose `uidMap` match the ones in the preview list.
	Moreover, deletion will only be performed on the resources whose `.metadata.uid` match the ones gathered in the preview list, to ensure we remove only the resources a user explicitly agreed to.

KEP-3895: kubectl delete: Add interactive(-i) flag #3896

KEP-3895: kubectl delete: Add interactive(-i) flag #3896

Conversation

ardaguclu commented Mar 2, 2023

ardaguclu commented Mar 7, 2023

ardaguclu commented May 22, 2023

ardaguclu commented May 29, 2023

soltysh 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

soltysh commented Jun 1, 2023

enj 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

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

johnbelamaric commented Jun 15, 2023

k8s-ci-robot commented Jun 15, 2023

wojtek-t commented Jun 15, 2023