OCPBUGS-65845: fix stale image digest race condition

Change the default pull policy from PullIfMissing to PullIfNewer in
buildDaemonlessImage and extractSourceFromImage. With PullIfMissing, if
an image was already pulled (e.g. during a pre-pull step), a newer
version pushed to the registry would not be detected. PullIfNewer adds a
lightweight registry manifest check that catches cases where a tag was
updated between Build object creation and the actual pull. For
digest-pinned references this is a no-op.

Also fix DaemonlessClient.PullImage to use '@' instead of ':' when the
tag field contains a digest (e.g. "sha256:..."), preventing incorrect
image name reconstruction.

Author: joepvd

pkg/build/builder/daemonless.go

@@ -255,7 +255,7 @@ func buildDaemonlessImage(sc types.SystemContext, store storage.Store, isolation
 		args[ev.Name] = ev.Value
 	}
 
-	pullPolicy := buildah.PullIfMissing
+	pullPolicy := buildah.PullIfNewer
 	if opts.Pull {
 		log.V(2).Infof("Forcing fresh pull of base image.")
 		pullPolicy = buildah.PullAlways
@@ -838,7 +838,11 @@ func (d *DaemonlessClient) RemoveContainer(opts docker.RemoveContainerOptions) e
 func (d *DaemonlessClient) PullImage(opts docker.PullImageOptions, searchPaths []string) error {
 	imageName := opts.Repository
 	if opts.Tag != "" {
-		imageName = imageName + ":" + opts.Tag
+		if strings.Contains(opts.Tag, ":") {
+			imageName = imageName + "@" + opts.Tag
+		} else {
+			imageName = imageName + ":" + opts.Tag
+		}
 	}
 	return pullDaemonlessImage(d.SystemContext, d.Store, imageName, searchPaths, d.BlobCacheDirectory)
 }

pkg/build/builder/daemonless_test.go

@@ -15,6 +15,65 @@ import (
 	builderutil "github.com/openshift/builder/pkg/build/builder/util"
 )
 
+func TestPullImageNameReconstruction(t *testing.T) {
+	tests := []struct {
+		name string
+		repo string
+		tag  string
+		want string
+	}{
+		{
+			name: "tag reference",
+			repo: "registry:5000/ns/image",
+			tag:  "latest",
+			want: "registry:5000/ns/image:latest",
+		},
+		{
+			name: "digest reference passed as tag",
+			repo: "registry:5000/ns/image",
+			tag:  "sha256:abc123def456",
+			want: "registry:5000/ns/image@sha256:abc123def456",
+		},
+		{
+			name: "digest reference in repository (normal flow)",
+			repo: "registry:5000/ns/image@sha256:abc123def456",
+			tag:  "",
+			want: "registry:5000/ns/image@sha256:abc123def456",
+		},
+		{
+			name: "simple image with tag",
+			repo: "busybox",
+			tag:  "latest",
+			want: "busybox:latest",
+		},
+		{
+			name: "image without tag",
+			repo: "busybox",
+			tag:  "",
+			want: "busybox",
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			opts := docker.PullImageOptions{
+				Repository: tt.repo,
+				Tag:        tt.tag,
+			}
+			imageName := opts.Repository
+			if opts.Tag != "" {
+				if strings.Contains(opts.Tag, ":") {
+					imageName = imageName + "@" + opts.Tag
+				} else {
+					imageName = imageName + ":" + opts.Tag
+				}
+			}
+			if imageName != tt.want {
+				t.Errorf("reconstructed image name = %q, want %q", imageName, tt.want)
+			}
+		})
+	}
+}
+
 func TestMergeNodeCredentials(t *testing.T) {
 	for _, tt := range []struct {
 		name      string

pkg/build/builder/docker_test.go

@@ -596,6 +596,79 @@ USER 1001`
 	}
 }
 
+func TestPullImageDigestReference(t *testing.T) {
+	tests := []struct {
+		name     string
+		input    string
+		wantRepo string
+		wantTag  string
+	}{
+		{
+			name:     "tag reference",
+			input:    "registry:5000/ns/image:latest",
+			wantRepo: "registry:5000/ns/image",
+			wantTag:  "latest",
+		},
+		{
+			name:     "digest reference preserves full name in repository",
+			input:    "registry:5000/ns/image@sha256:abc123def456",
+			wantRepo: "registry:5000/ns/image@sha256:abc123def456",
+			wantTag:  "",
+		},
+		{
+			name:     "internal registry digest reference",
+			input:    "image-registry.openshift-image-registry.svc:5000/ci-op-xxx/pipeline@sha256:aabbccdd",
+			wantRepo: "image-registry.openshift-image-registry.svc:5000/ci-op-xxx/pipeline@sha256:aabbccdd",
+			wantTag:  "",
+		},
+		{
+			name:     "simple tag",
+			input:    "myimage:v1",
+			wantRepo: "myimage",
+			wantTag:  "v1",
+		},
+		{
+			name:     "no tag",
+			input:    "myimage",
+			wantRepo: "myimage",
+			wantTag:  "",
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			var gotOpts docker.PullImageOptions
+			dockerClient := &FakeDocker{
+				pullImageFunc: func(opts docker.PullImageOptions, searchPaths []string) error {
+					gotOpts = opts
+					return nil
+				},
+			}
+			d := &DockerBuilder{
+				dockerClient: dockerClient,
+				build: &buildapiv1.Build{
+					Spec: buildapiv1.BuildSpec{
+						CommonSpec: buildapiv1.CommonSpec{
+							Strategy: buildapiv1.BuildStrategy{
+								DockerStrategy: &buildapiv1.DockerBuildStrategy{},
+							},
+						},
+					},
+				},
+			}
+			err := d.pullImage(tt.input, nil)
+			if err != nil {
+				t.Fatalf("unexpected error: %v", err)
+			}
+			if gotOpts.Repository != tt.wantRepo {
+				t.Errorf("Repository = %q, want %q", gotOpts.Repository, tt.wantRepo)
+			}
+			if gotOpts.Tag != tt.wantTag {
+				t.Errorf("Tag = %q, want %q", gotOpts.Tag, tt.wantTag)
+			}
+		})
+	}
+}
+
 // TestCopyLocalObject verifies that we are able to copy mounted Kubernetes Secret or ConfigMap
 // data to the build directory. The build directory is typically where git source code is cloned,
 // though other sources of code may be used as well.

pkg/build/builder/source.go

@@ -381,7 +381,7 @@ func copyImageSourceFromFilesytem(sourceDir, destDir string) error {
 func extractSourceFromImage(ctx context.Context, dockerClient DockerClient, store storage.Store, image, buildDir string, imageSecretIndex int, paths []buildapiv1.ImageSourcePath, forcePull bool, blobCacheDirectory string) error {
 	log.V(4).Infof("Extracting image source from image %s", image)
 
-	pullPolicy := buildah.PullIfMissing
+	pullPolicy := buildah.PullIfNewer
 	if forcePull {
 		pullPolicy = buildah.PullAlways
 	}