Update prune's behaviour for namespaces

[Affan-7]

What type of PR is this?

/kind bug
/kind deprecation

What this PR does / why we need it:

This PR changes the behaviour of --prune flag of kubectl apply.

Previously if you use --prune with a namespace flag like -n sandbox it also prunes other non-namespaced items as described in #110905.

Run kubectl apply --prune --dry-run=server --all -n sandbox -f a.yaml, this results in
service/nginx created (dry run)
namespace/cattle-system pruned (dry run)

After this PR, the --prune flag no longer prunes non-namespaced items if used with a namesapce falg like -n sandbox.

Run kubectl apply --prune --dry-run=server --all -n sandbox -f a.yaml, this results in
service/nginx created (dry run)

(It still prunes the non-namespaced items if namespace is not sepcifed )

Run kubectl apply --prune --dry-run=server --all -f b.yaml, this results in
service/nginx created (dry run)
namespace/cattle-system pruned (dry run)

Which issue(s) this PR fixes:

Fixes #110905

Does this PR introduce a user-facing change?

Action Required: kubectl apply will no longer prune non-namespaced resources by default when used with the --namespace flag. To preserve the current behaviour, list the resources you want to target explicitly in the --prune-allowlist flag.

[k8s-ci-robot]

Please note that we're already in Test Freeze for the release-1.28 branch. This means every merged PR will be automatically fast-forwarded via the periodic ci-fast-forward job to the release branch of the upcoming v1.28.0 release.

Fast forwards are scheduled to happen every 6 hours, whereas the most recent run was: Mon Jul 31 10:15:47 UTC 2023.

[k8s-ci-robot]

Welcome @Affan-7!

It looks like this is your first PR to kubernetes/kubernetes 🎉. 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/kubernetes 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]

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

[Affan-7]

/auto-cc

[Affan-7]

/cc pacoxu

[pacoxu]

/ok-to-test
/easycla
/check-cla

[pacoxu]

/assign @KnVerey @brianpursley

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Affan-7
Once this PR has been reviewed and has the lgtm label, please ask for approval from brianpursley. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

• staging/src/k8s.io/kubectl/OWNERS

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

[Affan-7]

/test pull-kubernetes-unit

[pacoxu]

This test case should be updated as well.

[Affan-7]

I think it should prune the non-namespaced items when a namespace and a allow list is not specified.

It's interesting that this test case is failing, but this test under the prune_test.go is passing.

{
	mapper:             &testRESTMapper{},
	pr:                 []Resource{},
	namespaceSpecified: false,
	expectedns:         14,
	expectednns:        2,
	expectederr:        nil,
},

[pacoxu]

It's interesting that this test case is failing, but this test under the prune_test.go is passing.

This case should pass.

• If the namespace is not specified, default 14 namespace resources and 2 none-namespaced resources will be pruned.
• If the namespace is specified, only the default 14 namespace resources will be pruned.

[Affan-7]

This case should pass.

• If the namespace is not specified, default 14 namespace resources and 2 none-namespaced resources will be pruned.
• If the namespace is specified, only the default 14 namespace resources will be pruned.

Yes that's true, but this test under the apply_test.go is failing.

=== FAIL: vendor/k8s.io/kubectl/pkg/cmd/apply TestApplyPruneObjectsWithAllowlist/prune_without_namespace_and_allowlist_should_delete_resources_that_are_not_in_the_specified_file (0.03s)
    apply_test.go:1040: expected output to contain "namespace/test-apply pruned", but it did not. Actual Output:
        replicationcontroller/test-rc unchanged
        configmap/test-cm pruned
        replicationcontroller/test-rc2 pruned

It should have pruned the namesapce.

[pacoxu]

Sorry, after investigation, I think we have made a mistake here.

kubernetes/staging/src/k8s.io/kubectl/pkg/cmd/apply/apply.go

Lines 462 to 463 in 3c4a813

	NamespaceParam(o.Namespace).DefaultNamespace().
	FilenameParam(o.EnforceNamespace, &o.DeleteOptions.FilenameOptions).

• the namespace will default to default if not set.

kubernetes/staging/src/k8s.io/kubectl/pkg/cmd/apply/prune.go

Lines 74 to 75 in 3c4a813

	namespacedRESTMappings, nonNamespacedRESTMappings, err := prune.GetRESTMappings(o.Mapper, o.PruneResources, o.Namespace != "")
	if err != nil {

namespaceSpecified o.Namespace != "" will always be true.

The warning message will be there for everyone(no matter users specified namespace for the kubectl apply --prune) if I understand correctly.

[pacoxu]

/hold

[Affan-7]

I wonder what should we do now?

[mashetty330]

@pacoxu I was referring to the original issue and I ended up in this PR. where does the validation for the namespace will default to default if not set. happens. I dont see it in DefaultNamespace() IIUC. Could you please provide the pointer to that

[pacoxu]

/priority important-soon
/triage accepted
As there is an action required release note here, we should take care of this before merging.

[Affan-7]

/remove-hold

[brianpursley]

Is anything else needed for this PR?

I took a look and it looks OK to me, but I'd like to see what @seans3 or @soltysh thinks about it since they are more familiar than I am with prune.

It could cause a problem if someone relies on the old behavior, but it has gone through the deprecation period with a warning, so it should be ok(?).

[k8s-triage-robot]

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

This bot triages 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 PR is closed

You can:

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

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

/lifecycle stale

[Affan-7]

/remove-lifecycle stale

[Affan-7]

I don't think that this PR is producing much impact, closing this.