OCPBUGS-104569: vSphere boot image reconciler overwrites/renames current custom-named templates

go.mod

@@ -360,7 +360,7 @@ require (
 	github.com/opencontainers/image-spec v1.1.1
 	github.com/opencontainers/runtime-spec v1.3.0 // indirect
 	github.com/pelletier/go-toml/v2 v2.2.4 // indirect
-	github.com/pkg/errors v0.9.1
+	github.com/pkg/errors v0.9.1 // indirect
 	github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2 // indirect
 	github.com/polyfloyd/go-errorlint v1.7.0 // indirect
 	github.com/proglottis/gpgme v0.1.4 // indirect

pkg/controller/bootimage/cache/cache.go

@@ -9,7 +9,6 @@ import (
 	"path/filepath"
 
 	"github.com/coreos/stream-metadata-go/stream"
-	"github.com/pkg/errors"
 	"golang.org/x/sys/unix"
 	"k8s.io/klog/v2"
 )
@@ -70,14 +69,10 @@ func getFileFromCache(fileName, cacheDir string) (string, string, error) {
 }
 
 // GetCacheDir returns a local path of the cache, where the installer should put the data:
-// /tmp/<applicationName>/<dataType>_cache
+// /tmp/<ImageBasedApplicationName>/<ImageDataType>_cache
 // If the directory doesn't exist, it will be automatically created.
-func getCacheDir(dataType, applicationName string) (string, error) {
-	if dataType == "" {
-		return "", errors.Errorf("data type can't be an empty string")
-	}
-
-	cacheDir := filepath.Join("/tmp", applicationName, dataType+"_cache")
+func getCacheDir() (string, error) {
+	cacheDir := filepath.Join("/tmp", ImageBasedApplicationName, ImageDataType+"_cache")
 
 	_, err := os.Stat(cacheDir)
 	if err != nil {
@@ -130,7 +125,7 @@ func cacheFile(ova *stream.Artifact, filePath, cacheDir string) (string, error)
 	return ova.Download(cacheDir)
 }
 
-// download obtains a file from a given URL, puts it in the cache folder, defined by dataType parameter,
+// download obtains a file from a given URL, puts it in the cache folder,
 // and returns the local file path.
 func DownloadOva(ova *stream.Artifact) (string, error) {
 
@@ -139,7 +134,7 @@ func DownloadOva(ova *stream.Artifact) (string, error) {
 		return "", err
 	}
 
-	cacheDir, err := getCacheDir(ImageDataType, ImageBasedApplicationName)
+	cacheDir, err := getCacheDir()
 	if err != nil {
 		return "", err
 	}

pkg/controller/bootimage/cache/cache_test.go

@@ -0,0 +1,152 @@
+package cache
+
+import (
+	"crypto/sha256"
+	"encoding/hex"
+	"fmt"
+	"net/http"
+	"net/http/httptest"
+	"os"
+	"path/filepath"
+	"sync/atomic"
+	"testing"
+
+	"github.com/coreos/stream-metadata-go/stream"
+)
+
+var testFileCounter atomic.Int64
+
+// newTestArtifact starts an httptest server serving content at a uniquely-named path (so
+// concurrent/sequential test cases never collide in the shared /tmp/imagebased/image_cache
+// directory DownloadOva always uses) and returns a stream.Artifact pointing at it, along with
+// a cleanup func that removes the resulting cached file.
+func newTestArtifact(t *testing.T, content []byte) (*stream.Artifact, *httptest.Server, func()) {
+	t.Helper()
+
+	n := testFileCounter.Add(1)
+	fileName := fmt.Sprintf("mco-cache-test-%d.ova", n)
+
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		_, _ = w.Write(content)
+	}))
+	t.Cleanup(srv.Close)
+
+	sum := sha256.Sum256(content)
+	artifact := &stream.Artifact{
+		Location: srv.URL + "/" + fileName,
+		Sha256:   hex.EncodeToString(sum[:]),
+	}
+
+	cacheDir, err := getCacheDir()
+	if err != nil {
+		t.Fatalf("newTestArtifact: failed to resolve cache dir: %v", err)
+	}
+	cleanup := func() {
+		_ = os.Remove(filepath.Join(cacheDir, fileName))
+	}
+	t.Cleanup(cleanup)
+
+	return artifact, srv, cleanup
+}
+
+func TestDownloadOva(t *testing.T) {
+	t.Run("fresh download", func(t *testing.T) {
+		content := []byte("fake-ova-content-fresh-download")
+		artifact, _, _ := newTestArtifact(t, content)
+
+		path, err := DownloadOva(artifact)
+		if err != nil {
+			t.Fatalf("DownloadOva() unexpected error: %v", err)
+		}
+		got, err := os.ReadFile(path)
+		if err != nil {
+			t.Fatalf("failed to read downloaded file: %v", err)
+		}
+		if string(got) != string(content) {
+			t.Errorf("downloaded content = %q, want %q", got, content)
+		}
+	})
+
+	t.Run("cache hit avoids re-downloading", func(t *testing.T) {
+		content := []byte("fake-ova-content-cache-hit")
+		artifact, srv, _ := newTestArtifact(t, content)
+
+		firstPath, err := DownloadOva(artifact)
+		if err != nil {
+			t.Fatalf("DownloadOva() (first call) unexpected error: %v", err)
+		}
+
+		// Shut the server down before the second call: if DownloadOva tried to re-fetch instead
+		// of serving from cache, it would fail with a connection error.
+		srv.Close()
+
+		secondPath, err := DownloadOva(artifact)
+		if err != nil {
+			t.Fatalf("DownloadOva() (second call) unexpected error: %v", err)
+		}
+		if secondPath != firstPath {
+			t.Errorf("DownloadOva() second call path = %q, want %q (same cached file)", secondPath, firstPath)
+		}
+		got, err := os.ReadFile(secondPath)
+		if err != nil {
+			t.Fatalf("failed to read cached file: %v", err)
+		}
+		if string(got) != string(content) {
+			t.Errorf("cached content = %q, want %q", got, content)
+		}
+	})
+
+	t.Run("corrupted cache is re-downloaded", func(t *testing.T) {
+		content := []byte("fake-ova-content-corruption-repair")
+		artifact, _, cleanup := newTestArtifact(t, content)
+
+		cacheDir, err := getCacheDir()
+		if err != nil {
+			t.Fatalf("failed to resolve cache dir: %v", err)
+		}
+		name, err := artifact.Name()
+		if err != nil {
+			t.Fatalf("failed to compute artifact name: %v", err)
+		}
+		cachedPath := filepath.Join(cacheDir, name)
+
+		// Pre-populate the cache with content that does NOT match artifact.Sha256.
+		if err := os.WriteFile(cachedPath, []byte("stale-corrupted-bytes"), 0o644); err != nil {
+			t.Fatalf("failed to seed corrupted cache file: %v", err)
+		}
+		defer cleanup()
+
+		path, err := DownloadOva(artifact)
+		if err != nil {
+			t.Fatalf("DownloadOva() unexpected error: %v", err)
+		}
+		got, err := os.ReadFile(path)
+		if err != nil {
+			t.Fatalf("failed to read repaired file: %v", err)
+		}
+		if string(got) != string(content) {
+			t.Errorf("repaired content = %q, want %q (freshly re-downloaded)", got, content)
+		}
+	})
+
+	t.Run("download failure propagates", func(t *testing.T) {
+		srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			w.WriteHeader(http.StatusInternalServerError)
+		}))
+		t.Cleanup(srv.Close)
+
+		artifact := &stream.Artifact{
+			Location: srv.URL + "/unreachable-mco-test.ova",
+			Sha256:   "0000000000000000000000000000000000000000000000000000000000000",
+		}
+		cacheDir, err := getCacheDir()
+		if err != nil {
+			t.Fatalf("failed to resolve cache dir: %v", err)
+		}
+		t.Cleanup(func() { _ = os.Remove(filepath.Join(cacheDir, "unreachable-mco-test.ova")) })
+
+		if _, err := DownloadOva(artifact); err == nil {
+			t.Fatalf("DownloadOva() expected an error for a failing download, got nil")
+		}
+	})
+}

