Merge pull request #4598 from fabriziopandini/clusterctl-move-infrast…

…ructure-secrets

🌱 clusterctl move should consider Secrets from provider's namespace

cmd/clusterctl/client/cluster/mover.go

@@ -60,7 +60,7 @@ func (o *objectMover) Move(namespace string, toCluster Client, dryRun bool) erro
 		log.Info("********************************************************")
 	}
 
-	objectGraph := newObjectGraph(o.fromProxy)
+	objectGraph := newObjectGraph(o.fromProxy, o.fromProviderInventory)
 
 	// checks that all the required providers in place in the target cluster.
 	if !o.dryRun {

cmd/clusterctl/client/cluster/objectgraph.go

@@ -105,15 +105,17 @@ func (n *node) isSoftOwnedBy(other *node) bool {
 
 // objectGraph manages the Kubernetes object graph that is generated during the discovery phase for the move operation.
 type objectGraph struct {
-	proxy     Proxy
-	uidToNode map[types.UID]*node
-	types     map[string]*discoveryTypeInfo
+	proxy             Proxy
+	providerInventory InventoryClient
+	uidToNode         map[types.UID]*node
+	types             map[string]*discoveryTypeInfo
 }
 
-func newObjectGraph(proxy Proxy) *objectGraph {
+func newObjectGraph(proxy Proxy, providerInventory InventoryClient) *objectGraph {
 	return &objectGraph{
-		proxy:     proxy,
-		uidToNode: map[types.UID]*node{},
+		proxy:             proxy,
+		providerInventory: providerInventory,
+		uidToNode:         map[types.UID]*node{},
 	}
 }
 
@@ -309,6 +311,26 @@ func (o *objectGraph) Discovery(namespace string) error {
 			return err
 		}
 
+		// if we are discovering Secrets, also secrets from the providers namespace should be included.
+		if discoveryType.typeMeta.GetObjectKind().GroupVersionKind().GroupKind() == corev1.SchemeGroupVersion.WithKind("SecretList").GroupKind() {
+			providers, err := o.providerInventory.List()
+			if err != nil {
+				return err
+			}
+			for _, p := range providers.Items {
+				if p.Type == string(clusterctlv1.InfrastructureProviderType) {
+					providerNamespaceSelector := []client.ListOption{client.InNamespace(p.Namespace)}
+					providerNamespaceSecretList := new(unstructured.UnstructuredList)
+					if err := retryWithExponentialBackoff(discoveryBackoff, func() error {
+						return getObjList(o.proxy, typeMeta, providerNamespaceSelector, providerNamespaceSecretList)
+					}); err != nil {
+						return err
+					}
+					objList.Items = append(objList.Items, providerNamespaceSecretList.Items...)
+				}
+			}
+		}
+
 		if len(objList.Items) == 0 {
 			continue
 		}

cmd/clusterctl/client/cluster/objectgraph_test.go

@@ -22,12 +22,11 @@ import (
 	"testing"
 
 	. "github.com/onsi/gomega"
-
 	"github.com/pkg/errors"
-
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/types"
+	clusterctlv1 "sigs.k8s.io/cluster-api/cmd/clusterctl/api/v1alpha3"
 	"sigs.k8s.io/cluster-api/cmd/clusterctl/internal/test"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
@@ -64,7 +63,7 @@ func TestObjectGraph_getDiscoveryTypeMetaList(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			g := NewWithT(t)
 
-			graph := newObjectGraph(tt.fields.proxy)
+			graph := newObjectGraph(tt.fields.proxy, nil)
 			err := graph.getDiscoveryTypes()
 			if tt.wantErr {
 				g.Expect(err).To(HaveOccurred())
@@ -304,7 +303,7 @@ func TestObjectGraph_addObj(t *testing.T) {
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			graph := newObjectGraph(nil)
+			graph := newObjectGraph(nil, nil)
 			for _, o := range tt.args.objs {
 				graph.addObj(o)
 			}
@@ -1012,44 +1011,47 @@ var objectGraphsTests = []struct {
 		},
 	},
 	{
-		name: "Cluster and Global + Namespaced External Objects",
+		// NOTE: External objects are CRD types installed by clusterctl, but not directly related with the CAPI hierarchy of objects. e.g. IPAM claims.
+		name: "Namespaced External Objects with force move label",
 		args: objectGraphTestArgs{
-			func() []client.Object {
-				objs := []client.Object{}
-				objs = append(objs, test.NewFakeCluster("ns1", "cluster1").Objs()...)
-				objs = append(objs, test.NewFakeExternalObject("ns1", "externalObject1").Objs()...)
-				objs = append(objs, test.NewFakeExternalObject("", "externalObject2").Objs()...)
-
-				return objs
-			}(),
+			objs: test.NewFakeExternalObject("ns1", "externalObject1").Objs(),
 		},
 		want: wantGraph{
 			nodes: map[string]wantGraphItem{
 				"external.cluster.x-k8s.io/v1alpha4, Kind=GenericExternalObject, ns1/externalObject1": {},
-				"external.cluster.x-k8s.io/v1alpha4, Kind=GenericExternalObject, /externalObject2":    {},
-				"cluster.x-k8s.io/v1alpha4, Kind=Cluster, ns1/cluster1":                               {},
-				"infrastructure.cluster.x-k8s.io/v1alpha4, Kind=GenericInfrastructureCluster, ns1/cluster1": {
-					owners: []string{
-						"cluster.x-k8s.io/v1alpha4, Kind=Cluster, ns1/cluster1",
-					},
-				},
-				"/v1, Kind=Secret, ns1/cluster1-ca": {
-					softOwners: []string{
-						"cluster.x-k8s.io/v1alpha4, Kind=Cluster, ns1/cluster1", // NB. this secret is not linked to the cluster through owner ref
-					},
-				},
-				"/v1, Kind=Secret, ns1/cluster1-kubeconfig": {
-					owners: []string{
-						"cluster.x-k8s.io/v1alpha4, Kind=Cluster, ns1/cluster1",
-					},
-				},
+			},
+		},
+	},
+	{
+		// NOTE: External objects are CRD types installed by clusterctl, but not directly related with the CAPI hierarchy of objects. e.g. IPAM claims.
+		name: "Global External Objects with force move label",
+		args: objectGraphTestArgs{
+			objs: test.NewFakeExternalObject("", "externalObject1").Objs(),
+		},
+		want: wantGraph{
+			nodes: map[string]wantGraphItem{
+				"external.cluster.x-k8s.io/v1alpha4, Kind=GenericExternalObject, /externalObject1": {},
+			},
+		},
+	},
+	{
+		// NOTE: Infrastructure providers global credentials are going to be stored in Secrets in the provider's namespaces.
+		name: "Secrets from provider's namespace",
+		args: objectGraphTestArgs{
+			objs: []client.Object{
+				test.NewSecret("infra1", "credentials"),
+			},
+		},
+		want: wantGraph{
+			nodes: map[string]wantGraphItem{
+				"/v1, Kind=Secret, infra1/credentials": {},
 			},
 		},
 	},
 }
 
 func getDetachedObjectGraphWihObjs(objs []client.Object) (*objectGraph, error) {
-	graph := newObjectGraph(nil) // detached from any cluster
+	graph := newObjectGraph(nil, nil) // detached from any cluster
 	for _, o := range objs {
 		u := &unstructured.Unstructured{}
 		if err := test.FakeScheme.Convert(o, u, nil); err != nil {
@@ -1084,7 +1086,10 @@ func getObjectGraphWithObjs(objs []client.Object) *objectGraph {
 		fromProxy.WithObjs(o)
 	}
 
-	return newObjectGraph(fromProxy)
+	fromProxy.WithProviderInventory("infra1", clusterctlv1.InfrastructureProviderType, "v1.2.3", "infra1")
+	inventory := newInventoryClient(fromProxy, fakePollImmediateWaiter)
+
+	return newObjectGraph(fromProxy, inventory)
 }
 
 func getFakeProxyWithCRDs() *test.FakeProxy {
@@ -1225,6 +1230,34 @@ func TestObjectGraph_DiscoveryByNamespace(t *testing.T) {
 				},
 			},
 		},
+		{
+			// NOTE: External objects are CRD types installed by clusterctl, but not directly related with the CAPI hierarchy of objects. e.g. IPAM claims.
+			name: "Namespaced External Objects with force move label",
+			args: args{
+				namespace: "ns1",                                                       // read only from ns1
+				objs:      test.NewFakeExternalObject("ns1", "externalObject1").Objs(), // Fake external object with
+			},
+			want: wantGraph{
+				nodes: map[string]wantGraphItem{
+					"external.cluster.x-k8s.io/v1alpha4, Kind=GenericExternalObject, ns1/externalObject1": {},
+				},
+			},
+		},
+		{
+			// NOTE: Infrastructure providers global credentials are going to be stored in Secrets in the provider's namespaces.
+			name: "Secrets from provider's namespace (e.g. credentials) should always be read",
+			args: args{
+				namespace: "ns1", // read only from ns1
+				objs: []client.Object{
+					test.NewSecret("infra1", "credentials"), // a secret in infra1 namespace, where an infrastructure provider is installed
+				},
+			},
+			want: wantGraph{
+				nodes: map[string]wantGraphItem{
+					"/v1, Kind=Secret, infra1/credentials": {},
+				},
+			},
+		},
 	}
 
 	for _, tt := range tests {

cmd/clusterctl/internal/test/fake_objects.go

@@ -1143,6 +1143,22 @@ func (f *FakeExternalObject) Objs() []client.Object {
 	return []client.Object{externalObj}
 }
 
+// NewSecret generates a new secret with the given namespace and name.
+func NewSecret(namespace, name string) *corev1.Secret {
+	s := &corev1.Secret{
+		TypeMeta: metav1.TypeMeta{
+			APIVersion: corev1.SchemeGroupVersion.String(),
+			Kind:       "Secret",
+		},
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      name,
+			Namespace: namespace,
+		},
+	}
+	setUID(s)
+	return s
+}
+
 // SelectClusterObj finds and returns a Cluster with the given name and namespace, if any.
 func SelectClusterObj(objs []client.Object, namespace, name string) *clusterv1.Cluster {
 	for _, o := range objs {

docs/book/src/clusterctl/provider-contract.md

@@ -311,10 +311,18 @@ functioning of `clusterctl` when using non-compliant component YAML or cluster t
 
 Provider authors should be aware that `clusterctl move` command implements a discovery mechanism that considers:
 
-* All the objects of Kind defined in one of the CRDs installed by clusterctl using `clusterctl init`.
-* `Secret` and `ConfigMap` objects.
-* The `OwnerReference` chain of the above objects.
-* Any object of Kind in which its CRD has the "move" label (`clusterctl.cluster.x-k8s.io/move`) attached to it.
+* All the objects of Kind defined in one of the CRDs installed by clusterctl using `clusterctl init` from the namespace being moved.
+* `ConfigMap` objects from the namespace being moved.
+* `Secret` objects from the namespace being moved and from the namespaces where infrastructure providers are installed.
+
+`clusterctl move` does NOT consider any objects:
+
+* Not included in the set of objects defined above.
+* Included in the set of objects defined above, but not:
+  * Directly or indirectly linked to a `Cluster` object through the `OwnerReference` chain.
+  * Directly or indirectly linked to a `ClusterResourceSet` object through the `OwnerReference` chain.
+  * Explicitly required to move via the "move" label (`clusterctl.cluster.x-k8s.io/move`) attached to the object or to
+    the CRD definition.
 
 <aside class="note warning">
 
@@ -324,15 +332,8 @@ When using the "move" label, if the CRD is a global resource, the object is copi
 
 </aside>
 
-`clusterctl move` does NOT consider any objects:
-
-* Not included in the set of objects defined above.
-* Included in the set of objects defined above, but not:
-  * Directly or indirectly linked to a `Cluster` object through the `OwnerReference` chain.
-  * Directly or indirectly linked to a `ClusterResourceSet` object through the `OwnerReference` chain.
-
 If moving some of excluded object is required, the provider authors should create documentation describing the
-the exact move sequence to be executed by the user.
+exact move sequence to be executed by the user.
 
 Additionally, provider authors should be aware that `clusterctl move` assumes all the provider's Controllers respect the
 `Cluster.Spec.Paused` field introduced in the v1alpha3 Cluster API specification.