Image policy is resolving images on replica sets by default

Even when local names are not requested, the image policy plugin is
deciding to rewrite image references in replica sets that point to the
integrated registry (with tags) to use digests. This causes the
deployment controller that created them to get wedged (because it
detects a change to the template) and become unable to update status on
that replica set.

https://bugzilla.redhat.com/show_bug.cgi?id=1481801

pkg/image/admission/imagepolicy/api/types.go

@@ -60,6 +60,11 @@ type ImageResolutionPolicyRule struct {
 	// stream tags, but only if the target image stream tag has the "resolveLocalNames" field
 	// set.
 	LocalNames bool
+	// Type controls whether resolution will happen if the rule doesn't match local names. The default
+	// value is DoNotAttempt. This overrides the global image policy for a matching resource. If there
+	// are multiple rules matching a resource, any rule that requests resolution or rewrite will
+	// result in rewrite.
+	Type ImageResolutionType
 }
 
 // ImageExecutionPolicyRule determines whether a provided image may be used on the platform.

pkg/image/admission/imagepolicy/api/v1/default-policy.yaml

@@ -17,4 +17,3 @@ executionRules:
   - key: images.openshift.io/deny-execution
     value: "true"
   skipOnResolutionFailure: true
-

pkg/image/admission/imagepolicy/api/v1/defaults.go