pkg/controller/bootimage/helpers.go

@@ -99,7 +99,7 @@ func upgradeStubIgnitionIfRequired(secretName string, secretClient clientset.Int
 	userData := secret.Data[ctrlcommon.UserDataKey]
 	var userDataIgn interface{}
 	if err := json.Unmarshal(userData, &userDataIgn); err != nil {
-		return fmt.Errorf("failed to unmarshal decoded user-data to json (secret %s): %wt", secret.Name, err)
+		return fmt.Errorf("failed to unmarshal decoded user-data to json (secret %s): %w", secret.Name, err)
 	}
 	versionPath := []string{ctrlcommon.IgnFieldIgnition, ctrlcommon.IgnFieldVersion}
 	version, _, err := unstructured.NestedString(userDataIgn.(map[string]any), versionPath...)

pkg/controller/bootimage/ms_helpers_test.go

@@ -0,0 +1,139 @@
+package bootimage
+
+import (
+	"encoding/json"
+	"testing"
+
+	"github.com/coreos/stream-metadata-go/stream"
+	osconfigv1 "github.com/openshift/api/config/v1"
+	machinev1beta1 "github.com/openshift/api/machine/v1beta1"
+	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+)
+
+func TestGetMAPIBootImageValue(t *testing.T) {
+	const rawProviderSpecJSON = `{"template":"some-raw-provider-spec"}`
+
+	machineSet := &machinev1beta1.MachineSet{
+		Spec: machinev1beta1.MachineSetSpec{
+			Template: machinev1beta1.MachineTemplateSpec{
+				Spec: machinev1beta1.MachineSpec{
+					ProviderSpec: machinev1beta1.ProviderSpec{
+						Value: &runtime.RawExtension{Raw: []byte(rawProviderSpecJSON)},
+					},
+				},
+			},
+		},
+	}
+
+	vsphereInfra := &osconfigv1.Infrastructure{
+		Status: osconfigv1.InfrastructureStatus{
+			PlatformStatus: &osconfigv1.PlatformStatus{Type: osconfigv1.VSpherePlatformType},
+		},
+	}
+	awsInfra := &osconfigv1.Infrastructure{
+		Status: osconfigv1.InfrastructureStatus{
+			PlatformStatus: &osconfigv1.PlatformStatus{Type: osconfigv1.AWSPlatformType},
+		},
+	}
+
+	streamConfigMap := func(t *testing.T, s *stream.Stream) *corev1.ConfigMap {
+		t.Helper()
+		data, err := json.Marshal(s)
+		if err != nil {
+			t.Fatalf("failed to marshal test stream: %v", err)
+		}
+		return &corev1.ConfigMap{
+			ObjectMeta: metav1.ObjectMeta{Name: "coreos-bootimages"},
+			Data:       map[string]string{StreamConfigMapKey: string(data)},
+		}
+	}
+
+	validVSphereStream := &stream.Stream{
+		Architectures: map[string]stream.Arch{
+			"x86_64": {
+				Artifacts: map[string]stream.PlatformArtifacts{
+					"vmware": {Release: "417.94.20250101"},
+				},
+			},
+		},
+	}
+
+	cases := []struct {
+		name      string
+		infra     *osconfigv1.Infrastructure
+		configMap *corev1.ConfigMap
+		arch      string
+		want      string // "" means "fall back to raw providerSpec bytes"
+	}{
+		{
+			name:      "non-vSphere platform: raw providerSpec bytes",
+			infra:     awsInfra,
+			configMap: streamConfigMap(t, validVSphereStream),
+			arch:      "x86_64",
+		},
+		{
+			name:      "nil infra: raw providerSpec bytes",
+			infra:     nil,
+			configMap: streamConfigMap(t, validVSphereStream),
+			arch:      "x86_64",
+		},
+		{
+			name:      "nil PlatformStatus: raw providerSpec bytes",
+			infra:     &osconfigv1.Infrastructure{},
+			configMap: streamConfigMap(t, validVSphereStream),
+			arch:      "x86_64",
+		},
+		{
+			name:      "nil configMap: raw providerSpec bytes",
+			infra:     vsphereInfra,
+			configMap: nil,
+			arch:      "x86_64",
+		},
+		{
+			name:      "vSphere with valid release for arch: release string",
+			infra:     vsphereInfra,
+			configMap: streamConfigMap(t, validVSphereStream),
+			arch:      "x86_64",
+			want:      "417.94.20250101",
+		},
+		{
+			name:      "vSphere but arch not present in stream: raw providerSpec bytes",
+			infra:     vsphereInfra,
+			configMap: streamConfigMap(t, validVSphereStream),
+			arch:      "arm64",
+		},
+		{
+			name:  "vSphere but vmware artifact has empty release: raw providerSpec bytes",
+			infra: vsphereInfra,
+			configMap: streamConfigMap(t, &stream.Stream{
+				Architectures: map[string]stream.Arch{
+					"x86_64": {Artifacts: map[string]stream.PlatformArtifacts{"vmware": {Release: ""}}},
+				},
+			}),
+			arch: "x86_64",
+		},
+		{
+			name:  "vSphere but configMap data is unparseable: raw providerSpec bytes",
+			infra: vsphereInfra,
+			configMap: &corev1.ConfigMap{
+				Data: map[string]string{StreamConfigMapKey: "not-json"},
+			},
+			arch: "x86_64",
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			got := getMAPIBootImageValue(machineSet, tc.configMap, tc.infra, tc.arch)
+			want := rawProviderSpecJSON
+			if tc.want != "" {
+				want = tc.want
+			}
+			if string(got) != want {
+				t.Errorf("getMAPIBootImageValue() = %q, want %q", got, want)
+			}
+		})
+	}
+}