Skip to content

fix: prevent registry mirror map races #9837

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
chdeshpa-hue wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: prevent registry mirror map races #9837

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions control-plane-operator/hostedclusterconfigoperator/cmd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
9 changes: 4 additions & 5 deletions control-plane-operator/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -304,17 +304,17 @@ 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{
Delegate: &releaseinfo.RegistryMirrorProviderDecorator{
Delegate: coreReleaseProvider,
RegistryOverrides: registryOverrides,
},
OpenShiftImageRegistryOverrides: imageRegistryOverrides,
}
cpReleaseProvider.SetOpenShiftImageRegistryOverrides(imageRegistryOverrides)

return cpReleaseProvider, userReleaseProvider
}
Expand Down Expand Up @@ -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)

Expand Down
5 changes: 2 additions & 3 deletions ignition-server/cmd/run_local_ignitionprovider.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
25 changes: 13 additions & 12 deletions ignition-server/cmd/start.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
7 changes: 3 additions & 4 deletions karpenter-operator/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
11 changes: 5 additions & 6 deletions support/globalconfig/imagecontentsource.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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
}
110 changes: 64 additions & 46 deletions support/globalconfig/imagecontentsource_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)
Expand All @@ -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
Expand All @@ -301,10 +280,6 @@ func TestReconcileMgmtImageRegistryOverrides(t *testing.T) {
"registry2": "override2",
},
},
OpenShiftImageRegistryOverrides: nil,
},
expectedMetadata: &imageregistry.RegistryClientImageMetadataProvider{
OpenShiftImageRegistryOverrides: nil,
},
expectedError: nil,
},
Expand Down Expand Up @@ -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))
})
}
39 changes: 33 additions & 6 deletions support/imageregistry/imagemetadata.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"fmt"
"net/http"
"sync"
"sync/atomic"
"time"

hyperv1 "github.com/openshift/hypershift/api/hypershift/v1beta1"
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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))
Expand All @@ -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
Expand Down Expand Up @@ -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)
}

Expand All @@ -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)
Expand Down Expand Up @@ -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
Expand Down
Loading