diff --git a/pkg/controller/bundle/bundle_unpacker.go b/pkg/controller/bundle/bundle_unpacker.go index bf797ba1fd..6abfc808ac 100644 --- 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 index 85fe2bbf62..8a4fc5c0a0 100644 --- 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 index d60f6dc668..1d61d47d13 100644 --- 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 new file mode 100644 index 0000000000..aa03e31c14 --- /dev/null +++ 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 new file mode 100644 index 0000000000..ecdc879356 --- /dev/null +++ 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) + }) + } +}