Repository: openshift/assisted-service (language: python)

Issue:
MGMT-17468: Use x86_64 over amd64 and arm64 over aarch64 for CPU architecture of both release and OS images to comply with ABI current behavior
Currently assisted-service `normalizes` only release images in `kubeAPI` flow. we want it to `normalize` in `restAPI` flow as well. This is due to the nature of `ABI` flow currently, which starts the service with a release image with CPU architecture `aarch64`, but registers a cluster with `arm64` release image, and same with OS imags. In order to neatralize these differentials, this PR changes the way release and OS images are created and later fetched by normalizing it in creation and then during fetching.

<!--
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

You can refer to [Kubernetes community documentation] on writing good commit messages, which provides good tips and ideas.

Some PRs address specific issues. Please, refer to the [CONTRIBUTING] documentation for more
information on how to link a PR to an existing issue.

It's recommended to take a few extra minutes to provide more information about
how this code was tested. Here are some questions that may be worth answering:

- Should this PR be tested by the reviewer?
- Is this PR relying on CI for an e2e test run?
- Should this PR be tested in a specific environment?
- Any logs, screenshots, etc that can help with the review process?

-->

## List all the issues related to this PR

- [ ] New Feature <!-- new functionality -->
- [ ] Enhancement <!-- refactor, code changes, improvement, that won't add new features -->
- [x] Bug fix
- [ ] Tests
- [ ] Documentation
- [ ] CI/CD <!-- Notice that changes for Dockerfiles/Jenkinsfiles aren't tested in CI due to a known bug. -->

## What environments does this code impact?

- [ ] Automation (CI, tools, etc)
- [x] Cloud
- [x] Operator Managed Deployments
- [ ] None

## How was this code tested?

<!-- Please, select one or more if needed: -->

- [ ] assisted-test-infra environment
- [ ] dev-scripts environment
- [ ] Reviewer's test appreciated
- [x] Waiting for CI to do a full test run
- [ ] Manual (Elaborate on how it was tested)
- [ ] No tests needed

## Checklist

- [x] Title and description added to both, commit and PR.
- [x] Relevant issues have been associated (see [CONTRIBUTING] guide)
- [x] This change does not require a documentation update (docstring, `docs`, README, etc)
- [ ] Does this change include unit-tests (note that code changes require unit-tests)

## Reviewers Checklist

- Are the title and description (in both PR and commit) meaningful and clear?
- Is there a bug required (and linked) for this change?
- Should this PR be backported?

[Kubernetes community documentation]: https://github.com/kubernetes/community/blob/master/contributors/guide/pull-requests.md#commit-message-guidelines
[CONTRIBUTING]: https://github.com/openshift/assisted-service/blob/master/CONTRIBUTING.md



Discussion / hints:

Fix patch:
diff --git a/internal/versions/common.go b/internal/versions/common.go
index 49ee013c100..bfa3e1725a0 100644
--- a/internal/versions/common.go
+++ b/internal/versions/common.go
@@ -6,6 +6,8 @@ import (
 	"fmt"
 	"sync"
 
+	"github.com/go-openapi/strfmt"
+	"github.com/go-openapi/swag"
 	"github.com/openshift/assisted-service/internal/common"
 	"github.com/openshift/assisted-service/internal/oc"
 	models "github.com/openshift/assisted-service/models"
@@ -41,6 +43,14 @@ func NewHandler(
 	enableKubeAPI bool,
 	releaseSources models.ReleaseSources,
 ) (Handler, error) {
+	for _, releaseImage := range releaseImages {
+		if err := validateReleaseImage(releaseImage); err != nil {
+			return nil, errors.Wrap(err, "error occurred while validating release images")
+		}
+
+		normalizeReleaseImageCPUArchitecture(releaseImage)
+	}
+
 	if enableKubeAPI {
 		h := &kubeAPIVersionsHandler{
 			mustGatherVersions: mustGatherVersions,
@@ -52,10 +62,6 @@ func NewHandler(
 			sem:                semaphore.NewWeighted(30),
 		}
 
-		if err := h.validateVersions(); err != nil {
-			return nil, err
-		}
-
 		return h, nil
 	}
 
@@ -235,3 +241,37 @@ func ParseReleaseImages(
 		}
 	})
 }
