Skip to content

OCPBUGS-53265: Use the image reference for the cache in the image metadata provider #6155

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
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

OCPBUGS-53265: Use the image reference for the cache in the image metadata provider #6155

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
107 changes: 27 additions & 80 deletions support/util/imagemetadata.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,52 +42,26 @@ type RegistryClientImageMetadataProvider struct {
OpenShiftImageRegistryOverrides map[string][]string
}

// ImageMetadata returns metadata 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 image metadata in the
// cache. When the image reference does not contain a digest, a lookup is made to the registry to
// fetch the digest of the image that the tag refers to. This is because the actual image that the
// tag is referring to could have changed. Once a digest is obtained, the cache is checked so that
// no further fetching occurs. Only if both cache lookups fail, the image metadata is fetched and
// stored in the cache.
// ImageMetadata returns metadata for a given image using the given pull secret to authenticate.
// The ICSPs/IDMSs are checked first for overrides and then the cache is checked using the image
// pull spec. If not found in the cache, the manifest is looked up and added to the cache.
func (r *RegistryClientImageMetadataProvider) ImageMetadata(ctx context.Context, imageRef string, pullSecret []byte) (*dockerv1client.DockerImageConfig, error) {

var (
repo distribution.Repository
ref *reference.DockerImageReference
parsedImageRef reference.DockerImageReference
err error
)

parsedImageRef, err = reference.Parse(imageRef)
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 {
// If the image reference contains a digest, immediately look it up in the cache
if parsedImageRef.ID != "" {
if imageConfigObject, exists := imageMetadataCache.Get(parsedImageRef.ID); exists {
return imageConfigObject.(*dockerv1client.DockerImageConfig), nil
}
}
ref = &parsedImageRef
}

// Get the image repo info based the source/mirrors in the ICSPs/IDMSs
ref = seekOverride(ctx, r.OpenShiftImageRegistryOverrides, parsedImageRef)
ref := seekOverride(ctx, r.OpenShiftImageRegistryOverrides, parsedImageRef)
refPullSpec := ref.String()

// If the image reference contains a digest, immediately look it up in the cache
if ref.ID != "" {
if imageConfigObject, exists := imageMetadataCache.Get(ref.ID); exists {
return imageConfigObject.(*dockerv1client.DockerImageConfig), nil
}
// Check the cache for the image
if imageConfigObject, exists := imageMetadataCache.Get(refPullSpec); exists {
return imageConfigObject.(*dockerv1client.DockerImageConfig), nil
}

repo, err = getRepository(ctx, *ref, pullSecret)
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)
}
Expand All @@ -98,18 +72,13 @@ func (r *RegistryClientImageMetadataProvider) ImageMetadata(ctx context.Context,
return nil, fmt.Errorf("failed to obtain root manifest for %s: %w", imageRef, err)
}

// If the image ref did not contain a digest, attempt looking it up by digest after we've fetched the digest
if ref.ID == "" {
if imageConfigObject, exists := imageMetadataCache.Get(string(location.Manifest)); exists {
return imageConfigObject.(*dockerv1client.DockerImageConfig), nil
}
}

config, _, err := manifest.ManifestToImageConfig(ctx, firstManifest, repo.Blobs(ctx), location)
if err != nil {
return nil, fmt.Errorf("failed to obtain image configuration for %s: %w", imageRef, err)
}
imageMetadataCache.Add(string(location.Manifest), config)

// Cache the image config using the image reference pull spec
imageMetadataCache.Add(refPullSpec, config)

