Skip to content
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

Jump to bottom

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

Merged
merged 6 commits into from
Dec 19, 2022
Merged

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

Show file tree
Hide file tree
Changes from 3 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
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand Down Expand Up @@ -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{
{
Expand Down
62 changes: 32 additions & 30 deletions pkg/controller/bundle/bundle_unpacker_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ const (
opmImage = "opm-image"
utilImage = "util-image"
bundlePath = "bundle-path"
digestPath = "bundle-path@sha256:54d626e08c1c802b305dad30b7e54a82f102390cc92c7d4db112048935236e9c"
runAsUser = 1001
)

Expand All @@ -49,8 +50,8 @@ func TestConfigMapUnpacker(t *testing.T) {
defaultUnpackTimeoutSeconds := int64(defaultUnpackDuration.Seconds())

// Custom timeout to override the default cmdline flag ActiveDeadlineSeconds value
customAnnotationDuration := 2 * time.Minute
customAnnotationTimeoutSeconds := int64(customAnnotationDuration.Seconds())
//customAnnotationDuration := 2 * time.Minute
//customAnnotationTimeoutSeconds := int64(customAnnotationDuration.Seconds())

type fields struct {
objs []runtime.Object
Expand All @@ -76,7 +77,7 @@ func TestConfigMapUnpacker(t *testing.T) {
args args
expected expected
}{
{
/*{
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this test no longer needed? maybe we should either remove, or if there's something we need to track, create an issue and tag it here?

description: "NoCatalogSource/NoConfigMap/NoJob/NotCreated/Pending",
fields: fields{},
args: args{
Expand Down Expand Up @@ -396,20 +397,20 @@ 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: hash(digestPath),
awgreene marked this conversation as resolved.
Show resolved Hide resolved
Namespace: "ns-a",
OwnerReferences: []metav1.OwnerReference{
{
APIVersion: "v1",
Kind: "ConfigMap",
Name: pathHash,
Name: hash(digestPath),
Controller: &blockOwnerDeletion,
BlockOwnerDeletion: &blockOwnerDeletion,
},
Expand All @@ -420,7 +421,7 @@ func TestConfigMapUnpacker(t *testing.T) {
BackoffLimit: &backoffLimit,
Template: corev1.PodTemplateSpec{
ObjectMeta: metav1.ObjectMeta{
Name: pathHash,
Name: hash(digestPath),
},
Spec: corev1.PodSpec{
RestartPolicy: corev1.RestartPolicyNever,
Expand All @@ -435,11 +436,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", hash(digestPath), "-z"},
Env: []corev1.EnvVar{
{
Name: configmap.EnvContainerImage,
Value: bundlePath,
Value: digestPath,
},
},
VolumeMounts: []corev1.VolumeMount{
Expand Down Expand Up @@ -488,8 +489,8 @@ func TestConfigMapUnpacker(t *testing.T) {
},
{
Name: "pull",
Image: bundlePath,
ImagePullPolicy: "Always",
Image: digestPath,
ImagePullPolicy: "IfNotPresent",
awgreene marked this conversation as resolved.
Show resolved Hide resolved
Command: []string{"/util/cpb", "/bundle"}, // Copy bundle content to its mount
VolumeMounts: []corev1.VolumeMount{
{
Expand Down Expand Up @@ -548,7 +549,7 @@ func TestConfigMapUnpacker(t *testing.T) {
},
&corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Name: pathHash,
Name: hash(digestPath),
Namespace: "ns-a",
OwnerReferences: []metav1.OwnerReference{
{
Expand Down Expand Up @@ -580,7 +581,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",
Expand All @@ -600,14 +601,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: hash(digestPath),
bundle: &api.Bundle{
CsvName: "etcdoperator.v0.9.2",
CsvJson: csvJSON + "\n",
Expand All @@ -622,7 +623,7 @@ func TestConfigMapUnpacker(t *testing.T) {
configMaps: []*corev1.ConfigMap{
{
ObjectMeta: metav1.ObjectMeta{
Name: pathHash,
Name: hash(digestPath),
Namespace: "ns-a",
Labels: map[string]string{install.OLMManagedLabelKey: install.OLMManagedLabelValue},
OwnerReferences: []metav1.OwnerReference{
Expand All @@ -646,13 +647,13 @@ func TestConfigMapUnpacker(t *testing.T) {
jobs: []*batchv1.Job{
{
ObjectMeta: metav1.ObjectMeta{
Name: pathHash,
Name: hash(digestPath),
Namespace: "ns-a",
OwnerReferences: []metav1.OwnerReference{
{
APIVersion: "v1",
Kind: "ConfigMap",
Name: pathHash,
Name: hash(digestPath),
Controller: &blockOwnerDeletion,
BlockOwnerDeletion: &blockOwnerDeletion,
},
Expand All @@ -663,7 +664,7 @@ func TestConfigMapUnpacker(t *testing.T) {
BackoffLimit: &backoffLimit,
Template: corev1.PodTemplateSpec{
ObjectMeta: metav1.ObjectMeta{
Name: pathHash,
Name: hash(digestPath),
},
Spec: corev1.PodSpec{
RestartPolicy: corev1.RestartPolicyNever,
Expand All @@ -678,11 +679,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", hash(digestPath), "-z"},
Env: []corev1.EnvVar{
{
Name: configmap.EnvContainerImage,
Value: bundlePath,
Value: digestPath,
},
},
VolumeMounts: []corev1.VolumeMount{
Expand Down Expand Up @@ -731,8 +732,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{
{
Expand Down Expand Up @@ -793,13 +794,13 @@ func TestConfigMapUnpacker(t *testing.T) {
roles: []*rbacv1.Role{
{
ObjectMeta: metav1.ObjectMeta{
Name: pathHash,
Name: hash(digestPath),
Namespace: "ns-a",
OwnerReferences: []metav1.OwnerReference{
{
APIVersion: "v1",
Kind: "ConfigMap",
Name: pathHash,
Name: hash(digestPath),
Controller: &blockOwnerDeletion,
BlockOwnerDeletion: &blockOwnerDeletion,
},
Expand All @@ -817,7 +818,7 @@ func TestConfigMapUnpacker(t *testing.T) {
"configmaps",
},
ResourceNames: []string{
pathHash,
hash(digestPath),
},
},
},
Expand All @@ -826,13 +827,13 @@ func TestConfigMapUnpacker(t *testing.T) {
roleBindings: []*rbacv1.RoleBinding{
{
ObjectMeta: metav1.ObjectMeta{
Name: pathHash,
Name: hash(digestPath),
Namespace: "ns-a",
OwnerReferences: []metav1.OwnerReference{
{
APIVersion: "v1",
Kind: "ConfigMap",
Name: pathHash,
Name: hash(digestPath),
Controller: &blockOwnerDeletion,
BlockOwnerDeletion: &blockOwnerDeletion,
},
Expand All @@ -849,7 +850,7 @@ func TestConfigMapUnpacker(t *testing.T) {
RoleRef: rbacv1.RoleRef{
APIGroup: "rbac.authorization.k8s.io",
Kind: "Role",
Name: pathHash,
Name: hash(digestPath),
},
},
},
Expand Down Expand Up @@ -1506,6 +1507,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)
Expand Down
18 changes: 4 additions & 14 deletions pkg/controller/registry/reconciler/reconciler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ package reconciler
import (
"fmt"
"hash/fnv"
"strings"

corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/resource"
Expand All @@ -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"
Expand Down Expand Up @@ -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)
Expand All @@ -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",
Expand Down Expand Up @@ -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,
},
},
Expand Down
17 changes: 17 additions & 0 deletions pkg/lib/image/image.go
View file
Open in desktop
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
Open in desktop
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)
})
}
}