From f3b49adadf2182ba0a7fb930e6340da725053520 Mon Sep 17 00:00:00 2001 From: Paco Xu Date: Wed, 9 Nov 2022 07:43:03 +0800 Subject: [PATCH 1/2] kubectl apply: warning that in future release, prune will ignores no-namespaced resource if -n is not empty Signed-off-by: Paco Xu --- .../src/k8s.io/kubectl/pkg/cmd/apply/prune.go | 3 +- .../src/k8s.io/kubectl/pkg/cmd/diff/diff.go | 2 +- .../src/k8s.io/kubectl/pkg/cmd/diff/prune.go | 4 +- .../k8s.io/kubectl/pkg/util/prune/prune.go | 52 ++++++++++++------- .../kubectl/pkg/util/prune/prune_test.go | 41 ++++++++++++--- 5 files changed, 71 insertions(+), 31 deletions(-) diff --git a/staging/src/k8s.io/kubectl/pkg/cmd/apply/prune.go b/staging/src/k8s.io/kubectl/pkg/cmd/apply/prune.go index 04ce54e5685..8c07d32b82e 100644 --- a/staging/src/k8s.io/kubectl/pkg/cmd/apply/prune.go +++ b/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) diff --git a/staging/src/k8s.io/kubectl/pkg/cmd/diff/diff.go b/staging/src/k8s.io/kubectl/pkg/cmd/diff/diff.go index 82cf5133f4f..0ba5b13f32e 100644 --- a/staging/src/k8s.io/kubectl/pkg/cmd/diff/diff.go +++ b/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) } diff --git a/staging/src/k8s.io/kubectl/pkg/cmd/diff/prune.go b/staging/src/k8s.io/kubectl/pkg/cmd/diff/prune.go index 64a6d4fee36..ffb37a2e571 100644 --- a/staging/src/k8s.io/kubectl/pkg/cmd/diff/prune.go +++ b/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) } diff --git a/staging/src/k8s.io/kubectl/pkg/util/prune/prune.go b/staging/src/k8s.io/kubectl/pkg/util/prune/prune.go index 0d49153fe4c..5b195632fe9 100644 --- a/staging/src/k8s.io/kubectl/pkg/util/prune/prune.go +++ b/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.") } } diff --git a/staging/src/k8s.io/kubectl/pkg/util/prune/prune_test.go b/staging/src/k8s.io/kubectl/pkg/util/prune/prune_test.go index cbc389b4af0..fba46ca412f 100644 --- a/staging/src/k8s.io/kubectl/pkg/util/prune/prune_test.go +++ b/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{}, + 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") } From 7263aba71bfdd2286200ffd7ef79766950c5fcd9 Mon Sep 17 00:00:00 2001 From: Paco Xu Date: Wed, 9 Nov 2022 07:43:43 +0800 Subject: [PATCH 2/2] kubectl-apply: add prune test cases for non-namespaced resources Signed-off-by: Paco Xu --- .../kubectl/pkg/cmd/apply/apply_test.go | 58 ++++++++++++++++--- .../kubectl/pkg/util/prune/prune_test.go | 2 +- 2 files changed, 52 insertions(+), 8 deletions(-) diff --git a/staging/src/k8s.io/kubectl/pkg/cmd/apply/apply_test.go b/staging/src/k8s.io/kubectl/pkg/cmd/apply/apply_test.go index 5d433177295..325e6dd2e29 100644 --- a/staging/src/k8s.io/kubectl/pkg/cmd/apply/apply_test.go +++ b/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) diff --git a/staging/src/k8s.io/kubectl/pkg/util/prune/prune_test.go b/staging/src/k8s.io/kubectl/pkg/util/prune/prune_test.go index fba46ca412f..a22afc03d99 100644 --- a/staging/src/k8s.io/kubectl/pkg/util/prune/prune_test.go +++ b/staging/src/k8s.io/kubectl/pkg/util/prune/prune_test.go @@ -67,7 +67,7 @@ func TestGetRESTMappings(t *testing.T) { mapper: &testRESTMapper{}, pr: []Resource{}, namespaceSpecified: true, - expectedns: 14, + 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,