OCPBUGS-44655: Fix multiarch validations using MetadataProvider

Signed-off-by: Juan Manuel Parrilla Madrid <jparrill@redhat.com>
openshift · Nov 26, 2024 · 77e573c · 77e573c
1 parent 2569f33
commit 77e573c
Show file tree

Hide file tree

Showing 11 changed files with 375 additions and 63 deletions.
diff --git a/cmd/cluster/core/create.go b/cmd/cluster/core/create.go
@@ -648,7 +648,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
 		}
 	}
@@ -1012,7 +1012,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
@@ -1021,8 +1021,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,15 @@ func TestValidateMgmtClusterAndNodePoolCPUArchitectures(t *testing.T) {
 
 	fakeKubeClient := fakekubeclient.NewSimpleClientset()
 	fakeDiscovery, ok := fakeKubeClient.Discovery().(*fakediscovery.FakeDiscovery)
-
 	if !ok {
 		t.Fatalf("failed to convert FakeDiscovery")
 	}
 
+	fakeMetadataProvider := &fakeimagemetadataprovider.FakeRegistryClientImageMetadataProvider{
+		Result:   &dockerv1client.DockerImageConfig{},
+		Manifest: fakeimagemetadataprovider.FakeManifest{},
+	}
+
 	// if you want to fake a specific version
 	fakeDiscovery.FakedServerVersion = &apiversion.Info{
 		Platform: "linux/amd64",
@@ -80,7 +86,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/hypershift-operator/controllers/hostedcluster/hostedcluster_controller_test.go b/hypershift-operator/controllers/hostedcluster/hostedcluster_controller_test.go
@@ -10,6 +10,8 @@ import (
 
 	"github.com/openshift/hypershift/cmd/util"
 
+	"github.com/docker/distribution"
+	"github.com/docker/distribution/manifest/manifestlist"
 	"github.com/go-logr/logr"
 	"github.com/google/go-cmp/cmp"
 	. "github.com/onsi/gomega"
@@ -30,6 +32,7 @@ import (
 	"github.com/openshift/hypershift/support/config"
 	"github.com/openshift/hypershift/support/releaseinfo"
 	fakereleaseprovider "github.com/openshift/hypershift/support/releaseinfo/fake"
+	"github.com/openshift/hypershift/support/releaseinfo/registryclient"
 	"github.com/openshift/hypershift/support/thirdparty/library-go/pkg/image/dockerv1client"
 	"github.com/openshift/hypershift/support/upsert"
 	hyperutil "github.com/openshift/hypershift/support/util"
@@ -57,8 +60,17 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 )
 
-var Now = metav1.NewTime(time.Now())
-var Later = metav1.NewTime(Now.Add(5 * time.Minute))
+var (
+	Now   = metav1.NewTime(time.Now())
+	Later = metav1.NewTime(Now.Add(5 * time.Minute))
+)
+
+const (
+	ManifestListMediaType = "application/vnd.docker.distribution.manifest.list.v2+json"
+	LinuxOS               = "linux"
+	ArchitectureAMD64     = "amd64"
+	ArchitecturePPC64LE   = "ppc64le"
+)
 
 func TestHasBeenAvailable(t *testing.T) {
 	now := time.Now().Truncate(time.Second)
@@ -155,7 +167,10 @@ func TestHasBeenAvailable(t *testing.T) {
 				createOrUpdate:                func(reconcile.Request) upsert.CreateOrUpdateFN { return ctrl.CreateOrUpdate },
 				ManagementClusterCapabilities: &fakecapabilities.FakeSupportNoCapabilities{},
 				ReconcileMetadataProviders: func(ctx context.Context, imgOverrides map[string]string) (releaseinfo.ProviderWithOpenShiftImageRegistryOverrides, hyperutil.ImageMetadataProvider, error) {
-					return &fakereleaseprovider.FakeReleaseProvider{}, &fakeimagemetadataprovider.FakeImageMetadataProvider{Result: &dockerv1client.DockerImageConfig{}}, nil
+					return &fakereleaseprovider.FakeReleaseProvider{}, &fakeimagemetadataprovider.FakeRegistryClientImageMetadataProvider{
+						Result:   &dockerv1client.DockerImageConfig{},
+						Manifest: fakeimagemetadataprovider.FakeManifest{},
+					}, nil
 				},
 				now: func() metav1.Time { return reconcilerNow },
 			}
@@ -899,6 +914,35 @@ func expectedRules(addRules []rbacv1.PolicyRule) []rbacv1.PolicyRule {
 
 func TestHostedClusterWatchesEverythingItCreates(t *testing.T) {
 	releaseImage, _ := version.LookupDefaultOCPVersion("")
+	manifests := []manifestlist.ManifestDescriptor{
+		{
+			Descriptor: distribution.Descriptor{
+				MediaType: ManifestListMediaType,
+				Digest:    "sha256:70fb4524d21e1b6c08477eb5d1ca2cf282b3270b1d008f70dd7e1cf13d8ba4ce",
+			},
+			Platform: manifestlist.PlatformSpec{
+				Architecture: ArchitectureAMD64,
+				OS:           LinuxOS,
+			},
+		},
+		{
+			Descriptor: distribution.Descriptor{
+				MediaType: ManifestListMediaType,
+				Digest:    "sha256:70fb4524d21e1b6c08477eb5d1ca2cf282b3270b1d008f70dd7e1cf13d8ba4ce",
+			},
+			Platform: manifestlist.PlatformSpec{
+				Architecture: ArchitecturePPC64LE,
+				OS:           LinuxOS,
+			},
+		},
+	}
+	deserializeFunc := func(payload []byte) (*manifestlist.DeserializedManifestList, error) {
+		return &manifestlist.DeserializedManifestList{
+			ManifestList: manifestlist.ManifestList{
+				Manifests: manifests,
+			},
+		}, nil
+	}
 	hostedClusters := []*hyperv1.HostedCluster{
 		{
 			ObjectMeta: metav1.ObjectMeta{
@@ -1083,7 +1127,12 @@ func TestHostedClusterWatchesEverythingItCreates(t *testing.T) {
 		),
 		createOrUpdate: func(reconcile.Request) upsert.CreateOrUpdateFN { return ctrl.CreateOrUpdate },
 		ReconcileMetadataProviders: func(ctx context.Context, imgOverrides map[string]string) (releaseinfo.ProviderWithOpenShiftImageRegistryOverrides, hyperutil.ImageMetadataProvider, error) {
-			return &fakereleaseprovider.FakeReleaseProvider{}, &fakeimagemetadataprovider.FakeImageMetadataProvider{Result: &dockerv1client.DockerImageConfig{}}, nil
+			return &fakereleaseprovider.FakeReleaseProvider{},
+				&fakeimagemetadataprovider.FakeRegistryClientImageMetadataProvider{
+					MediaType: ManifestListMediaType,
+					Result:    &dockerv1client.DockerImageConfig{},
+					Manifest:  fakeimagemetadataprovider.FakeManifest{},
+				}, nil
 		},
 		EnableEtcdRecovery: true,
 		now:                metav1.Now,
@@ -1099,7 +1148,8 @@ func TestHostedClusterWatchesEverythingItCreates(t *testing.T) {
 
 	for _, hc := range hostedClusters {
 		t.Run(hc.Name, func(t *testing.T) {
-			_, err := r.Reconcile(context.Background(), reconcile.Request{NamespacedName: types.NamespacedName{Namespace: hc.Namespace, Name: hc.Name}})
+			ctx := context.WithValue(context.Background(), registryclient.DeserializeFuncName, deserializeFunc)
+			_, err := r.Reconcile(ctx, reconcile.Request{NamespacedName: types.NamespacedName{Namespace: hc.Namespace, Name: hc.Name}})
 			if err != nil {
 				t.Fatalf("Reconcile failed: %v", err)
 			}
@@ -1910,7 +1960,7 @@ func TestValidateReleaseImage(t *testing.T) {
 								"image-4.18.0": "4.18.0",
 							},
 						},
-						&fakeimagemetadataprovider.FakeImageMetadataProvider{
+						&fakeimagemetadataprovider.FakeRegistryClientImageMetadataProvider{
 							Result: &dockerv1client.DockerImageConfig{},
 						},
 						nil
@@ -2252,7 +2302,7 @@ func TestIsUpgradeable(t *testing.T) {
 							"image-4.14":   "4.15.0",
 						},
 					},
-					&fakeimagemetadataprovider.FakeImageMetadataProvider{
+					&fakeimagemetadataprovider.FakeRegistryClientImageMetadataProvider{
 						Result: &dockerv1client.DockerImageConfig{},
 					},
 					nil
@@ -2609,7 +2659,7 @@ func TestIsProgressing(t *testing.T) {
 							"release-1.3": "1.3.0",
 						},
 					},
-					&fakeimagemetadataprovider.FakeImageMetadataProvider{
+					&fakeimagemetadataprovider.FakeRegistryClientImageMetadataProvider{
 						Result: &dockerv1client.DockerImageConfig{},
 					},
 					nil
@@ -3535,7 +3585,7 @@ func TestKubevirtETCDEncKey(t *testing.T) {
 				),
 				createOrUpdate: func(reconcile.Request) upsert.CreateOrUpdateFN { return ctrl.CreateOrUpdate },
 				ReconcileMetadataProviders: func(ctx context.Context, imgOverrides map[string]string) (releaseinfo.ProviderWithOpenShiftImageRegistryOverrides, hyperutil.ImageMetadataProvider, error) {
-					return &fakereleaseprovider.FakeReleaseProvider{}, &fakeimagemetadataprovider.FakeImageMetadataProvider{Result: &dockerv1client.DockerImageConfig{}}, nil
+					return &fakereleaseprovider.FakeReleaseProvider{}, &fakeimagemetadataprovider.FakeRegistryClientImageMetadataProvider{Result: &dockerv1client.DockerImageConfig{}}, nil
 				},
 				now: metav1.Now,
 			}

diff --git a/hypershift-operator/controllers/nodepool/secret_janitor_test.go b/hypershift-operator/controllers/nodepool/secret_janitor_test.go
@@ -156,7 +156,7 @@ spec:
 			//We need the ReleaseProvider to stay at 4.18 so that the token doesnt get updated when bumping releases,
 			// this protects us from possibly hiding other factors that might be causing the token to be updated
 			ReleaseProvider: &fakereleaseprovider.FakeReleaseProvider{Version: semver.MustParse("4.18.0").String()},
-			ImageMetadataProvider: &fakeimagemetadataprovider.FakeImageMetadataProvider{Result: &dockerv1client.DockerImageConfig{Config: &docker10.DockerConfig{
+			ImageMetadataProvider: &fakeimagemetadataprovider.FakeRegistryClientImageMetadataProvider{Result: &dockerv1client.DockerImageConfig{Config: &docker10.DockerConfig{
 				Labels: map[string]string{},
 			}}},
 		},

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
@@ -33,8 +33,15 @@ const (
 	ArchitectureS390X   = "s390x"
 	ArchitecturePPC64LE = "ppc64le"
 	ArchitectureARM64   = "arm64"
+
+	DeserializeFuncName deserializeFuncCtxKey = "deserializeFunc"
 )
 
+type deserializeFuncCtxKey string
+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 +322,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)
 	}
@@ -332,13 +339,19 @@ func IsMultiArchManifestList(ctx context.Context, imageRef string, pullSecret []
 		return false, nil
 	}
 
-	deserializedManifestList := new(manifestlist.DeserializedManifestList)
-	if err = deserializedManifestList.UnmarshalJSON(payload); err != nil {
-		return false, fmt.Errorf("failed to get unmarshalled manifest list: %w", err)
+	// Default to using the deserializeManifest function, but allow for a custom deserialization function to be passed in the context for testing purposes and avoiding paralelism issues
+	deserializeFunc := deserializeManifest
+	if ctx.Value(DeserializeFuncName) != nil {
+		deserializeFunc = ctx.Value(DeserializeFuncName).(func([]byte) (*manifestlist.DeserializedManifestList, error))
+	}
+
+	manifestList, err := deserializeFunc(payload)
+	if err != nil {
+		return false, fmt.Errorf("failed to deserialize payload: %w", err)
 	}
 
 	count := 0
-	for _, arch := range deserializedManifestList.ManifestList.Manifests {
+	for _, arch := range manifestList.ManifestList.Manifests {
 		switch arch.Platform.Architecture {
 		case ArchitectureAMD64, ArchitectureS390X, ArchitecturePPC64LE, ArchitectureARM64:
 			count = count + 1
@@ -351,9 +364,18 @@ func IsMultiArchManifestList(ctx context.Context, imageRef string, pullSecret []
 	return false, nil
 }
 
+func deserializeManifest(b []byte) (*manifestlist.DeserializedManifestList, error) {
+	deserializedManifestList := new(manifestlist.DeserializedManifestList)
+	if err := deserializedManifestList.UnmarshalJSON(b); err != nil {
+		return nil, fmt.Errorf("failed to get unmarshalled manifest list: %w", err)
+	}
+
+	return deserializedManifestList, nil
+}
+
 // 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 +444,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 +458,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)
 		}