fix: improve pull-secret namespace handling for hcp clusters

README.md

@@ -345,7 +345,10 @@ different credential:
 ```sh
 kubectl create secret generic crc-pull-secret --from-file=.dockerconfigjson=./pull-secret.json --type=kubernetes.io/dockerconfigjson
 ```
-then reference it via `template.pullSecretRef.name: crc-pull-secret`.
+then reference it via `template.pullSecretRef.name: crc-pull-secret`. The
+Secret must be in the same namespace as the `ClusterPool` or `ClusterInstance`.
+For `hcp`, the operator copies it to the HostedCluster namespace under a
+per-instance name before HyperShift uses it.
 
 #### Common to both paths
 
@@ -559,8 +562,9 @@ On the **management** OpenShift cluster:
   to have `get` access to that Secret. The RBAC in
   `config/openshift-config-rbac` grants this access; `make deploy`
   applies it automatically. To override the default, set
-  `template.pullSecretRef` explicitly, pointing at an opaque `Secret` in
-  the same namespace as the pool or instances.
+  `template.pullSecretRef` explicitly, pointing at a `Secret` in the same
+  namespace as the pool or instances. For `hcp`, the operator copies that
+  Secret into the HostedCluster namespace before provisioning.
 - For `crc` pools specifically: an extracted CRC bundle `crc.qcow2`,
   hosted at an HTTP-reachable URL, and a `Secret` holding its
   `id_ecdsa_crc` SSH key (`template.bundleSSHKeyRef`). See

api/v1alpha1/clusterpool_types.go

