diff --git a/control-plane-operator/hostedclusterconfigoperator/cmd.go b/control-plane-operator/hostedclusterconfigoperator/cmd.go index 169c19e88176..6f71a0887383 100644 --- a/control-plane-operator/hostedclusterconfigoperator/cmd.go +++ b/control-plane-operator/hostedclusterconfigoperator/cmd.go @@ -292,12 +292,11 @@ func (o *HostedClusterConfigOperator) Run(ctx context.Context) error { }, RegistryOverrides: nil, }, - OpenShiftImageRegistryOverrides: imageRegistryOverrides, } + releaseProvider.SetOpenShiftImageRegistryOverrides(imageRegistryOverrides) - imageMetaDataProvider := &imageregistry.RegistryClientImageMetadataProvider{ - OpenShiftImageRegistryOverrides: imageRegistryOverrides, - } + imageMetaDataProvider := &imageregistry.RegistryClientImageMetadataProvider{} + imageMetaDataProvider.SetOpenShiftImageRegistryOverrides(imageRegistryOverrides) apiReadingClient, err := client.New(mgr.GetConfig(), client.Options{Scheme: hyperapi.Scheme}) if err != nil { diff --git a/control-plane-operator/main.go b/control-plane-operator/main.go index 1dee4413d062..c9394b10cb23 100644 --- a/control-plane-operator/main.go +++ b/control-plane-operator/main.go @@ -304,8 +304,8 @@ func buildReleaseProviders(componentImages map[string]string, registryOverrides Delegate: coreReleaseProvider, RegistryOverrides: nil, // UserReleaseProvider shouldn't include registry overrides as they should not get propagated to the data plane. }, - OpenShiftImageRegistryOverrides: imageRegistryOverrides, } + userReleaseProvider.SetOpenShiftImageRegistryOverrides(imageRegistryOverrides) // It should be used to lookup spec.controlPlaneReleaseImage. cpReleaseProvider := &releaseinfo.ProviderWithOpenShiftImageRegistryOverridesDecorator{ @@ -313,8 +313,8 @@ func buildReleaseProviders(componentImages map[string]string, registryOverrides Delegate: coreReleaseProvider, RegistryOverrides: registryOverrides, }, - OpenShiftImageRegistryOverrides: imageRegistryOverrides, } + cpReleaseProvider.SetOpenShiftImageRegistryOverrides(imageRegistryOverrides) return cpReleaseProvider, userReleaseProvider } @@ -508,9 +508,8 @@ func NewStartCommand() *cobra.Command { cpReleaseProvider, userReleaseProvider := buildReleaseProviders(componentImages, registryOverrides, imageRegistryOverrides) - imageMetaDataProvider := &imageregistry.RegistryClientImageMetadataProvider{ - OpenShiftImageRegistryOverrides: imageRegistryOverrides, - } + imageMetaDataProvider := &imageregistry.RegistryClientImageMetadataProvider{} + imageMetaDataProvider.SetOpenShiftImageRegistryOverrides(imageRegistryOverrides) defaultIngressDomain := os.Getenv(config.DefaultIngressDomainEnvVar) diff --git a/ignition-server/cmd/run_local_ignitionprovider.go b/ignition-server/cmd/run_local_ignitionprovider.go index 8ad98cf0b8c1..980ae5c3f0af 100644 --- a/ignition-server/cmd/run_local_ignitionprovider.go +++ b/ignition-server/cmd/run_local_ignitionprovider.go @@ -96,9 +96,8 @@ func (o *RunLocalIgnitionProviderOptions) Run(ctx context.Context) error { return fmt.Errorf("unable to create image file cache: %w", err) } - imageMetaDataProvider := &imageregistry.RegistryClientImageMetadataProvider{ - OpenShiftImageRegistryOverrides: map[string][]string{}, - } + imageMetaDataProvider := &imageregistry.RegistryClientImageMetadataProvider{} + imageMetaDataProvider.SetOpenShiftImageRegistryOverrides(map[string][]string{}) p := &controllers.LocalIgnitionProvider{ Client: cl, diff --git a/ignition-server/cmd/start.go b/ignition-server/cmd/start.go index 4979aa6b58d4..698b7cfff1d1 100644 --- a/ignition-server/cmd/start.go +++ b/ignition-server/cmd/start.go @@ -180,24 +180,25 @@ func setUpPayloadStoreReconciler(ctx context.Context, registryOverrides map[stri return nil, fmt.Errorf("unable to create image file cache: %w", err) } - imageMetaDataProvider := &imageregistry.RegistryClientImageMetadataProvider{ - OpenShiftImageRegistryOverrides: imageregistry.ConvertImageRegistryOverrideStringToMap(os.Getenv("OPENSHIFT_IMG_OVERRIDES")), + imageRegistryOverrides := imageregistry.ConvertImageRegistryOverrideStringToMap(os.Getenv("OPENSHIFT_IMG_OVERRIDES")) + imageMetaDataProvider := &imageregistry.RegistryClientImageMetadataProvider{} + imageMetaDataProvider.SetOpenShiftImageRegistryOverrides(imageRegistryOverrides) + releaseProvider := &releaseinfo.ProviderWithOpenShiftImageRegistryOverridesDecorator{ + Delegate: &releaseinfo.RegistryMirrorProviderDecorator{ + Delegate: &releaseinfo.CachedProvider{ + Inner: &releaseinfo.RegistryClientProvider{}, + Cache: map[string]*releaseinfo.ReleaseImage{}, + }, + RegistryOverrides: registryOverrides, + }, } + releaseProvider.SetOpenShiftImageRegistryOverrides(imageRegistryOverrides) if err = (&controllers.TokenSecretReconciler{ Client: mgr.GetClient(), PayloadStore: payloadStore, IgnitionProvider: &controllers.LocalIgnitionProvider{ - ReleaseProvider: &releaseinfo.ProviderWithOpenShiftImageRegistryOverridesDecorator{ - Delegate: &releaseinfo.RegistryMirrorProviderDecorator{ - Delegate: &releaseinfo.CachedProvider{ - Inner: &releaseinfo.RegistryClientProvider{}, - Cache: map[string]*releaseinfo.ReleaseImage{}, - }, - RegistryOverrides: registryOverrides, - }, - OpenShiftImageRegistryOverrides: imageregistry.ConvertImageRegistryOverrideStringToMap(os.Getenv("OPENSHIFT_IMG_OVERRIDES")), - }, + ReleaseProvider: releaseProvider, Client: mgr.GetClient(), Namespace: os.Getenv(namespaceEnvVariableName), CloudProvider: cloudProvider, diff --git a/karpenter-operator/main.go b/karpenter-operator/main.go index 5e656cb5c337..53c28ca60df9 100644 --- a/karpenter-operator/main.go +++ b/karpenter-operator/main.go @@ -187,12 +187,11 @@ func run(ctx context.Context) error { }, RegistryOverrides: nil, }, - OpenShiftImageRegistryOverrides: imageRegistryOverrides, } + releaseProvider.SetOpenShiftImageRegistryOverrides(imageRegistryOverrides) - imageMetaDataProvider := &imageregistry.RegistryClientImageMetadataProvider{ - OpenShiftImageRegistryOverrides: imageRegistryOverrides, - } + imageMetaDataProvider := &imageregistry.RegistryClientImageMetadataProvider{} + imageMetaDataProvider.SetOpenShiftImageRegistryOverrides(imageRegistryOverrides) kir := karpenterignition.KarpenterIgnitionReconciler{ ReleaseProvider: releaseProvider, diff --git a/support/globalconfig/imagecontentsource.go b/support/globalconfig/imagecontentsource.go index c85664fbe443..71dd687fefdf 100644 --- a/support/globalconfig/imagecontentsource.go +++ b/support/globalconfig/imagecontentsource.go @@ -245,12 +245,11 @@ func NewCommonRegistryProvider(ctx context.Context, capChecker capabilities.Capa }, RegistryOverrides: registryOverrides, }, - OpenShiftImageRegistryOverrides: imageRegistryMirrors, } + releaseProvider.SetOpenShiftImageRegistryOverrides(imageRegistryMirrors) - metadataProvider := &imageregistry.RegistryClientImageMetadataProvider{ - OpenShiftImageRegistryOverrides: imageRegistryMirrors, - } + metadataProvider := &imageregistry.RegistryClientImageMetadataProvider{} + metadataProvider.SetOpenShiftImageRegistryOverrides(imageRegistryMirrors) provider := CommonRegistryProvider{ capChecker: capChecker, @@ -286,8 +285,8 @@ func (rp CommonRegistryProvider) Reconcile(ctx context.Context, client crclient. } } - rp.ReleaseProvider.OpenShiftImageRegistryOverrides = imageRegistryMirrors - rp.MetadataProvider.OpenShiftImageRegistryOverrides = imageRegistryMirrors + rp.ReleaseProvider.SetOpenShiftImageRegistryOverrides(imageRegistryMirrors) + rp.MetadataProvider.SetOpenShiftImageRegistryOverrides(imageRegistryMirrors) return nil } diff --git a/support/globalconfig/imagecontentsource_test.go b/support/globalconfig/imagecontentsource_test.go index 11154272f64f..c1be5bad7e9e 100644 --- a/support/globalconfig/imagecontentsource_test.go +++ b/support/globalconfig/imagecontentsource_test.go @@ -8,7 +8,6 @@ import ( "github.com/openshift/hypershift/support/api" "github.com/openshift/hypershift/support/capabilities" - "github.com/openshift/hypershift/support/imageregistry" "github.com/openshift/hypershift/support/releaseinfo" configv1 "github.com/openshift/api/config/v1" @@ -210,12 +209,12 @@ func TestReconcileMgmtImageRegistryOverrides(t *testing.T) { idms *configv1.ImageDigestMirrorSetList icsp *operatorv1alpha1.ImageContentSourcePolicyList expectedRelease *releaseinfo.ProviderWithOpenShiftImageRegistryOverridesDecorator - expectedMetadata *imageregistry.RegistryClientImageMetadataProvider + expectedOverrides map[string][]string expectedError error expectedErrorMatch string }{ { - name: "Test with ICSP and IDMS capabilities", + name: "When ICSP and IDMS are supported, it should initialize both providers with merged mirrors", capChecker: &capabilities.MockCapabilityChecker{ MockHas: func(capability ...capabilities.CapabilityType) bool { return slices.Contains(capability, capabilities.CapabilityICSP) || slices.Contains(capability, capabilities.CapabilityIDMS) @@ -238,49 +237,29 @@ func TestReconcileMgmtImageRegistryOverrides(t *testing.T) { "registry2": "override2", }, }, - OpenShiftImageRegistryOverrides: map[string][]string{ - "registry1": {"icsp-registry-mirrors-2/mirror1", "icsp-registry-mirrors-2/mirror2", "icsp-registry-mirrors-1/mirror1", "icsp-registry-mirrors-1/mirror2"}, - "registry2": {"mirror1", "mirror2"}, - "registry1.sample.com/samplens/sampleimage@sha256:123456": { - "mirror1.sample.com/samplens/sampleimage@sha256:123456", - "mirror1.sample.com/samplens/sampleimage@sha256:123456", - }, - "registry2.sample.com/samplens/sampleimage@sha256:123456": { - "mirror2.sample.com/samplens/sampleimage@sha256:123456", - "mirror2.sample.com/samplens/sampleimage@sha256:123456", - }, - "registry3.sample.com/samplens/sampleimage@sha256:123456": { - "mirror3.sample.com/samplens/sampleimage@sha256:123456", - "mirror3.sample.com/samplens/sampleimage@sha256:123456", - "mirroricsp3.sample.com/samplens/sampleimage@sha256:123456", - "mirroricsp3.sample.com/samplens/sampleimage@sha256:123456", - }, - }, }, - expectedMetadata: &imageregistry.RegistryClientImageMetadataProvider{ - OpenShiftImageRegistryOverrides: map[string][]string{ - "registry1": {"icsp-registry-mirrors-2/mirror1", "icsp-registry-mirrors-2/mirror2", "icsp-registry-mirrors-1/mirror1", "icsp-registry-mirrors-1/mirror2"}, - "registry2": {"mirror1", "mirror2"}, - "registry1.sample.com/samplens/sampleimage@sha256:123456": { - "mirror1.sample.com/samplens/sampleimage@sha256:123456", - "mirror1.sample.com/samplens/sampleimage@sha256:123456", - }, - "registry2.sample.com/samplens/sampleimage@sha256:123456": { - "mirror2.sample.com/samplens/sampleimage@sha256:123456", - "mirror2.sample.com/samplens/sampleimage@sha256:123456", - }, - "registry3.sample.com/samplens/sampleimage@sha256:123456": { - "mirror3.sample.com/samplens/sampleimage@sha256:123456", - "mirror3.sample.com/samplens/sampleimage@sha256:123456", - "mirroricsp3.sample.com/samplens/sampleimage@sha256:123456", - "mirroricsp3.sample.com/samplens/sampleimage@sha256:123456", - }, + expectedOverrides: map[string][]string{ + "registry1": {"icsp-registry-mirrors-2/mirror1", "icsp-registry-mirrors-2/mirror2", "icsp-registry-mirrors-1/mirror1", "icsp-registry-mirrors-1/mirror2"}, + "registry2": {"mirror1", "mirror2"}, + "registry1.sample.com/samplens/sampleimage@sha256:123456": { + "mirror1.sample.com/samplens/sampleimage@sha256:123456", + "mirror1.sample.com/samplens/sampleimage@sha256:123456", + }, + "registry2.sample.com/samplens/sampleimage@sha256:123456": { + "mirror2.sample.com/samplens/sampleimage@sha256:123456", + "mirror2.sample.com/samplens/sampleimage@sha256:123456", + }, + "registry3.sample.com/samplens/sampleimage@sha256:123456": { + "mirror3.sample.com/samplens/sampleimage@sha256:123456", + "mirror3.sample.com/samplens/sampleimage@sha256:123456", + "mirroricsp3.sample.com/samplens/sampleimage@sha256:123456", + "mirroricsp3.sample.com/samplens/sampleimage@sha256:123456", }, }, expectedError: nil, }, { - name: "Test without ICSP and IDMS capabilities", + name: "When ICSP and IDMS are unsupported, it should initialize both providers without mirrors", capChecker: &capabilities.MockCapabilityChecker{ MockHas: func(capability ...capabilities.CapabilityType) bool { return false @@ -301,10 +280,6 @@ func TestReconcileMgmtImageRegistryOverrides(t *testing.T) { "registry2": "override2", }, }, - OpenShiftImageRegistryOverrides: nil, - }, - expectedMetadata: &imageregistry.RegistryClientImageMetadataProvider{ - OpenShiftImageRegistryOverrides: nil, }, expectedError: nil, }, @@ -343,10 +318,53 @@ func TestReconcileMgmtImageRegistryOverrides(t *testing.T) { } // Check release provider - g.Expect(provider.GetReleaseProvider()).To(Equal(tc.expectedRelease)) + g.Expect(provider.ReleaseProvider.Delegate).To(Equal(tc.expectedRelease.Delegate)) + g.Expect(provider.GetReleaseProvider().GetOpenShiftImageRegistryOverrides()).To(Equal(tc.expectedOverrides)) // Check image metadata provider - g.Expect(provider.GetMetadataProvider()).To(Equal(tc.expectedMetadata)) + g.Expect(provider.GetMetadataProvider()).To(BeIdenticalTo(provider.MetadataProvider)) + g.Expect(provider.MetadataProvider.GetOpenShiftImageRegistryOverrides()).To(Equal(tc.expectedOverrides)) }) } } + +func TestCommonRegistryProvider_Reconcile(t *testing.T) { + t.Run("When ICSP and IDMS capabilities are present, it should publish one merged snapshot to both providers", func(t *testing.T) { + g := NewWithT(t) + ctx := t.Context() + idms := createFakeIDMS() + icsp := createFakeICSP() + var objects []client.Object + for i := range idms.Items { + objects = append(objects, &idms.Items[i]) + } + for i := range icsp.Items { + objects = append(objects, &icsp.Items[i]) + } + + client := fake.NewClientBuilder().WithScheme(api.Scheme).WithObjects(objects...).Build() + capChecker := &capabilities.MockCapabilityChecker{ + MockHas: func(capability ...capabilities.CapabilityType) bool { + return slices.Contains(capability, capabilities.CapabilityICSP) || slices.Contains(capability, capabilities.CapabilityIDMS) + }, + } + provider, err := NewCommonRegistryProvider(ctx, capChecker, client, nil) + g.Expect(err).ToNot(HaveOccurred()) + + g.Expect(provider.Reconcile(ctx, client)).To(Succeed()) + expected := map[string][]string{ + "registry1": {"icsp-registry-mirrors-2/mirror1", "icsp-registry-mirrors-2/mirror2", "icsp-registry-mirrors-1/mirror1", "icsp-registry-mirrors-1/mirror2"}, + "registry2": {"mirror1", "mirror2"}, + "registry1.sample.com/samplens/sampleimage@sha256:123456": {"mirror1.sample.com/samplens/sampleimage@sha256:123456", "mirror1.sample.com/samplens/sampleimage@sha256:123456"}, + "registry2.sample.com/samplens/sampleimage@sha256:123456": {"mirror2.sample.com/samplens/sampleimage@sha256:123456", "mirror2.sample.com/samplens/sampleimage@sha256:123456"}, + "registry3.sample.com/samplens/sampleimage@sha256:123456": { + "mirror3.sample.com/samplens/sampleimage@sha256:123456", + "mirror3.sample.com/samplens/sampleimage@sha256:123456", + "mirroricsp3.sample.com/samplens/sampleimage@sha256:123456", + "mirroricsp3.sample.com/samplens/sampleimage@sha256:123456", + }, + } + g.Expect(provider.ReleaseProvider.GetOpenShiftImageRegistryOverrides()).To(Equal(expected)) + g.Expect(provider.MetadataProvider.GetOpenShiftImageRegistryOverrides()).To(Equal(expected)) + }) +} diff --git a/support/imageregistry/imagemetadata.go b/support/imageregistry/imagemetadata.go index c9bf535dcb9f..12f568cd46fd 100644 --- a/support/imageregistry/imagemetadata.go +++ b/support/imageregistry/imagemetadata.go @@ -6,6 +6,7 @@ import ( "fmt" "net/http" "sync" + "sync/atomic" "time" hyperv1 "github.com/openshift/hypershift/api/hypershift/v1beta1" @@ -116,13 +117,33 @@ type ImageMetadataProvider interface { type metadataGetterFn func(ctx context.Context, imageRef string, pullSecret []byte) (*dockerv1client.DockerImageConfig, []distribution.Descriptor, distribution.BlobStore, error) type RegistryClientImageMetadataProvider struct { - OpenShiftImageRegistryOverrides map[string][]string + // overrides holds an immutable snapshot published by SetOpenShiftImageRegistryOverrides. + overrides atomic.Pointer[map[string][]string] // repoSetupFn overrides GetRepoSetup for testing; nil means use the real implementation. repoSetupFn func(ctx context.Context, imageRef string, pullSecret []byte) (distribution.Repository, *reference.DockerImageReference, error) // metadataGetter overrides getMetadata for testing; nil means use the real implementation. metadataGetter metadataGetterFn } +// SetOpenShiftImageRegistryOverrides publishes the current image registry mirror snapshot. +func (r *RegistryClientImageMetadataProvider) SetOpenShiftImageRegistryOverrides(overrides map[string][]string) { + snapshot := releaseinfo.CopyOpenShiftImageRegistryOverrides(overrides) + r.overrides.Store(&snapshot) +} + +// GetOpenShiftImageRegistryOverrides returns a defensive copy of the current mirror snapshot. +func (r *RegistryClientImageMetadataProvider) GetOpenShiftImageRegistryOverrides() map[string][]string { + return releaseinfo.CopyOpenShiftImageRegistryOverrides(r.loadOverrides()) +} + +// loadOverrides returns an immutable snapshot; callers must not modify it. +func (r *RegistryClientImageMetadataProvider) loadOverrides() map[string][]string { + if snapshot := r.overrides.Load(); snapshot != nil { + return *snapshot + } + return nil +} + func (r *RegistryClientImageMetadataProvider) getRepoSetup(ctx context.Context, imageRef string, pullSecret []byte) (distribution.Repository, *reference.DockerImageReference, error) { if r.repoSetupFn != nil { return r.repoSetupFn(ctx, imageRef, pullSecret) @@ -214,7 +235,8 @@ func (r *RegistryClientImageMetadataProvider) GetDigest(ctx context.Context, ima // There are no ICSPs/IDMSs to process. // That means the image reference should be pulled from the external registry - if len(r.OpenShiftImageRegistryOverrides) == 0 { + overrides := r.loadOverrides() + if len(overrides) == 0 { // If the image name is in the cache, return early if imageDigest, exists := digestCache.Get(imageRef); exists { parsedImageRef.ID = string(imageDigest.(digest.Digest)) @@ -225,7 +247,7 @@ func (r *RegistryClientImageMetadataProvider) GetDigest(ctx context.Context, ima } // Get the image repo info based the source/mirrors in the ICSPs/IDMSs - ref = r.seekOverride(ctx, parsedImageRef, pullSecret) + ref = r.seekOverrideWithOverrides(ctx, parsedImageRef, pullSecret, overrides) composedRef := ref.String() // If the overridden image name is in the cache, return early @@ -297,7 +319,8 @@ func (r *RegistryClientImageMetadataProvider) GetMetadata(ctx context.Context, i err error ) - if len(r.OpenShiftImageRegistryOverrides) == 0 { + overrides := r.loadOverrides() + if len(overrides) == 0 { return getMetadata(ctx, imageRef, pullSecret) } @@ -307,7 +330,7 @@ func (r *RegistryClientImageMetadataProvider) GetMetadata(ctx context.Context, i } // Get the image repo info based the source/mirrors in the ICSPs/IDMSs - ref = r.seekOverride(ctx, parsedImageRef, pullSecret) + ref = r.seekOverrideWithOverrides(ctx, parsedImageRef, pullSecret, overrides) composedRef := ref.String() return getMetadata(ctx, composedRef, pullSecret) @@ -538,9 +561,13 @@ func GetPayloadVersion(ctx context.Context, releaseImageProvider releaseinfo.Pro } func (r *RegistryClientImageMetadataProvider) seekOverride(ctx context.Context, parsedImageReference reference.DockerImageReference, pullSecret []byte) *reference.DockerImageReference { + return r.seekOverrideWithOverrides(ctx, parsedImageReference, pullSecret, r.loadOverrides()) +} + +func (r *RegistryClientImageMetadataProvider) seekOverrideWithOverrides(ctx context.Context, parsedImageReference reference.DockerImageReference, pullSecret []byte, overrides map[string][]string) *reference.DockerImageReference { getter := r.getMetadataGetter() log := ctrl.LoggerFrom(ctx) - for source, mirrors := range r.OpenShiftImageRegistryOverrides { + for source, mirrors := range overrides { // Skip empty sources if source == "" { continue diff --git a/support/imageregistry/imagemetadata_test.go b/support/imageregistry/imagemetadata_test.go index f9dd9079eada..287c58eb0df7 100644 --- a/support/imageregistry/imagemetadata_test.go +++ b/support/imageregistry/imagemetadata_test.go @@ -119,9 +119,8 @@ func TestGetDigest(t *testing.T) { ctrl := gomock.NewController(t) - provider := &RegistryClientImageMetadataProvider{ - OpenShiftImageRegistryOverrides: tc.overrides, - } + provider := &RegistryClientImageMetadataProvider{} + provider.SetOpenShiftImageRegistryOverrides(tc.overrides) if tc.setupMock != nil { provider.repoSetupFn = tc.setupMock(ctrl) } @@ -231,9 +230,8 @@ func TestGetManifest(t *testing.T) { ctrl := gomock.NewController(t) - provider := &RegistryClientImageMetadataProvider{ - OpenShiftImageRegistryOverrides: tc.overrides, - } + provider := &RegistryClientImageMetadataProvider{} + provider.SetOpenShiftImageRegistryOverrides(tc.overrides) if tc.setupMock != nil { provider.repoSetupFn = tc.setupMock(ctrl) } @@ -514,9 +512,9 @@ func TestSeekOverride(t *testing.T) { g := NewGomegaWithT(t) pullSecret := []byte(`{"auths":{}}`) provider := &RegistryClientImageMetadataProvider{ - OpenShiftImageRegistryOverrides: tc.overrides, - metadataGetter: fakeGetter, + metadataGetter: fakeGetter, } + provider.SetOpenShiftImageRegistryOverrides(tc.overrides) imgRef := provider.seekOverride(ctx, tc.imageRef, pullSecret) g.Expect(imgRef).To(Equal(tc.expectedImgRef), fmt.Sprintf("Expected image reference to be equal to: %v, \nbut got: %v", tc.expectedImgRef, imgRef)) }) @@ -548,6 +546,140 @@ func TestGetMetadataGetter(t *testing.T) { }) } +func TestRegistryClientImageMetadataProvider_ConcurrentOverrides(t *testing.T) { + t.Run("When mirror overrides are refreshed during lookup, it should use synchronized snapshots", func(t *testing.T) { + mirrorCache.mutex.Lock() + originalMirrorCache := mirrorCache.cache + mirrorCache.cache = make(map[string]mirrorCacheEntry) + mirrorCache.mutex.Unlock() + defer func() { + mirrorCache.mutex.Lock() + mirrorCache.cache = originalMirrorCache + mirrorCache.mutex.Unlock() + }() + + const waitTimeout = time.Second + const sourceRegistry = "quay.io" + const namespace = "registry-lock-test" + const oldMirrorRegistry = "mirror-a.registry-lock-test.example.com" + const newMirrorRegistry = "mirror-b.registry-lock-test.example.com" + oldMirrorImage := oldMirrorRegistry + "/" + namespace + "/image:latest" + newMirrorImage := newMirrorRegistry + "/" + namespace + "/image:latest" + canonicalImage := sourceRegistry + "/" + namespace + "/image:latest" + requestedImages := make([]string, 0, 2) + lookupStarted := make(chan struct{}, 1) + releaseLookup := make(chan struct{}) + lookupDone := make(chan struct{}) + firstResult := make(chan *reference.DockerImageReference, 1) + releaseLookupClosed := false + defer func() { + if !releaseLookupClosed { + close(releaseLookup) + } + }() + + provider := &RegistryClientImageMetadataProvider{ + metadataGetter: func(ctx context.Context, imageRef string, pullSecret []byte) (*dockerv1client.DockerImageConfig, []distribution.Descriptor, distribution.BlobStore, error) { + requestedImages = append(requestedImages, imageRef) + if imageRef == oldMirrorImage { + lookupStarted <- struct{}{} + select { + case <-releaseLookup: + case <-ctx.Done(): + return nil, nil, nil, ctx.Err() + } + return nil, nil, nil, fmt.Errorf("mirror unavailable") + } + return nil, nil, nil, nil + }, + } + provider.SetOpenShiftImageRegistryOverrides(map[string][]string{ + sourceRegistry: {oldMirrorRegistry}, + }) + parsedRef := reference.DockerImageReference{ + Registry: sourceRegistry, + Namespace: namespace, + Name: "image", + Tag: "latest", + } + + go func() { + defer close(lookupDone) + firstResult <- provider.seekOverride(t.Context(), parsedRef, []byte(`{"auths":{}}`)) + }() + + select { + case <-lookupStarted: + case <-time.After(waitTimeout): + t.Fatal("timed out waiting for metadata lookup to block") + } + + setDone := make(chan struct{}) + go func() { + provider.SetOpenShiftImageRegistryOverrides(map[string][]string{ + sourceRegistry: {newMirrorRegistry}, + }) + close(setDone) + }() + + select { + case <-setDone: + case <-time.After(waitTimeout): + t.Fatal("override publication waited for the blocked metadata lookup") + } + + close(releaseLookup) + releaseLookupClosed = true + select { + case <-lookupDone: + case <-time.After(waitTimeout): + t.Fatal("timed out waiting for the first metadata lookup to finish") + } + + g := NewWithT(t) + g.Expect((<-firstResult).String()).To(Equal(canonicalImage)) + g.Expect(requestedImages).To(Equal([]string{oldMirrorImage})) + g.Expect(provider.seekOverride(t.Context(), parsedRef, []byte(`{"auths":{}}`)).String()).To(Equal(newMirrorImage)) + g.Expect(requestedImages).To(Equal([]string{oldMirrorImage, newMirrorImage})) + }) +} + +func TestRegistryClientImageMetadataProvider_SetOpenShiftImageRegistryOverrides(t *testing.T) { + t.Run("When overrides are cleared, it should preserve nil and empty snapshots", func(t *testing.T) { + g := NewWithT(t) + provider := &RegistryClientImageMetadataProvider{} + g.Expect(provider.GetOpenShiftImageRegistryOverrides()).To(BeNil()) + for _, overrides := range []map[string][]string{ + {"quay.io": {"mirror.example.com"}}, + nil, + {}, + {"quay.io": nil, "registry.example.com": {}}, + } { + provider.SetOpenShiftImageRegistryOverrides(overrides) + g.Expect(provider.GetOpenShiftImageRegistryOverrides()).To(Equal(overrides)) + } + }) + + t.Run("When the input snapshot is modified, it should preserve the provider state", func(t *testing.T) { + g := NewWithT(t) + provider := &RegistryClientImageMetadataProvider{} + overrides := map[string][]string{ + "quay.io": {"mirror-a.example.com", "mirror-b.example.com"}, + } + + provider.SetOpenShiftImageRegistryOverrides(overrides) + overrides["quay.io"][0] = "mutated.example.com" + delete(overrides, "quay.io") + snapshot := provider.GetOpenShiftImageRegistryOverrides() + snapshot["quay.io"][0] = "mutated-output.example.com" + delete(snapshot, "quay.io") + + g.Expect(provider.GetOpenShiftImageRegistryOverrides()).To(Equal(map[string][]string{ + "quay.io": {"mirror-a.example.com", "mirror-b.example.com"}, + })) + }) +} + func TestImageLabels(t *testing.T) { testCases := []struct { name string @@ -1043,9 +1175,9 @@ func TestSeekOverrideWithCache(t *testing.T) { } provider := &RegistryClientImageMetadataProvider{ - OpenShiftImageRegistryOverrides: overrides, - metadataGetter: failingGetter, + metadataGetter: failingGetter, } + provider.SetOpenShiftImageRegistryOverrides(overrides) // First call will attempt verification (which will fail via mock) // and cache the result @@ -1073,20 +1205,20 @@ func TestSeekOverrideWithCache(t *testing.T) { // Test with first mirror provider1 := &RegistryClientImageMetadataProvider{ - OpenShiftImageRegistryOverrides: map[string][]string{ - "quay.io": {"mirror1.example.com"}, - }, metadataGetter: failingGetter, } + provider1.SetOpenShiftImageRegistryOverrides(map[string][]string{ + "quay.io": {"mirror1.example.com"}, + }) provider1.seekOverride(ctx, parsedRef, []byte(`{"auths":{}}`)) // Test with second mirror provider2 := &RegistryClientImageMetadataProvider{ - OpenShiftImageRegistryOverrides: map[string][]string{ - "quay.io": {"mirror2.example.com"}, - }, metadataGetter: failingGetter, } + provider2.SetOpenShiftImageRegistryOverrides(map[string][]string{ + "quay.io": {"mirror2.example.com"}, + }) provider2.seekOverride(ctx, parsedRef, []byte(`{"auths":{}}`)) // Both mirrors should be cached separately @@ -1110,14 +1242,14 @@ func TestSeekOverrideFallsBackWhenMirrorUnavailable(t *testing.T) { ctx := context.Background() provider := &RegistryClientImageMetadataProvider{ - OpenShiftImageRegistryOverrides: map[string][]string{ - "quay.io": {"nonexistent-mirror.invalid"}, - }, // Mock metadataGetter that simulates an unavailable mirror, avoiding real network calls. metadataGetter: func(ctx context.Context, imageRef string, pullSecret []byte) (*dockerv1client.DockerImageConfig, []distribution.Descriptor, distribution.BlobStore, error) { return nil, nil, nil, fmt.Errorf("simulated mirror unavailable") }, } + provider.SetOpenShiftImageRegistryOverrides(map[string][]string{ + "quay.io": {"nonexistent-mirror.invalid"}, + }) parsedRef := reference.DockerImageReference{ Registry: "quay.io", diff --git a/support/releaseinfo/registry_image_content_policies.go b/support/releaseinfo/registry_image_content_policies.go index 09179285b7cd..7d569b13fc78 100644 --- a/support/releaseinfo/registry_image_content_policies.go +++ b/support/releaseinfo/registry_image_content_policies.go @@ -4,6 +4,7 @@ import ( "context" "strings" "sync" + "sync/atomic" "github.com/openshift/hypershift/support/releaseinfo/registryclient" "github.com/openshift/hypershift/support/thirdparty/library-go/pkg/image/reference" @@ -16,9 +17,10 @@ import ( var _ ProviderWithOpenShiftImageRegistryOverrides = (*ProviderWithOpenShiftImageRegistryOverridesDecorator)(nil) type ProviderWithOpenShiftImageRegistryOverridesDecorator struct { - Delegate ProviderWithRegistryOverrides - OpenShiftImageRegistryOverrides map[string][]string - mirroredReleaseImage string + Delegate ProviderWithRegistryOverrides + // overrides holds an immutable snapshot published by SetOpenShiftImageRegistryOverrides. + overrides atomic.Pointer[map[string][]string] + mirroredReleaseImage atomic.Pointer[string] // repoSetupFn is an injectable function for verifying mirror availability. // When nil, defaults to registryclient.GetRepoSetup. @@ -27,6 +29,20 @@ type ProviderWithOpenShiftImageRegistryOverridesDecorator struct { lock sync.Mutex } +// SetOpenShiftImageRegistryOverrides publishes the current image registry mirror snapshot. +func (p *ProviderWithOpenShiftImageRegistryOverridesDecorator) SetOpenShiftImageRegistryOverrides(overrides map[string][]string) { + snapshot := CopyOpenShiftImageRegistryOverrides(overrides) + p.overrides.Store(&snapshot) +} + +// loadOverrides returns an immutable snapshot; callers must not modify it. +func (p *ProviderWithOpenShiftImageRegistryOverridesDecorator) loadOverrides() map[string][]string { + if snapshot := p.overrides.Load(); snapshot != nil { + return *snapshot + } + return nil +} + func (p *ProviderWithOpenShiftImageRegistryOverridesDecorator) Lookup(ctx context.Context, image string, pullSecret []byte) (*ReleaseImage, error) { p.lock.Lock() defer p.lock.Unlock() @@ -36,9 +52,11 @@ func (p *ProviderWithOpenShiftImageRegistryOverridesDecorator) Lookup(ctx contex repoSetup = registryclient.GetRepoSetup } + overrides := p.loadOverrides() + logger := ctrl.LoggerFrom(ctx) - for registrySource, registryDest := range p.OpenShiftImageRegistryOverrides { + for registrySource, registryDest := range overrides { if strings.Contains(image, registrySource) { for _, registryReplacement := range registryDest { replacedImage := strings.Replace(image, registrySource, registryReplacement, 1) @@ -48,7 +66,7 @@ func (p *ProviderWithOpenShiftImageRegistryOverridesDecorator) Lookup(ctx contex if releaseImage != nil { // Verify mirror image availability. if _, _, err = repoSetup(ctx, replacedImage, pullSecret); err == nil { - p.mirroredReleaseImage = replacedImage + p.mirroredReleaseImage.Store(&replacedImage) return releaseImage, nil } logger.Info("WARNING: The current mirrors image is unavailable, continue Scanning multiple mirrors", "error", err.Error(), "mirror image", image) @@ -61,7 +79,7 @@ func (p *ProviderWithOpenShiftImageRegistryOverridesDecorator) Lookup(ctx contex } // Reset mirrored release image when falling back to original - p.mirroredReleaseImage = "" + p.mirroredReleaseImage.Store(nil) return p.Delegate.Lookup(ctx, image, pullSecret) } @@ -70,9 +88,31 @@ func (p *ProviderWithOpenShiftImageRegistryOverridesDecorator) GetRegistryOverri } func (p *ProviderWithOpenShiftImageRegistryOverridesDecorator) GetOpenShiftImageRegistryOverrides() map[string][]string { - return p.OpenShiftImageRegistryOverrides + return CopyOpenShiftImageRegistryOverrides(p.loadOverrides()) } func (p *ProviderWithOpenShiftImageRegistryOverridesDecorator) GetMirroredReleaseImage() string { - return p.mirroredReleaseImage + if image := p.mirroredReleaseImage.Load(); image != nil { + return *image + } + return "" +} + +// CopyOpenShiftImageRegistryOverrides copies the map and its mirror slices, preserving nil values. +func CopyOpenShiftImageRegistryOverrides(overrides map[string][]string) map[string][]string { + if overrides == nil { + return nil + } + + copyOfOverrides := make(map[string][]string, len(overrides)) + for source, mirrors := range overrides { + if mirrors == nil { + copyOfOverrides[source] = nil + continue + } + copyOfOverrides[source] = make([]string, len(mirrors)) + copy(copyOfOverrides[source], mirrors) + } + + return copyOfOverrides } diff --git a/support/releaseinfo/registry_image_content_policies_test.go b/support/releaseinfo/registry_image_content_policies_test.go index b8f8b423b110..873ded3ebd81 100644 --- a/support/releaseinfo/registry_image_content_policies_test.go +++ b/support/releaseinfo/registry_image_content_policies_test.go @@ -4,6 +4,7 @@ import ( "context" "sync" "testing" + "time" . "github.com/onsi/gomega" @@ -38,9 +39,6 @@ func TestProviderWithOpenShiftImageRegistryOverridesDecorator_Lookup(t *testing. } provider := &ProviderWithOpenShiftImageRegistryOverridesDecorator{ Delegate: delegate, - OpenShiftImageRegistryOverrides: map[string][]string{ - canonicalReleaseImage: {mirroredReleaseImage}, - }, // Mock repoSetupFn to avoid real network calls for mirror verification. repoSetupFn: func(ctx context.Context, imageRef string, pullSecret []byte) (distribution.Repository, *reference.DockerImageReference, error) { ref, err := reference.Parse(imageRef) @@ -51,6 +49,9 @@ func TestProviderWithOpenShiftImageRegistryOverridesDecorator_Lookup(t *testing. }, lock: sync.Mutex{}, } + provider.SetOpenShiftImageRegistryOverrides(map[string][]string{ + canonicalReleaseImage: {mirroredReleaseImage}, + }) pullSecret := []byte(`{"auths":{}}`) // Call the Lookup method and validate GetMirroredReleaseImage. @@ -83,12 +84,12 @@ func TestProviderWithOpenShiftImageRegistryOverridesDecorator_LookupWithNilRepoS // is assigned but never called, avoiding real network calls. provider := &ProviderWithOpenShiftImageRegistryOverridesDecorator{ Delegate: delegate, - OpenShiftImageRegistryOverrides: map[string][]string{ - "no-match-source": {"no-match-mirror"}, - }, // repoSetupFn intentionally nil to exercise the default fallback. lock: sync.Mutex{}, } + provider.SetOpenShiftImageRegistryOverrides(map[string][]string{ + "no-match-source": {"no-match-mirror"}, + }) pullSecret := []byte(`{"auths":{}}`) result, err := provider.Lookup(t.Context(), directImage, pullSecret) @@ -96,3 +97,185 @@ func TestProviderWithOpenShiftImageRegistryOverridesDecorator_LookupWithNilRepoS g.Expect(result).To(Equal(releaseImage)) g.Expect(provider.GetMirroredReleaseImage()).To(BeEmpty()) } + +func TestProviderWithOpenShiftImageRegistryOverridesDecorator_GetOpenShiftImageRegistryOverrides(t *testing.T) { + t.Run("When overrides are cleared, it should preserve nil and empty snapshots", func(t *testing.T) { + g := NewWithT(t) + provider := &ProviderWithOpenShiftImageRegistryOverridesDecorator{} + g.Expect(provider.GetOpenShiftImageRegistryOverrides()).To(BeNil()) + for _, overrides := range []map[string][]string{ + {"quay.io": {"mirror.example.com"}}, + nil, + {}, + {"quay.io": nil, "registry.example.com": {}}, + } { + provider.SetOpenShiftImageRegistryOverrides(overrides) + g.Expect(provider.GetOpenShiftImageRegistryOverrides()).To(Equal(overrides)) + } + }) + + t.Run("When the returned snapshot is modified, it should preserve the provider state", func(t *testing.T) { + g := NewWithT(t) + provider := &ProviderWithOpenShiftImageRegistryOverridesDecorator{} + input := map[string][]string{ + "quay.io": {"mirror-a.example.com", "mirror-b.example.com"}, + } + provider.SetOpenShiftImageRegistryOverrides(input) + + input["quay.io"][0] = "mutated-input.example.com" + input["new-source.example.com"] = []string{"new-mirror.example.com"} + + overrides := provider.GetOpenShiftImageRegistryOverrides() + overrides["quay.io"][0] = "mutated.example.com" + delete(overrides, "quay.io") + + g.Expect(provider.GetOpenShiftImageRegistryOverrides()).To(Equal(map[string][]string{ + "quay.io": {"mirror-a.example.com", "mirror-b.example.com"}, + })) + }) +} + +func TestProviderWithOpenShiftImageRegistryOverridesDecorator_GetMirroredReleaseImage(t *testing.T) { + t.Run("When fallback fails, it should clear the mirrored image before delegation during concurrent reads", func(t *testing.T) { + g := NewWithT(t) + var provider *ProviderWithOpenShiftImageRegistryOverridesDecorator + provider = &ProviderWithOpenShiftImageRegistryOverridesDecorator{ + Delegate: &RegistryMirrorProviderDecorator{ + Delegate: &fakeProvider{lookupFn: func(_ context.Context, image string, _ []byte) (*ReleaseImage, error) { + if image == "quay.io/release:latest" { + g.Expect(provider.GetMirroredReleaseImage()).To(BeEmpty()) + return nil, context.Canceled + } + return &ReleaseImage{ImageStream: &imagev1.ImageStream{}, StreamMetadata: &stream.Stream{}}, nil + }}, + }, + repoSetupFn: func(context.Context, string, []byte) (distribution.Repository, *reference.DockerImageReference, error) { + return nil, nil, nil + }, + } + g.Expect(provider.GetMirroredReleaseImage()).To(BeEmpty()) + stop := make(chan struct{}) + var readers sync.WaitGroup + readers.Add(1) + go func() { + defer readers.Done() + for { + select { + case <-stop: + return + default: + provider.GetMirroredReleaseImage() + } + } + }() + defer func() { + close(stop) + readers.Wait() + }() + for range 100 { + provider.SetOpenShiftImageRegistryOverrides(map[string][]string{"quay.io": {"mirror.example.com"}}) + _, err := provider.Lookup(t.Context(), "quay.io/release:latest", nil) + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(provider.GetMirroredReleaseImage()).To(Equal("mirror.example.com/release:latest")) + provider.SetOpenShiftImageRegistryOverrides(nil) + _, err = provider.Lookup(t.Context(), "quay.io/release:latest", nil) + g.Expect(err).To(MatchError(context.Canceled)) + g.Expect(provider.GetMirroredReleaseImage()).To(BeEmpty()) + } + }) +} + +func TestProviderWithOpenShiftImageRegistryOverridesDecorator_ConcurrentOverrides(t *testing.T) { + t.Run("When mirror overrides are refreshed during lookup, it should use synchronized snapshots", func(t *testing.T) { + const waitTimeout = time.Second + canonicalReleaseImage := "canonical-release-image" + unavailableMirrorImage := "unavailable.example.com/release:latest" + oldMirrorImage := "mirror-a.example.com/release:latest" + newMirrorImage := "mirror-b.example.com/release:latest" + releaseImage := &ReleaseImage{ + ImageStream: &imagev1.ImageStream{}, + StreamMetadata: &stream.Stream{}, + } + requestedImages := make([]string, 0, 3) + lookupStarted := make(chan struct{}, 1) + releaseLookup := make(chan struct{}) + lookupDone := make(chan struct{}) + lookupErr := make(chan error, 1) + releaseLookupClosed := false + defer func() { + if !releaseLookupClosed { + close(releaseLookup) + } + }() + + provider := &ProviderWithOpenShiftImageRegistryOverridesDecorator{ + Delegate: &RegistryMirrorProviderDecorator{ + Delegate: &fakeProvider{ + lookupFn: func(_ context.Context, image string, _ []byte) (*ReleaseImage, error) { + requestedImages = append(requestedImages, image) + if image == unavailableMirrorImage { + lookupStarted <- struct{}{} + <-releaseLookup + return nil, context.Canceled + } + return releaseImage, nil + }, + }, + RegistryOverrides: map[string]string{}, + }, + repoSetupFn: func(ctx context.Context, imageRef string, pullSecret []byte) (distribution.Repository, *reference.DockerImageReference, error) { + ref, err := reference.Parse(imageRef) + if err != nil { + return nil, nil, err + } + return nil, &ref, nil + }, + } + provider.SetOpenShiftImageRegistryOverrides(map[string][]string{ + canonicalReleaseImage: {unavailableMirrorImage, oldMirrorImage}, + }) + + go func() { + defer close(lookupDone) + _, err := provider.Lookup(t.Context(), canonicalReleaseImage, []byte(`{"auths":{}}`)) + lookupErr <- err + }() + + select { + case <-lookupStarted: + case <-time.After(waitTimeout): + t.Fatal("timed out waiting for the delegate lookup to block") + } + + setDone := make(chan struct{}) + go func() { + provider.SetOpenShiftImageRegistryOverrides(map[string][]string{ + canonicalReleaseImage: {newMirrorImage}, + }) + close(setDone) + }() + + select { + case <-setDone: + case <-time.After(waitTimeout): + t.Fatal("override publication waited for the blocked delegate lookup") + } + + close(releaseLookup) + releaseLookupClosed = true + select { + case <-lookupDone: + case <-time.After(waitTimeout): + t.Fatal("timed out waiting for the first lookup to finish") + } + + g := NewWithT(t) + g.Expect(<-lookupErr).ToNot(HaveOccurred()) + g.Expect(requestedImages).To(Equal([]string{unavailableMirrorImage, oldMirrorImage})) + + _, err := provider.Lookup(t.Context(), canonicalReleaseImage, []byte(`{"auths":{}}`)) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(requestedImages).To(Equal([]string{unavailableMirrorImage, oldMirrorImage, newMirrorImage})) + g.Expect(provider.GetMirroredReleaseImage()).To(Equal(newMirrorImage)) + }) +}