From c26190e8821ffcb5b379ed9bfee244e9694658d8 Mon Sep 17 00:00:00 2001 From: killianmuldoon Date: Fri, 30 Jul 2021 11:27:21 +0100 Subject: [PATCH] Change delete behaviour to respect inventory Add an option to the delete functions in clusterctl too allow user configuration of inventory deletion. Clusterctl no longer deletes provider inventories during an upgrade. This reduces the chance of an unrecoverable error during clusterctl upgrade. Signed-off-by: killianmuldoon --- cmd/clusterctl/client/cluster/components.go | 13 +- .../client/cluster/components_test.go | 212 +++++++++++++++++- cmd/clusterctl/client/cluster/upgrader.go | 3 +- cmd/clusterctl/client/delete.go | 7 +- cmd/clusterctl/client/delete_test.go | 3 + 5 files changed, 227 insertions(+), 11 deletions(-) diff --git a/cmd/clusterctl/client/cluster/components.go b/cmd/clusterctl/client/cluster/components.go index ce4788691372..a41e2eaa90ee 100644 --- a/cmd/clusterctl/client/cluster/components.go +++ b/cmd/clusterctl/client/cluster/components.go @@ -39,6 +39,7 @@ const ( validatingWebhookConfigurationKind = "ValidatingWebhookConfiguration" mutatingWebhookConfigurationKind = "MutatingWebhookConfiguration" customResourceDefinitionKind = "CustomResourceDefinition" + providerGroupKind = "Provider.clusterctl.cluster.x-k8s.io" ) // DeleteOptions holds options for ComponentsClient.Delete func. @@ -46,6 +47,7 @@ type DeleteOptions struct { Provider clusterctlv1.Provider IncludeNamespace bool IncludeCRDs bool + SkipInventory bool } // ComponentsClient has methods to work with provider components in the cluster. @@ -152,7 +154,6 @@ func (p *providerComponents) Delete(options DeleteOptions) error { if !options.IncludeCRDs && isCRD { continue } - // If the resource is a namespace isNamespace := obj.GroupVersionKind().Kind == namespaceKind if isNamespace { @@ -168,6 +169,14 @@ func (p *providerComponents) Delete(options DeleteOptions) error { namespacesToDelete.Insert(obj.GetName()) } + // If the resource is part of the inventory for clusterctl don't delete it at this point as losing this information makes the + // upgrade function non-reentrant. Instead keep the inventory objects around until the upgrade is finished and working and + // delete them at the end of the upgrade flow. + isInventory := obj.GroupVersionKind().GroupKind().String() == providerGroupKind + if isInventory && options.SkipInventory { + continue + } + // If the resource is a cluster resource, skip it if the resource name does not start with the instance prefix. // This is required because there are cluster resources like e.g. ClusterRoles and ClusterRoleBinding, which are instance specific; // During the installation, clusterctl adds the instance namespace prefix to such resources (see fixRBAC), and so we can rely @@ -175,12 +184,12 @@ func (p *providerComponents) Delete(options DeleteOptions) error { // NOTE: namespace and CRD are special case managed above; webhook instead goes hand by hand with the controller they // should always be deleted. isWebhook := obj.GroupVersionKind().Kind == validatingWebhookConfigurationKind || obj.GroupVersionKind().Kind == mutatingWebhookConfigurationKind + if util.IsClusterResource(obj.GetKind()) && !isNamespace && !isCRD && !isWebhook && !strings.HasPrefix(obj.GetName(), instanceNamespacePrefix) { continue } - resourcesToDelete = append(resourcesToDelete, obj) } diff --git a/cmd/clusterctl/client/cluster/components_test.go b/cmd/clusterctl/client/cluster/components_test.go index cda8af22c25a..9b747ae2c0b0 100644 --- a/cmd/clusterctl/client/cluster/components_test.go +++ b/cmd/clusterctl/client/cluster/components_test.go @@ -17,8 +17,12 @@ limitations under the License. package cluster import ( + "fmt" "testing" + "github.com/google/go-cmp/cmp" + "sigs.k8s.io/cluster-api/cmd/clusterctl/internal/scheme" + . "github.com/onsi/gomega" corev1 "k8s.io/api/core/v1" @@ -42,7 +46,11 @@ func Test_providerComponents_Delete(t *testing.T) { crd.SetKind("CustomResourceDefinition") crd.SetName("crd1") crd.SetLabels(labels) - + providerInventory := unstructured.Unstructured{} + providerInventory.SetAPIVersion(clusterctlv1.GroupVersion.String()) + providerInventory.SetKind("Provider") + providerInventory.SetName("providerOne") + providerInventory.SetLabels(labels) mutatingWebhook := unstructured.Unstructured{} mutatingWebhook.SetAPIVersion("admissionregistration.k8s.io/v1beta1") mutatingWebhook.SetKind("MutatingWebhookConfiguration") @@ -83,6 +91,8 @@ func Test_providerComponents_Delete(t *testing.T) { }, // CRDs (should be deleted only if includeCRD) &crd, + // Inventory should be deleted ony if skipInventory + &providerInventory, &mutatingWebhook, // A cluster-wide provider component (should always be deleted) &rbacv1.ClusterRole{ @@ -120,7 +130,9 @@ func Test_providerComponents_Delete(t *testing.T) { provider clusterctlv1.Provider includeNamespace bool includeCRD bool + skipInventory bool } + type wantDiff struct { object corev1.ObjectReference deleted bool @@ -133,11 +145,12 @@ func Test_providerComponents_Delete(t *testing.T) { wantErr bool }{ { - name: "Delete provider while preserving Namespace and CRDs", + name: "Delete provider while preserving Namespace and CRDs and providerInventory", args: args{ provider: clusterctlv1.Provider{ObjectMeta: metav1.ObjectMeta{Name: "infrastructure-infra", Namespace: "ns1"}, ProviderName: "infra", Type: string(clusterctlv1.InfrastructureProviderType)}, includeNamespace: false, includeCRD: false, + skipInventory: true, }, wantDiff: []wantDiff{ {object: corev1.ObjectReference{APIVersion: "v1", Kind: "Namespace", Name: "ns1"}, deleted: false}, // namespace should be preserved @@ -148,15 +161,17 @@ func Test_providerComponents_Delete(t *testing.T) { {object: corev1.ObjectReference{APIVersion: "v1", Kind: "Pod", Namespace: "ns2", Name: "pod3"}, deleted: false}, // this object is in another namespace, and should never be touched by delete {object: corev1.ObjectReference{APIVersion: "rbac.authorization.k8s.io/v1", Kind: "ClusterRole", Name: "ns1-cluster-role"}, deleted: true}, // cluster-wide provider components should be deleted {object: corev1.ObjectReference{APIVersion: "rbac.authorization.k8s.io/v1", Kind: "ClusterRole", Name: "some-cluster-role"}, deleted: false}, // other cluster-wide objects should be preserved + {object: corev1.ObjectReference{APIVersion: clusterctlv1.GroupVersion.String(), Kind: "Provider", Name: "providerOne"}, deleted: false}, // providerInventory should be preserved }, wantErr: false, }, { - name: "Delete provider and provider namespace, while preserving CRDs", + name: "Delete provider and provider namespace, while preserving CRDs and providerInventory", args: args{ provider: clusterctlv1.Provider{ObjectMeta: metav1.ObjectMeta{Name: "infrastructure-infra", Namespace: "ns1"}, ProviderName: "infra", Type: string(clusterctlv1.InfrastructureProviderType)}, includeNamespace: true, includeCRD: false, + skipInventory: true, }, wantDiff: []wantDiff{ {object: corev1.ObjectReference{APIVersion: "v1", Kind: "Namespace", Name: "ns1"}, deleted: true}, // namespace should be deleted @@ -167,15 +182,17 @@ func Test_providerComponents_Delete(t *testing.T) { {object: corev1.ObjectReference{APIVersion: "v1", Kind: "Pod", Namespace: "ns2", Name: "pod3"}, deleted: false}, // this object is in another namespace, and should never be touched by delete {object: corev1.ObjectReference{APIVersion: "rbac.authorization.k8s.io/v1", Kind: "ClusterRole", Name: "ns1-cluster-role"}, deleted: true}, // cluster-wide provider components should be deleted {object: corev1.ObjectReference{APIVersion: "rbac.authorization.k8s.io/v1", Kind: "ClusterRole", Name: "some-cluster-role"}, deleted: false}, // other cluster-wide objects should be preserved + {object: corev1.ObjectReference{APIVersion: clusterctlv1.GroupVersion.String(), Kind: "Provider", Name: "providerOne"}, deleted: false}, // providerInventory should be preserved }, wantErr: false, }, { - name: "Delete provider and provider CRDs, while preserving the provider namespace", + name: "Delete provider and provider CRDs, while preserving the provider namespace and the providerInventory", args: args{ provider: clusterctlv1.Provider{ObjectMeta: metav1.ObjectMeta{Name: "infrastructure-infra", Namespace: "ns1"}, ProviderName: "infra", Type: string(clusterctlv1.InfrastructureProviderType)}, includeNamespace: false, includeCRD: true, + skipInventory: true, }, wantDiff: []wantDiff{ {object: corev1.ObjectReference{APIVersion: "v1", Kind: "Namespace", Name: "ns1"}, deleted: false}, // namespace should be preserved @@ -186,15 +203,38 @@ func Test_providerComponents_Delete(t *testing.T) { {object: corev1.ObjectReference{APIVersion: "v1", Kind: "Pod", Namespace: "ns2", Name: "pod3"}, deleted: false}, // this object is in another namespace, and should never be touched by delete {object: corev1.ObjectReference{APIVersion: "rbac.authorization.k8s.io/v1", Kind: "ClusterRole", Name: "ns1-cluster-role"}, deleted: true}, // cluster-wide provider components should be deleted {object: corev1.ObjectReference{APIVersion: "rbac.authorization.k8s.io/v1", Kind: "ClusterRole", Name: "some-cluster-role"}, deleted: false}, // other cluster-wide objects should be preserved + {object: corev1.ObjectReference{APIVersion: clusterctlv1.GroupVersion.String(), Kind: "Provider", Name: "providerOne"}, deleted: false}, // providerInventory should be preserved + }, + wantErr: false, + }, + { + name: "Delete providerInventory and provider while preserving provider CRDs and provider namespace", + args: args{ + provider: clusterctlv1.Provider{ObjectMeta: metav1.ObjectMeta{Name: "infrastructure-infra", Namespace: "ns1"}, ProviderName: "infra", Type: string(clusterctlv1.InfrastructureProviderType)}, + includeNamespace: false, + includeCRD: false, + skipInventory: false, + }, + wantDiff: []wantDiff{ + {object: corev1.ObjectReference{APIVersion: "v1", Kind: "Namespace", Name: "ns1"}, deleted: false}, // namespace should be deleted + {object: corev1.ObjectReference{APIVersion: "apiextensions.k8s.io/v1beta1", Kind: "CustomResourceDefinition", Name: "crd1"}, deleted: false}, // crd should not be deleted + {object: corev1.ObjectReference{APIVersion: "admissionregistration.k8s.io/v1beta1", Kind: "MutatingWebhookConfiguration", Name: "mwh1"}, deleted: true}, // MutatingWebhookConfiguration should be deleted + {object: corev1.ObjectReference{APIVersion: "v1", Kind: "Pod", Namespace: "ns1", Name: "pod1"}, deleted: true}, // provider components should be deleted + {object: corev1.ObjectReference{APIVersion: "v1", Kind: "Pod", Namespace: "ns1", Name: "pod2"}, deleted: false}, // other objects in the namespace should not be deleted + {object: corev1.ObjectReference{APIVersion: "v1", Kind: "Pod", Namespace: "ns2", Name: "pod3"}, deleted: false}, // this object is in another namespace, and should never be touched by delete + {object: corev1.ObjectReference{APIVersion: "rbac.authorization.k8s.io/v1", Kind: "ClusterRole", Name: "ns1-cluster-role"}, deleted: true}, // cluster-wide provider components should be deleted + {object: corev1.ObjectReference{APIVersion: "rbac.authorization.k8s.io/v1", Kind: "ClusterRole", Name: "some-cluster-role"}, deleted: false}, // other cluster-wide objects should be preserved + {object: corev1.ObjectReference{APIVersion: clusterctlv1.GroupVersion.String(), Kind: "Provider", Name: "providerOne"}, deleted: true}, // providerInventory should be deleted }, wantErr: false, }, { - name: "Delete provider, provider namespace and provider CRDs", + name: "Delete provider, provider namespace and provider CRDs and the providerInventory", args: args{ provider: clusterctlv1.Provider{ObjectMeta: metav1.ObjectMeta{Name: "infrastructure-infra", Namespace: "ns1"}, ProviderName: "infra", Type: string(clusterctlv1.InfrastructureProviderType)}, includeNamespace: true, includeCRD: true, + skipInventory: false, }, wantDiff: []wantDiff{ {object: corev1.ObjectReference{APIVersion: "v1", Kind: "Namespace", Name: "ns1"}, deleted: true}, // namespace should be deleted @@ -205,6 +245,7 @@ func Test_providerComponents_Delete(t *testing.T) { {object: corev1.ObjectReference{APIVersion: "v1", Kind: "Pod", Namespace: "ns2", Name: "pod3"}, deleted: false}, // this object is in another namespace, and should never be touched by delete {object: corev1.ObjectReference{APIVersion: "rbac.authorization.k8s.io/v1", Kind: "ClusterRole", Name: "ns1-cluster-role"}, deleted: true}, // cluster-wide provider components should be deleted {object: corev1.ObjectReference{APIVersion: "rbac.authorization.k8s.io/v1", Kind: "ClusterRole", Name: "some-cluster-role"}, deleted: false}, // other cluster-wide objects should be preserved + {object: corev1.ObjectReference{APIVersion: clusterctlv1.GroupVersion.String(), Kind: "Provider", Name: "providerOne"}, deleted: true}, // providerInventory should be deleted }, wantErr: false, }, @@ -212,13 +253,15 @@ func Test_providerComponents_Delete(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { g := NewWithT(t) - proxy := test.NewFakeProxy().WithObjs(initObjs...) + c := newComponentsClient(proxy) + err := c.Delete(DeleteOptions{ Provider: tt.args.provider, IncludeNamespace: tt.args.includeNamespace, IncludeCRDs: tt.args.includeCRD, + SkipInventory: tt.args.skipInventory, }) if tt.wantErr { g.Expect(err).To(HaveOccurred()) @@ -295,3 +338,160 @@ func Test_providerComponents_DeleteCoreProviderWebhookNamespace(t *testing.T) { g.Expect(len(nsList.Items)).Should(Equal(0)) }) } + +func Test_providerComponents_Create(t *testing.T) { + labelsOne := map[string]string{ + clusterv1.ProviderLabelName: "infrastructure-infra", + } + commonObjects := []client.Object{ + // Namespace for the provider + &corev1.Namespace{ + TypeMeta: metav1.TypeMeta{ + Kind: "Namespace", + APIVersion: "v1", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "ns1", + Labels: labelsOne, + }, + }, + // A cluster-wide provider component. + &rbacv1.ClusterRole{ + TypeMeta: metav1.TypeMeta{ + Kind: "ClusterRole", + APIVersion: rbacv1.SchemeGroupVersion.WithKind("ClusterRole").GroupVersion().String(), + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "ns1-cluster-role", // global objects belonging to the provider have a namespace prefix. + Labels: labelsOne, + }, + }, + } + // A namespaced provider component as a pod. + podOne := &corev1.Pod{ + TypeMeta: metav1.TypeMeta{ + Kind: "Pod", + APIVersion: "v1", + }, + ObjectMeta: metav1.ObjectMeta{ + Namespace: "ns1", + Name: "pod1", + Labels: labelsOne, + }, + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Image: "pod1-v1", + }, + }, + }, + } + // podTwo is the same as podTwo but has an image titled pod1-v2. + podTwo := &corev1.Pod{ + TypeMeta: metav1.TypeMeta{ + Kind: "Pod", + APIVersion: "v1", + }, + ObjectMeta: metav1.ObjectMeta{ + Namespace: "ns1", + Name: "pod1", + Labels: labelsOne, + }, + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Image: "pod1-v2", + }, + }, + }, + } + type args struct { + objectsToCreate []client.Object + initObjects []client.Object + } + + tests := []struct { + name string + args args + want []client.Object + wantErr bool + }{ + { + name: "Create Provider Pod, Namespace and ClusterRole", + args: args{ + objectsToCreate: append(commonObjects, podOne), + initObjects: []client.Object{}, + }, + want: append(commonObjects, podOne), + wantErr: false, + }, + { + name: "Upgrade Provider Pod, Namespace and ClusterRole", + args: args{ + objectsToCreate: append(commonObjects, podTwo), + initObjects: append(commonObjects, podOne), + }, + want: append(commonObjects, podTwo), + wantErr: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + proxy := test.NewFakeProxy().WithObjs(tt.args.initObjects...) + c := newComponentsClient(proxy) + var unstructuredObjectsToCreate []unstructured.Unstructured + for _, obj := range tt.args.objectsToCreate { + uns := &unstructured.Unstructured{} + if err := scheme.Scheme.Convert(obj, uns, nil); err != nil { + g.Expect(fmt.Errorf("%v %v could not be converted to unstructured", err.Error(), obj)).NotTo(HaveOccurred()) + } + unstructuredObjectsToCreate = append(unstructuredObjectsToCreate, *uns) + } + err := c.Create(unstructuredObjectsToCreate) + if tt.wantErr { + g.Expect(err).To(HaveOccurred()) + return + } + + g.Expect(err).NotTo(HaveOccurred()) + + cs, err := proxy.NewClient() + g.Expect(err).NotTo(HaveOccurred()) + + for _, item := range tt.want { + obj := &unstructured.Unstructured{} + obj.SetKind(item.GetObjectKind().GroupVersionKind().Kind) + obj.SetAPIVersion(item.GetObjectKind().GroupVersionKind().GroupVersion().String()) + key := client.ObjectKey{ + Namespace: item.GetNamespace(), + Name: item.GetName(), + } + + err := cs.Get(ctx, key, obj) + + if err != nil && !apierrors.IsNotFound(err) { + t.Fatalf("Failed to get %v from the cluster: %v", key, err) + } + g.Expect(obj.GetNamespace()).To(Equal(item.GetNamespace()), cmp.Diff(obj.GetNamespace(), item.GetNamespace())) + g.Expect(obj.GetName()).To(Equal(item.GetName()), cmp.Diff(obj.GetName(), item.GetName())) + g.Expect(obj.GetAPIVersion()).To(Equal(item.GetObjectKind().GroupVersionKind().GroupVersion().String()), cmp.Diff(obj.GetAPIVersion(), item.GetObjectKind().GroupVersionKind().GroupVersion().String())) + if item.GetObjectKind().GroupVersionKind().Kind == "Pod" { + p1, okp1 := item.(*corev1.Pod) + if !(okp1) { + g.Expect(fmt.Errorf("%v %v could retrieve pod", err.Error(), obj)).NotTo(HaveOccurred()) + } + p2 := &corev1.Pod{} + if err := scheme.Scheme.Convert(obj, p2, nil); err != nil { + g.Expect(fmt.Errorf("%v %v could not be converted to unstructured", err.Error(), obj)).NotTo(HaveOccurred()) + } + if len(p1.Spec.Containers) == 0 || len(p2.Spec.Containers) == 0 { + g.Expect(fmt.Errorf("%v %v could not be converted to unstructured", err.Error(), obj)).NotTo(HaveOccurred()) + } + g.Expect(p1.Spec.Containers[0].Image).To(Equal(p2.Spec.Containers[0].Image), cmp.Diff(obj.GetNamespace(), item.GetNamespace())) + } + } + }) + } +} diff --git a/cmd/clusterctl/client/cluster/upgrader.go b/cmd/clusterctl/client/cluster/upgrader.go index 70ea9dac2753..6a57d25d2c70 100644 --- a/cmd/clusterctl/client/cluster/upgrader.go +++ b/cmd/clusterctl/client/cluster/upgrader.go @@ -356,11 +356,12 @@ func (u *providerUpgrader) doUpgrade(upgradePlan *UpgradePlan) error { return err } - // Delete the provider, preserving CRD and namespace. + // Delete the provider, preserving CRD, namespace and the inventory. if err := u.providerComponents.Delete(DeleteOptions{ Provider: upgradeItem.Provider, IncludeNamespace: false, IncludeCRDs: false, + SkipInventory: true, }); err != nil { return err } diff --git a/cmd/clusterctl/client/delete.go b/cmd/clusterctl/client/delete.go index 73067f7560e2..2bf461db07ae 100644 --- a/cmd/clusterctl/client/delete.go +++ b/cmd/clusterctl/client/delete.go @@ -53,6 +53,9 @@ type DeleteOptions struct { // IncludeCRDs forces the deletion of the provider's CRDs (and of all the related objects). IncludeCRDs bool + + // SkipInventory forces the deletion of the inventory items used by clusterctl to track providers. + SkipInventory bool } func (c *clusterctlClient) Delete(options DeleteOptions) error { @@ -110,9 +113,9 @@ func (c *clusterctlClient) Delete(options DeleteOptions) error { } } - // Delete the selected providers + // Delete the selected providers. for _, provider := range providersToDelete { - if err := clusterClient.ProviderComponents().Delete(cluster.DeleteOptions{Provider: provider, IncludeNamespace: options.IncludeNamespace, IncludeCRDs: options.IncludeCRDs}); err != nil { + if err := clusterClient.ProviderComponents().Delete(cluster.DeleteOptions{Provider: provider, IncludeNamespace: options.IncludeNamespace, IncludeCRDs: options.IncludeCRDs, SkipInventory: options.SkipInventory}); err != nil { return err } } diff --git a/cmd/clusterctl/client/delete_test.go b/cmd/clusterctl/client/delete_test.go index e8d241bcd51a..4e3eaf966f81 100644 --- a/cmd/clusterctl/client/delete_test.go +++ b/cmd/clusterctl/client/delete_test.go @@ -51,6 +51,7 @@ func Test_clusterctlClient_Delete(t *testing.T) { Kubeconfig: Kubeconfig{Path: "kubeconfig", Context: "mgmt-context"}, IncludeNamespace: false, IncludeCRDs: false, + SkipInventory: false, CoreProvider: "", BootstrapProviders: nil, InfrastructureProviders: nil, @@ -71,6 +72,7 @@ func Test_clusterctlClient_Delete(t *testing.T) { Kubeconfig: Kubeconfig{Path: "kubeconfig", Context: "mgmt-context"}, IncludeNamespace: false, IncludeCRDs: false, + SkipInventory: false, CoreProvider: "", BootstrapProviders: []string{bootstrapProviderConfig.Name()}, InfrastructureProviders: nil, @@ -94,6 +96,7 @@ func Test_clusterctlClient_Delete(t *testing.T) { Kubeconfig: Kubeconfig{Path: "kubeconfig", Context: "mgmt-context"}, IncludeNamespace: false, IncludeCRDs: false, + SkipInventory: false, CoreProvider: capiProviderConfig.Name(), BootstrapProviders: []string{bootstrapProviderConfig.Name()}, InfrastructureProviders: nil,