Skip to content

OCPBUGS-4955: Set ImagePullPolicy of bundle unpacker to "IfNotPresent" for image digests #2908

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Dec 19, 2022
+87 −41
Merged

OCPBUGS-4955: Set ImagePullPolicy of bundle unpacker to "IfNotPresent" for image digests #2908

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
cae1e0b
Capture ImagePullPolicy selection from image as func, export for wide…
dtfranz Dec 15, 2022
23b37eb
Fixed using wrong image as reference for pull policy inference
dtfranz Dec 16, 2022
5b6a543
Fixed unit test using wrong vars for path/image/etc.
dtfranz Dec 16, 2022
9816763
Remove comment block
dtfranz Dec 16, 2022
2a5c923
Generate hash just once for digestPath
dtfranz Dec 16, 2022
c336d48
Merge branch 'master' into bundle-digest-ifnotpresent
dtfranz Dec 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion pkg/controller/bundle/bundle_unpacker.go
View file
Original file line number Diff line number Diff line change
@@ -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{
{
55 changes: 29 additions & 26 deletions pkg/controller/bundle/bundle_unpacker_test.go
View file
Original file line number Diff line number Diff line change
@@ -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",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe double-check if tests are needed for non-digest images too?

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)
18 changes: 4 additions & 14 deletions pkg/controller/registry/reconciler/reconciler.go
View file
Original file line number Diff line number Diff line change
@@ -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,
},
},
17 changes: 17 additions & 0 deletions pkg/lib/image/image.go
View file
Original file line number Diff line number Diff line change
@@ -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
}
35 changes: 35 additions & 0 deletions pkg/lib/image/image_test.go
View file
Original file line number Diff line number Diff line change
@@ -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)
})
}
}