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.

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

Fixes #75706 - Show warning message when namespace is specified for deleting a cluster-scoped resource #75820

Merged
merged 1 commit into from Apr 6, 2019

Conversation

@YoubingLi
Copy link
Contributor

commented Mar 28, 2019

What type of PR is this?

/kind feature

What this PR does / why we need it:

Show warning message when namespace is specified for deleting a cluster-scoped resource

Which issue(s) this PR fixes:

Fixes # 75706

Special notes for your reviewer:
/assign @liggitt
/assign @yue9944882

Does this PR introduce a user-facing change?:

no

NONE
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Mar 28, 2019

Hi @YoubingLi. Thanks for your PR.

I'm waiting for a kubernetes 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.

func (o *DeleteOptions) Complete(f cmdutil.Factory, args []string, cmd *cobra.Command) error {
cmdNamespace, enforceNamespace, err := f.ToRawKubeConfigLoader().Namespace()
if err != nil {
return err
}

err = checkNamespaceWithArgs(f, args[0], cmd)

This comment has been minimized.

Copy link
@liggitt

liggitt Mar 28, 2019

Member

Rejecting a command that specified -n along with cluster-scoped resources is not backwards compatible. at most, we could warn at this point. It would also be much simpler to check this after you have o.Result and o.Mapper resolved (or in the delete visit loop).

In Complete():

o.WarnClusterScope = enforceNamespace

in DeleteResult():

if !warnedClusterScope && o.WarnClusterScope && info.Mapping.Scope.Name() == meta.RESTScopeNameRoot {
	warnedClusterScope = true
	fmt.Fprintf(o.ErrOut, "Warning: deleting cluster-scoped resources, not scoped to the provided namespace\n")
}

response, err := o.deleteResource(info, options)
...

that would also give us a place to require an explicit opt-in in the future to delete cluster-scoped things in combination with an explicit --namespace arg (after a suitable warning/deprecation period)

This comment has been minimized.

Copy link
@YoubingLi

YoubingLi Mar 29, 2019

Author Contributor

@liggitt

 This approach is simple, but the warning message is appear alternately.

  Meanwhile, we don't need to show warning message for "default", "kube-system", "public" namespace who is rejected by server.


 BTW, do we need to show warning message if --all-namespaces is specified in the command line?

 I prefer to show one warning message for the command and handle --all-namespaces as --namespace.

./kubectl delete namespace -n test --all
Warning: deleting cluster-scoped resources , not scoped to the provided namespace
namespace "test" deleted
Warning: deleting cluster-scoped resources , not scoped to the provided namespace
namespace "test1" deleted
Warning: deleting cluster-scoped resources , not scoped to the provided namespace
namespace "test2" deleted
Error from server (Forbidden): namespaces "default" is forbidden: this namespace may not be deleted
Error from server (Forbidden): namespaces "kube-public" is forbidden: this namespace may not be deleted
Error from server (Forbidden): namespaces "kube-system" is forbidden: this namespace may not be deleted

This comment has been minimized.

Copy link
@liggitt

liggitt Mar 29, 2019

Member

The "warnedClusterScope = true" marker could let us show the warning once per invocation

This comment has been minimized.

Copy link
@liggitt

liggitt Mar 29, 2019

Member

I could see skipping the warning if --all-namespaces was specified. Is that allowed in combination with an explicit --namespace arg?

This comment has been minimized.

Copy link
@YoubingLi

YoubingLi Mar 29, 2019

Author Contributor

-n name and --all-namespaces={true|false} is allowed to specifed together in command.

This comment has been minimized.

Copy link
@YoubingLi

YoubingLi Mar 29, 2019

Author Contributor

@liggitt @smarterclayton @yue9944882

Diff has been updated and please help review it.

@YoubingLi YoubingLi force-pushed the YoubingLi:bugfix branch from 921e946 to 50b0c01 Mar 29, 2019

@k8s-ci-robot k8s-ci-robot added size/M and removed size/L labels Mar 29, 2019

@YoubingLi YoubingLi changed the title Fixes #75706 - "--namespace" or "--all-namespaces" cannot be specifie… Fixes #75706 - Show warning message when namespace is specified for deleting a cluster-scoped resource Mar 29, 2019

@@ -149,6 +150,8 @@ func (o *DeleteOptions) Complete(f cmdutil.Factory, args []string, cmd *cobra.Co
return err
}

o.WarnClusterScope = enforceNamespace

This comment has been minimized.

Copy link
@liggitt

liggitt Mar 29, 2019

Member

o.WarnClusterScope = enforceNamespace && !o.DeleteAllNamespaces?

This comment has been minimized.

Copy link
@YoubingLi

YoubingLi Mar 29, 2019

Author Contributor

If -n / --namespace is specified, enforceNamespace would be true.

If --all-namespace is specified, I prefer not to show warning message.

This comment has been minimized.

Copy link
@YoubingLi

YoubingLi Mar 29, 2019

Author Contributor

Even "--alll" is not specified, warning message shoule be thrown when the user deletes a cluster-scoped resource with -n option

This comment has been minimized.

Copy link
@liggitt

liggitt Mar 29, 2019

Member

If --all-namespace is specified, I prefer not to show warning message.

Correct, that's what that does

pkg/kubectl/cmd/delete/delete.go Outdated Show resolved Hide resolved
pkg/kubectl/cmd/delete/delete.go Outdated Show resolved Hide resolved
pkg/kubectl/cmd/delete/delete.go Outdated Show resolved Hide resolved
test/cmd/core.sh Show resolved Hide resolved

@YoubingLi YoubingLi force-pushed the YoubingLi:bugfix branch from 50b0c01 to d9635ad Apr 1, 2019

@k8s-ci-robot k8s-ci-robot added size/S and removed size/M labels Apr 1, 2019

@YoubingLi

This comment has been minimized.

Copy link
Contributor Author

commented Apr 1, 2019

@liggitt

Diff has been updated.
pkg/kubectl/cmd/delete/delete.go Outdated Show resolved Hide resolved
test/cmd/rbac.sh Outdated Show resolved Hide resolved
test/cmd/storage.sh Outdated Show resolved Hide resolved
@SataQiu

This comment has been minimized.

Copy link
Member

commented Apr 1, 2019

/kind feature
/release-note-none

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Apr 1, 2019

@SataQiu: you can only set the release note label to release-note-none if the release-note block in the PR body text is empty or "none".

In response to this:

/kind feature
/release-note-none

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 k8s-ci-robot added kind/feature and removed needs-kind labels Apr 1, 2019

@YoubingLi YoubingLi force-pushed the YoubingLi:bugfix branch from d9635ad to f1e85f4 Apr 2, 2019

@liggitt

This comment has been minimized.

Copy link
Member

commented Apr 2, 2019

/ok-to-test

test/cmd/core.sh Outdated Show resolved Hide resolved
@YoubingLi

This comment has been minimized.

Copy link
Contributor Author

commented Apr 2, 2019

All failure cases are not related to my code change.

/retest

@liggitt

This comment has been minimized.

Copy link
Member

commented Apr 2, 2019

run_namespace_tests is failing, seems related

@YoubingLi

This comment has been minimized.

Copy link
Contributor Author

commented Apr 2, 2019

According to following log

I0402 01:37:40.255] �[32mcore.sh:1330: Successful get namespaces/my-namespace {{.metadata.name}}: my-namespace
I0402 01:37:40.510] �(B�[m+++ exit code: 1
I0402 01:37:40.516] +++ error: 1
I0402 01:37:40.553] Error when running run_namespace_tests