@@ -16,15 +16,19 @@ func SetDefaults_ImagePolicyConfig(obj *ImagePolicyConfig) {
 
 	if obj.ResolutionRules == nil {
 		obj.ResolutionRules = []ImageResolutionPolicyRule{
-			{TargetResource: GroupResource{Resource: "pods"}, LocalNames: true},
-			{TargetResource: GroupResource{Group: "build.openshift.io", Resource: "builds"}, LocalNames: true},
-			{TargetResource: GroupResource{Resource: "replicationcontrollers"}, LocalNames: true},
-			{TargetResource: GroupResource{Group: "extensions", Resource: "replicasets"}, LocalNames: true},
+			// pods are rewrite attempted for backwards compatibility.
+			{TargetResource: GroupResource{Resource: "pods"}, LocalNames: true, Type: Attempt},
+
+			{TargetResource: GroupResource{Group: "build.openshift.io", Resource: "builds"}, LocalNames: true, Type: Attempt},
 			{TargetResource: GroupResource{Group: "batch", Resource: "jobs"}, LocalNames: true},
 
-			// TODO: consider adding these
-			// {TargetResource: GroupResource{Group: "extensions", Resource: "deployments"}, LocalNames: true},
-			// {TargetResource: GroupResource{Group: "apps", Resource: "statefulsets"}, LocalNames: true},
+			{TargetResource: GroupResource{Group: "extensions", Resource: "replicasets"}, LocalNames: true},
+			{TargetResource: GroupResource{Resource: "replicationcontrollers"}, LocalNames: true},
+
+			{TargetResource: GroupResource{Group: "apps", Resource: "deployments"}, LocalNames: true},
+			{TargetResource: GroupResource{Group: "extensions", Resource: "deployments"}, LocalNames: true},
+			{TargetResource: GroupResource{Group: "apps", Resource: "statefulsets"}, LocalNames: true},
+			{TargetResource: GroupResource{Group: "extensions", Resource: "daemonsets"}, LocalNames: true},
 		}
 	}
 

pkg/image/admission/imagepolicy/api/v1/swagger_doc.go

@@ -57,6 +57,7 @@ var map_ImageResolutionPolicyRule = map[string]string{
 	"":               "ImageResolutionPolicyRule describes resolution rules based on resource.",
 	"targetResource": "TargetResource is the identified group and resource. If Resource is *, this rule will apply to all resources in that group.",
 	"localNames":     "LocalNames will allow single segment names to be interpreted as namespace local image stream tags, but only if the target image stream tag has the \"resolveLocalNames\" field set.",
+	"type":           "Type controls whether resolution will happen if the rule doesn't match local names. The default value is DoNotAttempt. This overrides the global image policy for a matching resource.",
 }
 
 func (ImageResolutionPolicyRule) SwaggerDoc() map[string]string {

pkg/image/admission/imagepolicy/api/v1/types.go

@@ -49,6 +49,9 @@ type ImageResolutionPolicyRule struct {
 	// stream tags, but only if the target image stream tag has the "resolveLocalNames" field
 	// set.
 	LocalNames bool `json:"localNames"`
+	// Type controls whether resolution will happen if the rule doesn't match local names. The default
+	// value is DoNotAttempt. This overrides the global image policy for a matching resource.
+	Type ImageResolutionType `json:"type"`
 }
 
 // ImageExecutionPolicyRule determines whether a provided image may be used on the platform.

pkg/image/admission/imagepolicy/imagepolicy.go

@@ -451,26 +451,32 @@ var skipImageRewriteOnUpdate = map[schema.GroupResource]struct{}{
 }
 
 // RewriteImagePullSpec applies to implicit rewrite attributes and local resources as well as if the policy requires it.
+// If a local name check is requested and a rule matches true is returned. The policy default resolution is only respected
+// if a resource isn't covered by a rule - if pods have a rule with DoNotAttempt and the global policy is RequiredRewrite,
+// no pods will be rewritten.
 func (config resolutionConfig) RewriteImagePullSpec(attr *rules.ImagePolicyAttributes, isUpdate bool, gr schema.GroupResource) bool {
 	if isUpdate {
 		if _, ok := skipImageRewriteOnUpdate[gr]; ok {
 			return false
 		}
 	}
-	if api.RequestsResolution(config.config.ResolveImages) {
-		return true
-	}
-	if attr.LocalRewrite {
-		for _, rule := range config.config.ResolutionRules {
-			if !rule.LocalNames {
-				continue
-			}
-			if resolutionRuleCoversResource(rule.TargetResource, gr) {
-				return true
-			}
+	hasMatchingRule := false
+	for _, rule := range config.config.ResolutionRules {
+		if !resolutionRuleCoversResource(rule.TargetResource, gr) {
+			continue
 		}
+		if rule.LocalNames && attr.LocalRewrite {
+			return true
+		}
+		if api.RewriteImagePullSpec(rule.Type) {
+			return true
+		}
+		hasMatchingRule = true
 	}
-	return false
+	if hasMatchingRule {
+		return false
+	}
+	return api.RewriteImagePullSpec(config.config.ResolveImages)
 }
 
 // resolutionRuleCoversResource implements wildcard checking on Resource names

pkg/image/admission/imagepolicy/imagepolicy_test.go

@@ -1,6 +1,7 @@
 package imagepolicy
 
 import (
+	"bytes"
 	"os"
 	"reflect"
 	"strings"
@@ -436,9 +437,16 @@ func TestAdmissionResolveImages(t *testing.T) {
 		DockerImageReference: "integrated.registry/image1/image1:latest",
 	}
 
+	obj, err := configlatest.ReadYAML(bytes.NewBufferString(`{"kind":"ImagePolicyConfig","apiVersion":"v1"}`))
+	if err != nil || obj == nil {
+		t.Fatal(err)
+	}
+	defaultPolicyConfig := obj.(*api.ImagePolicyConfig)
+
 	testCases := []struct {
 		client *testclient.Fake
 		policy api.ImageResolutionType
+		config *api.ImagePolicyConfig
 		attrs  admission.Attributes
 		admit  bool
 		expect runtime.Object
@@ -585,6 +593,168 @@ func TestAdmissionResolveImages(t *testing.T) {
 				},
 			},
 		},
+
+		// resolves images in the integrated registry on builds without altering their ref (avoids looking up the tag)
+		{
+			policy: api.RequiredRewrite,
+			client: testclient.NewSimpleFake(
+				image1,
+			),
+			attrs: admission.NewAttributesRecord(
+				&buildapi.Build{
+					Spec: buildapi.BuildSpec{
+						CommonSpec: buildapi.CommonSpec{
+							Strategy: buildapi.BuildStrategy{
+								SourceStrategy: &buildapi.SourceBuildStrategy{
+									From: kapi.ObjectReference{Kind: "DockerImage", Name: "integrated.registry/test/mysql@sha256:0000000000000000000000000000000000000000000000000000000000000001"},
+								},
+							},
+						},
+					},
+				}, nil, schema.GroupVersionKind{Version: "v1", Kind: "Build"},
+				"default", "build1", schema.GroupVersionResource{Version: "v1", Resource: "builds"},
+				"", admission.Create, nil,
+			),
+			admit: true,
+			expect: &buildapi.Build{
+				Spec: buildapi.BuildSpec{
+					CommonSpec: buildapi.CommonSpec{
+						Strategy: buildapi.BuildStrategy{
+							SourceStrategy: &buildapi.SourceBuildStrategy{
+								From: kapi.ObjectReference{Kind: "DockerImage", Name: "integrated.registry/test/mysql@sha256:0000000000000000000000000000000000000000000000000000000000000001"},
+							},
+						},
+					},
+				},
+			},
+		},
+		// does not rewrite the config because build has DoNotAttempt by default, which overrides global policy
+		{
+			config: &api.ImagePolicyConfig{
+				ResolveImages: api.RequiredRewrite,
+				ResolutionRules: []api.ImageResolutionPolicyRule{
+					{TargetResource: metav1.GroupResource{Group: "", Resource: "builds"}},
+				},
+			},
+			client: testclient.NewSimpleFake(
+				&imageapi.ImageStreamTag{
+					ObjectMeta: metav1.ObjectMeta{Name: "test:other", Namespace: "default"},
+					Image:      *image1,
+				},
+			),
+			attrs: admission.NewAttributesRecord(
+				&buildapi.Build{
+					Spec: buildapi.BuildSpec{
+						CommonSpec: buildapi.CommonSpec{
+							Strategy: buildapi.BuildStrategy{
+								CustomStrategy: &buildapi.CustomBuildStrategy{
+									From: kapi.ObjectReference{Kind: "ImageStreamTag", Name: "test:other"},
+								},
+							},
+						},
+					},
+				}, nil, schema.GroupVersionKind{Version: "v1", Kind: "Build"},
+				"default", "build1", schema.GroupVersionResource{Version: "v1", Resource: "builds"},
+				"", admission.Create, nil,
+			),
+			admit: true,
+			expect: &buildapi.Build{
+				Spec: buildapi.BuildSpec{
+					CommonSpec: buildapi.CommonSpec{
+						Strategy: buildapi.BuildStrategy{
+							CustomStrategy: &buildapi.CustomBuildStrategy{
+								From: kapi.ObjectReference{Kind: "ImageStreamTag", Name: "test:other"},
+							},
+						},
+					},
+				},
+			},
+		},
+		// does not rewrite the config because build has Attempt by default, which overrides global policy
+		{
+			config: &api.ImagePolicyConfig{
+				ResolveImages: api.RequiredRewrite,
+				ResolutionRules: []api.ImageResolutionPolicyRule{
+					{TargetResource: metav1.GroupResource{Group: "", Resource: "builds"}, Type: api.Attempt},
+				},
+			},
+			client: testclient.NewSimpleFake(
+				&imageapi.ImageStreamTag{
+					ObjectMeta: metav1.ObjectMeta{Name: "test:other", Namespace: "default"},
+					Image:      *image1,
+				},
+			),
+			attrs: admission.NewAttributesRecord(
+				&buildapi.Build{
+					Spec: buildapi.BuildSpec{
+						CommonSpec: buildapi.CommonSpec{
+							Strategy: buildapi.BuildStrategy{
+								CustomStrategy: &buildapi.CustomBuildStrategy{
+									From: kapi.ObjectReference{Kind: "ImageStreamTag", Name: "test:other"},
+								},
+							},
+						},
+					},
+				}, nil, schema.GroupVersionKind{Version: "v1", Kind: "Build"},
+				"default", "build1", schema.GroupVersionResource{Version: "v1", Resource: "builds"},
+				"", admission.Create, nil,
+			),
+			admit: true,
+			expect: &buildapi.Build{
+				Spec: buildapi.BuildSpec{
+					CommonSpec: buildapi.CommonSpec{
+						Strategy: buildapi.BuildStrategy{
+							CustomStrategy: &buildapi.CustomBuildStrategy{
+								From: kapi.ObjectReference{Kind: "ImageStreamTag", Name: "test:other"},
+							},
+						},
+					},
+				},
+			},
+		},
+		// does rewrite the config because build has AttemptRewrite
+		{
+			config: &api.ImagePolicyConfig{
+				ResolveImages: api.DoNotAttempt,
+				ResolutionRules: []api.ImageResolutionPolicyRule{
+					{TargetResource: metav1.GroupResource{Group: "", Resource: "builds"}, Type: api.AttemptRewrite},
+				},
+			},
+			client: testclient.NewSimpleFake(
+				&imageapi.ImageStreamTag{
+					ObjectMeta: metav1.ObjectMeta{Name: "test:other", Namespace: "default"},
+					Image:      *image1,
+				},
+			),
+			attrs: admission.NewAttributesRecord(
+				&buildapi.Build{
+					Spec: buildapi.BuildSpec{
+						CommonSpec: buildapi.CommonSpec{
+							Strategy: buildapi.BuildStrategy{
+								CustomStrategy: &buildapi.CustomBuildStrategy{
+									From: kapi.ObjectReference{Kind: "ImageStreamTag", Name: "test:other"},
+								},
+							},
+						},
+					},
+				}, nil, schema.GroupVersionKind{Version: "v1", Kind: "Build"},
+				"default", "build1", schema.GroupVersionResource{Version: "v1", Resource: "builds"},
+				"", admission.Create, nil,
+			),
+			admit: true,
+			expect: &buildapi.Build{
+				Spec: buildapi.BuildSpec{
+					CommonSpec: buildapi.CommonSpec{
+						Strategy: buildapi.BuildStrategy{
+							CustomStrategy: &buildapi.CustomBuildStrategy{
+								From: kapi.ObjectReference{Kind: "DockerImage", Name: "integrated.registry/image1/image1@sha256:0000000000000000000000000000000000000000000000000000000000000001"},
+							},
+						},
+					},
+				},
+			},
+		},
+
 		// resolves builds.build.openshift.io with image stream tags, uses the image DockerImageReference with SHA set.
 		{
 			policy: api.RequiredRewrite,
@@ -735,6 +905,43 @@ func TestAdmissionResolveImages(t *testing.T) {
 				},
 			},
 		},
+		// does not resolve replica sets by default
+		{
+			config: defaultPolicyConfig,
+			client: testclient.NewSimpleFake(
+				&imageapi.ImageStreamTag{
+					ObjectMeta: metav1.ObjectMeta{Name: "test:other", Namespace: "default"},
+					Image:      *image1,
+				},
+			),
+			attrs: admission.NewAttributesRecord(
+				&kapiextensions.ReplicaSet{
+					Spec: kapiextensions.ReplicaSetSpec{
+						Template: kapi.PodTemplateSpec{
+							Spec: kapi.PodSpec{
+								Containers: []kapi.Container{
+									{Image: "integrated.registry/default/test:other"},
+								},
+							},
+						},
+					},
+				}, nil, schema.GroupVersionKind{Version: "v1", Kind: "ReplicaSet", Group: "extensions"},
+				"default", "rs1", schema.GroupVersionResource{Version: "v1", Resource: "replicasets", Group: "extensions"},
+				"", admission.Create, nil,
+			),
+			admit: true,
+			expect: &kapiextensions.ReplicaSet{
+				Spec: kapiextensions.ReplicaSetSpec{
+					Template: kapi.PodTemplateSpec{
+						Spec: kapi.PodSpec{
+							Containers: []kapi.Container{
+								{Image: "integrated.registry/default/test:other"},
+							},
+						},
+					},
+				},
+			},
+		},
 		// resolves replica sets that specifically request lookup
 		{
 			policy: api.RequiredRewrite,
@@ -925,16 +1132,21 @@ func TestAdmissionResolveImages(t *testing.T) {
 	}
 	for i, test := range testCases {
 		onResources := []schema.GroupResource{{Resource: "builds"}, {Resource: "pods"}}
-		p, err := newImagePolicyPlugin(&api.ImagePolicyConfig{
-			ResolveImages: test.policy,
-			ResolutionRules: []api.ImageResolutionPolicyRule{
-				{LocalNames: true, TargetResource: metav1.GroupResource{Resource: "*"}},
-				{LocalNames: true, TargetResource: metav1.GroupResource{Group: "extensions", Resource: "*"}},
-			},
-			ExecutionRules: []api.ImageExecutionPolicyRule{
-				{ImageCondition: api.ImageCondition{OnResources: onResources}},
-			},
-		})
+		config := test.config
+		if config == nil {
+			// old style config
+			config = &api.ImagePolicyConfig{
+				ResolveImages: test.policy,
+				ResolutionRules: []api.ImageResolutionPolicyRule{
+					{LocalNames: true, TargetResource: metav1.GroupResource{Resource: "*"}, Type: test.policy},
+					{LocalNames: true, TargetResource: metav1.GroupResource{Group: "extensions", Resource: "*"}, Type: test.policy},
+				},
+				ExecutionRules: []api.ImageExecutionPolicyRule{
+					{ImageCondition: api.ImageCondition{OnResources: onResources}},
+				},
+			}
+		}
+		p, err := newImagePolicyPlugin(config)
 		if err != nil {
 			t.Fatal(err)
 		}