OCPBUGS-44655: Fix multiarch validations using MetadataProvider

Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com>
openshift · Nov 21, 2024 · e20a560 · e20a560
1 parent d419048
commit e20a560
Show file tree

Hide file tree

Showing 9 changed files with 154 additions and 28 deletions.
diff --git a/cmd/cluster/core/create.go b/cmd/cluster/core/create.go
@@ -642,7 +642,7 @@ func (opts *RawCreateOptions) Validate(ctx context.Context) (*ValidatedCreateOpt
 		if err != nil {
 			return nil, fmt.Errorf("could not retrieve kube clientset: %w", err)
 		}
-		if err := validateMgmtClusterAndNodePoolCPUArchitectures(ctx, opts, kc); err != nil {
+		if err := validateMgmtClusterAndNodePoolCPUArchitectures(ctx, opts, kc, &hyperutil.RegistryClientImageMetadataProvider{}); err != nil {
 			return nil, err
 		}
 	}
@@ -1006,7 +1006,7 @@ func parseTolerationString(str string) (*corev1.Toleration, error) {
 // validateMgmtClusterAndNodePoolCPUArchitectures checks if a multi-arch release image or release stream was provided.
 // If none were provided, checks to make sure the NodePool CPU arch and the management cluster CPU arch match; if they
 // do not, the CLI will return an error since the NodePool will fail to complete during runtime.
-func validateMgmtClusterAndNodePoolCPUArchitectures(ctx context.Context, opts *RawCreateOptions, kc kubeclient.Interface) error {
+func validateMgmtClusterAndNodePoolCPUArchitectures(ctx context.Context, opts *RawCreateOptions, kc kubeclient.Interface, imageMetadataProvider hyperutil.ImageMetadataProvider) error {
 	validMultiArchImage := false
 
 	// Check if the release image is multi-arch
@@ -1015,8 +1015,7 @@ func validateMgmtClusterAndNodePoolCPUArchitectures(ctx context.Context, opts *R
 		if err != nil {
 			return fmt.Errorf("failed to read pull secret file: %w", err)
 		}
-
-		validMultiArchImage, err = registryclient.IsMultiArchManifestList(ctx, opts.ReleaseImage, pullSecret)
+		validMultiArchImage, err = registryclient.IsMultiArchManifestList(ctx, opts.ReleaseImage, pullSecret, imageMetadataProvider)
 		if err != nil {
 			return err
 		}

diff --git a/cmd/cluster/core/create_test.go b/cmd/cluster/core/create_test.go
@@ -6,6 +6,8 @@ import (
 
 	. "github.com/onsi/gomega"
 	hyperv1 "github.com/openshift/hypershift/api/hypershift/v1beta1"
+	"github.com/openshift/hypershift/support/thirdparty/library-go/pkg/image/dockerv1client"
+	"github.com/openshift/hypershift/support/util/fakeimagemetadataprovider"
 	crclient "sigs.k8s.io/controller-runtime/pkg/client"
 
 	corev1 "k8s.io/api/core/v1"
@@ -19,11 +21,14 @@ func TestValidateMgmtClusterAndNodePoolCPUArchitectures(t *testing.T) {
 
 	fakeKubeClient := fakekubeclient.NewSimpleClientset()
 	fakeDiscovery, ok := fakeKubeClient.Discovery().(*fakediscovery.FakeDiscovery)
-
 	if !ok {
 		t.Fatalf("failed to convert FakeDiscovery")
 	}
 
+	fakeMetadataProvider := &fakeimagemetadataprovider.FakeImageMetadataProvider{
+		Result: &dockerv1client.DockerImageConfig{},
+	}
+
 	// if you want to fake a specific version
 	fakeDiscovery.FakedServerVersion = &apiversion.Info{
 		Platform: "linux/amd64",
@@ -80,7 +85,7 @@ func TestValidateMgmtClusterAndNodePoolCPUArchitectures(t *testing.T) {
 	for _, tc := range tests {
 		t.Run(tc.name, func(t *testing.T) {
 			g := NewWithT(t)
-			err := validateMgmtClusterAndNodePoolCPUArchitectures(ctx, tc.opts, fakeKubeClient)
+			err := validateMgmtClusterAndNodePoolCPUArchitectures(ctx, tc.opts, fakeKubeClient, fakeMetadataProvider)
 			if tc.expectError {
 				g.Expect(err).To(HaveOccurred())
 			} else {

diff --git a/ignition-server/cmd/run_local_ignitionprovider.go b/ignition-server/cmd/run_local_ignitionprovider.go
@@ -92,15 +92,20 @@ func (o *RunLocalIgnitionProviderOptions) Run(ctx context.Context) error {
 		return fmt.Errorf("unable to create image file cache: %w", err)
 	}
 
+	imageMetaDataProvider := &util.RegistryClientImageMetadataProvider{
+		OpenShiftImageRegistryOverrides: map[string][]string{},
+	}
+
 	p := &controllers.LocalIgnitionProvider{
-		Client:              cl,
-		ReleaseProvider:     &releaseinfo.ProviderWithOpenShiftImageRegistryOverridesDecorator{},
-		CloudProvider:       "",
-		Namespace:           o.Namespace,
-		WorkDir:             o.WorkDir,
-		PreserveOutput:      true,
-		ImageFileCache:      imageFileCache,
-		FeatureGateManifest: o.FeatureGateManifest,
+		Client:                cl,
+		ReleaseProvider:       &releaseinfo.ProviderWithOpenShiftImageRegistryOverridesDecorator{},
+		ImageMetadataProvider: imageMetaDataProvider,
+		CloudProvider:         "",
+		Namespace:             o.Namespace,
+		WorkDir:               o.WorkDir,
+		PreserveOutput:        true,
+		ImageFileCache:        imageFileCache,
+		FeatureGateManifest:   o.FeatureGateManifest,
 	}
 
 	payload, err := p.GetPayload(ctx, o.Image, config.String(), "", "", "")

diff --git a/ignition-server/controllers/local_ignitionprovider.go b/ignition-server/controllers/local_ignitionprovider.go
@@ -72,6 +72,10 @@ type LocalIgnitionProvider struct {
 	// to render the ignition payload.
 	FeatureGateManifest string
 
+	// ImageMetaDataProvider is used to get the image metadata for the images
+	// used in the ignition payload.
+	ImageMetadataProvider *util.RegistryClientImageMetadataProvider
+
 	ImageFileCache *imageFileCache
 
 	lock sync.Mutex
@@ -191,7 +195,7 @@ func (p *LocalIgnitionProvider) GetPayload(ctx context.Context, releaseImage, cu
 		return nil, fmt.Errorf("release image does not contain machine-config-operator (images: %v)", imageProvider.ComponentImages())
 	}
 
-	mcoImage, err = registryclient.GetCorrectArchImage(ctx, component, mcoImage, pullSecret)
+	mcoImage, err = registryclient.GetCorrectArchImage(ctx, component, mcoImage, pullSecret, p.ImageMetadataProvider)
 	if err != nil {
 		return nil, err
 	}
@@ -329,7 +333,7 @@ func (p *LocalIgnitionProvider) GetPayload(ctx context.Context, releaseImage, cu
 			return fmt.Errorf("release image does not contain $%s (images: %v)", clusterConfigComponent, imageProvider.ComponentImages())
 		}
 
-		clusterConfigImage, err = registryclient.GetCorrectArchImage(ctx, clusterConfigComponent, clusterConfigImage, pullSecret)
+		clusterConfigImage, err = registryclient.GetCorrectArchImage(ctx, clusterConfigComponent, clusterConfigImage, pullSecret, p.ImageMetadataProvider)
 		if err != nil {
 			return err
 		}
@@ -750,7 +754,7 @@ EOF
    --asset-input-dir %[2]s/input \
    --asset-output-dir %[2]s/output \
    --rendered-manifest-files=%[2]s/manifests \
-   --payload-version=%[4]s 
+   --payload-version=%[4]s
 cp %[2]s/manifests/99_feature-gate.yaml %[3]s/99_feature-gate.yaml
 `
 	}

diff --git a/support/releaseinfo/registryclient/client.go b/support/releaseinfo/registryclient/client.go
@@ -35,6 +35,10 @@ const (
 	ArchitectureARM64   = "arm64"
 )
 
+type ManifestProvider interface {
+	GetManifest(ctx context.Context, imageRef string, pullSecret []byte) (distribution.Manifest, error)
+}
+
 // ExtractImageFiles extracts a list of files from a registry image given the image reference, pull secret and the
 // list of files to extract. It returns a map with file contents or an error.
 func ExtractImageFiles(ctx context.Context, imageRef string, pullSecret []byte, files ...string) (map[string][]byte, error) {
@@ -315,8 +319,8 @@ func GetManifest(ctx context.Context, imageRef string, pullSecret []byte) (distr
 }
 
 // IsMultiArchManifestList determines whether an image is a manifest listed image and contains manifests the following processor architectures: amd64, arm64, s390x, ppc64le
-func IsMultiArchManifestList(ctx context.Context, imageRef string, pullSecret []byte) (bool, error) {
-	srcManifest, err := GetManifest(ctx, imageRef, pullSecret)
+func IsMultiArchManifestList(ctx context.Context, imageRef string, pullSecret []byte, imageMetadataProvider ManifestProvider) (bool, error) {
+	srcManifest, err := imageMetadataProvider.GetManifest(ctx, imageRef, pullSecret)
 	if err != nil {
 		return false, fmt.Errorf("failed to retrieve manifest %s: %w", imageRef, err)
 	}
@@ -352,8 +356,8 @@ func IsMultiArchManifestList(ctx context.Context, imageRef string, pullSecret []
 }
 
 // findImageRefByArch finds the appropriate image reference in a multi-arch manifest image based on the current platform's OS and processor architecture
-func findImageRefByArch(ctx context.Context, imageRef string, pullSecret []byte, osToFind string, archToFind string) (manifestImageRef string, err error) {
-	manifestList, err := GetManifest(ctx, imageRef, pullSecret)
+func findImageRefByArch(ctx context.Context, imageRef string, pullSecret []byte, osToFind string, archToFind string, imageMetadataPorvider ManifestProvider) (manifestImageRef string, err error) {
+	manifestList, err := imageMetadataPorvider.GetManifest(ctx, imageRef, pullSecret)
 	if err != nil {
 		return "", fmt.Errorf("failed to retrieve manifest from image ref, %s: %w", imageRef, err)
 	}
@@ -422,10 +426,10 @@ func findMatchingManifest(ctx context.Context, imageRef string, deserializedMani
 
 // GetCorrectArchImage returns the appropriate image related to the system os/arch if the image reference is manifest
 // listed, else returns the original image reference
-func GetCorrectArchImage(ctx context.Context, component string, imageRef string, pullSecret []byte) (manifestImageRef string, err error) {
+func GetCorrectArchImage(ctx context.Context, component string, imageRef string, pullSecret []byte, imageMetadataProvider ManifestProvider) (manifestImageRef string, err error) {
 	log := ctrl.LoggerFrom(ctx)
 
-	isMultiArchImage, err := IsMultiArchManifestList(ctx, imageRef, pullSecret)
+	isMultiArchImage, err := IsMultiArchManifestList(ctx, imageRef, pullSecret, imageMetadataProvider)
 	if err != nil {
 		return "", fmt.Errorf("failed to determine if image is manifest listed: %w", err)
 	}
@@ -436,7 +440,7 @@ func GetCorrectArchImage(ctx context.Context, component string, imageRef string,
 		log.Info(component + " image is a manifest listed image; extracting manifest for os/arch: " + operatingSystem + "/" + arch)
 
 		// Verify MF Image has the right os/arch image
-		imageRef, err = findImageRefByArch(ctx, imageRef, pullSecret, operatingSystem, arch)
+		imageRef, err = findImageRefByArch(ctx, imageRef, pullSecret, operatingSystem, arch, imageMetadataProvider)
 		if err != nil {
 			return "", fmt.Errorf("failed to extract appropriate os/arch manifest from %s: %w", imageRef, err)
 		}

diff --git a/support/releaseinfo/registryclient/client_test.go b/support/releaseinfo/registryclient/client_test.go
@@ -16,6 +16,18 @@ const (
 	LinuxOS           = "linux"
 )
 
+type fakeMetadataProvider interface {
+	GetManifest(ctx context.Context, imageRef string, pullSecret []byte) (distribution.Manifest, error)
+}
+
+type fakeRegistryClientImageMetadataProvider struct {
+	OpenShiftImageRegistryOverrides map[string][]string
+}
+
+func (f *fakeRegistryClientImageMetadataProvider) GetManifest(ctx context.Context, imageRef string, pullSecret []byte) (distribution.Manifest, error) {
+	return nil, nil
+}
+
 func TestFindMatchingManifest(t *testing.T) {
 	deserializedManifestList1 := &manifestlist.DeserializedManifestList{
 		ManifestList: manifestlist.ManifestList{
@@ -198,7 +210,9 @@ func TestFindMatchingManifest(t *testing.T) {
 
 func TestIsMultiArchManifestList(t *testing.T) {
 	pullSecretBytes := []byte("{\"auths\":{\"quay.io\":{\"auth\":\"\",\"email\":\"\"}}}")
-
+	imageMetadataProvider := &fakeRegistryClientImageMetadataProvider{
+		OpenShiftImageRegistryOverrides: map[string][]string{},
+	}
 	testCases := []struct {
 		name                   string
 		image                  string
@@ -239,7 +253,7 @@ func TestIsMultiArchManifestList(t *testing.T) {
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
 			g := NewWithT(t)
-			isMultiArchImage, err := IsMultiArchManifestList(context.TODO(), tc.image, tc.pullSecretBytes)
+			isMultiArchImage, err := IsMultiArchManifestList(context.TODO(), tc.image, tc.pullSecretBytes, imageMetadataProvider)
 			if tc.expectErr {
 				g.Expect(err).To(HaveOccurred())
 			} else {

diff --git a/support/util/fakeimagemetadataprovider/fakeimagemetadataprovider.go b/support/util/fakeimagemetadataprovider/fakeimagemetadataprovider.go
@@ -3,13 +3,19 @@ package fakeimagemetadataprovider
 import (
 	"context"
 
+	"github.com/docker/distribution"
 	"github.com/openshift/hypershift/support/thirdparty/library-go/pkg/image/dockerv1client"
 )
 
 type FakeImageMetadataProvider struct {
-	Result *dockerv1client.DockerImageConfig
+	Result   *dockerv1client.DockerImageConfig
+	Manifest distribution.Manifest
 }
 
 func (f *FakeImageMetadataProvider) ImageMetadata(ctx context.Context, imageRef string, pullSecret []byte) (*dockerv1client.DockerImageConfig, error) {
 	return f.Result, nil
 }
+
+func (f *FakeImageMetadataProvider) GetManifest(ctx context.Context, imageRef string, pullSecret []byte) (distribution.Manifest, error) {
+	return f.Manifest, nil
+}
diff --git a/support/util/imagemetadata.go b/support/util/imagemetadata.go
@@ -28,6 +28,7 @@ var (
 
 type ImageMetadataProvider interface {
 	ImageMetadata(ctx context.Context, imageRef string, pullSecret []byte) (*dockerv1client.DockerImageConfig, error)
+	GetManifest(ctx context.Context, imageRef string, pullSecret []byte) (distribution.Manifest, error)
 }
 
 type RegistryClientImageMetadataProvider struct {
@@ -131,6 +132,94 @@ func (r *RegistryClientImageMetadataProvider) ImageMetadata(ctx context.Context,
 	return config, nil
 }
 
+// GetManifest returns the manifest for a given image using the given pull secret
+// to authenticate. This lookup uses a cache based on the image digest. If The
+// reference of the image contains a digest (which is the mainline case for images in a release payload),
+// the digest is parsed from the image reference and then used to lookup the manifest in the
+// cache and return it with the ImageOverrides already included.
+func (r *RegistryClientImageMetadataProvider) GetManifest(ctx context.Context, imageRef string, pullSecret []byte) (distribution.Manifest, error) {
+	log := ctrl.LoggerFrom(ctx)
+
+	var (
+		repo           distribution.Repository
+		ref            *reference.DockerImageReference
+		parsedImageRef reference.DockerImageReference
+		err            error
+		overrideFound  bool
+	)
+
+	parsedImageRef, err = reference.Parse(imageRef)
+	if err != nil {
+		return nil, fmt.Errorf("failed to parse image reference %q: %w", imageRef, err)
+	}
+
+	// There are no ICSPs/IDMSs to process.
+	// That means the image reference should be pulled from the external registry
+	if len(r.OpenShiftImageRegistryOverrides) == 0 {
+		parsedImageRef, err = reference.Parse(imageRef)
+		if err != nil {
+			return nil, fmt.Errorf("failed to parse image reference %q: %w", imageRef, err)
+		}
+
+		// If the image reference contains a digest, immediately look it up in the cache
+		if parsedImageRef.ID != "" {
+			if manifest, exists := imageMetadataCache.Get("manifest-" + parsedImageRef.ID); exists {
+				return manifest.(distribution.Manifest), nil
+			}
+		}
+
+		ref = &parsedImageRef
+		repo, err = getRepository(ctx, *ref, pullSecret)
+		if err != nil {
+			return nil, err
+		}
+	}
+
+	// Get the image repo info based the source/mirrors in the ICSPs/IDMSs
+	for source, mirrors := range r.OpenShiftImageRegistryOverrides {
+		for _, mirror := range mirrors {
+			ref, overrideFound, err = GetRegistryOverrides(ctx, parsedImageRef, source, mirror)
+			if err != nil {
+				log.Info(fmt.Sprintf("failed to find registry override for image reference %q with source, %s, mirror %s: %s", imageRef, source, mirror, err.Error()))
+				continue
+			}
+			break
+		}
+		// We found a successful source/mirror combo so break continuing any further source/mirror combos
+		if overrideFound {
+			break
+		}
+	}
+
+	// If the image reference contains a digest, immediately look it up in the cache
+	if ref.ID != "" {
+		if manifest, exists := imageMetadataCache.Get("manifest-" + ref.ID); exists {
+			return manifest.(distribution.Manifest), nil
+		}
+	}
+
+	repo, err = getRepository(ctx, *ref, pullSecret)
+	if err != nil || repo == nil {
+		return nil, fmt.Errorf("failed to create repository client for %s: %w", ref.DockerClientDefaults().RegistryURL(), err)
+	}
+
+	ref.ID = parsedImageRef.ID
+	firstManifest, location, err := manifest.FirstManifest(ctx, *ref, repo)
+	if err != nil {
+		return nil, fmt.Errorf("failed to obtain root manifest for %s: %w", imageRef, err)
+	}
+
+	if ref.ID == "" {
+		if manifest, exists := imageMetadataCache.Get(string("manifest-" + location.Manifest)); exists {
+			return manifest.(distribution.Manifest), nil
+		}
+	}
+
+	imageMetadataCache.Add("manifest-"+string(location.Manifest), firstManifest)
+
+	return firstManifest, nil
+}
+
 func getRepository(ctx context.Context, ref reference.DockerImageReference, pullSecret []byte) (distribution.Repository, error) {
 	credStore, err := dockercredentials.NewFromBytes(pullSecret)
 	if err != nil {

diff --git a/support/util/util.go b/support/util/util.go
@@ -398,7 +398,7 @@ func DetermineHostedClusterPayloadArch(ctx context.Context, c client.Client, hc
 		return "", fmt.Errorf("expected %s key in pull secret", corev1.DockerConfigJsonKey)
 	}
 
-	isMultiArchReleaseImage, err := registryclient.IsMultiArchManifestList(ctx, hc.Spec.Release.Image, pullSecretBytes)
+	isMultiArchReleaseImage, err := registryclient.IsMultiArchManifestList(ctx, hc.Spec.Release.Image, pullSecretBytes, imageMetadataProvider)
 	if err != nil {
 		return "", fmt.Errorf("failed to determine if release image multi-arch: %w", err)
 	}