return config, nil
}
Expand Down Expand Up @@ -197,56 +166,34 @@ func (r *RegistryClientImageMetadataProvider) GetDigest(ctx context.Context, ima
return srcDigest, composedParsedRef, 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.
// GetManifest returns the manifest for a given image using the given pull secret to authenticate.
// The ICSPs/IDMSs are checked first for overrides and then the cache is checked using the image
// pull spec. If not found in the cache, the manifest is looked up and added to the cache.
func (r *RegistryClientImageMetadataProvider) GetManifest(ctx context.Context, imageRef string, pullSecret []byte) (distribution.Manifest, error) {

var (
ref *reference.DockerImageReference
parsedImageRef reference.DockerImageReference
err error
srcDigest digest.Digest
)

parsedImageRef, err = reference.Parse(imageRef)
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 {
// If the image reference contains a digest, immediately look it up in the cache
if parsedImageRef.ID != "" {
if manifest, exists := manifestsCache.Get(parsedImageRef.ID); exists {
return manifest.(distribution.Manifest), nil
}
}
ref = &parsedImageRef
}

// Get the image repo info based the source/mirrors in the ICSPs/IDMSs
ref = seekOverride(ctx, r.OpenShiftImageRegistryOverrides, parsedImageRef)
ref := seekOverride(ctx, r.OpenShiftImageRegistryOverrides, parsedImageRef)

// If the image reference contains a digest, immediately look it up in the cache
if ref.ID != "" {
if manifest, exists := manifestsCache.Get(ref.ID); exists {
return manifest.(distribution.Manifest), nil
}
// Check the cache for the image
if manifest, exists := manifestsCache.Get(ref.String()); exists {
return manifest.(distribution.Manifest), nil
}

composedRef := ref.String()

digestsManifest, srcDigest, err := getManifest(ctx, composedRef, pullSecret)
// Look up the manifest
manifest, _, err := getManifest(ctx, ref.String(), pullSecret)
if err != nil {
return nil, err
}
manifestsCache.Add(srcDigest, digestsManifest)

return digestsManifest, nil
// Cache the mainfest using the image reference pull spec
manifestsCache.Add(ref.String(), manifest)

return manifest, nil
}

func (r *RegistryClientImageMetadataProvider) GetMetadata(ctx context.Context, imageRef string, pullSecret []byte) (*dockerv1client.DockerImageConfig, []distribution.Descriptor, distribution.BlobStore, error) {
Expand Down
44 changes: 25 additions & 19 deletions support/util/imagemetadata_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,12 +92,11 @@ func TestGetManifest(t *testing.T) {
pullSecret := []byte("{}")

testsCases := []struct {
name string
imageRef string
pullSecret []byte
expectedErr bool
validateCache bool
expectedDigest digest.Digest
name string
imageRef string
pullSecret []byte
expectedErr bool
validateCache bool
}{
{
name: "if failed to parse image reference",
Expand All @@ -112,20 +111,25 @@ func TestGetManifest(t *testing.T) {
expectedErr: false,
},
{
name: "Pull x86 manifest from cache",
imageRef: "quay.io/openshift-release-dev/ocp-release:4.16.12-x86_64",
pullSecret: pullSecret,
expectedErr: false,
validateCache: true,
expectedDigest: "sha256:2a50e5d5267916078145731db740bbc85ee764e1a194715fd986ab5bf9a3414e",
name: "Pull x86 manifest from cache",
imageRef: "quay.io/openshift-release-dev/ocp-release:4.16.12-x86_64",
pullSecret: pullSecret,
expectedErr: false,
validateCache: true,
},
{
name: "Pull Multiarch manifest",
imageRef: "quay.io/openshift-release-dev/ocp-release:4.16.12-multi",
pullSecret: pullSecret,
expectedErr: false,
validateCache: true,
expectedDigest: "sha256:727276732f03d8d5a2374efa3d01fb0ed9f65b32488b862e9a9d2ff4cde89ff6",
name: "Pull Multiarch manifest",
imageRef: "quay.io/openshift-release-dev/ocp-release:4.16.12-multi",
pullSecret: pullSecret,
expectedErr: false,
validateCache: true,
},
{
name: "Pull Multiarch manifest with Shah",
imageRef: "quay.io/openshift-release-dev/ocp-release@sha256:be8bcea2ab176321a4e1e54caab4709f9024bc437e52ca5bc088e729367cd0cf",
pullSecret: pullSecret,
expectedErr: false,
validateCache: true,
},
}

Expand All @@ -146,7 +150,9 @@ func TestGetManifest(t *testing.T) {
}

if tc.validateCache {
_, exists := manifestsCache.Get(tc.expectedDigest)
parsedImageRef, err := reference.Parse(tc.imageRef)
g.Expect(err).NotTo(HaveOccurred())
_, exists := manifestsCache.Get(parsedImageRef.String())
g.Expect(exists).To(BeTrue())
}
})
Expand Down
16 changes: 8 additions & 8 deletions support/util/util_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -564,6 +564,14 @@ func TestGetImageArchitecture(t *testing.T) {
expectedArch hyperv1.PayloadArchType
expectErr bool
}{
{
name: "Bad pull secret, cache empty; err",
image: "quay.io/openshift-release-dev/ocp-release:4.16.11-ppc64le",
pullSecretBytes: []byte(""),
imageMetadataProvider: &RegistryClientImageMetadataProvider{},
expectedArch: "",
expectErr: true,
},
{
name: "Get amd64 from amd64 image; no err",
image: "quay.io/openshift-release-dev/ocp-release:4.16.10-x86_64",
Expand All @@ -580,14 +588,6 @@ func TestGetImageArchitecture(t *testing.T) {
expectedArch: hyperv1.PPC64LE,
expectErr: false,
},
{
name: "Bad pull secret; err",
image: "quay.io/openshift-release-dev/ocp-release:4.16.11-ppc64le",
pullSecretBytes: []byte(""),
imageMetadataProvider: &RegistryClientImageMetadataProvider{},
expectedArch: "",
expectErr: true,
},
}

for _, tc := range testCases {
Expand Down