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 <[email protected]>

cmd/clusterctl/client/cluster/components.go

@@ -39,13 +39,15 @@ const (
 	validatingWebhookConfigurationKind = "ValidatingWebhookConfiguration"
 	mutatingWebhookConfigurationKind   = "MutatingWebhookConfiguration"
 	customResourceDefinitionKind       = "CustomResourceDefinition"
+	providerGroupKind                  = "Provider.clusterctl.cluster.x-k8s.io"
 )
 
 // DeleteOptions holds options for ComponentsClient.Delete func.
 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,19 +169,27 @@ 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
 		// on that for deleting only the global resources belonging the the instance we are processing.
 		// 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)
 	}
 

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"
@@ -295,3 +299,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()))
+				}
+			}
+		})
+	}
+}

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
 		}

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
 		}
 	}

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,