Merge pull request #5487 from csrwng/fix_progressing_condition

OCPBUGS-48532: Fix IsProgressing condition in HostedClusters

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,
 		},