diff --git a/README.md b/README.md index 75e81f2..5b7e55f 100644 --- a/README.md +++ b/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 diff --git a/api/v1alpha1/clusterpool_types.go b/api/v1alpha1/clusterpool_types.go index 1c62ecb..0b8c33f 100644 --- a/api/v1alpha1/clusterpool_types.go +++ b/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"` diff --git a/config/crd/bases/guestcluster.opdev.io_clusterinstances.yaml b/config/crd/bases/guestcluster.opdev.io_clusterinstances.yaml index 8e411f5..45fb556 100644 --- a/config/crd/bases/guestcluster.opdev.io_clusterinstances.yaml +++ b/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: "" diff --git a/config/crd/bases/guestcluster.opdev.io_clusterpools.yaml b/config/crd/bases/guestcluster.opdev.io_clusterpools.yaml index 9c6f09b..33a70fb 100644 --- a/config/crd/bases/guestcluster.opdev.io_clusterpools.yaml +++ b/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: "" diff --git a/docs/reference/crd-api.md b/docs/reference/crd-api.md index a6f0c2b..ac65fca 100644 --- a/docs/reference/crd-api.md +++ b/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
worker VM). E.g. "16Gi" for CRC, "6Gi" for a HyperShift worker. | | Required: \{\}
| | `cores` _integer_ | Cores is the number of vCPUs allocated per VM. | | Minimum: 1
Required: \{\}
| | `rootVolumeSize` _string_ | RootVolumeSize is the size of the root disk (e.g. "35Gi"). | | Optional: \{\}
| -| `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
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). | | Optional: \{\}
| +| `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
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. 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: \{\}
| | `idmsRef` _[LocalObjectReference](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.33/#localobjectreference-v1-core)_ | IDMSRef optionally references an ImageDigestMirrorSet-shaped ConfigMap applied at
provision time for disconnected/mirrored registries. Interpreted per-topology:
for hcp it seeds HostedCluster.spec.imageContentSources; for crc it is applied
as an ImageDigestMirrorSet inside the guest cluster post-boot. | | Optional: \{\}
| | `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
CRC bundle's SSH private key (the "id_ecdsa_crc" file shipped inside an official
.crcbundle, used to reach the booted VM as user "core"), under a data key named
"id_ecdsa" or "ssh-privatekey". This field is a FALLBACK. When CRCVersion is set,
the operator derives the SSH key automatically from the referenced CRCBundle
instead, and this field is ignored. When CRCVersion is unset, this field is
required for topology=crc: the crc-agent Job uses this key to SSH into the
freshly booted CRC VM and run the post-boot fixups natively (start kubelet,
approve kubelet CSRs, inject the real pull secret, set credentials, rewrite the
kubeconfig server to the externally-routable API Route hostname the
ClusterInstance controller provisions). Ignored for topology=hcp. | | Optional: \{\}
| | `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)
containing an SSH public key, under a data key named "id_rsa.pub", to inject as
an authorized key for the "core" user on every hcp NodePool worker (via
HostedCluster.spec.sshKey, see HyperShift's own ignition machine-config
generation). This field is only a debugging convenience (e.g. to inspect a
worker that is stuck before ever registering as a Node) and has no effect on
cluster function. Most deployments should leave it unset. Ignored for
topology=crc (see BundleSSHKeyRef for that path's own, unrelated SSH mechanism). | | Optional: \{\}
| diff --git a/internal/controller/clusterinstance_controller.go b/internal/controller/clusterinstance_controller.go index d4da5f5..0678457 100644 --- a/internal/controller/clusterinstance_controller.go +++ b/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) diff --git a/internal/controller/clusterinstance_hypershift.go b/internal/controller/clusterinstance_hypershift.go index 0dc9bd0..248ddf4 100644 --- a/internal/controller/clusterinstance_hypershift.go +++ b/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 } diff --git a/internal/controller/clusterinstance_hypershift_test.go b/internal/controller/clusterinstance_hypershift_test.go index fc7b8eb..58dafcc 100644 --- a/internal/controller/clusterinstance_hypershift_test.go +++ b/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() +} diff --git a/internal/resources/common.go b/internal/resources/common.go index 4285de3..671da81 100644 --- a/internal/resources/common.go +++ b/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" }