Merge pull request #110907 from pacoxu/kubectl-apply

kubectl apply: warning that kubectl will ignores no-namespaced resource in future release with namespace specified and with default pruneAllowlist

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

@@ -760,6 +760,22 @@ func TestApplyPruneObjectsWithAllowlist(t *testing.T) {
 		t.Fatal(err)
 	}
 
+	// Create Namespace that can be pruned
+	ns := &unstructured.Unstructured{
+		Object: map[string]interface{}{
+			"kind":       "Namespace",
+			"apiVersion": "v1",
+			"metadata": map[string]interface{}{
+				"name": "test-apply",
+				"uid":  "uid-ns",
+			},
+		},
+	}
+	err = setLastAppliedConfigAnnotation(ns)
+	if err != nil {
+		t.Fatal(err)
+	}
+
 	// Create a ConfigMap without a UID. Resources without a UID will not be pruned.
 	cmNoUID := &unstructured.Unstructured{
 		Object: map[string]interface{}{
@@ -788,28 +804,53 @@ func TestApplyPruneObjectsWithAllowlist(t *testing.T) {
 			},
 		},
 	}
-	if err != nil {
-		t.Fatal(err)
-	}
 
 	testCases := map[string]struct {
 		currentResources        []runtime.Object
 		pruneAllowlist          []string
+		namespace               string
 		expectedPrunedResources []string
 		expectedOutputs         []string
 	}{
-		"prune without allowlist should delete resources that are not in the specified file": {
-			currentResources:        []runtime.Object{rc, rc2, cm},
-			expectedPrunedResources: []string{"test/test-cm", "test/test-rc2"},
+		"prune without namespace and allowlist should delete resources that are not in the specified file": {
+			currentResources:        []runtime.Object{rc, rc2, cm, ns},
+			expectedPrunedResources: []string{"test/test-cm", "test/test-rc2", "/test-apply"},
 			expectedOutputs: []string{
 				"replicationcontroller/test-rc unchanged",
 				"configmap/test-cm pruned",
 				"replicationcontroller/test-rc2 pruned",
+				"namespace/test-apply pruned",
+			},
+		},
+		// Deprecated: kubectl apply will no longer prune non-namespaced resources by default when used with the --namespace flag in a future release
+		// namespace is a non-namespaced resource and will not be pruned in the future
+		"prune with namespace and without allowlist should delete resources that are not in the specified file": {
+			currentResources:        []runtime.Object{rc, rc2, cm, ns},
+			namespace:               "test",
+			expectedPrunedResources: []string{"test/test-cm", "test/test-rc2", "/test-apply"},
+			expectedOutputs: []string{
+				"replicationcontroller/test-rc unchanged",
+				"configmap/test-cm pruned",
+				"replicationcontroller/test-rc2 pruned",
+				"namespace/test-apply pruned",
+			},
+		},
+		// Even namespace is a non-namespaced resource, it will be pruned if specified in pruneAllowList in the future
+		"prune with namespace and allowlist should delete all matching resources": {
+			currentResources:        []runtime.Object{rc, cm, ns},
+			pruneAllowlist:          []string{"core/v1/ConfigMap", "core/v1/Namespace"},
+			namespace:               "test",
+			expectedPrunedResources: []string{"test/test-cm", "/test-apply"},
+			expectedOutputs: []string{
+				"replicationcontroller/test-rc unchanged",
+				"configmap/test-cm pruned",
+				"namespace/test-apply pruned",
 			},
 		},
 		"prune with allowlist should delete only matching resources": {
 			currentResources:        []runtime.Object{rc, rc2, cm},
 			pruneAllowlist:          []string{"core/v1/ConfigMap"},
+			namespace:               "test",
 			expectedPrunedResources: []string{"test/test-cm"},
 			expectedOutputs: []string{
 				"replicationcontroller/test-rc unchanged",
@@ -819,6 +860,7 @@ func TestApplyPruneObjectsWithAllowlist(t *testing.T) {
 		"prune with allowlist specifying the same resource type multiple times should not fail": {
 			currentResources:        []runtime.Object{rc, rc2, cm},
 			pruneAllowlist:          []string{"core/v1/ConfigMap", "core/v1/ConfigMap"},
+			namespace:               "test",
 			expectedPrunedResources: []string{"test/test-cm"},
 			expectedOutputs: []string{
 				"replicationcontroller/test-rc unchanged",
@@ -828,6 +870,7 @@ func TestApplyPruneObjectsWithAllowlist(t *testing.T) {
 		"prune with allowlist should not delete resources that exist in the specified file": {
 			currentResources:        []runtime.Object{rc, rc2, cm},
 			pruneAllowlist:          []string{"core/v1/ReplicationController"},
+			namespace:               "test",
 			expectedPrunedResources: []string{"test/test-rc2"},
 			expectedOutputs: []string{
 				"replicationcontroller/test-rc unchanged",
@@ -837,6 +880,7 @@ func TestApplyPruneObjectsWithAllowlist(t *testing.T) {
 		"prune with allowlist specifying multiple resource types should delete matching resources": {
 			currentResources:        []runtime.Object{rc, rc2, cm},
 			pruneAllowlist:          []string{"core/v1/ConfigMap", "core/v1/ReplicationController"},
+			namespace:               "test",
 			expectedPrunedResources: []string{"test/test-cm", "test/test-rc2"},
 			expectedOutputs: []string{
 				"replicationcontroller/test-rc unchanged",
@@ -900,7 +944,7 @@ func TestApplyPruneObjectsWithAllowlist(t *testing.T) {
 				cmd := NewCmdApply("kubectl", tf, ioStreams)
 				cmd.Flags().Set("filename", filenameRC)
 				cmd.Flags().Set("prune", "true")
-				cmd.Flags().Set("namespace", "test")
+				cmd.Flags().Set("namespace", tc.namespace)
 				cmd.Flags().Set("all", "true")
 				for _, allow := range tc.pruneAllowlist {
 					cmd.Flags().Set("prune-allowlist", allow)

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

@@ -70,7 +70,7 @@ func newPruner(o *ApplyOptions) pruner {
 
 func (p *pruner) pruneAll(o *ApplyOptions) error {
 
-	namespacedRESTMappings, nonNamespacedRESTMappings, err := prune.GetRESTMappings(o.Mapper, o.PruneResources)
+	namespacedRESTMappings, nonNamespacedRESTMappings, err := prune.GetRESTMappings(o.Mapper, o.PruneResources, o.Namespace != "")
 	if err != nil {
 		return fmt.Errorf("error retrieving RESTMappings to prune: %v", err)
 	}
@@ -82,6 +82,7 @@ func (p *pruner) pruneAll(o *ApplyOptions) error {
 			}
 		}
 	}
+
 	for _, m := range nonNamespacedRESTMappings {
 		if err := p.prune(metav1.NamespaceNone, m); err != nil {
 			return fmt.Errorf("error pruning nonNamespaced object %v: %v", m.GroupVersionKind, err)

staging/src/k8s.io/kubectl/pkg/cmd/diff/diff.go

@@ -747,7 +747,7 @@ func (o *DiffOptions) Run() error {
 	})
 
 	if o.pruner != nil {
-		prunedObjs, err := o.pruner.pruneAll()
+		prunedObjs, err := o.pruner.pruneAll(o.CmdNamespace != "")
 		if err != nil {
 			klog.Warningf("pruning failed and could not be evaluated err: %v", err)
 		}

staging/src/k8s.io/kubectl/pkg/cmd/diff/prune.go

@@ -50,9 +50,9 @@ func newPruner(dc dynamic.Interface, m meta.RESTMapper, r []prune.Resource) *pru
 	}
 }
 
-func (p *pruner) pruneAll() ([]runtime.Object, error) {
+func (p *pruner) pruneAll(namespaceSpecified bool) ([]runtime.Object, error) {
 	var allPruned []runtime.Object
-	namespacedRESTMappings, nonNamespacedRESTMappings, err := prune.GetRESTMappings(p.mapper, p.resources)
+	namespacedRESTMappings, nonNamespacedRESTMappings, err := prune.GetRESTMappings(p.mapper, p.resources, namespaceSpecified)
 	if err != nil {
 		return allPruned, fmt.Errorf("error retrieving RESTMappings to prune: %v", err)
 	}

staging/src/k8s.io/kubectl/pkg/util/prune/prune.go

@@ -22,8 +22,33 @@ import (
 
 	"k8s.io/apimachinery/pkg/api/meta"
 	"k8s.io/apimachinery/pkg/runtime/schema"
+	"k8s.io/klog/v2"
 )
 
+// default allowlist of namespaced resources
+var defaultNamespacedPruneResources = []Resource{
+	{"", "v1", "ConfigMap", true},
+	{"", "v1", "Endpoints", true},
+	{"", "v1", "PersistentVolumeClaim", true},
+	{"", "v1", "Pod", true},
+	{"", "v1", "ReplicationController", true},
+	{"", "v1", "Secret", true},
+	{"", "v1", "Service", true},
+	{"batch", "v1", "Job", true},
+	{"batch", "v1", "CronJob", true},
+	{"networking.k8s.io", "v1", "Ingress", true},
+	{"apps", "v1", "DaemonSet", true},
+	{"apps", "v1", "Deployment", true},
+	{"apps", "v1", "ReplicaSet", true},
+	{"apps", "v1", "StatefulSet", true},
+}
+
+// default allowlist of non-namespaced resources
+var defaultNonNamespacedPruneResources = []Resource{
+	{"", "v1", "Namespace", false},
+	{"", "v1", "PersistentVolume", false},
+}
+
 type Resource struct {
 	group      string
 	version    string
@@ -35,26 +60,15 @@ func (pr Resource) String() string {
 	return fmt.Sprintf("%v/%v, Kind=%v, Namespaced=%v", pr.group, pr.version, pr.kind, pr.namespaced)
 }
 
-func GetRESTMappings(mapper meta.RESTMapper, pruneResources []Resource) (namespaced, nonNamespaced []*meta.RESTMapping, err error) {
+// if namespace is explicitly specified, the default allow list should not include non-namespaced resources.
+// if pruneResources is specified by user, respect the user setting.
+func GetRESTMappings(mapper meta.RESTMapper, pruneResources []Resource, namespaceSpecified bool) (namespaced, nonNamespaced []*meta.RESTMapping, err error) {
 	if len(pruneResources) == 0 {
-		// default allowlist
-		pruneResources = []Resource{
-			{"", "v1", "ConfigMap", true},
-			{"", "v1", "Endpoints", true},
-			{"", "v1", "Namespace", false},
-			{"", "v1", "PersistentVolumeClaim", true},
-			{"", "v1", "PersistentVolume", false},
-			{"", "v1", "Pod", true},
-			{"", "v1", "ReplicationController", true},
-			{"", "v1", "Secret", true},
-			{"", "v1", "Service", true},
-			{"batch", "v1", "Job", true},
-			{"batch", "v1", "CronJob", true},
-			{"networking.k8s.io", "v1", "Ingress", true},
-			{"apps", "v1", "DaemonSet", true},
-			{"apps", "v1", "Deployment", true},
-			{"apps", "v1", "ReplicaSet", true},
-			{"apps", "v1", "StatefulSet", true},
+		pruneResources = defaultNamespacedPruneResources
+		// TODO in kubectl v1.29, add back non-namespaced resource only if namespace is not specified
+		pruneResources = append(pruneResources, defaultNonNamespacedPruneResources...)
+		if namespaceSpecified {
+			klog.Warning("Deprecated: kubectl apply will no longer prune non-namespaced resources by default when used with the --namespace flag in a future release. To preserve the current behaviour, list the resources you want to target explicitly in the --prune-allowlist flag.")
 		}
 	}
 

staging/src/k8s.io/kubectl/pkg/util/prune/prune_test.go

@@ -48,23 +48,48 @@ func (m *testRESTMapper) RESTMapping(gk schema.GroupKind, versions ...string) (*
 
 func TestGetRESTMappings(t *testing.T) {
 	tests := []struct {
-		mapper      *testRESTMapper
-		pr          []Resource
-		expectedns  int
-		expectednns int
-		expectederr error
+		mapper             *testRESTMapper
+		pr                 []Resource
+		namespaceSpecified bool
+		expectedns         int
+		expectednns        int
+		expectederr        error
 	}{
 		{
-			mapper:      &testRESTMapper{},
-			pr:          []Resource{},
-			expectedns:  14,
+			mapper:             &testRESTMapper{},
+			pr:                 []Resource{},
+			namespaceSpecified: false,
+			expectedns:         14,
+			expectednns:        2,
+			expectederr:        nil,
+		},
+		{
+			mapper:             &testRESTMapper{},
+			pr:                 []Resource{},
+			namespaceSpecified: true,
+			expectedns:         14,
+			// it will be 0 non-namespaced resources after the deprecation period has passed.
+			// for details, refer to https://github.com/kubernetes/kubernetes/pull/110907/.
 			expectednns: 2,
 			expectederr: nil,
 		},
+		{
+			mapper: &testRESTMapper{},
+			pr: []Resource{
+				{"apps", "v1", "DaemonSet", true},
+				{"core", "v1", "Pod", true},
+				{"", "v1", "Foo2", false},
+				{"foo", "v1", "Foo3", false},
+			},
+			namespaceSpecified: false,
+			expectedns:         2,
+			expectednns:        2,
+			expectederr:        nil,
+		},
 	}
 
 	for _, tc := range tests {
-		actualns, actualnns, actualerr := GetRESTMappings(tc.mapper, tc.pr)
+		actualns, actualnns, actualerr := GetRESTMappings(tc.mapper, tc.pr, tc.namespaceSpecified)
 		if tc.expectederr != nil {
 			assert.NotEmptyf(t, actualerr, "getRESTMappings error expected but not fired")
 		}