From ac6b215a79c6c683194de0b482ee8c484bba90c5 Mon Sep 17 00:00:00 2001
From: Ricardo Mestre <ricardo.mestre@suse.com>
Date: Thu, 31 Jul 2025 16:31:34 +0100
Subject: [PATCH] Fix recomputing proxy images when installing a ptf or test
 (bsc#1246553)

---
 mgrpxy/cmd/support/ptf/podman/utils.go        |  65 +++-------
 mgrpxy/shared/utils/flags_test.go             | 120 +++++++++++++-----
 shared/types/images.go                        |   9 +-
 shared/utils/utils.go                         |   7 +-
 shared/utils/utils_test.go                    | 113 +++++++++++++----
 uyuni-tools.changes.rjpmestre.main-bsc1246553 |   2 +
 6 files changed, 211 insertions(+), 105 deletions(-)
 create mode 100644 uyuni-tools.changes.rjpmestre.main-bsc1246553
#L3PD: bsc#1246553

diff --git a/mgrpxy/cmd/support/ptf/podman/utils.go b/mgrpxy/cmd/support/ptf/podman/utils.go
index a3ca7b9c..8c8258c6 100644
--- a/mgrpxy/cmd/support/ptf/podman/utils.go
+++ b/mgrpxy/cmd/support/ptf/podman/utils.go
@@ -45,55 +45,32 @@ func (flags *podmanPTFFlags) checkParameters() error {
 		suffix = "test"
 		projectID = flags.TestID
 	}
-	httpdImage, err := podman_shared.GetRunningImage("httpd")
-	if err != nil {
-		return err
-	}
-	flags.UpgradeFlags.Httpd.Name, err = utils.ComputePTF(flags.CustomerID, projectID, httpdImage, suffix)
-	if err != nil {
-		return err
-	}
-	log.Info().Msgf(L("The httpd ptf image computed is: %s"), flags.UpgradeFlags.Httpd.Name)
 
-	sshImage, err := podman_shared.GetRunningImage("ssh")
-	if err != nil {
-		return err
+	proxyImages := []struct {
+		serviceName string
+		imageFlag   *types.ImageFlags
+	}{
+		{"httpd", &flags.UpgradeFlags.Httpd},
+		{"ssh", &flags.UpgradeFlags.SSH},
+		{"tftpd", &flags.UpgradeFlags.Tftpd},
+		{"salt-broker", &flags.UpgradeFlags.SaltBroker},
+		{"squid", &flags.UpgradeFlags.Squid},
 	}
-	flags.UpgradeFlags.SSH.Name, err = utils.ComputePTF(flags.CustomerID, projectID, sshImage, suffix)
-	if err != nil {
-		return err
-	}
-	log.Info().Msgf(L("The ssh ptf image computed is: %s"), flags.UpgradeFlags.SSH.Name)
 
-	tftpdImage, err := podman_shared.GetRunningImage("tftpd")
-	if err != nil {
-		return err
-	}
-	flags.UpgradeFlags.Tftpd.Name, err = utils.ComputePTF(flags.CustomerID, projectID, tftpdImage, suffix)
-	if err != nil {
-		return err
-	}
-	log.Info().Msgf(L("The tftpd ptf image computed is: %s"), flags.UpgradeFlags.Tftpd.Name)
+	// Process each pxy image
+	for _, config := range proxyImages {
+		runningImage, err := podman_shared.GetRunningImage(config.serviceName)
+		if err != nil {
+			return err
+		}
+		config.imageFlag.Name, err = utils.ComputePTF(flags.CustomerID, projectID, runningImage, suffix)
+		if err != nil {
+			return err
+		}
+		config.imageFlag.SkipComputation = true
 
-	saltBrokerImage, err := podman_shared.GetRunningImage("salt-broker")
-	if err != nil {
-		return err
-	}
-	flags.UpgradeFlags.SaltBroker.Name, err = utils.ComputePTF(flags.CustomerID, projectID, saltBrokerImage, suffix)
-	if err != nil {
-		return err
-	}
-	log.Info().Msgf(L("The salt-broker ptf image computed is: %s"), flags.UpgradeFlags.SaltBroker.Name)
-
-	squidImage, err := podman_shared.GetRunningImage("squid")
-	if err != nil {
-		return err
-	}
-	flags.UpgradeFlags.Squid.Name, err = utils.ComputePTF(flags.CustomerID, projectID, squidImage, suffix)
-	if err != nil {
-		return err
+		log.Info().Msgf(L("The %[1]s ptf image computed is: %[2]s"), config.serviceName, config.imageFlag.Name)
 	}
-	log.Info().Msgf(L("The squid ptf image computed is: %s"), flags.UpgradeFlags.Squid.Name)
 
 	return nil
 }
diff --git a/mgrpxy/shared/utils/flags_test.go b/mgrpxy/shared/utils/flags_test.go
index 9ac4418d..835205c7 100644
--- a/mgrpxy/shared/utils/flags_test.go
+++ b/mgrpxy/shared/utils/flags_test.go
@@ -1,4 +1,4 @@
-// SPDX-FileCopyrightText: 2024 SUSE LLC
+// SPDX-FileCopyrightText: 2025 SUSE LLC
 //
 // SPDX-License-Identifier: Apache-2.0
 
@@ -10,40 +10,102 @@ import (
 	"github.com/uyuni-project/uyuni-tools/shared/types"
 )
 
+// TestGetContainerImage tests GetContainerImage method
+// Covering different scenarios: defaults, empty values, and overriding values.
 func TestGetContainerImage(t *testing.T) {
-	data := [][]string{
-		// Expectect image, value of --registry, value of --tag, value of --http-image, value of --http-tag
+	tests := []struct {
+		name           string
+		proxyFlags     ProxyImageFlags
+		expectedResult string
+		description    string
+	}{
+		// Defaults and overiding values
 		{
-			"registry/default/image/proxy-httpd:tag",
-			"registry/default/image/",
-			"tag",
-			"registry/default/image/proxy-httpd",
-			"",
+			name: "no image details",
+			proxyFlags: ProxyImageFlags{
+				Registry: "default/image",
+				Tag:      "tag",
+				Httpd: types.ImageFlags{
+					Name: "",
+					Tag:  "",
+				},
+			},
+			expectedResult: "default/image:tag",
 		},
-		{"registry/default/image/proxy-httpd:tag", "registry", "tag", "registry/default/image/proxy-httpd", ""},
 		{
-			"myregistry.example.com/default/image/proxy-httpd:tag",
-			"myregistry.example.com",
-			"tag",
-			"default/image/proxy-httpd",
-			"",
+			name: "httpd image details overrule defaults",
+			proxyFlags: ProxyImageFlags{
+				Registry: "default/image",
+				Tag:      "tag",
+				Httpd: types.ImageFlags{
+					Name: "default/image/proxy-httpd",
+					Tag:  "mytag",
+				},
+			},
+			expectedResult: "default/image/proxy-httpd:mytag",
+		},
+
+		// registry and image name matching
+		{
+			name: "httpd image name overrule when contains full registry",
+			proxyFlags: ProxyImageFlags{
+				Registry: "default",
+				Tag:      "tag",
+				Httpd: types.ImageFlags{
+					Name: "default/image/proxy-httpd",
+					Tag:  "mytag",
+				},
+			},
+			expectedResult: "default/image/proxy-httpd:mytag",
+		},
+		{
+			name: "httpd image name is appended to registry when it does not include registry",
+			proxyFlags: ProxyImageFlags{
+				Registry: "default/extra/image",
+				Tag:      "tag",
+				Httpd: types.ImageFlags{
+					Name: "default/image/proxy-httpd",
+					Tag:  "mytag",
+				},
+			},
+			expectedResult: "default/extra/image/default/image/proxy-httpd:mytag",
 		},
-		{"default/image/proxy-httpd:mytag", "default/image", "tag", "default/image/proxy-httpd", "mytag"},
-		{"myregistry/path/proxy-httpd:tag", "registry/path", "tag", "myregistry/path/proxy-httpd", ""},
-	}
 
-	for i, testCase := range data {
-		proxyFlags := ProxyImageFlags{
-			Tag:      testCase[2],
-			Registry: testCase[1],
-			Httpd: types.ImageFlags{
-				Name: testCase[3],
-				Tag:  testCase[4],
+		// domain usage
+		{
+			name: "custom full httpd registry image name",
+			proxyFlags: ProxyImageFlags{
+				Registry: "registry.suse.com/suse/some/paths/",
+				Tag:      "1.0.0",
+				Httpd: types.ImageFlags{
+					Name: "registry.opensuse.org/uyuni/proxy-httpd",
+					Tag:  "2.0.0",
+				},
+			},
+			expectedResult: "registry.suse.com/suse/some/paths/uyuni/proxy-httpd:2.0.0",
+			// expectedResult: "registry.opensuse.org/uyuni/proxy-httpd:2.0.0", // this should be the expected result
+		},
+		{
+			name: "httpd with path-only image name",
+			proxyFlags: ProxyImageFlags{
+				Registry: "registry.suse.com/uyuni",
+				Tag:      "1.0.0",
+				Httpd: types.ImageFlags{
+					Name: "path/to/proxy-httpd",
+					Tag:  "",
+				},
 			},
-		}
-		imageName := proxyFlags.GetContainerImage("httpd")
-		if imageName != testCase[0] {
-			t.Errorf("Testcase %d: Image name %s does not match expected %s", i, imageName, testCase[0])
-		}
+			expectedResult: "registry.suse.com/uyuni/path/to/proxy-httpd:1.0.0",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			actual := tt.proxyFlags.GetContainerImage("httpd")
+
+			if actual != tt.expectedResult {
+				t.Errorf("GetContainerImage('httpd') = %s, expected: %s", actual, tt.expectedResult)
+			}
+		})
 	}
 }
diff --git a/shared/types/images.go b/shared/types/images.go
index 0940a866..be66fa6a 100644
--- a/shared/types/images.go
+++ b/shared/types/images.go
@@ -6,10 +6,11 @@ package types
 
 // ImageFlags represents the flags used by an image.
 type ImageFlags struct {
-	Registry   string `mapstructure:"registry"`
-	Name       string `mapstructure:"image"`
-	Tag        string `mapstructure:"tag"`
-	PullPolicy string `mapstructure:"pullPolicy"`
+	Registry        string `mapstructure:"registry"`
+	Name            string `mapstructure:"image"`
+	Tag             string `mapstructure:"tag"`
+	PullPolicy      string `mapstructure:"pullPolicy"`
+	SkipComputation bool   // Internal field, not a command flag, indicates if the image should be computed or not
 }
 
 // ImageMetadata represents the image metadata of an RPM image.
diff --git a/shared/utils/utils.go b/shared/utils/utils.go
index ab5dd7a2..ef9fdb78 100644
--- a/shared/utils/utils.go
+++ b/shared/utils/utils.go
@@ -205,6 +205,11 @@ func ComputeImage(
 	imageFlags types.ImageFlags,
 	appendToName ...string,
 ) (string, error) {
+	if imageFlags.SkipComputation {
+		log.Debug().Msgf("Skipping image recomputation for %s", imageFlags.Name)
+		return imageFlags.Name, nil
+	}
+
 	if !strings.Contains(DefaultRegistry, registry) {
 		log.Info().Msgf(L("Registry %[1]s would be used instead of namespace %[2]s"), registry, DefaultRegistry)
 	}
@@ -244,7 +249,7 @@ func ComputeImage(
 
 // ComputePTF returns a PTF or Test image from registry.suse.com.
 func ComputePTF(user string, ptfID string, fullImage string, suffix string) (string, error) {
-	prefix := fmt.Sprintf("registry.suse.com/a/%s/%s/", user, ptfID)
+	prefix := fmt.Sprintf("registry.suse.com/a/%s/%s/", strings.ToLower(user), ptfID)
 	submatches := prodVersionArchRegex.FindStringSubmatch(fullImage)
 	if submatches == nil || len(submatches) > 1 {
 		return "", fmt.Errorf(L("invalid image name: %s"), fullImage)
diff --git a/shared/utils/utils_test.go b/shared/utils/utils_test.go
index c7838c8b..71e1307a 100644
--- a/shared/utils/utils_test.go
+++ b/shared/utils/utils_test.go
@@ -167,37 +167,96 @@ func sendInput(
 }
 
 func TestComputePTF(t *testing.T) {
-	data := [][]string{
+	// Constants
+	const (
+		defaultPtfID      = "27977"
+		defaultUser       = "150158"
+		defaultSuffix     = "ptf"
+		baseRegistryHost  = "registry.suse.com"
+		defaultRegistry50 = "registry.suse.com/suse/manager/5.0/x86_64"
+		defaultRegistry51 = "registry.suse.com/suse/multi-linux-manager/5.1/x86_64"
+	)
+
+	tests := []struct {
+		name                 string
+		registry             string
+		user                 string
+		ptfID                string
+		fullImage            string
+		suffix               string
+		expected             string
+		expectedErrorMessage string
+	}{
+		// Success cases - 5.0 Manager
 		{
-			"registry.suse.com/a/a196136/27977/suse/manager/5.0/x86_64/proxy-helm:latest-ptf-27977",
-			"a196136",
-			"27977",
-			"registry.suse.com/suse/manager/5.0/x86_64/proxy-helm:latest",
-			"ptf",
+			name:      "success 5.0 container with 5.0 registry",
+			registry:  defaultRegistry50,
+			fullImage: defaultRegistry50 + "/proxy-tftpd:5.0.0",
+			expected:  "registry.suse.com/a/150158/27977/suse/manager/5.0/x86_64/proxy-tftpd:latest-ptf-27977",
+		},
+		{
+			name:      "success 5.0 rpm container with 5.0 registry",
+			registry:  defaultRegistry50,
+			fullImage: "localhost/suse/manager/5.0/x86_64/proxy-ssh:5.0.0",
+			expected:  "registry.suse.com/a/150158/27977/suse/manager/5.0/x86_64/proxy-ssh:latest-ptf-27977",
+		},
+		{
+			name:      "success 5.0 container and base registry host",
+			registry:  baseRegistryHost,
+			fullImage: defaultRegistry50 + "/proxy-tftpd:latest",
+			expected:  "registry.suse.com/a/150158/27977/suse/manager/5.0/x86_64/proxy-tftpd:latest-ptf-27977",
+		},
+		{
+			name:      "success 5.0 container and custom registry",
+			registry:  "mysccregistry.com",
+			fullImage: defaultRegistry50 + "/proxy-helm:latest",
+			expected:  "registry.suse.com/a/150158/27977/suse/manager/5.0/x86_64/proxy-helm:latest-ptf-27977",
+		},
+		{
+			name:      "success 5.0 rpm container and custom registry",
+			registry:  "mysccregistry.com",
+			fullImage: "localhost/suse/manager/5.0/x86_64/proxy-helm:latest",
+			expected:  "registry.suse.com/a/150158/27977/suse/manager/5.0/x86_64/proxy-helm:latest-ptf-27977",
 		},
-	}
-
-	for i, testCase := range data {
-		result := testCase[0]
-		user := testCase[1]
-		ptfID := testCase[2]
-		fullImage := testCase[3]
-		suffix := testCase[4]
 
-		actual, err := ComputePTF(user, ptfID, fullImage, suffix)
+		// Failure cases
+		{
+			name:                 "fail invalid image",
+			registry:             baseRegistryHost,
+			fullImage:            "some.domain.com/not/matching/suse/proxy-helm:latest",
+			expectedErrorMessage: "invalid image name: some.domain.com/not/matching/suse/proxy-helm:latest",
+		},
+	}
 
-		if err != nil {
-			t.Errorf(
-				"Testcase %d: Unexpected error while computing image with %s, %s, %s, %s: %s",
-				i, user, ptfID, fullImage, suffix, err,
-			)
-		}
-		if actual != result {
-			t.Errorf(
-				"Testcase %d: Expected %s got %s when computing image with %s, %s, %s, %s",
-				i, result, actual, user, ptfID, fullImage, suffix,
-			)
-		}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			ptfID := defaultPtfID
+			if tt.ptfID != "" {
+				ptfID = tt.ptfID
+			}
+			user := defaultUser
+			if tt.user != "" {
+				user = tt.user
+			}
+			suffix := defaultSuffix
+			if tt.suffix != "" {
+				suffix = tt.suffix
+			}
+
+			actual, err := ComputePTF(user, ptfID, tt.fullImage, suffix)
+			if err != nil {
+				if tt.expectedErrorMessage == "" {
+					t.Errorf("Unexpected error while executing ComputePTF('%s', '%s', '%s', '%s', '%s'): %s",
+						tt.registry, tt.user, tt.ptfID, tt.fullImage, tt.suffix, err)
+				} else if !strings.Contains(err.Error(), tt.expectedErrorMessage) {
+					t.Errorf("Expected error message to contain '%s', but got: %s",
+						tt.expectedErrorMessage, err.Error())
+				}
+			} else if actual != tt.expected {
+				t.Errorf("ComputePTF('%s', '%s', '%s', '%s', '%s') = %s\nexpected: %s",
+					tt.registry, tt.user, tt.ptfID, tt.fullImage, tt.suffix, actual, tt.expected)
+			}
+		})
 	}
 }
 
