From 41407b14e82987afe1b70b27d6ccc14f61c6593a Mon Sep 17 00:00:00 2001 From: Brian Pursley Date: Sun, 30 Oct 2022 20:50:19 -0400 Subject: [PATCH] kubectl apply: Deprecate --prune-whitelist in favor of --prune-allowlist Changes in kubectl apply --prune to support k8s Inclusive Naming Initiative: * Deprecated the --prune-whitelist flag. * Deprecated the PruneWhitelist field on ApplyFlags struct. * Removed PruneWhitelist field (not used anywhere) from ApplyOptions struct. * Added --prune-allowlist flag. * Added PruneAllowlist field on ApplyFlags struct. * Added unit tests for prune with allowlist This commit also fixes a bug where the command would fail if you specified the sameGVK multiple times for --allow-whitelist. Now it only attempts to prune the unique set of allowed GVKs. Kubernetes-commit: f7ebf4d8852d4500f24100ca9a4ca665efc1fada --- pkg/cmd/apply/apply.go | 17 ++- pkg/cmd/apply/apply_test.go | 246 +++++++++++++++++++++++++++++++++++ pkg/util/slice/slice.go | 19 +++ pkg/util/slice/slice_test.go | 42 ++++++ 4 files changed, 318 insertions(+), 6 deletions(-) diff --git a/pkg/cmd/apply/apply.go b/pkg/cmd/apply/apply.go index 6aa846f3..e951194a 100644 --- a/pkg/cmd/apply/apply.go +++ b/pkg/cmd/apply/apply.go @@ -46,6 +46,7 @@ import ( "k8s.io/kubectl/pkg/util/i18n" "k8s.io/kubectl/pkg/util/openapi" "k8s.io/kubectl/pkg/util/prune" + "k8s.io/kubectl/pkg/util/slice" "k8s.io/kubectl/pkg/util/templates" "k8s.io/kubectl/pkg/validation" ) @@ -68,7 +69,10 @@ type ApplyFlags struct { All bool Overwrite bool OpenAPIPatch bool - PruneWhitelist []string + + // DEPRECATED: Use PruneAllowlist instead + PruneWhitelist []string // TODO: Remove this in kubectl 1.28 or later + PruneAllowlist []string genericclioptions.IOStreams } @@ -95,7 +99,6 @@ type ApplyOptions struct { All bool Overwrite bool OpenAPIPatch bool - PruneWhitelist []string ValidationDirective string Validator validation.Schema @@ -161,7 +164,7 @@ var ( kubectl apply --prune -f manifest.yaml -l app=nginx # Apply the configuration in manifest.yaml and delete all the other config maps that are not in the file - kubectl apply --prune -f manifest.yaml --all --prune-whitelist=core/v1/ConfigMap`)) + kubectl apply --prune -f manifest.yaml --all --prune-allowlist=core/v1/ConfigMap`)) warningNoLastAppliedConfigAnnotation = "Warning: resource %[1]s is missing the %[2]s annotation which is required by %[3]s apply. %[3]s apply should only be used on resources created declaratively by either %[3]s create --save-config or %[3]s apply. The missing annotation will be patched automatically.\n" warningChangesOnDeletingResource = "Warning: Detected changes to resource %[1]s which is currently being deleted.\n" @@ -229,7 +232,9 @@ func (flags *ApplyFlags) AddFlags(cmd *cobra.Command) { cmd.Flags().BoolVar(&flags.Overwrite, "overwrite", flags.Overwrite, "Automatically resolve conflicts between the modified and live configuration by using values from the modified configuration") cmd.Flags().BoolVar(&flags.Prune, "prune", flags.Prune, "Automatically delete resource objects, that do not appear in the configs and are created by either apply or create --save-config. Should be used with either -l or --all.") cmd.Flags().BoolVar(&flags.All, "all", flags.All, "Select all resources in the namespace of the specified resource types.") - cmd.Flags().StringArrayVar(&flags.PruneWhitelist, "prune-whitelist", flags.PruneWhitelist, "Overwrite the default whitelist with for --prune") + cmd.Flags().StringArrayVar(&flags.PruneAllowlist, "prune-allowlist", flags.PruneAllowlist, "Overwrite the default allowlist with for --prune") + cmd.Flags().StringArrayVar(&flags.PruneWhitelist, "prune-whitelist", flags.PruneWhitelist, "Overwrite the default whitelist with for --prune") // TODO: Remove this in kubectl 1.28 or later + cmd.Flags().MarkDeprecated("prune-whitelist", "Use --prune-allowlist instead.") cmd.Flags().BoolVar(&flags.OpenAPIPatch, "openapi-patch", flags.OpenAPIPatch, "If true, use openapi to calculate diff when the openapi presents and the resource can be found in the openapi spec. Otherwise, fall back to use baked-in types.") } @@ -300,7 +305,8 @@ func (flags *ApplyFlags) ToOptions(cmd *cobra.Command, baseName string, args []s } if flags.Prune { - flags.PruneResources, err = prune.ParseResources(mapper, flags.PruneWhitelist) + pruneAllowlist := slice.ToSet(flags.PruneAllowlist, flags.PruneWhitelist) + flags.PruneResources, err = prune.ParseResources(mapper, pruneAllowlist) if err != nil { return nil, err } @@ -326,7 +332,6 @@ func (flags *ApplyFlags) ToOptions(cmd *cobra.Command, baseName string, args []s All: flags.All, Overwrite: flags.Overwrite, OpenAPIPatch: flags.OpenAPIPatch, - PruneWhitelist: flags.PruneWhitelist, Recorder: recorder, Namespace: namespace, diff --git a/pkg/cmd/apply/apply_test.go b/pkg/cmd/apply/apply_test.go index d426f524..5d433177 100644 --- a/pkg/cmd/apply/apply_test.go +++ b/pkg/cmd/apply/apply_test.go @@ -49,12 +49,14 @@ import ( dynamicfakeclient "k8s.io/client-go/dynamic/fake" restclient "k8s.io/client-go/rest" "k8s.io/client-go/rest/fake" + testing2 "k8s.io/client-go/testing" "k8s.io/client-go/util/csaupgrade" cmdtesting "k8s.io/kubectl/pkg/cmd/testing" cmdutil "k8s.io/kubectl/pkg/cmd/util" "k8s.io/kubectl/pkg/scheme" "k8s.io/kubectl/pkg/util/openapi" utilpointer "k8s.io/utils/pointer" + "k8s.io/utils/strings/slices" ) var ( @@ -714,6 +716,250 @@ func TestApplyPruneObjects(t *testing.T) { } } +func TestApplyPruneObjectsWithAllowlist(t *testing.T) { + cmdtesting.InitTestErrorHandler(t) + + // Read ReplicationController from the file we will use to apply. This one will not be pruned because it exists in the file. + rc := readUnstructuredFromFile(t, filenameRC) + err := setLastAppliedConfigAnnotation(rc) + if err != nil { + t.Fatal(err) + } + + // Create another ReplicationController that can be pruned + rc2 := &unstructured.Unstructured{ + Object: map[string]interface{}{ + "kind": "ReplicationController", + "apiVersion": "v1", + "metadata": map[string]interface{}{ + "name": "test-rc2", + "namespace": "test", + "uid": "uid-rc2", + }, + }, + } + err = setLastAppliedConfigAnnotation(rc2) + if err != nil { + t.Fatal(err) + } + + // Create a ConfigMap that can be pruned + cm := &unstructured.Unstructured{ + Object: map[string]interface{}{ + "kind": "ConfigMap", + "apiVersion": "v1", + "metadata": map[string]interface{}{ + "name": "test-cm", + "namespace": "test", + "uid": "uid-cm", + }, + }, + } + err = setLastAppliedConfigAnnotation(cm) + 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{}{ + "kind": "ConfigMap", + "apiVersion": "v1", + "metadata": map[string]interface{}{ + "name": "test-cm-nouid", + "namespace": "test", + }, + }, + } + err = setLastAppliedConfigAnnotation(cmNoUID) + if err != nil { + t.Fatal(err) + } + + // Create a ConfigMap without a last applied annotation. Resources without a last applied annotation will not be pruned. + cmNoLastApplied := &unstructured.Unstructured{ + Object: map[string]interface{}{ + "kind": "ConfigMap", + "apiVersion": "v1", + "metadata": map[string]interface{}{ + "name": "test-cm-nolastapplied", + "namespace": "test", + "uid": "uid-cm-nolastapplied", + }, + }, + } + if err != nil { + t.Fatal(err) + } + + testCases := map[string]struct { + currentResources []runtime.Object + pruneAllowlist []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"}, + expectedOutputs: []string{ + "replicationcontroller/test-rc unchanged", + "configmap/test-cm pruned", + "replicationcontroller/test-rc2 pruned", + }, + }, + "prune with allowlist should delete only matching resources": { + currentResources: []runtime.Object{rc, rc2, cm}, + pruneAllowlist: []string{"core/v1/ConfigMap"}, + expectedPrunedResources: []string{"test/test-cm"}, + expectedOutputs: []string{ + "replicationcontroller/test-rc unchanged", + "configmap/test-cm pruned", + }, + }, + "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"}, + expectedPrunedResources: []string{"test/test-cm"}, + expectedOutputs: []string{ + "replicationcontroller/test-rc unchanged", + "configmap/test-cm pruned", + }, + }, + "prune with allowlist should not delete resources that exist in the specified file": { + currentResources: []runtime.Object{rc, rc2, cm}, + pruneAllowlist: []string{"core/v1/ReplicationController"}, + expectedPrunedResources: []string{"test/test-rc2"}, + expectedOutputs: []string{ + "replicationcontroller/test-rc unchanged", + "replicationcontroller/test-rc2 pruned", + }, + }, + "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"}, + expectedPrunedResources: []string{"test/test-cm", "test/test-rc2"}, + expectedOutputs: []string{ + "replicationcontroller/test-rc unchanged", + "configmap/test-cm pruned", + "replicationcontroller/test-rc2 pruned", + }, + }, + "prune should not delete resources that are missing a UID": { + currentResources: []runtime.Object{rc, cm, cmNoUID}, + expectedPrunedResources: []string{"test/test-cm"}, + expectedOutputs: []string{ + "replicationcontroller/test-rc unchanged", + "configmap/test-cm pruned", + }, + }, + "prune should not delete resources that are missing the last applied config annotation": { + currentResources: []runtime.Object{rc, cm, cmNoLastApplied}, + expectedPrunedResources: []string{"test/test-cm"}, + expectedOutputs: []string{ + "replicationcontroller/test-rc unchanged", + "configmap/test-cm pruned", + }, + }, + } + + for testCaseName, tc := range testCases { + for _, testingOpenAPISchema := range testingOpenAPISchemas { + t.Run(testCaseName, func(t *testing.T) { + tf := cmdtesting.NewTestFactory().WithNamespace("test") + defer tf.Cleanup() + + tf.UnstructuredClient = &fake.RESTClient{ + NegotiatedSerializer: resource.UnstructuredPlusDefaultContentConfig().NegotiatedSerializer, + Client: fake.CreateHTTPClient(func(req *http.Request) (*http.Response, error) { + switch p, m := req.URL.Path, req.Method; { + case p == "/namespaces/test/replicationcontrollers/test-rc" && m == "GET": + encoded := runtime.EncodeOrDie(unstructured.UnstructuredJSONScheme, rc) + bodyRC := io.NopCloser(strings.NewReader(encoded)) + return &http.Response{StatusCode: http.StatusOK, Header: cmdtesting.DefaultHeader(), Body: bodyRC}, nil + case p == "/namespaces/test/replicationcontrollers/test-rc" && m == "PATCH": + encoded := runtime.EncodeOrDie(unstructured.UnstructuredJSONScheme, rc) + bodyRC := io.NopCloser(strings.NewReader(encoded)) + return &http.Response{StatusCode: http.StatusOK, Header: cmdtesting.DefaultHeader(), Body: bodyRC}, nil + default: + t.Fatalf("unexpected request: %#v\n%#v", req.URL, req) + return nil, nil + } + }), + } + tf.OpenAPISchemaFunc = testingOpenAPISchema.OpenAPISchemaFn + tf.FakeOpenAPIGetter = testingOpenAPISchema.OpenAPIGetter + tf.ClientConfigVal = cmdtesting.DefaultClientConfig() + + for _, resource := range tc.currentResources { + if err := tf.FakeDynamicClient.Tracker().Add(resource); err != nil { + t.Fatal(err) + } + } + + ioStreams, _, buf, errBuf := genericclioptions.NewTestIOStreams() + cmd := NewCmdApply("kubectl", tf, ioStreams) + cmd.Flags().Set("filename", filenameRC) + cmd.Flags().Set("prune", "true") + cmd.Flags().Set("namespace", "test") + cmd.Flags().Set("all", "true") + for _, allow := range tc.pruneAllowlist { + cmd.Flags().Set("prune-allowlist", allow) + } + cmd.Run(cmd, []string{}) + + if errBuf.String() != "" { + t.Fatalf("unexpected error output: %s", errBuf.String()) + } + + actualOutput := buf.String() + for _, expectedOutput := range tc.expectedOutputs { + if !strings.Contains(actualOutput, expectedOutput) { + t.Fatalf("expected output to contain %q, but it did not. Actual Output:\n%s", expectedOutput, actualOutput) + } + } + + var prunedResources []string + for _, action := range tf.FakeDynamicClient.Actions() { + if action.GetVerb() == "delete" { + deleteAction := action.(testing2.DeleteAction) + prunedResources = append(prunedResources, deleteAction.GetNamespace()+"/"+deleteAction.GetName()) + } + } + + // Make sure nothing unexpected was pruned + for _, resource := range prunedResources { + if !slices.Contains(tc.expectedPrunedResources, resource) { + t.Fatalf("expected %s not to be pruned, but it was", resource) + } + } + + // Make sure everything that was expected to be pruned was pruned + for _, resource := range tc.expectedPrunedResources { + if !slices.Contains(prunedResources, resource) { + t.Fatalf("expected %s to be pruned, but it was not", resource) + } + } + + }) + } + } +} + +func setLastAppliedConfigAnnotation(obj runtime.Object) error { + accessor, err := meta.Accessor(obj) + if err != nil { + return err + } + annotations := accessor.GetAnnotations() + if annotations == nil { + annotations = make(map[string]string) + accessor.SetAnnotations(annotations) + } + annotations[corev1.LastAppliedConfigAnnotation] = runtime.EncodeOrDie(unstructured.NewJSONFallbackEncoder(codec), obj) + accessor.SetAnnotations(annotations) + return nil +} + // Tests that apply of object in need of CSA migration results in a call // to patch it. func TestApplyCSAMigration(t *testing.T) { diff --git a/pkg/util/slice/slice.go b/pkg/util/slice/slice.go index f997d5cb..d02bb345 100644 --- a/pkg/util/slice/slice.go +++ b/pkg/util/slice/slice.go @@ -36,3 +36,22 @@ func ContainsString(slice []string, s string, modifier func(s string) string) bo } return false } + +// ToSet returns a single slice containing the unique values from one or more slices. The order of the items in the +// result is not guaranteed. +func ToSet[T comparable](slices ...[]T) []T { + if len(slices) == 0 { + return nil + } + m := map[T]struct{}{} + for _, slice := range slices { + for _, value := range slice { + m[value] = struct{}{} + } + } + result := []T{} + for k := range m { + result = append(result, k) + } + return result +} diff --git a/pkg/util/slice/slice_test.go b/pkg/util/slice/slice_test.go index 7e3bec6e..0adb6b3a 100644 --- a/pkg/util/slice/slice_test.go +++ b/pkg/util/slice/slice_test.go @@ -18,6 +18,7 @@ package slice import ( "reflect" + "sort" "testing" ) @@ -29,3 +30,44 @@ func TestSortInts64(t *testing.T) { t.Errorf("func Ints64 didnt sort correctly, %v !- %v", src, expected) } } + +func TestToSet(t *testing.T) { + testCases := map[string]struct { + input [][]string + expected []string + }{ + "nil should be returned if no slices are passed to the function": { + input: [][]string{}, + expected: nil, + }, + "empty slice should be returned if an empty slice is passed to the function": { + input: [][]string{{}}, + expected: []string{}, + }, + "a single slice with no duplicates should have the same values": { + input: [][]string{{"a", "b", "c"}}, + expected: []string{"a", "b", "c"}, + }, + "duplicates should be removed from a single slice": { + input: [][]string{{"a", "b", "a", "c", "b"}}, + expected: []string{"a", "b", "c"}, + }, + "multiple slices with no duplicates should be combined": { + input: [][]string{{"a", "b", "c"}, {"d", "e", "f"}}, + expected: []string{"a", "b", "c", "d", "e", "f"}, + }, + "duplicates should be removed from multiple slices": { + input: [][]string{{"a", "b", "c"}, {"d", "b", "e"}, {"e", "f", "a"}}, + expected: []string{"a", "b", "c", "d", "e", "f"}, + }, + } + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + actual := ToSet(tc.input...) + sort.Strings(actual) // Sort is needed to compare the output because ToSet is non-deterministic + if !reflect.DeepEqual(actual, tc.expected) { + t.Errorf("wrong output. Actual=%v, Expected=%v", actual, tc.expected) + } + }) + } +}