Skip to content

MCO-1956: Extract image utils from build package #5400

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

Merged
merged 1 commit into from
Nov 19, 2025
+324 −209
Merged

MCO-1956: Extract image utils from build package #5400

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
52 changes: 11 additions & 41 deletions pkg/controller/build/imagepruner/imageinspect.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,13 @@ package imagepruner
import (
"context"
"fmt"
"strings"

"github.com/containers/common/pkg/retry"
"github.com/containers/image/v5/docker"
"github.com/containers/image/v5/image"
"github.com/containers/image/v5/manifest"
"github.com/containers/image/v5/types"
digest "github.com/opencontainers/go-digest"
"github.com/openshift/machine-config-operator/pkg/imageutils"
)

const (
Expand Down Expand Up @@ -58,24 +57,6 @@ func (i *imageInspectorImpl) DeleteImage(ctx context.Context, sysCtx *types.Syst
return deleteImage(ctx, sysCtx, image)
}

// parseImageName parses the image name string into an ImageReference,
// handling various prefix formats like "docker://" and ensuring a standard format.
func parseImageName(imgName string) (types.ImageReference, error) {
if strings.Contains(imgName, "//") && !strings.HasPrefix(imgName, "docker://") {
return nil, fmt.Errorf("unknown transport for pullspec %s", imgName)
}

if strings.HasPrefix(imgName, "docker://") {
imgName = strings.ReplaceAll(imgName, "docker://", "//")
}

if !strings.HasPrefix(imgName, "//") {
imgName = "//" + imgName
}

return docker.Transport.ParseReference(imgName)
}

// deleteImage attempts to delete the specified image with retries,
// using the provided context and system context.
//
Expand All @@ -85,7 +66,7 @@ func deleteImage(ctx context.Context, sysCtx *types.SystemContext, imageName str
MaxRetry: cmdRetriesCount,
}

ref, err := parseImageName(imageName)
ref, err := imageutils.ParseImageName(imageName)
if err != nil {
return err
}
Expand All @@ -105,29 +86,19 @@ func deleteImage(ctx context.Context, sysCtx *types.SystemContext, imageName str
//
//nolint:unparam
func imageInspect(ctx context.Context, sysCtx *types.SystemContext, imageName string) (*types.ImageInspectInfo, *digest.Digest, error) {
var (
src types.ImageSource
imgInspect *types.ImageInspectInfo
err error
)
ref, err := imageutils.ParseImageName(imageName)
if err != nil {
return nil, nil, fmt.Errorf("error parsing image name %q: %w", imageName, err)
}

retryOpts := retry.RetryOptions{
MaxRetry: cmdRetriesCount,
}

ref, err := parseImageName(imageName)
src, err := imageutils.GetImageSourceFromReference(ctx, sysCtx, ref, &retryOpts)
if err != nil {
return nil, nil, fmt.Errorf("error parsing image name %q: %w", imageName, err)
}

// retry.IfNecessary takes into account whether the error is "retryable"
// so we don't keep looping on errors that will never resolve
if err := retry.RetryIfNecessary(ctx, func() error {
src, err = ref.NewImageSource(ctx, sysCtx)
return err
}, &retryOpts); err != nil {
return nil, nil, newErrImage(imageName, fmt.Errorf("error getting image source: %w", err))
return nil, nil, fmt.Errorf("error getting image source for %s: %w", imageName, err)
}
defer src.Close()

var rawManifest []byte
unparsedInstance := image.UnparsedInstance(src, nil)
Expand All @@ -144,14 +115,13 @@ func imageInspect(ctx context.Context, sysCtx *types.SystemContext, imageName st
return nil, nil, fmt.Errorf("error retrieving image digest: %q: %w", imageName, err)
}

defer src.Close()
Copy link
Contributor Author

@pablintino pablintino Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved this line a bit earlier as I think that the previous error conditions may make the function return without proper cleanup.


img, err := image.FromUnparsedImage(ctx, sysCtx, unparsedInstance)
if err != nil {
return nil, nil, newErrImage(imageName, fmt.Errorf("error parsing manifest for image: %w", err))
}

if err := retry.RetryIfNecessary(ctx, func() error {
var imgInspect *types.ImageInspectInfo
if err := retry.IfNecessary(ctx, func() error {
Copy link
Contributor Author

@pablintino pablintino Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to IfNecessary as I saw we were already using it and RetryIfNecessary is deprecated.

Copy link
Member

@isabella-janssen isabella-janssen Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! 💯 I think there are a few things we use in our code base that are deprecated. Maybe a good task for AI would be getting it to find what's deprecated and suggest solutions. 🤔 Maybe I'll try that out once the release freeze is behind us.

Copy link
Contributor Author

@pablintino pablintino Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I catched it myself this time, but Claude has already told me about deprecated code. When I was coding the mco-sanitize Claude told me the GPG packet was about to be discontinued.
If you have the time... Everybody will welcome your effort :)

imgInspect, err = img.Inspect(ctx)
return err
}, &retryOpts); err != nil {
Expand Down
156 changes: 7 additions & 149 deletions pkg/controller/build/imagepruner/imagepruner.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,18 +3,14 @@ package imagepruner
import (
"context"
"fmt"
"os"
"path/filepath"
"strings"

"github.com/openshift/machine-config-operator/pkg/imageutils"
corev1 "k8s.io/api/core/v1"
"k8s.io/klog/v2"

"github.com/containers/image/v5/types"
"github.com/opencontainers/go-digest"
mcfgv1 "github.com/openshift/api/machineconfiguration/v1"
"github.com/openshift/machine-config-operator/pkg/controller/template"
"github.com/openshift/machine-config-operator/pkg/secrets"
)

// ImagePruner defines the interface for inspecting and deleting container images.
Expand Down Expand Up @@ -42,18 +38,18 @@ func NewImagePruner() ImagePruner {
// InspectImage inspects the given image using the provided secret. It also accepts a
// ControllerConfig so that certificates may be placed on the filesystem for authentication.
func (i *imagePrunerImpl) InspectImage(ctx context.Context, pullspec string, secret *corev1.Secret, cc *mcfgv1.ControllerConfig) (*types.ImageInspectInfo, *digest.Digest, error) {
sysCtx, err := i.prepareSystemContext(secret, cc)
sysCtx, err := imageutils.NewSysContextFromControllerConfig(secret, cc)
if err != nil {
return nil, nil, fmt.Errorf("could not prepare for image inspection: %w", err)
}

defer func() {
if err := i.cleanup(sysCtx); err != nil {
if err := sysCtx.Cleanup(); err != nil {
klog.Warningf("Unable to clean up after inspection of %s: %s", pullspec, err)
}
}()

info, digest, err := i.images.ImageInspect(ctx, sysCtx, pullspec)
info, digest, err := i.images.ImageInspect(ctx, sysCtx.SysContext, pullspec)
if err != nil {
return nil, nil, fmt.Errorf("could not inspect image: %w", err)
}
Expand All @@ -64,158 +60,20 @@ func (i *imagePrunerImpl) InspectImage(ctx context.Context, pullspec string, sec
// DeleteImage deletes the given image using the provided secret. It also accepts a
// ControllerConfig so that certificates may be placed on the filesystem for authentication.
func (i *imagePrunerImpl) DeleteImage(ctx context.Context, pullspec string, secret *corev1.Secret, cc *mcfgv1.ControllerConfig) error {
sysCtx, err := i.prepareSystemContext(secret, cc)
sysCtx, err := imageutils.NewSysContextFromControllerConfig(secret, cc)
if err != nil {
return fmt.Errorf("could not prepare for image deletion: %w", err)
}

defer func() {
if err := i.cleanup(sysCtx); err != nil {
if err := sysCtx.Cleanup(); err != nil {
klog.Warningf("Unable to clean up after deletion of %s: %s", pullspec, err)
}
}()

if err := i.images.DeleteImage(ctx, sysCtx, pullspec); err != nil {
if err := i.images.DeleteImage(ctx, sysCtx.SysContext, pullspec); err != nil {
return fmt.Errorf("could not delete image: %w", err)
}

return nil
}

// prepareSystemContext prepares to perform the requested operation by first creating the
// certificate directory and then writing the authfile to the appropriate path.
func (i *imagePrunerImpl) prepareSystemContext(secret *corev1.Secret, cc *mcfgv1.ControllerConfig) (*types.SystemContext, error) {
// Make a deep copy of the ControllerConfig because the write process mutates
// the ControllerConfig in-place.
certsDir, err := i.prepareCerts(cc.DeepCopy())
if err != nil {
return nil, fmt.Errorf("could not prepare certs: %w", err)
}

authfilePath, err := i.prepareAuthfile(secret)
if err != nil {
return nil, fmt.Errorf("could not get authfile path for secret %s: %w", secret.Name, err)
}

return &types.SystemContext{
AuthFilePath: authfilePath,
DockerPerHostCertDirPath: certsDir,
}, nil
}

// cleanup cleans up after an operation by removing the temporary certificates directory
// and the temporary authfile.
func (i *imagePrunerImpl) cleanup(sysCtx *types.SystemContext) error {
if err := os.RemoveAll(sysCtx.DockerPerHostCertDirPath); err != nil && !os.IsNotExist(err) {
return fmt.Errorf("could not clean up certs directory %s: %w", sysCtx.DockerPerHostCertDirPath, err)
}

if err := os.RemoveAll(sysCtx.AuthFilePath); err != nil && !os.IsNotExist(err) {
return fmt.Errorf("could not clean up authfile directory %s: %w", sysCtx.AuthFilePath, err)
}

return nil
}

// prepareCerts prepares the certificates by first creating a temporary directory for them
// and then writing the certs from the ControllerConfig to that directory.
func (i *imagePrunerImpl) prepareCerts(cc *mcfgv1.ControllerConfig) (string, error) {
certsDir, err := os.MkdirTemp("", "imagepruner-certs-dir")
if err != nil {
return "", fmt.Errorf("could not create temp dir: %w", err)
}

if err := i.writeCerts(certsDir, cc); err != nil {
return "", fmt.Errorf("could not write certs: %w", err)
}

return certsDir, nil
}

// writeCerts extracts the certificates from the ControllerConfig and writes them
// to the appropriate directory, which defaults to /etc/docker/certs.d.
func (i *imagePrunerImpl) writeCerts(certsDir string, cc *mcfgv1.ControllerConfig) error {
template.UpdateControllerConfigCerts(cc)

for _, irb := range cc.Spec.ImageRegistryBundleData {
if err := i.writeCertFromImageRegistryBundle(certsDir, irb); err != nil {
return fmt.Errorf("could not write image registry bundle from ImageRegistryBundleData: %w", err)
}
}

for _, irb := range cc.Spec.ImageRegistryBundleUserData {
if err := i.writeCertFromImageRegistryBundle(certsDir, irb); err != nil {
return fmt.Errorf("could not write image registry bundle from ImageRegistryBundleUserData: %w", err)
}
}

return nil
}

// writeCertFromImageRegistryBundle writes a certificate from an image registry bundle
// to the specified certificates directory, creating necessary subdirectories.
func (i *imagePrunerImpl) writeCertFromImageRegistryBundle(certsDir string, irb mcfgv1.ImageRegistryBundle) error {
caFile := strings.ReplaceAll(irb.File, "..", ":")

certDir := filepath.Join(certsDir, caFile)

if err := os.MkdirAll(certDir, 0o755); err != nil {
return fmt.Errorf("could not create cert dir %q: %w", certDir, err)
}

certFile := filepath.Join(certDir, "ca.crt")

if err := os.WriteFile(certFile, irb.Data, 0o644); err != nil {
return fmt.Errorf("could not write cert file %q: %w", certFile, err)
}

return nil
}

// prepareAuthfile creates a temporary directory and writes the Docker secret
// (authfile) into a file named "authfile.json" within that directory.
// It returns the path to the created authfile.
func (i *imagePrunerImpl) prepareAuthfile(secret *corev1.Secret) (string, error) {
authfileDir, err := os.MkdirTemp("", "imagepruner-authfile")
if err != nil {
return "", fmt.Errorf("could not create temp dir for authfile: %w", err)
}

authfilePath := filepath.Join(authfileDir, "authfile.json")

is, err := secrets.NewImageRegistrySecret(secret)
if err != nil {
return "", fmt.Errorf("could not create an ImageRegistrySecret for '%s/%s': %w", secret.Namespace, secret.Name, err)
}

secretBytes, err := is.JSONBytes(corev1.SecretTypeDockerConfigJson)
if err != nil {
return "", fmt.Errorf("could not normalize secret '%s/%s' to %s: %w", secret.Namespace, secret.Name, corev1.SecretTypeDockerConfigJson, err)
}

if err := os.WriteFile(authfilePath, secretBytes, 0o644); err != nil {
return "", fmt.Errorf("could not write temp authfile %q for secret %q: %w", authfilePath, secret.Name, err)
}

return authfilePath, nil
}

// writeAuthfile ensures that the image registry secret is in the dockerconfigjson format
// and writes it to the specified path.
func (i *imagePrunerImpl) writeAuthfile(secret *corev1.Secret, authfilePath string) error {
is, err := secrets.NewImageRegistrySecret(secret)
if err != nil {
return fmt.Errorf("could not create an ImageRegistrySecret for '%s/%s': %w", secret.Namespace, secret.Name, err)
}

secretBytes, err := is.JSONBytes(corev1.SecretTypeDockerConfigJson)
if err != nil {
return fmt.Errorf("could not normalize secret '%s/%s' to %s: %w", secret.Namespace, secret.Name, corev1.SecretTypeDockerConfigJson, err)
}

if err := os.WriteFile(authfilePath, secretBytes, 0o644); err != nil {
return fmt.Errorf("could not write temp authfile %q for secret %q: %w", authfilePath, secret.Name, err)
}

return nil
}
Loading