Fix infra secret lookup so that it defaults to the namespace of the referencing resource

controllers/kubevirtcluster_controller_test.go

@@ -7,10 +7,7 @@ import (
 	"github.com/golang/mock/gomock"
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
-	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/runtime"
-	kubevirtv1 "kubevirt.io/api/core/v1"
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 	. "sigs.k8s.io/controller-runtime"
 	ctrl "sigs.k8s.io/controller-runtime"
@@ -44,7 +41,7 @@ var _ = Describe("Reconcile", func() {
 	})
 
 	setupClient := func(objects []client.Object) {
-		fakeClient = fake.NewClientBuilder().WithScheme(setupScheme()).WithObjects(objects...).Build()
+		fakeClient = fake.NewClientBuilder().WithScheme(testing.SetupScheme()).WithObjects(objects...).Build()
 		kubevirtClusterReconciler = controllers.KubevirtClusterReconciler{
 			Client:       fakeClient,
 			InfraCluster: infraClusterMock,
@@ -62,7 +59,7 @@ var _ = Describe("Reconcile", func() {
 				cluster,
 				kubevirtCluster,
 			}
-			fakeClient = fake.NewClientBuilder().WithScheme(setupScheme()).WithObjects(objects...).Build()
+			fakeClient = fake.NewClientBuilder().WithScheme(testing.SetupScheme()).WithObjects(objects...).Build()
 		})
 
 		AfterEach(func() {})
@@ -179,20 +176,3 @@ var _ = Describe("Reconcile", func() {
 		})
 	})
 })
-
-func setupScheme() *runtime.Scheme {
-	s := runtime.NewScheme()
-	if err := clusterv1.AddToScheme(s); err != nil {
-		panic(err)
-	}
-	if err := infrav1.AddToScheme(s); err != nil {
-		panic(err)
-	}
-	if err := kubevirtv1.AddToScheme(s); err != nil {
-		panic(err)
-	}
-	if err := corev1.AddToScheme(s); err != nil {
-		panic(err)
-	}
-	return s
-}

controllers/kubevirtmachine_controller_test.go

