K8SPG-792 Make patroni check check respect default image settings (#1167)

* K8SPG-792 Make patroni check check respect default image settings

Signed-off-by: Jakub Jaruszewski <jjaruszewski@man.poznan.pl>

* K8SPG-792 Add unit tests for default image behaviour

Signed-off-by: Jakub Jaruszewski <jjaruszewski@man.poznan.pl>

* K8SPG-792 Add comments regarding changed upstream func

Signed-off-by: Jakub Jaruszewski <jjaruszewski@man.poznan.pl>

* K8SPG-792 Use table for tests

Signed-off-by: Jakub Jaruszewski <jjaruszewski@man.poznan.pl>

* K8SPG-792 Fix gofmt warning

Signed-off-by: Jakub Jaruszewski <jjaruszewski@man.poznan.pl>

---------

Signed-off-by: Jakub Jaruszewski <jjaruszewski@man.poznan.pl>
Co-authored-by: Viacheslav Sarzhan <slava.sarzhan@percona.com>
Co-authored-by: George Kechagias <geo.kechagias@gmail.com>

Author: 3 people

internal/config/config.go

@@ -99,18 +99,28 @@ func PGExporterContainerImage(cluster *v1beta1.PostgresCluster) string {
 	return defaultFromEnv(image, "RELATED_IMAGE_PGEXPORTER")
 }
 
-// PostgresContainerImage returns the container image to use for PostgreSQL.
-func PostgresContainerImage(cluster *v1beta1.PostgresCluster) string {
-	image := cluster.Spec.Image
-	key := "RELATED_IMAGE_POSTGRES_" + fmt.Sprint(cluster.Spec.PostgresVersion)
+// PostgresContainerImageString returns the container image to use for PostgreSQL (from string params).
+// This func copies logic from original PostgresContainerImage as is, leaving PostgresContainerImage as a wrapper for upstream compatibility
+func PostgresContainerImageString(image string, postgresVersion int, postGISVersion string) string {
+	key := "RELATED_IMAGE_POSTGRES_" + fmt.Sprint(postgresVersion)
 
-	if version := cluster.Spec.PostGISVersion; version != "" {
-		key += "_GIS_" + version
+	if postGISVersion != "" {
+		key += "_GIS_" + postGISVersion
 	}
 
 	return defaultFromEnv(image, key)
 }
 
+// PostgresContainerImage returns the container image to use for PostgreSQL.
+// Made as a wrapper of PostgresContainerImageString for compat reasons
+func PostgresContainerImage(cluster *v1beta1.PostgresCluster) string {
+	image := cluster.Spec.Image
+	postgresVersion := cluster.Spec.PostgresVersion
+	postGISVersion := cluster.Spec.PostGISVersion
+
+	return PostgresContainerImageString(image, postgresVersion, postGISVersion)
+}
+
 // PGONamespace returns the namespace where the PGO is running,
 // based on the env var from the DownwardAPI
 // If no env var is found, returns ""

percona/controller/pgcluster/controller.go

@@ -424,7 +424,7 @@ func (r *PGClusterReconciler) reconcilePatroniVersionCheck(ctx context.Context,
 				Containers: []corev1.Container{
 					{
 						Name:  pNaming.ContainerPatroniVersionCheck,
-						Image: cr.Spec.Image,
+						Image: cr.PostgresImage(),
 						Command: []string{
 							"bash",
 						},
@@ -807,7 +807,7 @@ func (r *PGClusterReconciler) reconcileCustomExtensions(ctx context.Context, cr
 	for i := 0; i < len(cr.Spec.InstanceSets); i++ {
 		set := &cr.Spec.InstanceSets[i]
 		set.InitContainers = append(set.InitContainers, extensions.ExtensionRelocatorContainer(
-			cr, cr.Spec.Image, cr.Spec.ImagePullPolicy, cr.Spec.PostgresVersion,
+			cr, cr.PostgresImage(), cr.Spec.ImagePullPolicy, cr.Spec.PostgresVersion,
 		))
 		set.InitContainers = append(set.InitContainers, extensions.ExtensionInstallerContainer(
 			cr,

pkg/apis/pgv2.percona.com/v2/perconapgcluster_types.go

@@ -10,6 +10,7 @@ import (
 	"k8s.io/apimachinery/pkg/util/intstr"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 
+	"github.com/percona/percona-postgresql-operator/internal/config"
 	"github.com/percona/percona-postgresql-operator/internal/logging"
 	"github.com/percona/percona-postgresql-operator/internal/naming"
 	pNaming "github.com/percona/percona-postgresql-operator/percona/naming"
@@ -245,6 +246,12 @@ func (cr *PerconaPGCluster) Default() {
 	}
 }
 
+func (cr *PerconaPGCluster) PostgresImage() string {
+	image := cr.Spec.Image
+	postgresVersion := cr.Spec.PostgresVersion
+	return config.PostgresContainerImageString(image, postgresVersion, "")
+}
+
 func (cr *PerconaPGCluster) ToCrunchy(ctx context.Context, postgresCluster *crunchyv1beta1.PostgresCluster, scheme *runtime.Scheme) (*crunchyv1beta1.PostgresCluster, error) {
 	log := logging.FromContext(ctx)
 

pkg/apis/pgv2.percona.com/v2/perconapgcluster_types_test.go

@@ -1,6 +1,8 @@
 package v2
 
 import (
+	"fmt"
+	"os"
 	"testing"
 
 	"gotest.tools/v3/assert"
@@ -41,3 +43,61 @@ func TestPerconaPGCluster_BackupsEnabled(t *testing.T) {
 		})
 	}
 }
+
+func TestPerconaPGCluster_PostgresImage(t *testing.T) {
+	cluster := new(PerconaPGCluster)
+	cluster.Default()
+
+	postgresVersion := 16
+	testDefaultImage := fmt.Sprintf("test_default_image:%d", postgresVersion)
+	testSpecificImage := fmt.Sprintf("test_defined_image:%d", postgresVersion)
+	testEnv := fmt.Sprintf("RELATED_IMAGE_POSTGRES_%d", postgresVersion)
+
+	cluster.Spec.PostgresVersion = postgresVersion
+
+	tests := map[string]struct {
+		expectedImage string
+		setImage      string
+		envImage      string
+	}{
+		"Spec.Image should be empty by default": {
+			expectedImage: "",
+			setImage:      "",
+			envImage:      "",
+		},
+		"Spec.Image should use env variables if present": {
+			expectedImage: testDefaultImage,
+			setImage:      "",
+			envImage:      testDefaultImage,
+		},
+		"Spec.Image should use defined variable": {
+			expectedImage: testSpecificImage,
+			setImage:      testSpecificImage,
+			envImage:      testDefaultImage,
+		},
+	}
+
+	for name, tt := range tests {
+		t.Run(name, func(t *testing.T) {
+
+			cluster.Spec.Image = tt.setImage
+
+			if tt.envImage != "" {
+				err := os.Setenv(testEnv, tt.envImage)
+
+				if err != nil {
+					t.Fatalf("Failed to set %s env variable: %v", testEnv, err)
+				}
+
+				defer func() {
+					err := os.Unsetenv(testEnv)
+					if err != nil {
+						t.Errorf("Failed to unset %s env variable: %v", testEnv, err)
+					}
+				}()
+			}
+
+			assert.Equal(t, cluster.PostgresImage(), tt.expectedImage)
+		})
+	}
+}