Fix IsProgressing condition in HostedClusters

After #5168, the release image passed to the CVO is an image with a
digest (which is needed for offline). Because this doesn't always match
the release image specified in the HostedCluster.spec.release.image
field, code that relies on comparing the image in CVO status with what's
in the spec is now not matching. One effect is that the Progressing
condition remains set to "true" even if a HostedCluster has completed
provisioning.

This PR makes the following changes in order to fix it:
- Adds code to the HyperShift operator to compare either the image in
  spec as is or converted to a digest with the image reported by the CVO
  in status.
- Removes the call to obtain the release image for a specific
  architecture when obtaining an image with digest for the release.
- Removes code that uses fmt.Sprintf to obtain an image ref string from
  a docker reference in favor of ref.String() except in places where we
  can't rely on ref.String() to give us the expected reference.

control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go

@@ -3676,31 +3676,19 @@ func (r *HostedControlPlaneReconciler) reconcileClusterVersionOperator(ctx conte
 	if err := r.Get(ctx, client.ObjectKeyFromObject(pullSecret), pullSecret); err != nil {
 		return fmt.Errorf("failed to get pull secret for namespace %s: %w", hcp.Namespace, err)
 	}
-
-	cpRef, err := registryclient.GetCorrectArchImage(ctx, "cluster-version-operator", p.ControlPlaneImage, pullSecret.Data[corev1.DockerConfigJsonKey], r.ImageMetadataProvider)
+	pullSecretBytes := pullSecret.Data[corev1.DockerConfigJsonKey]
+	_, controlPlaneReleaseImageRef, err := r.ImageMetadataProvider.GetDigest(ctx, p.ControlPlaneImage, pullSecretBytes)
 	if err != nil {
-		return fmt.Errorf("failed to parse control plane release image %s: %w", cpRef, err)
+		return fmt.Errorf("failed to get control plane release image digest %s: %w", p.ControlPlaneImage, err)
 	}
-
-	_, cpReleaseImageRef, err := r.ImageMetadataProvider.GetDigest(ctx, cpRef, pullSecret.Data[corev1.DockerConfigJsonKey])
-	if err != nil {
-		return fmt.Errorf("failed to get control plane release image digest %s: %w", cpRef, err)
-	}
-
-	controlPlaneReleaseImage = fmt.Sprintf("%s/%s/%s", cpReleaseImageRef.Registry, cpReleaseImageRef.Namespace, cpReleaseImageRef.NameString())
+	controlPlaneReleaseImage = controlPlaneReleaseImageRef.String()
 
 	if p.ControlPlaneImage != hcp.Spec.ReleaseImage {
-		dpRef, err := registryclient.GetCorrectArchImage(ctx, "cluster-version-operator", hcp.Spec.ReleaseImage, pullSecret.Data[corev1.DockerConfigJsonKey], r.ImageMetadataProvider)
+		_, dataPlaneReleaseImageRef, err := r.ImageMetadataProvider.GetDigest(ctx, hcp.Spec.ReleaseImage, pullSecretBytes)
 		if err != nil {
-			return fmt.Errorf("failed to parse data plane release image %s: %w", dpRef, err)
+			return fmt.Errorf("failed to get release image digest %s: %w", hcp.Spec.ReleaseImage, err)
 		}
-
-		_, dpReleaseImageRef, err := r.ImageMetadataProvider.GetDigest(ctx, dpRef, pullSecret.Data[corev1.DockerConfigJsonKey])
-		if err != nil {
-			return fmt.Errorf("failed to get data plane release image digest %s: %w", dpRef, err)
-		}
-
-		dataPlaneReleaseImage = fmt.Sprintf("%s/%s/%s", dpReleaseImageRef.Registry, dpReleaseImageRef.Namespace, dpReleaseImageRef.NameString())
+		dataPlaneReleaseImage = dataPlaneReleaseImageRef.String()
 	} else {
 		dataPlaneReleaseImage = controlPlaneReleaseImage
 	}

control-plane-operator/controllers/hostedcontrolplane/v2/cvo/deployment.go

@@ -10,7 +10,6 @@ import (
 	hyperapi "github.com/openshift/hypershift/support/api"
 	"github.com/openshift/hypershift/support/config"
 	component "github.com/openshift/hypershift/support/controlplane-component"
-	"github.com/openshift/hypershift/support/releaseinfo/registryclient"
 	"github.com/openshift/hypershift/support/util"
 
 	configv1 "github.com/openshift/api/config/v1"
@@ -260,31 +259,26 @@ func discoverCVOReleaseImages(cpContext component.WorkloadContext) (string, stri
 	if err := cpContext.Client.Get(cpContext.Context, client.ObjectKeyFromObject(pullSecret), pullSecret); err != nil {
 		return "", "", fmt.Errorf("failed to get pull secret for namespace %s: %w", cpContext.HCP.Namespace, err)
 	}
+	pullSecretBytes := pullSecret.Data[corev1.DockerConfigJsonKey]
 
-	cpRef, err := registryclient.GetCorrectArchImage(cpContext.Context, "cluster-version-operator", cpContext.HCP.Spec.ReleaseImage, pullSecret.Data[corev1.DockerConfigJsonKey], cpContext.ImageMetadataProvider)
-	if err != nil {
-		return "", "", fmt.Errorf("failed to parse control plane release image %s: %w", cpRef, err)
+	cpReleaseImage := cpContext.HCP.Spec.ReleaseImage
+	if cpContext.HCP.Spec.ControlPlaneReleaseImage != nil {
+		cpReleaseImage = *cpContext.HCP.Spec.ControlPlaneReleaseImage
 	}
 
-	_, cpReleaseImageRef, err := cpContext.ImageMetadataProvider.GetDigest(cpContext.Context, cpRef, pullSecret.Data[corev1.DockerConfigJsonKey])
+	_, controlPlaneReleaseImageRef, err := cpContext.ImageMetadataProvider.GetDigest(cpContext.Context, cpReleaseImage, pullSecretBytes)
 	if err != nil {
-		return "", "", fmt.Errorf("failed to get control plane release image digest %s: %w", cpRef, err)
+		return "", "", fmt.Errorf("failed to get control plane release image digest %s: %w", controlPlaneReleaseImageRef, err)
 	}
+	controlPlaneReleaseImage = controlPlaneReleaseImageRef.String()
 
-	controlPlaneReleaseImage = fmt.Sprintf("%s/%s/%s", cpReleaseImageRef.Registry, cpReleaseImageRef.Namespace, cpReleaseImageRef.NameString())
-
-	if cpContext.HCP.Spec.ControlPlaneReleaseImage != nil && *cpContext.HCP.Spec.ControlPlaneReleaseImage != cpContext.HCP.Spec.ReleaseImage {
-		dpRef, err := registryclient.GetCorrectArchImage(cpContext.Context, "cluster-version-operator", cpContext.HCP.Spec.ReleaseImage, pullSecret.Data[corev1.DockerConfigJsonKey], cpContext.ImageMetadataProvider)
-		if err != nil {
-			return "", "", fmt.Errorf("failed to parse data plane release image %s: %w", dpRef, err)
-		}
-
-		_, dpReleaseImageRef, err := cpContext.ImageMetadataProvider.GetDigest(cpContext.Context, dpRef, pullSecret.Data[corev1.DockerConfigJsonKey])
+	if cpReleaseImage != cpContext.HCP.Spec.ReleaseImage {
+		_, dataPlaneReleaseImageRef, err := cpContext.ImageMetadataProvider.GetDigest(cpContext.Context, cpContext.HCP.Spec.ReleaseImage, pullSecret.Data[corev1.DockerConfigJsonKey])
 		if err != nil {
-			return "", "", fmt.Errorf("failed to get data plane release image digest %s: %w", dpRef, err)
+			return "", "", fmt.Errorf("failed to get data plane release image digest %s: %w", cpContext.HCP.Spec.ReleaseImage, err)
 		}
 
-		dataPlaneReleaseImage = fmt.Sprintf("%s/%s/%s", dpReleaseImageRef.Registry, dpReleaseImageRef.Namespace, dpReleaseImageRef.NameString())
+		dataPlaneReleaseImage = dataPlaneReleaseImageRef.String()
 	} else {
 		dataPlaneReleaseImage = controlPlaneReleaseImage
 	}

hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go

@@ -1066,6 +1066,10 @@ func (r *HostedClusterReconciler) reconcile(ctx context.Context, req ctrl.Reques
 	}
 
 	hcluster.Status.PayloadArch = payloadArch
+	pullSecretBytes, err := hyperutil.GetPullSecretBytes(ctx, r.Client, hcluster)
+	if err != nil {
+		return ctrl.Result{}, err
+	}
 
 	releaseImage, err := r.lookupReleaseImage(ctx, hcluster, releaseProvider)
 	if err != nil {
@@ -1080,7 +1084,15 @@ func (r *HostedClusterReconciler) reconcile(ctx context.Context, req ctrl.Reques
 			Message:            "HostedCluster is at expected version",
 			Reason:             hyperv1.AsExpectedReason,
 		}
-		progressing, err := isProgressing(hcluster, releaseImage)
+		refWithDigest := func() (string, error) {
+			_, ref, err := registryClientImageMetadataProvider.GetDigest(ctx, hcluster.Spec.Release.Image, pullSecretBytes)
+			if err != nil {
+				return "", err
+			}
+			return ref.String(), nil
+		}
+
+		progressing, err := isProgressing(hcluster, releaseImage, refWithDigest)
 		if err != nil {
 			condition.Status = metav1.ConditionFalse
 			condition.Message = err.Error()
@@ -1172,10 +1184,6 @@ func (r *HostedClusterReconciler) reconcile(ctx context.Context, req ctrl.Reques
 		}
 	}
 
-	pullSecretBytes, err := hyperutil.GetPullSecretBytes(ctx, r.Client, hcluster)
-	if err != nil {
-		return ctrl.Result{}, err
-	}
 	controlPlaneOperatorImage, err := GetControlPlaneOperatorImage(ctx, hcluster, releaseProvider, r.HypershiftOperatorImage, pullSecretBytes)
 	if err != nil {
 		return ctrl.Result{}, fmt.Errorf("failed to get controlPlaneOperatorImage: %w", err)
@@ -4239,7 +4247,7 @@ func (r *HostedClusterReconciler) validateReleaseImage(ctx context.Context, hc *
 	return supportedversion.IsValidReleaseVersion(&version, currentVersion, &supportedversion.LatestSupportedVersion, &minSupportedVersion, hc.Spec.Networking.NetworkType, hc.Spec.Platform.Type)
 }
 
-func isProgressing(hc *hyperv1.HostedCluster, releaseImage *releaseinfo.ReleaseImage) (bool, error) {
+func isProgressing(hc *hyperv1.HostedCluster, releaseImage *releaseinfo.ReleaseImage, refWithDigest func() (string, error)) (bool, error) {
 	for _, condition := range hc.Status.Conditions {
 		switch string(condition.Type) {
 		case string(hyperv1.SupportedHostedCluster), string(hyperv1.ValidHostedClusterConfiguration), string(hyperv1.ValidReleaseImage), string(hyperv1.ReconciliationActive):
@@ -4254,7 +4262,12 @@ func isProgressing(hc *hyperv1.HostedCluster, releaseImage *releaseinfo.ReleaseI
 		}
 	}
 
-	if hc.Status.Version == nil || hc.Spec.Release.Image != hc.Status.Version.Desired.Image {
+	withDigest, err := refWithDigest()
+	if err != nil {
+		return false, err
+	}
+
+	if hc.Status.Version == nil || (hc.Spec.Release.Image != hc.Status.Version.Desired.Image && withDigest != hc.Status.Version.Desired.Image) {
 		// cluster is doing initial rollout or upgrading
 		return true, nil
 	}

hypershift-operator/controllers/hostedcluster/hostedcluster_controller_test.go

@@ -2485,10 +2485,11 @@ func TestReconciliationSuccessConditionSetting(t *testing.T) {
 
 func TestIsProgressing(t *testing.T) {
 	tests := []struct {
-		name    string
-		hc      *hyperv1.HostedCluster
-		want    bool
-		wantErr bool
+		name       string
+		hc         *hyperv1.HostedCluster
+		withDigest string
+		want       bool
+		wantErr    bool
 	}{
 		{
 			name: "stable at release",
@@ -2513,6 +2514,30 @@ func TestIsProgressing(t *testing.T) {
 			want:    false,
 			wantErr: false,
 		},
+		{
+			name: "stable at release with digest",
+			hc: &hyperv1.HostedCluster{
+				Spec: hyperv1.HostedClusterSpec{
+					Release: hyperv1.Release{
+						Image: "release-1.2",
+					},
+					PullSecret: corev1.LocalObjectReference{
+						Name: "pull-secret",
+					},
+				},
+				Status: hyperv1.HostedClusterStatus{
+					Version: &hyperv1.ClusterVersionStatus{
+						Desired: configv1.Release{
+							Image:   "release-1.2@sha12345",
+							Version: "1.2.0",
+						},
+					},
+				},
+			},
+			withDigest: "release-1.2@sha12345",
+			want:       false,
+			wantErr:    false,
+		},
 		{
 			name: "cluster is rolling out",
 			hc: &hyperv1.HostedCluster{
@@ -2528,6 +2553,30 @@ func TestIsProgressing(t *testing.T) {
 			want:    true,
 			wantErr: false,
 		},
+		{
+			name: "cluster is upgrading with digest",
+			hc: &hyperv1.HostedCluster{
+				Spec: hyperv1.HostedClusterSpec{
+					Release: hyperv1.Release{
+						Image: "release-1.3",
+					},
+					PullSecret: corev1.LocalObjectReference{
+						Name: "pull-secret",
+					},
+				},
+				Status: hyperv1.HostedClusterStatus{
+					Version: &hyperv1.ClusterVersionStatus{
+						Desired: configv1.Release{
+							Image:   "release-1.2@sha12345",
+							Version: "1.2.0",
+						},
+					},
+				},
+			},
+			withDigest: "release-1.3@sha67890",
+			want:       true,
+			wantErr:    false,
+		},
 		{
 			name: "cluster is upgrading",
 			hc: &hyperv1.HostedCluster{
@@ -2679,7 +2728,9 @@ func TestIsProgressing(t *testing.T) {
 			if err != nil {
 				t.Errorf("isProgressing() internal err = %v", err)
 			}
-			got, err := isProgressing(tt.hc, releaseImage)
+			got, err := isProgressing(tt.hc, releaseImage, func() (string, error) {
+				return tt.withDigest, nil
+			})
 			if (err != nil) != tt.wantErr {
 				t.Errorf("isProgressing() error = %v, wantErr %v", err, tt.wantErr)
 				return

ignition-server/controllers/local_ignitionprovider.go

@@ -225,7 +225,7 @@ func (p *LocalIgnitionProvider) GetPayload(ctx context.Context, releaseImage, cu
 		return nil, err
 	}
 
-	mcoComposedImage := fmt.Sprintf("%s/%s/%s", checkedMcoImage.Registry, checkedMcoImage.Namespace, checkedMcoImage.NameString())
+	mcoComposedImage := checkedMcoImage.String()
 	if mcoComposedImage != mcoImage {
 		mcoImage = mcoComposedImage
 		log.Info(fmt.Sprintf("using mirrored %s image %v", component, mcoImage))
@@ -376,7 +376,7 @@ func (p *LocalIgnitionProvider) GetPayload(ctx context.Context, releaseImage, cu
 			return err
 		}
 
-		ccaComposedImage := fmt.Sprintf("%s/%s/%s", checkedClusterConfigImage.Registry, checkedClusterConfigImage.Namespace, checkedClusterConfigImage.NameString())
+		ccaComposedImage := checkedClusterConfigImage.String()
 		if ccaComposedImage != clusterConfigImage {
 			clusterConfigImage = ccaComposedImage
 			log.Info(fmt.Sprintf("using mirrored %s image %v", clusterConfigComponent, ccaComposedImage))

support/util/imagemetadata.go

@@ -163,7 +163,7 @@ func (r *RegistryClientImageMetadataProvider) GetDigest(ctx context.Context, ima
 	// Get the image repo info based the source/mirrors in the ICSPs/IDMSs
 	ref = seekOverride(ctx, r.OpenShiftImageRegistryOverrides, parsedImageRef)
 
-	composedRef := fmt.Sprintf("%s/%s/%s", ref.Registry, ref.Namespace, ref.NameString())
+	composedRef := ref.String()
 
 	// If the overridden image name is in the cache, return early
 	if imageDigest, exists := digestCache.Get(composedRef); exists {
@@ -236,7 +236,7 @@ func (r *RegistryClientImageMetadataProvider) GetManifest(ctx context.Context, i
 		}
 	}
 
-	composedRef := fmt.Sprintf("%s/%s/%s", ref.Registry, ref.Namespace, ref.NameString())
+	composedRef := ref.String()
 
 	digestsManifest, srcDigest, err := getManifest(ctx, composedRef, pullSecret)
 	if err != nil {
@@ -266,7 +266,7 @@ func (r *RegistryClientImageMetadataProvider) GetMetadata(ctx context.Context, i
 
 	// Get the image repo info based the source/mirrors in the ICSPs/IDMSs
 	ref = seekOverride(ctx, r.OpenShiftImageRegistryOverrides, parsedImageRef)
-	composedRef := fmt.Sprintf("%s/%s/%s", ref.Registry, ref.Namespace, ref.NameString())
+	composedRef := ref.String()
 
 	return getMetadata(ctx, composedRef, pullSecret)
 }
@@ -398,25 +398,25 @@ func GetRegistryOverrides(ctx context.Context, ref reference.DockerImageReferenc
 	// and it will assume that is the Name instead
 	if sourceRef.Namespace == "" {
 		if sourceRef.Name == ref.Namespace {
-			composedImage := fmt.Sprintf("%s/%s/%s", mirrorRef.Registry, ref.Namespace, ref.NameString())
+			composedImage := buildComposedRef(mirrorRef.Registry, ref.Namespace, ref.NameString())
 			composedRef, err := reference.Parse(composedImage)
 			if err != nil {
 				return nil, false, fmt.Errorf("failed to parse composed image reference (partial match) %q: %w", source, err)
 			}
-			log.Info("registry override coincidence found (namespace)", "original", fmt.Sprintf("%s/%s/%s", ref.Registry, ref.Namespace, ref.NameString()), "mirror", mirror, "composed", composedRef)
+			log.Info("registry override coincidence found (namespace)", "original", buildComposedRef(ref.Registry, ref.Namespace, ref.NameString()), "mirror", mirror, "composed", composedRef)
 			return &composedRef, true, nil
 		}
 	}
 
 	if ref.Namespace == sourceRef.Namespace && ref.Name == sourceRef.Name {
 		mirrorRef.ID = ref.ID
 		mirrorRef.Tag = ref.Tag
-		composedImage := fmt.Sprintf("%s/%s/%s", mirrorRef.Registry, mirrorRef.Namespace, mirrorRef.NameString())
+		composedImage := buildComposedRef(mirrorRef.Registry, mirrorRef.Namespace, mirrorRef.NameString())
 		composedRef, err := reference.Parse(composedImage)
 		if err != nil {
 			return nil, false, fmt.Errorf("failed to parse composed image reference (exact match) %q: %w", source, err)
 		}
-		log.Info("registry override coincidence found (exact match)", "original", fmt.Sprintf("%s/%s/%s", ref.Registry, ref.Namespace, ref.NameString()), "mirror", mirror, "composed", composedRef)
+		log.Info("registry override coincidence found (exact match)", "original", buildComposedRef(ref.Registry, ref.Namespace, ref.NameString()), "mirror", mirror, "composed", composedRef)
 		return &composedRef, true, nil
 	}
 
@@ -465,3 +465,9 @@ func seekOverride(ctx context.Context, openshiftImageRegistryOverrides map[strin
 	}
 	return &parsedImageReference
 }
+
+// buildComposedRef creates a docker image pull reference given its
+// separate components
+func buildComposedRef(registry, namespace, name string) string {
+	return fmt.Sprintf("%s/%s/%s", registry, namespace, name)
+}

support/util/imagemetadata_test.go

@@ -152,6 +152,7 @@ func TestGetManifest(t *testing.T) {
 		})
 	}
 }
+
 func TestGetDigest(t *testing.T) {
 	ctx := context.TODO()
 	pullSecret := []byte("{}")
@@ -166,7 +167,7 @@ func TestGetDigest(t *testing.T) {
 	}{
 		{
 			name:        "if failed to parse image reference",
-			imageRef:    "invalid-image-ref",
+			imageRef:    "::invalid-image-ref",
 			pullSecret:  pullSecret,
 			expectedErr: true,
 		},