@@ -128,13 +128,14 @@ type ClusterTemplate struct {
 	// +optional
 	RootVolumeSize string `json:"rootVolumeSize,omitempty"`
 
-	// PullSecretRef references a Secret (in the operator's namespace) containing the
-	// pull-secret used to pull the release payload / CRC bundle images. This field is
-	// optional. When unset, the operator defaults to the management cluster's own
+	// PullSecretRef references a Secret in the ClusterInstance's namespace containing
+	// the pull-secret used to pull the release payload / CRC bundle images. This field
+	// is optional. When unset, the operator defaults to the management cluster's own
 	// global pull secret (the "pull-secret" Secret in the "openshift-config"
-	// namespace), which is present on every OpenShift cluster. Set this field
-	// explicitly to use a different/narrower credential (e.g. one scoped to a
-	// disconnected mirror).
+	// namespace), which is present on every OpenShift cluster. For topology=hcp, an
+	// explicit Secret is copied to the HostedCluster's namespace under a per-instance
+	// name before the HostedCluster uses it. Set this field explicitly to use a
+	// different/narrower credential (e.g. one scoped to a disconnected mirror).
 	// +optional
 	PullSecretRef corev1.LocalObjectReference `json:"pullSecretRef,omitempty"`
 

config/crd/bases/guestcluster.opdev.io_clusterinstances.yaml

@@ -211,13 +211,14 @@ spec:
                     type: string
                   pullSecretRef:
                     description: |-
-                      PullSecretRef references a Secret (in the operator's namespace) containing the
-                      pull-secret used to pull the release payload / CRC bundle images. This field is
-                      optional. When unset, the operator defaults to the management cluster's own
+                      PullSecretRef references a Secret in the ClusterInstance's namespace containing
+                      the pull-secret used to pull the release payload / CRC bundle images. This field
+                      is optional. When unset, the operator defaults to the management cluster's own
                       global pull secret (the "pull-secret" Secret in the "openshift-config"
-                      namespace), which is present on every OpenShift cluster. Set this field
-                      explicitly to use a different/narrower credential (e.g. one scoped to a
-                      disconnected mirror).
+                      namespace), which is present on every OpenShift cluster. For topology=hcp, an
+                      explicit Secret is copied to the HostedCluster's namespace under a per-instance
+                      name before the HostedCluster uses it. Set this field explicitly to use a
+                      different/narrower credential (e.g. one scoped to a disconnected mirror).
                     properties:
                       name:
                         default: ""

config/crd/bases/guestcluster.opdev.io_clusterpools.yaml

@@ -219,13 +219,14 @@ spec:
                     type: string
                   pullSecretRef:
                     description: |-
-                      PullSecretRef references a Secret (in the operator's namespace) containing the
-                      pull-secret used to pull the release payload / CRC bundle images. This field is
-                      optional. When unset, the operator defaults to the management cluster's own
+                      PullSecretRef references a Secret in the ClusterInstance's namespace containing
+                      the pull-secret used to pull the release payload / CRC bundle images. This field
+                      is optional. When unset, the operator defaults to the management cluster's own
                       global pull secret (the "pull-secret" Secret in the "openshift-config"
-                      namespace), which is present on every OpenShift cluster. Set this field
-                      explicitly to use a different/narrower credential (e.g. one scoped to a
-                      disconnected mirror).
+                      namespace), which is present on every OpenShift cluster. For topology=hcp, an
+                      explicit Secret is copied to the HostedCluster's namespace under a per-instance
+                      name before the HostedCluster uses it. Set this field explicitly to use a
+                      different/narrower credential (e.g. one scoped to a disconnected mirror).
                     properties:
                       name:
                         default: ""

docs/reference/crd-api.md

@@ -487,7 +487,7 @@ _Appears in:_
 | `memory` _string_ | Memory is the amount of memory allocated per VM (CRC VM, or each HyperShift KubeVirt<br />worker VM). E.g. "16Gi" for CRC, "6Gi" for a HyperShift worker. |  | Required: \{\} <br /> |
 | `cores` _integer_ | Cores is the number of vCPUs allocated per VM. |  | Minimum: 1 <br />Required: \{\} <br /> |
 | `rootVolumeSize` _string_ | RootVolumeSize is the size of the root disk (e.g. "35Gi"). |  | Optional: \{\} <br /> |
-| `pullSecretRef` _[LocalObjectReference](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.33/#localobjectreference-v1-core)_ | PullSecretRef references a Secret (in the operator's namespace) containing the<br />pull-secret used to pull the release payload / CRC bundle images. This field is<br />optional. When unset, the operator defaults to the management cluster's own<br />global pull secret (the "pull-secret" Secret in the "openshift-config"<br />namespace), which is present on every OpenShift cluster. Set this field<br />explicitly to use a different/narrower credential (e.g. one scoped to a<br />disconnected mirror). |  | Optional: \{\} <br /> |
+| `pullSecretRef` _[LocalObjectReference](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.33/#localobjectreference-v1-core)_ | PullSecretRef references a Secret in the ClusterInstance's namespace containing<br />the pull-secret used to pull the release payload / CRC bundle images. This field<br />is optional. When unset, the operator defaults to the management cluster's own<br />global pull secret (the "pull-secret" Secret in the "openshift-config"<br />namespace), which is present on every OpenShift cluster. For topology=hcp, an<br />explicit Secret is copied to the HostedCluster's namespace under a per-instance<br />name before the HostedCluster uses it. Set this field explicitly to use a<br />different/narrower credential (e.g. one scoped to a disconnected mirror). |  | Optional: \{\} <br /> |
 | `idmsRef` _[LocalObjectReference](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.33/#localobjectreference-v1-core)_ | IDMSRef optionally references an ImageDigestMirrorSet-shaped ConfigMap applied at<br />provision time for disconnected/mirrored registries. Interpreted per-topology:<br />for hcp it seeds HostedCluster.spec.imageContentSources; for crc it is applied<br />as an ImageDigestMirrorSet inside the guest cluster post-boot. |  | Optional: \{\} <br /> |
 | `bundleSSHKeyRef` _[LocalObjectReference](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.33/#localobjectreference-v1-core)_ | BundleSSHKeyRef references a Secret (in the operator's namespace) containing the<br />CRC bundle's SSH private key (the "id_ecdsa_crc" file shipped inside an official<br />.crcbundle, used to reach the booted VM as user "core"), under a data key named<br />"id_ecdsa" or "ssh-privatekey". This field is a FALLBACK. When CRCVersion is set,<br />the operator derives the SSH key automatically from the referenced CRCBundle<br />instead, and this field is ignored. When CRCVersion is unset, this field is<br />required for topology=crc: the crc-agent Job uses this key to SSH into the<br />freshly booted CRC VM and run the post-boot fixups natively (start kubelet,<br />approve kubelet CSRs, inject the real pull secret, set credentials, rewrite the<br />kubeconfig server to the externally-routable API Route hostname the<br />ClusterInstance controller provisions). Ignored for topology=hcp. |  | Optional: \{\} <br /> |
 | `hcpWorkerSSHKeyRef` _[LocalObjectReference](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.33/#localobjectreference-v1-core)_ | HCPWorkerSSHKeyRef optionally references a Secret (in the operator's namespace)<br />containing an SSH public key, under a data key named "id_rsa.pub", to inject as<br />an authorized key for the "core" user on every hcp NodePool worker (via<br />HostedCluster.spec.sshKey, see HyperShift's own ignition machine-config<br />generation). This field is only a debugging convenience (e.g. to inspect a<br />worker that is stuck before ever registering as a Node) and has no effect on<br />cluster function. Most deployments should leave it unset. Ignored for<br />topology=crc (see BundleSSHKeyRef for that path's own, unrelated SSH mechanism). |  | Optional: \{\} <br /> |

internal/controller/clusterinstance_controller.go

@@ -492,7 +492,9 @@ func (r *ClusterInstanceReconciler) ensureNamespace(ctx context.Context, name st
 //
 // If Template.PullSecretRef is set, resolvePullSecret uses that Secret
 // (which must exist in instance.Namespace and carry the dockerconfigjson
-// data key) as-is. This is the explicit-override path, for example for
+// data key) directly when targetNamespace is the instance namespace. When
+// targetNamespace differs, it copies the Secret there under the deterministic
+// per-instance name. This is the explicit-override path, for example for
 // disconnected or mirrored registries that need a narrower or different
 // credential than the management cluster's own.
 //
@@ -513,6 +515,7 @@ func (r *ClusterInstanceReconciler) ensureNamespace(ctx context.Context, name st
 // provisioning error.
 func (r *ClusterInstanceReconciler) resolvePullSecret(ctx context.Context, instance *brokerv1alpha1.ClusterInstance, targetNamespace string) (string, error) {
 	ref := instance.Spec.Template.PullSecretRef
+	var data []byte
 	if ref.Name != "" {
 		secret := &corev1.Secret{}
 		if err := r.Get(ctx, types.NamespacedName{Name: ref.Name, Namespace: instance.Namespace}, secret); err != nil {
@@ -524,19 +527,22 @@ func (r *ClusterInstanceReconciler) resolvePullSecret(ctx context.Context, insta
 		if len(secret.Data[resources.PullSecretDataKey]) == 0 {
 			return "", fmt.Errorf("pull secret %s/%s is missing data key %q", instance.Namespace, ref.Name, resources.PullSecretDataKey)
 		}
-		return ref.Name, nil
-	}
-
-	clusterSecret := &corev1.Secret{}
-	if err := r.Get(ctx, types.NamespacedName{Name: resources.ClusterPullSecretName, Namespace: resources.ClusterPullSecretNamespace}, clusterSecret); err != nil {
-		if apierrors.IsNotFound(err) {
-			return "", fmt.Errorf("template.pullSecretRef is unset and the cluster's default pull secret %s/%s was not found", resources.ClusterPullSecretNamespace, resources.ClusterPullSecretName)
+		if targetNamespace == instance.Namespace {
+			return ref.Name, nil
+		}
+		data = secret.Data[resources.PullSecretDataKey]
+	} else {
+		clusterSecret := &corev1.Secret{}
+		if err := r.Get(ctx, types.NamespacedName{Name: resources.ClusterPullSecretName, Namespace: resources.ClusterPullSecretNamespace}, clusterSecret); err != nil {
+			if apierrors.IsNotFound(err) {
+				return "", fmt.Errorf("template.pullSecretRef is unset and the cluster's default pull secret %s/%s was not found", resources.ClusterPullSecretNamespace, resources.ClusterPullSecretName)
+			}
+			return "", fmt.Errorf("getting cluster default pull secret %s/%s: %w", resources.ClusterPullSecretNamespace, resources.ClusterPullSecretName, err)
+		}
+		data = clusterSecret.Data[resources.PullSecretDataKey]
+		if len(data) == 0 {
+			return "", fmt.Errorf("cluster default pull secret %s/%s is missing data key %q", resources.ClusterPullSecretNamespace, resources.ClusterPullSecretName, resources.PullSecretDataKey)
 		}
-		return "", fmt.Errorf("getting cluster default pull secret %s/%s: %w", resources.ClusterPullSecretNamespace, resources.ClusterPullSecretName, err)
-	}
-	data := clusterSecret.Data[resources.PullSecretDataKey]
-	if len(data) == 0 {
-		return "", fmt.Errorf("cluster default pull secret %s/%s is missing data key %q", resources.ClusterPullSecretNamespace, resources.ClusterPullSecretName, resources.PullSecretDataKey)
 	}
 
 	copyName := resources.DefaultPullSecretName(instance.Name)

internal/controller/clusterinstance_hypershift.go

@@ -442,16 +442,14 @@ func (r *ClusterInstanceReconciler) teardownHyperShiftBacking(ctx context.Contex
 		return false, err
 	}
 
-	// resolvePullSecret materializes the default pull-secret copy in the
-	// HostedCluster's namespace only when Template.PullSecretRef is unset.
-	// That copy cannot carry an owner reference across namespaces, so it is
-	// not garbage-collected automatically like the same-namespace crc copy
-	// is. Delete it explicitly here. This delete is a harmless no-op
-	// (NotFound) when an explicit PullSecretRef was used instead, because
-	// that name never collides with resources.DefaultPullSecretName.
+	// resolvePullSecret materializes the per-instance pull-secret copy in the
+	// HostedCluster's namespace whenever the source and target namespaces
+	// differ. That copy cannot carry an owner reference across namespaces, so
+	// it is not garbage-collected automatically like the same-namespace crc
+	// copy is. Delete it explicitly here.
 	pullSecretName := resources.DefaultPullSecretName(instance.Name)
 	pullSecret := &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: pullSecretName, Namespace: namespace}}
-	if err := deleteObject(pullSecret, "default pull secret copy"); err != nil {
+	if err := deleteObject(pullSecret, "pull secret copy"); err != nil {
 		return false, err
 	}
 

internal/controller/clusterinstance_hypershift_test.go

@@ -17,11 +17,20 @@ limitations under the License.
 package controller
 
 import (
+	"context"
 	"testing"
 
+	configv1 "github.com/openshift/api/config/v1"
+	hyperv1beta1 "github.com/openshift/hypershift/api/hypershift/v1beta1"
+	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/client-go/kubernetes/scheme"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 
 	brokerv1alpha1 "github.com/caxu-rh/guestcluster-operator/api/v1alpha1"
+	"github.com/caxu-rh/guestcluster-operator/internal/resources"
 )
 
 // TestDesiredReplicas is a plain testing.T table test (not ginkgo/envtest --
@@ -58,3 +67,82 @@ func TestDesiredReplicas(t *testing.T) {
 		})
 	}
 }
+
+func TestReconcileHyperShiftCopiesExplicitPullSecret(t *testing.T) {
+	ctx := context.Background()
+	instance := &brokerv1alpha1.ClusterInstance{
+		ObjectMeta: metav1.ObjectMeta{Name: "hcp-pull-secret", Namespace: "tenant"},
+		Spec: brokerv1alpha1.ClusterInstanceSpec{
+			Type: brokerv1alpha1.TopologyHCP,
+			Template: brokerv1alpha1.ClusterTemplate{
+				OCPVersion:   "4.16.0",
+				ReleaseImage: "quay.io/openshift-release-dev/ocp-release:4.16.0-x86_64",
+				Memory:       "8Gi",
+				Cores:        2,
+				PullSecretRef: corev1.LocalObjectReference{
+					Name: "explicit-pull-secret",
+				},
+			},
+		},
+	}
+	pullSecretData := []byte(`{"auths":{"quay.io":{"auth":"explicit"}}}`)
+	pullSecret := &corev1.Secret{
+		ObjectMeta: metav1.ObjectMeta{Name: "explicit-pull-secret", Namespace: instance.Namespace},
+		Type:       corev1.SecretTypeDockerConfigJson,
+		Data:       map[string][]byte{resources.PullSecretDataKey: pullSecretData},
+	}
+	ingress := &configv1.Ingress{
+		ObjectMeta: metav1.ObjectMeta{Name: "cluster"},
+		Spec:       configv1.IngressSpec{Domain: "apps.example.test"},
+	}
+	node := &corev1.Node{
+		ObjectMeta: metav1.ObjectMeta{Name: "worker-0"},
+		Status: corev1.NodeStatus{
+			Conditions: []corev1.NodeCondition{{Type: corev1.NodeReady, Status: corev1.ConditionTrue}},
+			Addresses:  []corev1.NodeAddress{{Type: corev1.NodeInternalIP, Address: "192.0.2.10"}},
+		},
+	}
+	c := newHyperShiftFakeClient(t, instance, pullSecret, ingress, node)
+	r := &ClusterInstanceReconciler{Client: c, Scheme: c.Scheme()}
+
+	if _, err := r.reconcileHyperShift(ctx, instance); err != nil {
+		t.Fatalf("reconcileHyperShift: %v", err)
+	}
+
+	copyName := resources.DefaultPullSecretName(instance.Name)
+	copy := &corev1.Secret{}
+	if err := c.Get(ctx, client.ObjectKey{Name: copyName, Namespace: resources.DefaultHostedClusterNamespace}, copy); err != nil {
+		t.Fatalf("getting copied pull secret: %v", err)
+	}
+	if got := string(copy.Data[resources.PullSecretDataKey]); got != string(pullSecretData) {
+		t.Errorf("copied pull secret data = %q, want %q", got, pullSecretData)
+	}
+
+	hostedCluster := &hyperv1beta1.HostedCluster{}
+	if err := c.Get(ctx, client.ObjectKey{Name: resources.HostedClusterName(instance.Name), Namespace: resources.DefaultHostedClusterNamespace}, hostedCluster); err != nil {
+		t.Fatalf("getting HostedCluster: %v", err)
+	}
+	if got := hostedCluster.Spec.PullSecret.Name; got != copyName {
+		t.Errorf("HostedCluster.Spec.PullSecret.Name = %q, want %q", got, copyName)
+	}
+}
+
+func newHyperShiftFakeClient(t *testing.T, objects ...client.Object) client.Client {
+	t.Helper()
+	s := runtime.NewScheme()
+	if err := scheme.AddToScheme(s); err != nil {
+		t.Fatalf("adding core scheme: %v", err)
+	}
+	if err := brokerv1alpha1.AddToScheme(s); err != nil {
+		t.Fatalf("adding GuestCluster scheme: %v", err)
+	}
+	if err := configv1.AddToScheme(s); err != nil {
+		t.Fatalf("adding OpenShift Config scheme: %v", err)
+	}
+	if err := hyperv1beta1.AddToScheme(s); err != nil {
+		t.Fatalf("adding HyperShift scheme: %v", err)
+	}
+	return fake.NewClientBuilder().WithScheme(s).
+		WithStatusSubresource(&brokerv1alpha1.ClusterInstance{}).
+		WithObjects(objects...).Build()
+}

internal/resources/common.go

@@ -240,11 +240,10 @@ const (
 )
 
 // DefaultPullSecretName is the deterministic name of the per-instance Secret
-// the operator creates as a copy of ClusterPullSecretName, when
-// ClusterTemplate.PullSecretRef is left unset. The operator places it in
-// whatever namespace the topology needs: the instance's own namespace for
-// crc, the HostedCluster's namespace for hcp-*. See resolvePullSecret in the
-// ClusterInstance controller.
+// the operator creates when it must materialize a pull secret in a different
+// namespace. The operator places it in whatever namespace the topology needs:
+// the instance's own namespace for crc, or the HostedCluster's namespace for
+// hcp. See resolvePullSecret in the ClusterInstance controller.
 func DefaultPullSecretName(instanceName string) string {
 	return instanceName + "-pull-secret"
 }