@@ -29,7 +29,6 @@ import (
 
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
 	kubevirtv1 "kubevirt.io/api/core/v1"
 
@@ -109,7 +108,7 @@ var _ = Describe("KubevirtClusterToKubevirtMachines", func() {
 			// add one more machine without corresponding kubevirt machine, to test that no request is created for it
 			testing.NewMachine(clusterName, "machine-without-corresponding-kubevirt-machine", nil),
 		}
-		fakeClient = fake.NewClientBuilder().WithScheme(setupScheme()).WithObjects(objects...).Build()
+		fakeClient = fake.NewClientBuilder().WithScheme(testing.SetupScheme()).WithObjects(objects...).Build()
 		kubevirtMachineReconciler = KubevirtMachineReconciler{
 			Client:         fakeClient,
 			MachineFactory: kubevirt.DefaultMachineFactory{},
@@ -324,7 +323,7 @@ var _ = Describe("reconcile a kubevirt machine", func() {
 			Logger:          testLogger,
 		}
 
-		fakeClient = fake.NewClientBuilder().WithScheme(setupScheme()).WithObjects(objects...).Build()
+		fakeClient = fake.NewClientBuilder().WithScheme(testing.SetupScheme()).WithObjects(objects...).Build()
 		kubevirtMachineReconciler = KubevirtMachineReconciler{
 			Client:          fakeClient,
 			WorkloadCluster: workloadClusterMock,
@@ -1064,7 +1063,7 @@ var _ = Describe("updateNodeProviderID", func() {
 		objects := []client.Object{
 			kubevirtMachine,
 		}
-		fakeClient = fake.NewClientBuilder().WithScheme(setupScheme()).WithObjects(objects...).Build()
+		fakeClient = fake.NewClientBuilder().WithScheme(testing.SetupScheme()).WithObjects(objects...).Build()
 		kubevirtMachineReconciler = KubevirtMachineReconciler{
 			Client:          fakeClient,
 			WorkloadCluster: workloadClusterMock,
@@ -1082,7 +1081,7 @@ var _ = Describe("updateNodeProviderID", func() {
 				},
 			},
 		}
-		fakeWorkloadClusterClient = fake.NewClientBuilder().WithScheme(setupScheme()).WithObjects(workloadClusterObjects...).Build()
+		fakeWorkloadClusterClient = fake.NewClientBuilder().WithScheme(testing.SetupScheme()).WithObjects(workloadClusterObjects...).Build()
 	})
 
 	AfterEach(func() {})
@@ -1132,20 +1131,3 @@ var _ = Describe("updateNodeProviderID", func() {
 		Expect(kubevirtMachine.Status.NodeUpdated).To(Equal(false))
 	})
 })
-
-func setupScheme() *runtime.Scheme {
-	s := runtime.NewScheme()
-	if err := clusterv1.AddToScheme(s); err != nil {
-		panic(err)
-	}
-	if err := infrav1.AddToScheme(s); err != nil {
-		panic(err)
-	}
-	if err := kubevirtv1.AddToScheme(s); err != nil {
-		panic(err)
-	}
-	if err := corev1.AddToScheme(s); err != nil {
-		panic(err)
-	}
-	return s
-}

pkg/infracluster/infracluster.go

@@ -6,41 +6,56 @@ import (
 
 	"github.com/pkg/errors"
 	corev1 "k8s.io/api/core/v1"
+	"k8s.io/client-go/rest"
 	"k8s.io/client-go/tools/clientcmd"
-	"sigs.k8s.io/controller-runtime/pkg/client"
+	k8sclient "sigs.k8s.io/controller-runtime/pkg/client"
 )
 
 //go:generate mockgen -source=./infracluster.go -destination=./mock/infracluster_generated.go -package=mock
 type InfraCluster interface {
-	GenerateInfraClusterClient(infraClusterSecretRef *corev1.ObjectReference, ownerNamespace string, context gocontext.Context) (client.Client, string, error)
+	GenerateInfraClusterClient(infraClusterSecretRef *corev1.ObjectReference, ownerNamespace string, context gocontext.Context) (k8sclient.Client, string, error)
 }
 
+// ClientFactoryFunc defines the function to create a new client
+type ClientFactoryFunc func(config *rest.Config, options k8sclient.Options) (k8sclient.Client, error)
+
 // New creates new InfraCluster instance
-func New(client client.Client) InfraCluster {
+func New(client k8sclient.Client) InfraCluster {
+	return NewWithFactory(client, k8sclient.New)
+}
+
+// NewWithFactory creates new InfraCluster instance that uses the provided client factory function.
+func NewWithFactory(client k8sclient.Client, factory ClientFactoryFunc) InfraCluster {
 	return &infraCluster{
-		Client: client,
+		Client:        client,
+		ClientFactory: factory,
 	}
 }
 
 type infraCluster struct {
-	client.Client
+	k8sclient.Client
+	ClientFactory ClientFactoryFunc
 }
 
 // GenerateInfraClusterClient creates a client for infra cluster.
-func (w *infraCluster) GenerateInfraClusterClient(infraClusterSecretRef *corev1.ObjectReference, ownerNamespace string, context gocontext.Context) (client.Client, string, error) {
+func (w *infraCluster) GenerateInfraClusterClient(infraClusterSecretRef *corev1.ObjectReference, ownerNamespace string, context gocontext.Context) (k8sclient.Client, string, error) {
 	if infraClusterSecretRef == nil {
 		return w.Client, ownerNamespace, nil
 	}
 
 	infraKubeconfigSecret := &corev1.Secret{}
-	infraKubeconfigSecretKey := client.ObjectKey{Namespace: infraClusterSecretRef.Namespace, Name: infraClusterSecretRef.Name}
+	secretNamespace := infraClusterSecretRef.Namespace
+	if secretNamespace == "" {
+		secretNamespace = ownerNamespace
+	}
+	infraKubeconfigSecretKey := k8sclient.ObjectKey{Namespace: secretNamespace, Name: infraClusterSecretRef.Name}
 	if err := w.Client.Get(context, infraKubeconfigSecretKey, infraKubeconfigSecret); err != nil {
 		return nil, "", errors.Wrapf(err, "failed to fetch infra kubeconfig secret %s/%s", infraClusterSecretRef.Namespace, infraClusterSecretRef.Name)
 	}
 
 	kubeConfig, ok := infraKubeconfigSecret.Data["kubeconfig"]
 	if !ok {
-		return nil, "", errors.New("Failed to retrieve infra kubeconfig from secret: 'kubeconfig' key is missing.")
+		return nil, "", errors.New("failed to retrieve infra kubeconfig from secret: 'kubeconfig' key is missing")
 	}
 
 	clientConfig, err := clientcmd.NewClientConfigFromBytes(kubeConfig)
@@ -62,7 +77,7 @@ func (w *infraCluster) GenerateInfraClusterClient(infraClusterSecretRef *corev1.
 		return nil, "", errors.Wrap(err, "failed to create REST config")
 	}
 
-	infraClusterClient, err := client.New(restConfig, client.Options{Scheme: w.Client.Scheme()})
+	infraClusterClient, err := w.ClientFactory(restConfig, k8sclient.Options{Scheme: w.Client.Scheme()})
 	if err != nil {
 		return nil, "", errors.Wrap(err, "failed to create infra cluster client")
 	}

pkg/infracluster/infracluster_test.go

@@ -0,0 +1,177 @@
+package infracluster_test
+
+import (
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/errors"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/client-go/rest"
+	. "sigs.k8s.io/cluster-api-provider-kubevirt/pkg/infracluster"
+	"sigs.k8s.io/cluster-api-provider-kubevirt/pkg/testing"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+	k8sclient "sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
+)
+
+var (
+	fakeClient         client.Client
+	infraClusterSecret *corev1.Secret
+	ownerNamespace     = "Mordor"
+	infraSecretName    = "external-infra-kubeconfig"
+	kubeconfig         = `apiVersion: v1
+clusters:
+- cluster:
+    insecure-skip-tls-verify: true
+    server: https://gondor.com
+  name: gondor
+contexts:
+- context:
+    cluster: gondor
+    namespace: minastirith
+    user: aragorn
+  name: gondor
+current-context: gondor
+kind: Config
+preferences: {}
+users:
+- name: aragorn
+`
+)
+
+var _ = Describe("InfraCluster", func() {
+
+	It("should return the management client and namespace when the infrastructure secret reference is nil", func() {
+		fakeClient = fake.NewClientBuilder().WithScheme(testing.SetupScheme()).Build()
+
+		infraCluster := New(fakeClient)
+		infraClient, infraNamespace, err := infraCluster.GenerateInfraClusterClient(nil, ownerNamespace, nil)
+		Expect(err).NotTo(HaveOccurred())
+		Expect(infraClient).To(BeIdenticalTo(fakeClient))
+		Expect(infraNamespace).To(Equal(ownerNamespace))
+	})
+
+	It("should failed when the referenced infrastructure secret cannot be found", func() {
+		fakeClient := fake.NewFakeClientWithScheme(testing.SetupScheme())
+
+		infraClusterSecretRef := &corev1.ObjectReference{
+			APIVersion: "v1",
+			Kind:       "Secret",
+			Name:       infraSecretName,
+		}
+		infraCluster := New(fakeClient)
+
+		_, _, err := infraCluster.GenerateInfraClusterClient(infraClusterSecretRef, ownerNamespace, nil)
+		Expect(errors.IsNotFound(err)).To(BeTrue())
+	})
+
+	It("should fail when the referenced infrastructure secret doesn't have a kubeconfig data in it", func() {
+		infraClusterSecret = &corev1.Secret{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      infraSecretName,
+				Namespace: ownerNamespace,
+			},
+			Data: map[string][]byte{},
+		}
+		fakeClient = fake.NewClientBuilder().WithScheme(testing.SetupScheme()).WithObjects(infraClusterSecret).Build()
+
+		infraClusterSecretRef := &corev1.ObjectReference{
+			APIVersion: "v1",
+			Kind:       "Secret",
+			Name:       infraSecretName,
+		}
+
+		infraCluster := New(fakeClient)
+		_, _, err := infraCluster.GenerateInfraClusterClient(infraClusterSecretRef, ownerNamespace, nil)
+		Expect(err).To(HaveOccurred())
+		Expect(err.Error()).To(Equal("failed to retrieve infra kubeconfig from secret: 'kubeconfig' key is missing"))
+	})
+
+	It("should fail when the referenced infrastructure secret kubeconfig data is invalid", func() {
+		infraClusterSecret = &corev1.Secret{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      infraSecretName,
+				Namespace: ownerNamespace,
+			},
+			Data: map[string][]byte{
+				"kubeconfig": []byte("hello world"),
+			},
+		}
+		fakeClient = fake.NewClientBuilder().WithScheme(testing.SetupScheme()).WithObjects(infraClusterSecret).Build()
+
+		infraClusterSecretRef := &corev1.ObjectReference{
+			APIVersion: "v1",
+			Kind:       "Secret",
+			Name:       infraSecretName,
+		}
+
+		infraCluster := New(fakeClient)
+		_, _, err := infraCluster.GenerateInfraClusterClient(infraClusterSecretRef, ownerNamespace, nil)
+		Expect(err).To(HaveOccurred())
+		Expect(err.Error()).To(ContainSubstring("failed to create K8s-API client config"))
+	})
+
+	It("should return the infra-client and the namespace defined in the secret, when set", func() {
+
+		infraClusterSecret = &corev1.Secret{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      infraSecretName,
+				Namespace: ownerNamespace,
+			},
+			Data: map[string][]byte{
+				"kubeconfig": []byte(kubeconfig),
+				"namespace":  []byte("Shire"),
+			},
+		}
+		fakeClient = fake.NewClientBuilder().WithScheme(testing.SetupScheme()).WithObjects(infraClusterSecret).Build()
+
+		infraClusterSecretRef := &corev1.ObjectReference{
+			APIVersion: "v1",
+			Kind:       "Secret",
+			Name:       infraSecretName,
+		}
+
+		fakeInfraClient := fake.NewFakeClient()
+		infraCluster := NewWithFactory(fakeClient,
+			func(config *rest.Config, options k8sclient.Options) (k8sclient.Client, error) {
+				return fakeInfraClient, nil
+			},
+		)
+		infraClient, namespace, err := infraCluster.GenerateInfraClusterClient(infraClusterSecretRef, ownerNamespace, nil)
+		Expect(err).NotTo(HaveOccurred())
+		Expect(infraClient).To(BeIdenticalTo(fakeInfraClient))
+		Expect(namespace).To(Equal("Shire"))
+	})
+
+	It("should return the infra-client and kubeconfig namespace when the secret doesn't specified one", func() {
+
+		infraClusterSecret = &corev1.Secret{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      infraSecretName,
+				Namespace: ownerNamespace,
+			},
+			Data: map[string][]byte{
+				"kubeconfig": []byte(kubeconfig),
+			},
+		}
+		fakeClient = fake.NewClientBuilder().WithScheme(testing.SetupScheme()).WithObjects(infraClusterSecret).Build()
+
+		infraClusterSecretRef := &corev1.ObjectReference{
+			APIVersion: "v1",
+			Kind:       "Secret",
+			Name:       infraSecretName,
+		}
+
+		fakeInfraClient := fake.NewFakeClient()
+		infraCluster := NewWithFactory(fakeClient,
+			func(config *rest.Config, options k8sclient.Options) (k8sclient.Client, error) {
+				return fakeInfraClient, nil
+			},
+		)
+		infraClient, namespace, err := infraCluster.GenerateInfraClusterClient(infraClusterSecretRef, ownerNamespace, nil)
+		Expect(err).NotTo(HaveOccurred())
+		Expect(infraClient).To(BeIdenticalTo(fakeInfraClient))
+		Expect(namespace).To(Equal("minastirith"))
+	})
+
+})

pkg/infracluster/kubevirt_suite_test.go

@@ -0,0 +1,13 @@
+package infracluster_test
+
+import (
+	"testing"
+
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+)
+
+func TestKubevirt(t *testing.T) {
+	RegisterFailHandler(Fail)
+	RunSpecs(t, "InfraCluster Suite")
+}