Seems like line 1332 failed

1329 kubectl create namespace my-namespace
1330 kube::test::get_object_assert 'namespaces/my-namespace' "{{$id_field}}" 'my-namespace'
1331 output_message=$(kubectl delete namespace -n my-namespace --all 2>&1 "${kube_flags[@]}")
1332 kube::test::if_has_string "${output_message}" 'warning: deleting cluster-scoped resources'
1333 kube::test::if_has_string "${output_message}" 'namespace "my-namespace" deleted'

@liggitt

This comment has been minimized.

Copy link
Member

commented Apr 2, 2019

line 1331 failed, because kubectl delete namespace --all never succeeds, because kube-system is not allowed to be deleted

@YoubingLi

This comment has been minimized.

Copy link
Contributor Author

commented Apr 2, 2019

Yeah. I just figured it out

==== ./kubectl delete namespace -n my-namespace --all

warning: deleting cluster-scoped resources, not scoped to the provided namespace
namespace "my-namespace" deleted
Error from server (Forbidden): namespaces "default" is forbidden: this namespace may not be deleted
Error from server (Forbidden): namespaces "kube-public" is forbidden: this namespace may not be deleted
Error from server (Forbidden): namespaces "kube-system" is forbidden: this namespace may not be deleted

===== echo $?
1

@YoubingLi YoubingLi force-pushed the YoubingLi:bugfix branch from f1e85f4 to c438d7b Apr 2, 2019

@YoubingLi

This comment has been minimized.

Copy link
Contributor Author

commented Apr 3, 2019

@liggitt @yue9944882 @smarterclayton

All test are passed
@liggitt

This comment has been minimized.

Copy link
Member

commented Apr 6, 2019

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm label Apr 6, 2019

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, YoubingLi

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:

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

@k8s-ci-robot k8s-ci-robot merged commit 9c2df99 into kubernetes:master Apr 6, 2019

17 checks passed

cla/linuxfoundation YoubingLi authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details
@YoubingLi YoubingLi referenced this pull request Jul 4, 2019
6 of 6 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.