OCPBUGS-4955: Set ImagePullPolicy of bundle unpacker to "IfNotPresent…

…" for image digests (#2908) * Capture ImagePullPolicy selection from image as func, export for wider use, implement in bundle unpacker, add tests Signed-off-by: Daniel Franz <dfranz@redhat.com> * Fixed using wrong image as reference for pull policy inference Signed-off-by: Daniel Franz <dfranz@redhat.com> * Fixed unit test using wrong vars for path/image/etc. Signed-off-by: Daniel Franz <dfranz@redhat.com> * Remove comment block Signed-off-by: Daniel Franz <dfranz@redhat.com> * Generate hash just once for digestPath Signed-off-by: Daniel Franz <dfranz@redhat.com> Signed-off-by: Daniel Franz <dfranz@redhat.com>
operator-framework · Dec 19, 2022 · cf85bf7 · cf85bf7
1 parent d82537c
commit cf85bf7
Show file tree

Hide file tree

Showing 5 changed files with 87 additions and 41 deletions.
diff --git a/pkg/controller/bundle/bundle_unpacker.go b/pkg/controller/bundle/bundle_unpacker.go
@@ -29,6 +29,7 @@ import (
 	listersoperatorsv1alpha1 "github.com/operator-framework/operator-lifecycle-manager/pkg/api/client/listers/operators/v1alpha1"
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/controller/install"
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/controller/registry/resolver/projection"
+	"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/image"
 )
 
 const (
@@ -170,7 +171,7 @@ func (c *ConfigMapUnpacker) job(cmRef *corev1.ObjectReference, bundlePath string
 						{
 							Name:            "pull",
 							Image:           bundlePath,
-							ImagePullPolicy: "Always",
+							ImagePullPolicy: image.InferImagePullPolicy(bundlePath),
 							Command:         []string{"/util/cpb", "/bundle"}, // Copy bundle content to its mount
 							VolumeMounts: []corev1.VolumeMount{
 								{

diff --git a/pkg/controller/bundle/bundle_unpacker_test.go b/pkg/controller/bundle/bundle_unpacker_test.go
@@ -33,11 +33,13 @@ const (
 	opmImage    = "opm-image"
 	utilImage   = "util-image"
 	bundlePath  = "bundle-path"
+	digestPath  = "bundle-path@sha256:54d626e08c1c802b305dad30b7e54a82f102390cc92c7d4db112048935236e9c"
 	runAsUser   = 1001
 )
 
 func TestConfigMapUnpacker(t *testing.T) {
 	pathHash := hash(bundlePath)
+	digestHash := hash(digestPath)
 	start := metav1.Now()
 	now := func() metav1.Time {
 		return start
@@ -398,18 +400,18 @@ func TestConfigMapUnpacker(t *testing.T) {
 			},
 		},
 		{
-			description: "CatalogSourcePresent/ConfigMapPresent/JobPresent/Unpacked",
+			description: "CatalogSourcePresent/ConfigMapPresent/JobPresent/DigestImage/Unpacked",
 			fields: fields{
 				objs: []runtime.Object{
 					&batchv1.Job{
 						ObjectMeta: metav1.ObjectMeta{
-							Name:      pathHash,
+							Name:      digestHash,
 							Namespace: "ns-a",
 							OwnerReferences: []metav1.OwnerReference{
 								{
 									APIVersion:         "v1",
 									Kind:               "ConfigMap",
-									Name:               pathHash,
+									Name:               digestHash,
 									Controller:         &blockOwnerDeletion,
 									BlockOwnerDeletion: &blockOwnerDeletion,
 								},
@@ -420,7 +422,7 @@ func TestConfigMapUnpacker(t *testing.T) {
 							BackoffLimit:          &backoffLimit,
 							Template: corev1.PodTemplateSpec{
 								ObjectMeta: metav1.ObjectMeta{
-									Name: pathHash,
+									Name: digestHash,
 								},
 								Spec: corev1.PodSpec{
 									RestartPolicy: corev1.RestartPolicyNever,
@@ -435,11 +437,11 @@ func TestConfigMapUnpacker(t *testing.T) {
 										{
 											Name:    "extract",
 											Image:   opmImage,
-											Command: []string{"opm", "alpha", "bundle", "extract", "-m", "/bundle/", "-n", "ns-a", "-c", pathHash, "-z"},
+											Command: []string{"opm", "alpha", "bundle", "extract", "-m", "/bundle/", "-n", "ns-a", "-c", digestHash, "-z"},
 											Env: []corev1.EnvVar{
 												{
 													Name:  configmap.EnvContainerImage,
-													Value: bundlePath,
+													Value: digestPath,
 												},
 											},
 											VolumeMounts: []corev1.VolumeMount{
@@ -488,8 +490,8 @@ func TestConfigMapUnpacker(t *testing.T) {
 										},
 										{
 											Name:            "pull",
-											Image:           bundlePath,
-											ImagePullPolicy: "Always",
+											Image:           digestPath,
+											ImagePullPolicy: "IfNotPresent",
 											Command:         []string{"/util/cpb", "/bundle"}, // Copy bundle content to its mount
 											VolumeMounts: []corev1.VolumeMount{
 												{
@@ -548,7 +550,7 @@ func TestConfigMapUnpacker(t *testing.T) {
 					},
 					&corev1.ConfigMap{
 						ObjectMeta: metav1.ObjectMeta{
-							Name:      pathHash,
+							Name:      digestHash,
 							Namespace: "ns-a",
 							OwnerReferences: []metav1.OwnerReference{
 								{
@@ -580,7 +582,7 @@ func TestConfigMapUnpacker(t *testing.T) {
 			args: args{
 				annotationTimeout: -1 * time.Minute,
 				lookup: &operatorsv1alpha1.BundleLookup{
-					Path:     bundlePath,
+					Path:     digestPath,
 					Replaces: "",
 					CatalogSourceRef: &corev1.ObjectReference{
 						Namespace: "ns-a",
@@ -600,14 +602,14 @@ func TestConfigMapUnpacker(t *testing.T) {
 			expected: expected{
 				res: &BundleUnpackResult{
 					BundleLookup: &operatorsv1alpha1.BundleLookup{
-						Path:     bundlePath,
+						Path:     digestPath,
 						Replaces: "",
 						CatalogSourceRef: &corev1.ObjectReference{
 							Namespace: "ns-a",
 							Name:      "src-a",
 						},
 					},
-					name: pathHash,
+					name: digestHash,
 					bundle: &api.Bundle{
 						CsvName: "etcdoperator.v0.9.2",
 						CsvJson: csvJSON + "\n",
@@ -622,7 +624,7 @@ func TestConfigMapUnpacker(t *testing.T) {
 				configMaps: []*corev1.ConfigMap{
 					{
 						ObjectMeta: metav1.ObjectMeta{
-							Name:      pathHash,
+							Name:      digestHash,
 							Namespace: "ns-a",
 							Labels:    map[string]string{install.OLMManagedLabelKey: install.OLMManagedLabelValue},
 							OwnerReferences: []metav1.OwnerReference{
@@ -646,13 +648,13 @@ func TestConfigMapUnpacker(t *testing.T) {
 				jobs: []*batchv1.Job{
 					{
 						ObjectMeta: metav1.ObjectMeta{
-							Name:      pathHash,
+							Name:      digestHash,
 							Namespace: "ns-a",
 							OwnerReferences: []metav1.OwnerReference{
 								{
 									APIVersion:         "v1",
 									Kind:               "ConfigMap",
-									Name:               pathHash,
+									Name:               digestHash,
 									Controller:         &blockOwnerDeletion,
 									BlockOwnerDeletion: &blockOwnerDeletion,
 								},
@@ -663,7 +665,7 @@ func TestConfigMapUnpacker(t *testing.T) {
 							BackoffLimit:          &backoffLimit,
 							Template: corev1.PodTemplateSpec{
 								ObjectMeta: metav1.ObjectMeta{
-									Name: pathHash,
+									Name: digestHash,
 								},
 								Spec: corev1.PodSpec{
 									RestartPolicy: corev1.RestartPolicyNever,
@@ -678,11 +680,11 @@ func TestConfigMapUnpacker(t *testing.T) {
 										{
 											Name:    "extract",
 											Image:   opmImage,
-											Command: []string{"opm", "alpha", "bundle", "extract", "-m", "/bundle/", "-n", "ns-a", "-c", pathHash, "-z"},
+											Command: []string{"opm", "alpha", "bundle", "extract", "-m", "/bundle/", "-n", "ns-a", "-c", digestHash, "-z"},
 											Env: []corev1.EnvVar{
 												{
 													Name:  configmap.EnvContainerImage,
-													Value: bundlePath,
+													Value: digestPath,
 												},
 											},
 											VolumeMounts: []corev1.VolumeMount{
@@ -731,8 +733,8 @@ func TestConfigMapUnpacker(t *testing.T) {
 										},
 										{
 											Name:            "pull",
-											Image:           bundlePath,
-											ImagePullPolicy: "Always",
+											Image:           digestPath,
+											ImagePullPolicy: "IfNotPresent",
 											Command:         []string{"/util/cpb", "/bundle"}, // Copy bundle content to its mount
 											VolumeMounts: []corev1.VolumeMount{
 												{
@@ -793,13 +795,13 @@ func TestConfigMapUnpacker(t *testing.T) {
 				roles: []*rbacv1.Role{
 					{
 						ObjectMeta: metav1.ObjectMeta{
-							Name:      pathHash,
+							Name:      digestHash,
 							Namespace: "ns-a",
 							OwnerReferences: []metav1.OwnerReference{
 								{
 									APIVersion:         "v1",
 									Kind:               "ConfigMap",
-									Name:               pathHash,
+									Name:               digestHash,
 									Controller:         &blockOwnerDeletion,
 									BlockOwnerDeletion: &blockOwnerDeletion,
 								},
@@ -817,7 +819,7 @@ func TestConfigMapUnpacker(t *testing.T) {
 									"configmaps",
 								},
 								ResourceNames: []string{
-									pathHash,
+									digestHash,
 								},
 							},
 						},
@@ -826,13 +828,13 @@ func TestConfigMapUnpacker(t *testing.T) {
 				roleBindings: []*rbacv1.RoleBinding{
 					{
 						ObjectMeta: metav1.ObjectMeta{
-							Name:      pathHash,
+							Name:      digestHash,
 							Namespace: "ns-a",
 							OwnerReferences: []metav1.OwnerReference{
 								{
 									APIVersion:         "v1",
 									Kind:               "ConfigMap",
-									Name:               pathHash,
+									Name:               digestHash,
 									Controller:         &blockOwnerDeletion,
 									BlockOwnerDeletion: &blockOwnerDeletion,
 								},
@@ -849,7 +851,7 @@ func TestConfigMapUnpacker(t *testing.T) {
 						RoleRef: rbacv1.RoleRef{
 							APIGroup: "rbac.authorization.k8s.io",
 							Kind:     "Role",
-							Name:     pathHash,
+							Name:     digestHash,
 						},
 					},
 				},
@@ -1506,6 +1508,7 @@ func TestConfigMapUnpacker(t *testing.T) {
 				if tt.expected.res.bundle == nil {
 					require.Nil(t, res.bundle)
 				} else {
+					require.NotNil(t, res.bundle)
 					require.Equal(t, tt.expected.res.bundle.CsvJson, res.bundle.CsvJson)
 					require.Equal(t, tt.expected.res.bundle.CsvName, res.bundle.CsvName)
 					require.Equal(t, tt.expected.res.bundle.Version, res.bundle.Version)

diff --git a/pkg/controller/registry/reconciler/reconciler.go b/pkg/controller/registry/reconciler/reconciler.go
@@ -4,7 +4,6 @@ package reconciler
 import (
 	"fmt"
 	"hash/fnv"
-	"strings"
 
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/resource"
@@ -14,6 +13,7 @@ import (
 
 	operatorsv1alpha1 "github.com/operator-framework/api/pkg/operators/v1alpha1"
 	controllerclient "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/controller-runtime/client"
+	"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/image"
 	hashutil "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/kubernetes/pkg/util/hash"
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorclient"
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorlister"
@@ -107,17 +107,7 @@ func NewRegistryReconcilerFactory(lister operatorlister.OperatorLister, opClient
 	}
 }
 
-func Pod(source *operatorsv1alpha1.CatalogSource, name string, image string, saName string, labels map[string]string, annotations map[string]string, readinessDelay int32, livenessDelay int32, runAsUser int64) *corev1.Pod {
-	// Ensure the catalog image is always pulled if the image is not based on a digest, measured by whether an "@" is included.
-	// See https://github.com/docker/distribution/blob/master/reference/reference.go for more info.
-	// This means recreating non-digest based catalog pods will result in the latest version of the catalog content being delivered on-cluster.
-	var pullPolicy corev1.PullPolicy
-	if strings.Contains(image, "@") {
-		pullPolicy = corev1.PullIfNotPresent
-	} else {
-		pullPolicy = corev1.PullAlways
-	}
-
+func Pod(source *operatorsv1alpha1.CatalogSource, name string, img string, saName string, labels map[string]string, annotations map[string]string, readinessDelay int32, livenessDelay int32, runAsUser int64) *corev1.Pod {
 	// make a copy of the labels and annotations to avoid mutating the input parameters
 	podLabels := make(map[string]string)
 	podAnnotations := make(map[string]string)
@@ -141,7 +131,7 @@ func Pod(source *operatorsv1alpha1.CatalogSource, name string, image string, saN
 			Containers: []corev1.Container{
 				{
 					Name:  name,
-					Image: image,
+					Image: img,
 					Ports: []corev1.ContainerPort{
 						{
 							Name:          "grpc",
@@ -184,7 +174,7 @@ func Pod(source *operatorsv1alpha1.CatalogSource, name string, image string, saN
 					SecurityContext: &corev1.SecurityContext{
 						ReadOnlyRootFilesystem: pointer.Bool(false),
 					},
-					ImagePullPolicy:          pullPolicy,
+					ImagePullPolicy:          image.InferImagePullPolicy(img),
 					TerminationMessagePolicy: corev1.TerminationMessageFallbackToLogsOnError,
 				},
 			},

diff --git a/pkg/lib/image/image.go b/pkg/lib/image/image.go
@@ -0,0 +1,17 @@
+package image
+
+import (
+	"strings"
+
+	corev1 "k8s.io/api/core/v1"
+)
+
+func InferImagePullPolicy(image string) corev1.PullPolicy {
+	// Ensure the image is always pulled if the image is not based on a digest, measured by whether an "@" is included.
+	// See https://github.com/docker/distribution/blob/master/reference/reference.go for more info.
+	// This means recreating non-digest based pods will result in the latest version of the content being delivered on-cluster.
+	if strings.Contains(image, "@") {
+		return corev1.PullIfNotPresent
+	}
+	return corev1.PullAlways
+}
diff --git a/pkg/lib/image/image_test.go b/pkg/lib/image/image_test.go
@@ -0,0 +1,35 @@
+package image_test
+
+import (
+	"testing"
+
+	"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/image"
+	"github.com/stretchr/testify/require"
+	corev1 "k8s.io/api/core/v1"
+)
+
+func TestInferImagePullPolicy(t *testing.T) {
+	tests := []struct {
+		description    string
+		img            string
+		expectedPolicy corev1.PullPolicy
+	}{
+		{
+			description:    "WithImageTag",
+			img:            "my-image:my-tag",
+			expectedPolicy: corev1.PullAlways,
+		},
+		{
+			description:    "WithImageDigest",
+			img:            "my-image@sha256:54d626e08c1c802b305dad30b7e54a82f102390cc92c7d4db112048935236e9c",
+			expectedPolicy: corev1.PullIfNotPresent,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.description, func(t *testing.T) {
+			policy := image.InferImagePullPolicy(tt.img)
+			require.Equal(t, tt.expectedPolicy, policy)
+		})
+	}
+}