Merge pull request #9957 from CrystalChun/use-clusterimageset

MGMT-23339: Use release image URL from ClusterImageSet for KubeAPI clusters

Author: openshift-merge-bot[bot]

cmd/agentbasedinstaller/register.go

@@ -105,7 +105,7 @@ func RegisterCluster(ctx context.Context, log *log.Logger, bmInventory *client.A
 		operatorInfo = operatorsToArray(operatorList)
 	}
 
-	clusterParams := controllers.CreateClusterParams(&cd, &aci, pullSecret, releaseImageVersion, releaseImageCPUArch, nil, operatorInfo)
+	clusterParams := controllers.CreateClusterParams(&cd, &aci, pullSecret, &models.ReleaseImage{Version: &releaseImageVersion, CPUArchitecture: &releaseImageCPUArch, URL: &releaseImage}, nil, operatorInfo)
 
 	if aci.Spec.Networking.NetworkType != "" {
 		clusterParams.NetworkType = &aci.Spec.Networking.NetworkType

internal/bminventory/inventory.go

@@ -564,11 +564,9 @@ func (b *bareMetalInventory) getOLMMonitoredOperators(log *logrus.Entry, cluster
 }
 
 func (b *bareMetalInventory) getNewClusterReleaseImage(ctx context.Context, params *models.ClusterCreateParams, arch string) (*models.ReleaseImage, error) {
-	releaseImage, err := b.versionsHandler.GetReleaseImage(ctx, swag.StringValue(params.OpenshiftVersion),
-		arch, swag.StringValue(params.PullSecret))
+	releaseImage, err := b.getReleaseImage(ctx, params, arch)
 	if err != nil {
-		return nil, errors.Wrapf(err, "Openshift version %s for CPU architecture %s is not supported",
-			swag.StringValue(params.OpenshiftVersion), arch)
+		return nil, err
 	}
 
 	if !b.EnableImageService {
@@ -590,6 +588,37 @@ func (b *bareMetalInventory) getNewClusterReleaseImage(ctx context.Context, para
 	return releaseImage, nil
 }
 
+func (b *bareMetalInventory) getReleaseImage(ctx context.Context, params *models.ClusterCreateParams, arch string) (*models.ReleaseImage, error) {
+	if params.OcpReleaseImage != "" {
+		releaseImage, err := b.versionsHandler.GetReleaseImageByURL(ctx, params.OcpReleaseImage, swag.StringValue(params.PullSecret))
+		if err != nil {
+			return nil, errors.Wrapf(err, "failed to get release image by URL %s during cluster registration", params.OcpReleaseImage)
+		}
+
+		normalizedArch := common.NormalizeCPUArchitecture(arch)
+		if normalizedArch == common.MultiCPUArchitecture {
+			if len(releaseImage.CPUArchitectures) <= 1 {
+				return nil, errors.Errorf("release image URL %s does not support requested CPU architecture %s", params.OcpReleaseImage, normalizedArch)
+			}
+			return releaseImage, nil
+		}
+		for _, cpuArch := range releaseImage.CPUArchitectures {
+			normalizedCpuArch := common.NormalizeCPUArchitecture(cpuArch)
+			if normalizedCpuArch == normalizedArch {
+				return releaseImage, nil
+			}
+		}
+		return nil, errors.Errorf("Requested CPU architecture %s is not supported for release image URL %s. Supported architectures in release image: %v", normalizedArch, params.OcpReleaseImage, releaseImage.CPUArchitectures)
+	}
+
+	releaseImage, err := b.versionsHandler.GetReleaseImage(ctx, swag.StringValue(params.OpenshiftVersion), arch, swag.StringValue(params.PullSecret))
+	if err != nil {
+		return nil, errors.Wrapf(err, "Openshift version %s for CPU architecture %s is not supported",
+			swag.StringValue(params.OpenshiftVersion), arch)
+	}
+	return releaseImage, nil
+}
+
 func MarshalNewClusterParamsNoPullSecret(params installer.V2RegisterClusterParams, log *logrus.Entry) []byte {
 	// Masked the Pull Secret to prevent its value from being displayed in the logs.
 	clusterParamsNoPullSecret := *params.NewClusterParams

internal/bminventory/inventory_test.go

@@ -265,17 +265,30 @@ func toMac(macStr string) *strfmt.MAC {
 	return &mac
 }
 
-func mockClusterRegisterSteps() {
+func mockClusterRegisterSteps(withReleaseImageURL bool) {
 	mockSecretValidator.EXPECT().ValidatePullSecret(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).Times(1)
-	mockVersions.EXPECT().GetReleaseImage(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(common.TestDefaultConfig.ReleaseImage, nil).Times(1)
+	if withReleaseImageURL {
+		mockVersions.EXPECT().GetReleaseImageByURL(gomock.Any(), gomock.Any(), gomock.Any()).Return(common.TestDefaultConfig.ReleaseImage, nil).Times(1)
+	} else {
+		mockVersions.EXPECT().GetReleaseImage(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(common.TestDefaultConfig.ReleaseImage, nil).Times(1)
+	}
 
 	mockOSImages.EXPECT().GetOsImage(gomock.Any(), gomock.Any()).Return(common.TestDefaultConfig.OsImage, nil).Times(1)
 	mockOperatorManager.EXPECT().GetSupportedOperatorsByType(models.OperatorTypeBuiltin).Return([]*models.MonitoredOperator{&common.TestDefaultConfig.MonitoredOperator}).Times(1)
 	mockProviderRegistry.EXPECT().SetPlatformUsages(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil)
 }
 
 func mockClusterRegisterSuccess(withEvents bool) {
-	mockClusterRegisterSteps()
+	mockClusterRegisterSteps(false)
+	mockMetric.EXPECT().ClusterRegistered().Times(1)
+
+	if withEvents {
+		mockEvents.EXPECT().SendClusterEvent(gomock.Any(), eventstest.NewEventMatcher(
+			eventstest.WithNameMatcher(eventgen.ClusterRegistrationSucceededEventName))).Times(1)
+	}
+}
+func mockClusterRegisterSuccessWithReleaseImageURL(withEvents bool) {
+	mockClusterRegisterSteps(true)
 	mockMetric.EXPECT().ClusterRegistered().Times(1)
 
 	if withEvents {
@@ -14676,7 +14689,7 @@ var _ = Describe("RegisterCluster", func() {
 		})
 		Context("Pull secret validation", func() {
 			It("Successfully validates the pull secret if it does not contain auth for a mirrored registry", func() {
-				mockClusterRegisterSuccess(true)
+				mockClusterRegisterSuccessWithReleaseImageURL(true)
 				mockAMSSubscription(ctx)
 				conf, _ := getMirrorRegistryConfigurations(getSecureRegistryToml("fake-registry.example.com", mirrorRegistry), mirrorRegistryCertificate)
 				params := getClusterCreateParams()
@@ -16373,7 +16386,7 @@ var _ = Describe("RegisterCluster", func() {
 	It("cluster api failed to register", func() {
 		bm.clusterApi = mockClusterApi
 		mockClusterApi.EXPECT().RegisterCluster(ctx, gomock.Any()).Return(errors.Errorf("error")).Times(1)
-		mockClusterRegisterSteps()
+		mockClusterRegisterSteps(false)
 
 		reply := bm.V2RegisterCluster(ctx, installer.V2RegisterClusterParams{
 			NewClusterParams: getDefaultClusterCreateParams(),
@@ -16433,6 +16446,20 @@ var _ = Describe("RegisterCluster", func() {
 		Expect(actual.Payload.OcpReleaseImage).To(Equal(common.TestDefaultConfig.ReleaseImageUrl))
 	})
 
+	It("successfully registers cluster with openshift release image URL defined in the cluster create params", func() {
+		mockClusterRegisterSuccessWithReleaseImageURL(true)
+		mockAMSSubscription(ctx)
+		clusterParams := getDefaultClusterCreateParams()
+		clusterParams.OcpReleaseImage = common.TestDefaultConfig.ReleaseImageUrl
+		reply := bm.V2RegisterCluster(ctx, installer.V2RegisterClusterParams{
+			NewClusterParams: clusterParams,
+		})
+		Expect(reflect.TypeOf(reply)).Should(Equal(reflect.TypeOf(installer.NewV2RegisterClusterCreated())))
+		actual := reply.(*installer.V2RegisterClusterCreated)
+		Expect(actual.Payload.OpenshiftVersion).To(Equal(common.TestDefaultConfig.ReleaseVersion))
+		Expect(actual.Payload.OcpReleaseImage).To(Equal(common.TestDefaultConfig.ReleaseImageUrl))
+	})
+
 	It("Register cluster with default CPU architecture", func() {
 		mockClusterRegisterSuccess(true)
 		mockAMSSubscription(ctx)
@@ -16531,6 +16558,38 @@ var _ = Describe("RegisterCluster", func() {
 		actual := reply.(*installer.V2RegisterClusterCreated)
 		Expect(actual.Payload.CPUArchitecture).To(Equal(common.TestDefaultConfig.CPUArchitecture))
 	})
+	Context("Register with release image URL", func() {
+		It("should register cluster with release image URL successfully", func() {
+			mockClusterRegisterSuccessWithReleaseImageURL(true)
+			mockAMSSubscription(ctx)
+			clusterParams := getDefaultClusterCreateParams()
+			clusterParams.OcpReleaseImage = common.TestDefaultConfig.ReleaseImageUrl
+			reply := bm.V2RegisterCluster(ctx, installer.V2RegisterClusterParams{
+				NewClusterParams: clusterParams,
+			})
+			Expect(reflect.TypeOf(reply)).Should(Equal(reflect.TypeOf(installer.NewV2RegisterClusterCreated())))
+			actual := reply.(*installer.V2RegisterClusterCreated)
+			Expect(actual.Payload.OpenshiftVersion).To(Equal(common.TestDefaultConfig.ReleaseVersion))
+			Expect(actual.Payload.OcpReleaseImage).To(Equal(common.TestDefaultConfig.ReleaseImageUrl))
+		})
+
+		It("should fail if openshift release image is different from cluster architecture", func() {
+			mockVersions.EXPECT().GetReleaseImageByURL(gomock.Any(), gomock.Any(), gomock.Any()).Return(&models.ReleaseImage{
+				CPUArchitecture:  swag.String(common.ARM64CPUArchitecture),
+				CPUArchitectures: []string{common.ARM64CPUArchitecture},
+				OpenshiftVersion: swag.String("4.12.0"),
+				URL:              swag.String(common.TestDefaultConfig.ReleaseImageUrl),
+				Version:          swag.String("4.12.0"),
+			}, nil).Times(1)
+
+			clusterParams := getDefaultClusterCreateParams()
+			clusterParams.OcpReleaseImage = common.TestDefaultConfig.ReleaseImageUrl
+			reply := bm.V2RegisterCluster(ctx, installer.V2RegisterClusterParams{
+				NewClusterParams: clusterParams,
+			})
+			Expect(reply).Should(BeAssignableToTypeOf(common.NewApiError(http.StatusBadRequest, errors.Errorf("Requested CPU architecture %s is not supported for release image URL %s. Supported architectures in release image: %v", common.ARM64CPUArchitecture, common.TestDefaultConfig.ReleaseImageUrl, []string{common.ARM64CPUArchitecture}))))
+		})
+	})
 
 	Context("Cluster Tags", func() {
 		It("Register cluster with tags", func() {
@@ -16649,7 +16708,7 @@ var _ = Describe("RegisterCluster", func() {
 			payload.Username = userName1
 			payload.Organization = orgID1
 			authCtx = context.WithValue(ctx, restapi.AuthKey, payload)
-			mockClusterRegisterSteps()
+			mockClusterRegisterSteps(false)
 			mockUsageReports()
 			mockAMSSubscription(authCtx)
 			mockMetric.EXPECT().ClusterRegistered().Times(1)
@@ -17512,7 +17571,7 @@ var _ = Describe("AMS subscriptions", func() {
 		It("register cluster - deregister if we failed to create AMS subscription", func() {
 			bm.clusterApi = mockClusterApi
 			mockClusterApi.EXPECT().RegisterCluster(ctx, gomock.Any()).Return(nil)
-			mockClusterRegisterSteps()
+			mockClusterRegisterSteps(false)
 			mockAccountsMgmt.EXPECT().CreateSubscription(ctx, gomock.Any(), clusterName).Return(nil, errors.New("dummy"))
 			mockClusterApi.EXPECT().DeregisterCluster(ctx, gomock.Any())
 
@@ -17527,7 +17586,7 @@ var _ = Describe("AMS subscriptions", func() {
 		It("register cluster - delete AMS subscription if we failed to patch DB with ams_subscription_id", func() {
 			bm.clusterApi = mockClusterApi
 			mockClusterApi.EXPECT().RegisterCluster(ctx, gomock.Any()).Return(nil)
-			mockClusterRegisterSteps()
+			mockClusterRegisterSteps(false)
 			mockAMSSubscription(ctx)
 			mockClusterApi.EXPECT().UpdateAmsSubscriptionID(ctx, gomock.Any(), strfmt.UUID("")).Return(common.NewApiError(http.StatusInternalServerError, errors.New("dummy")))
 			mockClusterApi.EXPECT().DeregisterCluster(ctx, gomock.Any())

internal/controller/controllers/clusterdeployments_controller.go

@@ -212,9 +212,10 @@ func (r *ClusterDeploymentsReconciler) Reconcile(origCtx context.Context, req ct
 		log.WithError(err).Errorf("failed to get pull secret for cluster deployment %s", clusterDeployment.Name)
 		return r.updateStatus(ctx, log, clusterInstall, clusterDeployment, nil, err)
 	}
-	releaseImage, err := r.getReleaseImage(ctx, clusterDeployment, clusterInstall, pullSecret)
+
+	clusterImageSet, err := r.ensureClusterImageSetRef(ctx, log, clusterDeployment, clusterInstall)
 	if err != nil {
-		log.WithError(err).Errorf("failed to get release image for cluster %s, ensure AgentClusterInstall.Spec.ImageSetRef references a ClusterImageSet", clusterDeployment.Name)
+		log.WithError(err).Errorf("failed to ensure release image for cluster %s, AgentClusterInstall.Spec.ImageSetRef must reference a valid ClusterImageSet", clusterDeployment.Name)
 		return r.updateStatus(ctx, log, clusterInstall, clusterDeployment, nil, err)
 	}
 
@@ -229,6 +230,11 @@ func (r *ClusterDeploymentsReconciler) Reconcile(origCtx context.Context, req ct
 
 	cluster, err := r.Installer.GetClusterByKubeKey(req.NamespacedName)
 	if errors.Is(err, gorm.ErrRecordNotFound) {
+		releaseImage, releaseImageErr := r.getReleaseImage(ctx, clusterImageSet, pullSecret)
+		if releaseImageErr != nil {
+			log.WithError(releaseImageErr).Errorf("failed to get release image for cluster %s", clusterDeployment.Name)
+			return r.updateStatus(ctx, log, clusterInstall, clusterDeployment, nil, releaseImageErr)
+		}
 		if !isInstalled(clusterDeployment, clusterInstall) {
 			return r.createNewCluster(ctx, log, req.NamespacedName, pullSecret, releaseImage, clusterDeployment, clusterInstall, mirrorRegistryConfiguration)
 		}
@@ -1378,25 +1384,26 @@ func (r *ClusterDeploymentsReconciler) addCustomManifests(ctx context.Context, l
 }
 
 func CreateClusterParams(clusterDeployment *hivev1.ClusterDeployment, clusterInstall *hiveext.AgentClusterInstall,
-	pullSecret string, releaseImageVersion string, releaseImageCPUArch string,
+	pullSecret string, releaseImage *models.ReleaseImage,
 	ignitionEndpoint *models.IgnitionEndpoint, olmOperators []*models.OperatorCreateParams) *models.ClusterCreateParams {
 	spec := clusterDeployment.Spec
 	platform, _ := getPlatform(clusterInstall.Spec)
 
 	clusterParams := &models.ClusterCreateParams{
 		BaseDNSDomain:         spec.BaseDomain,
 		Name:                  swag.String(spec.ClusterName),
-		OpenshiftVersion:      &releaseImageVersion,
+		OpenshiftVersion:      releaseImage.Version,
 		PullSecret:            swag.String(pullSecret),
 		VipDhcpAllocation:     swag.Bool(false),
 		APIVips:               ApiVipsEntriesToArray(clusterInstall.Spec.APIVIPs),
 		IngressVips:           IngressVipsEntriesToArray(clusterInstall.Spec.IngressVIPs),
 		SSHPublicKey:          clusterInstall.Spec.SSHPublicKey,
-		CPUArchitecture:       releaseImageCPUArch,
+		CPUArchitecture:       *releaseImage.CPUArchitecture,
 		UserManagedNetworking: swag.Bool(isUserManagedNetwork(clusterInstall)),
 		Platform:              platform,
 		SchedulableMasters:    swag.Bool(clusterInstall.Spec.MastersSchedulable),
 		ControlPlaneCount:     swag.Int64(int64(clusterInstall.Spec.ProvisionRequirements.ControlPlaneAgents)),
+		OcpReleaseImage:       *releaseImage.URL,
 	}
 
 	if len(clusterInstall.Spec.Networking.ClusterNetwork) > 0 {
@@ -1484,7 +1491,10 @@ func (r *ClusterDeploymentsReconciler) createNewCluster(
 	mirrorRegistryConfiguration *common.MirrorRegistryConfiguration) (ctrl.Result, error) {
 
 	log.Infof("Creating a new cluster %s %s", clusterDeployment.Name, clusterDeployment.Namespace)
-
+	if releaseImage == nil { // releaseImage shoudn't be nil, but guard against it just in case
+		log.Errorf("release image cannot be nil for day 1 cluster %s %s", clusterDeployment.Name, clusterDeployment.Namespace)
+		return r.updateStatus(ctx, log, clusterInstall, clusterDeployment, nil, fmt.Errorf("release image cannot be nil for day 1 cluster %s %s. Ensure AgentClusterInstall.Spec.ImageSetRef references a valid ClusterImageSet", clusterDeployment.Name, clusterDeployment.Namespace))
+	}
 	var ignitionEndpoint *models.IgnitionEndpoint
 	var err error
 	if clusterInstall.Spec.IgnitionEndpoint != nil {
@@ -1495,8 +1505,7 @@ func (r *ClusterDeploymentsReconciler) createNewCluster(
 		}
 	}
 
-	clusterParams := CreateClusterParams(clusterDeployment, clusterInstall, pullSecret, *releaseImage.Version,
-		*releaseImage.CPUArchitecture, ignitionEndpoint, nil)
+	clusterParams := CreateClusterParams(clusterDeployment, clusterInstall, pullSecret, releaseImage, ignitionEndpoint, nil)
 
 	c, err := r.Installer.RegisterClusterInternal(ctx, &key, mirrorRegistryConfiguration, installer.V2RegisterClusterParams{
 		NewClusterParams: clusterParams,
@@ -1555,12 +1564,7 @@ func (r *ClusterDeploymentsReconciler) createNewDay2Cluster(
 	return r.updateStatus(ctx, log, clusterInstall, clusterDeployment, c, err)
 }
 
-func (r *ClusterDeploymentsReconciler) getReleaseImage(
-	ctx context.Context,
-	clusterDeployment *hivev1.ClusterDeployment,
-	clusterInstall *hiveext.AgentClusterInstall,
-	pullSecret string) (*models.ReleaseImage, error) {
-
+func (r *ClusterDeploymentsReconciler) ensureClusterImageSetRef(ctx context.Context, log logrus.FieldLogger, clusterDeployment *hivev1.ClusterDeployment, clusterInstall *hiveext.AgentClusterInstall) (*hivev1.ClusterImageSet, error) {
 	// Make sure that the ImageSetRef is set before continuing
 	if clusterInstall.Spec.ImageSetRef == nil {
 		// ImageSetRef is not required for already installed clusters
@@ -1577,9 +1581,23 @@ func (r *ClusterDeploymentsReconciler) getReleaseImage(
 		Name:      clusterInstall.Spec.ImageSetRef.Name,
 	}
 	if err := r.Client.Get(ctx, key, clusterImageSet); err != nil {
+		if k8serrors.IsNotFound(err) {
+			return nil, errors.Errorf("ClusterImageSet %s not found. Please ensure the ClusterImageSet exists", key.Name)
+		}
 		return nil, errors.Wrapf(err, "failed to get cluster image set %s", key.Name)
 	}
 
+	return clusterImageSet, nil
+}
+
+func (r *ClusterDeploymentsReconciler) getReleaseImage(
+	ctx context.Context,
+	clusterImageSet *hivev1.ClusterImageSet,
+	pullSecret string) (*models.ReleaseImage, error) {
+	if clusterImageSet == nil {
+		r.Log.Debugf("cluster image set is nil for cluster")
+		return nil, nil
+	}
 	releaseImage, err := r.VersionsHandler.GetReleaseImageByURL(ctx, clusterImageSet.Spec.ReleaseImage, pullSecret)
 	if err != nil {
 		errMsgSuffix := ""
@@ -1592,7 +1610,7 @@ func (r *ClusterDeploymentsReconciler) getReleaseImage(
 			}
 		}
 		errMsg := "failed to get release image '%s'. Please ensure the releaseImage field in ClusterImageSet '%s' is valid, %s (error: %s)."
-		return nil, errors.New(fmt.Sprintf(errMsg, clusterImageSet.Spec.ReleaseImage, key.Name, errMsgSuffix, err.Error()))
+		return nil, errors.New(fmt.Sprintf(errMsg, clusterImageSet.Spec.ReleaseImage, clusterImageSet.Name, errMsgSuffix, err.Error()))
 	}
 
 	return releaseImage, nil