+
+func normalizeReleaseImageCPUArchitecture(releaseImage *models.ReleaseImage) {
+	// Normalize release.CPUArchitecture and release.CPUArchitectures
+	// TODO: remove this block when AI starts using aarch64 instead of arm64
+	if swag.StringValue(releaseImage.CPUArchitecture) == common.MultiCPUArchitecture || swag.StringValue(releaseImage.CPUArchitecture) == common.AARCH64CPUArchitecture {
+		*releaseImage.CPUArchitecture = common.NormalizeCPUArchitecture(*releaseImage.CPUArchitecture)
+		for i := 0; i < len(releaseImage.CPUArchitectures); i++ {
+			releaseImage.CPUArchitectures[i] = common.NormalizeCPUArchitecture(releaseImage.CPUArchitectures[i])
+		}
+	}
+}
+
+// validateReleaseImage ensures no missing values in Release image.
+func validateReleaseImage(releaseImage *models.ReleaseImage) error {
+	// Release images are not mandatory (dynamically added in kube-api flow),
+	// validating fields for those specified in list.
+	missingValueTemplate := "Missing value in ReleaseImage for '%s' field"
+	if swag.StringValue(releaseImage.CPUArchitecture) == "" {
+		return errors.Errorf(fmt.Sprintf(missingValueTemplate, "cpu_architecture"))
+	}
+
+	if swag.StringValue(releaseImage.OpenshiftVersion) == "" {
+		return errors.Errorf(fmt.Sprintf(missingValueTemplate, "openshift_version"))
+	}
+	if swag.StringValue(releaseImage.URL) == "" {
+		return errors.Errorf(fmt.Sprintf(missingValueTemplate, "url"))
+	}
+	if swag.StringValue(releaseImage.Version) == "" {
+		return errors.Errorf(fmt.Sprintf(missingValueTemplate, "version"))
+	}
+
+	// To validate CPU architecture enum
+	return releaseImage.Validate(strfmt.Default)
+}
diff --git a/internal/versions/kube_api_versions.go b/internal/versions/kube_api_versions.go
index 4d523329b6f..b56abf888fb 100644
--- a/internal/versions/kube_api_versions.go
+++ b/internal/versions/kube_api_versions.go
@@ -2,7 +2,6 @@ package versions
 
 import (
 	context "context"
-	"fmt"
 	"regexp"
 	"runtime/debug"
 	"strings"
@@ -53,6 +52,7 @@ func (h *kubeAPIVersionsHandler) GetMustGatherImages(openshiftVersion, cpuArchit
 // It first tries to find the image in the local cache. If the image is not found in the cache and a Kubernetes client is available (KubeAPI mode),
 // it then fetches all cluster image sets, updates the cache with these image sets, and attempts the cache lookup again.
 func (h *kubeAPIVersionsHandler) GetReleaseImage(ctx context.Context, openshiftVersion, cpuArchitecture, pullSecret string) (*models.ReleaseImage, error) {
+	cpuArchitecture = common.NormalizeCPUArchitecture(cpuArchitecture)
 	image, err := h.getReleaseImageFromCache(openshiftVersion, cpuArchitecture)
 	if err == nil || h.kubeClient == nil {
 		return image, err
@@ -248,38 +248,6 @@ func (h *kubeAPIVersionsHandler) addReleaseImage(releaseImageUrl, pullSecret str
 	return releaseImage.(*models.ReleaseImage), nil
 }
 
-// Ensure no missing values in Release images.
-func (h *kubeAPIVersionsHandler) validateVersions() error {
-	// Release images are not mandatory (dynamically added in kube-api flow),
-	// validating fields for those specified in list.
-	missingValueTemplate := "Missing value in ReleaseImage for '%s' field"
-	for _, release := range h.releaseImages {
-		if swag.StringValue(release.CPUArchitecture) == "" {
-			return errors.Errorf(fmt.Sprintf(missingValueTemplate, "cpu_architecture"))
-		}
-		if swag.StringValue(release.OpenshiftVersion) == "" {
-			return errors.Errorf(fmt.Sprintf(missingValueTemplate, "openshift_version"))
-		}
-		if swag.StringValue(release.URL) == "" {
-			return errors.Errorf(fmt.Sprintf(missingValueTemplate, "url"))
-		}
-		if swag.StringValue(release.Version) == "" {
-			return errors.Errorf(fmt.Sprintf(missingValueTemplate, "version"))
-		}
-		// Normalize release.CPUArchitecture and release.CPUArchitectures
-		// TODO: remove this block when AI starts using aarch64 instead of arm64
-		if swag.StringValue(release.CPUArchitecture) == common.MultiCPUArchitecture || swag.StringValue(release.CPUArchitecture) == common.AARCH64CPUArchitecture {
-			*release.CPUArchitecture = common.NormalizeCPUArchitecture(*release.CPUArchitecture)
-			for i := 0; i < len(release.CPUArchitectures); i++ {
-				release.CPUArchitectures[i] = common.NormalizeCPUArchitecture(release.CPUArchitectures[i])
-			}
-
-		}
-	}
-
-	return nil
-}
-
 // GetRevision returns the overall codebase version. It's for detecting
 // what code a binary was built from.
 func GetRevision() string {
diff --git a/internal/versions/osimages.go b/internal/versions/osimages.go
index e251b4e7536..4c5fd0bcd05 100644
--- a/internal/versions/osimages.go
+++ b/internal/versions/osimages.go
@@ -4,6 +4,7 @@ import (
 	"fmt"
 	"sort"
 
+	"github.com/go-openapi/strfmt"
 	"github.com/go-openapi/swag"
 	"github.com/hashicorp/go-version"
 	"github.com/openshift/assisted-service/internal/common"
@@ -24,45 +25,37 @@ type OSImages interface {
 type osImageList models.OsImages
 
 func NewOSImages(images models.OsImages) (OSImages, error) {
-	i := osImageList(images)
-	if err := i.validate(); err != nil {
-		return nil, err
+	if len(images) == 0 {
+		return nil, errors.New("No OS images provided")
 	}
-	return i, nil
-}
-
-func (images osImageList) validate() error {
 	for _, osImage := range images {
-		if swag.StringValue(osImage.OpenshiftVersion) == "" {
-			return errors.Errorf("Missing openshift_version in OsImage: %v", osImage)
+		if err := validateOSImage(osImage); err != nil {
+			return nil, err
 		}
-	}
 
-	openshiftVersions := images.GetOpenshiftVersions()
-	if len(openshiftVersions) == 0 {
-		return errors.Errorf("No OS images are available")
+		normalizeOSImageCPUArchitecture(osImage)
 	}
 
+	return osImageList(images), nil
+}
+
+func validateOSImage(osImage *models.OsImage) error {
 	missingValueTemplate := "Missing value in OSImage for '%s' field (openshift_version: %s)"
-	for _, key := range openshiftVersions {
-		architectures := images.GetCPUArchitectures(key)
-		for _, architecture := range architectures {
-			osImage, err := images.GetOsImage(key, architecture)
-			if err != nil {
-				return errors.Wrap(err, fmt.Sprintf("Failed to get OSImage for openshift version: %s", key))
-			}
-			if swag.StringValue(osImage.URL) == "" {
-				return errors.Errorf(fmt.Sprintf(missingValueTemplate, "url", key))
-			}
-			if swag.StringValue(osImage.Version) == "" {
-				return errors.Errorf(fmt.Sprintf(missingValueTemplate, "version", key))
-			}
-			// Normalize osImage.CPUArchitecture
-			// TODO: remove this block when AI starts using aarch64 instead of arm64
-			if architecture == common.AARCH64CPUArchitecture {
-				*osImage.CPUArchitecture = common.NormalizeCPUArchitecture(*osImage.CPUArchitecture)
-			}
-		}
+	if swag.StringValue(osImage.OpenshiftVersion) == "" {
+		return errors.Errorf("Missing openshift_version in OsImage: %v", osImage)
+	}
+
+	if swag.StringValue(osImage.URL) == "" {
+		return errors.Errorf(fmt.Sprintf(missingValueTemplate, "url", *osImage.OpenshiftVersion))
+	}
+	if swag.StringValue(osImage.Version) == "" {
+		return errors.Errorf(fmt.Sprintf(missingValueTemplate, "version", *osImage.OpenshiftVersion))
+	}
+	if swag.StringValue(osImage.CPUArchitecture) == "" {
+		osImage.CPUArchitecture = swag.String(common.DefaultCPUArchitecture)
+	}
+	if err := osImage.Validate(strfmt.Default); err != nil {
+		return errors.Wrapf(err, "osImage version '%s' CPU architecture is not valid", *osImage.OpenshiftVersion)
 	}
 
 	return nil
@@ -70,10 +63,13 @@ func (images osImageList) validate() error {
 
 // Returns the OsImage entity
 func (images osImageList) GetOsImage(openshiftVersion, cpuArchitecture string) (*models.OsImage, error) {
+	cpuArchitecture = common.NormalizeCPUArchitecture(cpuArchitecture)
+
 	if cpuArchitecture == "" {
 		// Empty implies default CPU architecture
 		cpuArchitecture = common.DefaultCPUArchitecture
 	}
+
 	// Filter OS images by specified CPU architecture
 	archImages := funk.Filter(images, func(osImage *models.OsImage) bool {
 		if swag.StringValue(osImage.CPUArchitecture) == "" {
@@ -205,3 +201,11 @@ func (images osImageList) GetOpenshiftVersions() []string {
 	}
 	return versions
 }
+
+func normalizeOSImageCPUArchitecture(osImage *models.OsImage) {
+	// Normalize osImage.CPUArchitecture
+	// TODO: remove this block when AI starts using aarch64 instead of arm64
+	if *osImage.CPUArchitecture == common.AARCH64CPUArchitecture {
+		*osImage.CPUArchitecture = common.NormalizeCPUArchitecture(*osImage.CPUArchitecture)
+	}
+}
diff --git a/internal/versions/rest_api_versions.go b/internal/versions/rest_api_versions.go
index 51a76812934..d715bb88904 100644
--- a/internal/versions/rest_api_versions.go
+++ b/internal/versions/rest_api_versions.go
@@ -31,6 +31,7 @@ type restAPIVersionsHandler struct {
 // For OpenShift versions specified as major.minor, it fetches the latest matching release image.
 // The function returns an error Returns an error for other formats of OpenShift version or if no matching image can be found.
 func (h *restAPIVersionsHandler) GetReleaseImage(_ context.Context, openshiftVersion, cpuArchitecture, _ string) (*models.ReleaseImage, error) {
+	cpuArchitecture = common.NormalizeCPUArchitecture(cpuArchitecture)
 	// validations
 	if err := validateCPUArchitecture(cpuArchitecture); err != nil {
 		return nil, err


Test patch:
diff --git a/internal/versions/common_test.go b/internal/versions/common_test.go
index fa6d3729902..35f573f5e5e 100644
--- a/internal/versions/common_test.go
+++ b/internal/versions/common_test.go
@@ -95,6 +95,42 @@ var _ = Describe("NewHandler", func() {
 		Expect(err).Should(HaveOccurred())
 		Expect(err.Error()).To(ContainSubstring("version"))
 	})
+
+	It("Normalizes CPU architecture", func() {
+		releaseImages := models.ReleaseImages{
+			&models.ReleaseImage{
+				OpenshiftVersion: swag.String("4.14"),
+				Version:          swag.String("4.14.1"),
+				CPUArchitecture:  swag.String(common.X86CPUArchitecture),
+				URL:              swag.String("release_4.14"),
+			},
+			&models.ReleaseImage{
+				OpenshiftVersion: swag.String("4.14"),
+				Version:          swag.String("4.14.1"),
+				CPUArchitecture:  swag.String(common.AARCH64CPUArchitecture),
+				URL:              swag.String("release_4.14"),
+			},
+		}
+
+		_, err := NewHandler(common.GetTestLog(), nil, releaseImages, nil, "", nil, nil, nil, false, nil)
+		Expect(err).ToNot(HaveOccurred())
+		Expect(*releaseImages[0].CPUArchitecture).To(Equal(common.X86CPUArchitecture))
+		Expect(*releaseImages[1].CPUArchitecture).To(Equal(common.ARM64CPUArchitecture))
+	})
+
+	It("Validates CPU architecture", func() {
+		releaseImages := models.ReleaseImages{
+			&models.ReleaseImage{
+				OpenshiftVersion: swag.String("4.14"),
+				Version:          swag.String("4.14.1"),
+				CPUArchitecture:  swag.String(common.AMD64CPUArchitecture),
+				URL:              swag.String("release_4.14"),
+			},
+		}
+
+		_, err := NewHandler(common.GetTestLog(), nil, releaseImages, nil, "", nil, nil, nil, false, nil)
+		Expect(err).To(HaveOccurred())
+	})
 })
 
 var _ = Describe("validateReleaseImageForRHCOS", func() {
diff --git a/internal/versions/kube_api_versions_test.go b/internal/versions/kube_api_versions_test.go
index 3c9082eb68c..407557747ac 100644
--- a/internal/versions/kube_api_versions_test.go
+++ b/internal/versions/kube_api_versions_test.go
@@ -267,6 +267,12 @@ var _ = Describe("GetReleaseImage", func() {
 		Expect(*releaseImage.Version).Should(Equal("4.12.999-rc.4"))
 	})
 
+	It("gets successfuly single-arch image with normaliznig CPU architecture", func() {
+		releaseImage, err := h.GetReleaseImage(ctx, "4.9-candidate_arm64", common.AARCH64CPUArchitecture, pullSecret)
+		Expect(err).ShouldNot(HaveOccurred())
+		Expect(*releaseImage.OpenshiftVersion).Should(Equal("4.9"))
+	})
+
 	It("returns the matching CPU architecture over multi-arch if it is present", func() {
 		h.releaseImages = models.ReleaseImages{
 			&models.ReleaseImage{
diff --git a/internal/versions/osimages_test.go b/internal/versions/osimages_test.go
index 673e42a5a98..842e8f3499e 100644
--- a/internal/versions/osimages_test.go
+++ b/internal/versions/osimages_test.go
@@ -8,6 +8,74 @@ import (
 	models "github.com/openshift/assisted-service/models"
 )
 
+var _ = Describe("NewOSImages", func() {
+	It("should fail when missing OpenshiftVersion", func() {
+		osImages := models.OsImages{
+			{
+				Version:         swag.String("4.14.213113"),
+				CPUArchitecture: swag.String(common.X86CPUArchitecture),
+				URL:             swag.String("foobar-4.14"),
+			},
+		}
+
+		_, err := NewOSImages(osImages)
+		Expect(err).Should(HaveOccurred())
+	})
+
+	It("should fail when missing Version", func() {
+		osImages := models.OsImages{
+			{
+				OpenshiftVersion: swag.String("4.14"),
+				CPUArchitecture:  swag.String(common.X86CPUArchitecture),
+				URL:              swag.String("foobar-4.14"),
+			},
+		}
+
+		_, err := NewOSImages(osImages)
+		Expect(err).Should(HaveOccurred())
+	})
+
+	It("should not fail when missing CPU architecture, should set to default", func() {
+		osImages := models.OsImages{
+			{
+				OpenshiftVersion: swag.String("4.14"),
+				Version:          swag.String("4.14.213113"),
+				URL:              swag.String("foobar-4.14"),
+			},
+		}
+
+		_, err := NewOSImages(osImages)
+		Expect(err).ShouldNot(HaveOccurred())
+		Expect(*osImages[0].CPUArchitecture).To(Equal(common.X86CPUArchitecture))
+	})
+
+	It("should fail when missing URL", func() {
+		osImages := models.OsImages{
+			{
+				OpenshiftVersion: swag.String("4.14"),
+				Version:          swag.String("4.14.213113"),
+				CPUArchitecture:  swag.String(common.X86CPUArchitecture),
+			},
+		}
+
+		_, err := NewOSImages(osImages)
+		Expect(err).Should(HaveOccurred())
+	})
+
+	It("should fail when CPU architecture is not valid", func() {
+		osImages := models.OsImages{
+			{
+				OpenshiftVersion: swag.String("4.14"),
+				Version:          swag.String("4.14.213113"),
+				CPUArchitecture:  swag.String(common.AMD64CPUArchitecture),
+			},
+		}
+
+		_, err := NewOSImages(osImages)
+		Expect(err).Should(HaveOccurred())
+	})
+})
+
 var _ = Describe("GetOsImage", func() {
 	var (
 		images OSImages
@@ -52,6 +120,13 @@ var _ = Describe("GetOsImage", func() {
 			Expect(image.OpenshiftVersion).To(HaveValue(Equal("4.9")))
 		})
 
+		It("With normalizing the CPU architecture", func() {
+			image, err := images.GetOsImage("4.9", common.AARCH64CPUArchitecture)
+			Expect(err).ShouldNot(HaveOccurred())
+			Expect(image.Version).To(HaveValue(Equal("version-49.123-0_arm64")))
+			Expect(*image.CPUArchitecture).To(Equal(common.ARM64CPUArchitecture))
+		})
+
 		It("parses the image list correctly", func() {
 			for _, version := range images.GetOpenshiftVersions() {
 				architectures := images.GetCPUArchitectures(version)
diff --git a/internal/versions/rest_api_versions_test.go b/internal/versions/rest_api_versions_test.go
index c819945a338..20c25531c01 100644
--- a/internal/versions/rest_api_versions_test.go
+++ b/internal/versions/rest_api_versions_test.go
@@ -381,6 +381,27 @@ var _ = Describe("GetReleaseImage", func() {
 		Expect(releaseImage).To(BeNil())
 	})
 
+	It("gets successfully with normalizing CPU architecture", func() {
+		releaseImages := models.ReleaseImages{
+			{
+				OpenshiftVersion: swag.String("4.14"),
+				Version:          swag.String("4.14.1"),
+				CPUArchitecture:  swag.String(common.ARM64CPUArchitecture),
+				CPUArchitectures: []string{common.ARM64CPUArchitecture},
+				URL:              swag.String("quay.io/openshift-release-dev/ocp-release:4.14.1-aarch64"),
+				SupportLevel:     models.ReleaseImageSupportLevelProduction,
+				Default:          false,
+			},
+		}
+
+		err := db.Create(releaseImages).Error
+		Expect(err).ShouldNot(HaveOccurred())
+
+		releaseImage, err := handler.GetReleaseImage(ctx, "4.14.1", common.AARCH64CPUArchitecture, pullSecret)
+		Expect(err).ShouldNot(HaveOccurred())
+		Expect(*releaseImage.Version).To(Equal("4.14.1"))
+	})
+
 	It("filters ignored release images successfully with major.minor ignored versions", func() {
 		releaseImages := models.ReleaseImages{
 			{