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

Author: smarterclayton

pkg/bootstrap/bindata.go

@@ -27,7 +27,9 @@ type ImagePolicyConfig struct {
 
 	// ResolutionRules allows more specific image resolution rules to be applied per resource. If
 	// empty, it defaults to allowing local image stream lookups - "mysql" will map to the image stream
-	// tag "mysql:latest" in the current namespace if the stream supports it.
+	// tag "mysql:latest" in the current namespace if the stream supports it. The default for this
+	// field is all known types that support image resolution, and the policy for those rules will be
+	// set to the overall resolution policy if an execution rule references the same resource.
 	ResolutionRules []ImageResolutionPolicyRule
 
 	// ExecutionRules determine whether the use of an image is allowed in an object with a pod spec.
@@ -53,6 +55,9 @@ var (
 
 // ImageResolutionPolicyRule describes resolution rules based on resource.
 type ImageResolutionPolicyRule struct {
+	// Policy controls whether resolution will happen if the rule doesn't match local names. This value
+	// overrides the global image policy for all target resources.
+	Policy ImageResolutionType
 	// TargetResource is the identified group and resource. If Resource is *, this rule will apply
 	// to all resources in that group.
 	TargetResource metav1.GroupResource

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

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

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

@@ -14,30 +14,52 @@ func SetDefaults_ImagePolicyConfig(obj *ImagePolicyConfig) {
 		obj.ResolveImages = Attempt
 	}
 
+	for i := range obj.ExecutionRules {
+		if len(obj.ExecutionRules[i].OnResources) == 0 {
+			obj.ExecutionRules[i].OnResources = []GroupResource{{Resource: "pods", Group: kapi.GroupName}}
+		}
+	}
+
 	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},
 			{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},
 		}
-	}
-
-	for i := range obj.ExecutionRules {
-		if len(obj.ExecutionRules[i].OnResources) == 0 {
-			obj.ExecutionRules[i].OnResources = []GroupResource{{Resource: "pods", Group: kapi.GroupName}}
+		// default the resolution policy to the global default
+		for i := range obj.ResolutionRules {
+			if len(obj.ResolutionRules[i].Policy) != 0 {
+				continue
+			}
+			obj.ResolutionRules[i].Policy = DoNotAttempt
+			for _, rule := range obj.ExecutionRules {
+				if executionRuleCoversResource(rule, obj.ResolutionRules[i].TargetResource) {
+					obj.ResolutionRules[i].Policy = obj.ResolveImages
+					break
+				}
+			}
 		}
 	}
-
 }
 
 func addDefaultingFuncs(scheme *runtime.Scheme) error {
 	return scheme.AddDefaultingFuncs(
 		SetDefaults_ImagePolicyConfig,
 	)
 }
+
+// executionRuleCoversResource returns true if gr is covered by rule.
+func executionRuleCoversResource(rule ImageExecutionPolicyRule, gr GroupResource) bool {
+	for _, target := range rule.OnResources {
+		if target.Group == gr.Group && (target.Resource == gr.Resource || target.Resource == "*") {
+			return true
+		}
+	}
+	return false
+}

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

@@ -45,7 +45,7 @@ func (ImageExecutionPolicyRule) SwaggerDoc() map[string]string {
 var map_ImagePolicyConfig = map[string]string{
 	"":                "ImagePolicyConfig is the configuration for control of images running on the platform.",
 	"resolveImages":   "ResolveImages indicates the default image resolution behavior.  If a rewriting policy is chosen, then the image pull specs will be updated.",
-	"resolutionRules": "ResolutionRules allows more specific image resolution rules to be applied per resource. If empty, it defaults to allowing local image stream lookups - \"mysql\" will map to the image stream tag \"mysql:latest\" in the current namespace if the stream supports it.",
+	"resolutionRules": "ResolutionRules allows more specific image resolution rules to be applied per resource. If empty, it defaults to allowing local image stream lookups - \"mysql\" will map to the image stream tag \"mysql:latest\" in the current namespace if the stream supports it. The default for this field is all known types that support image resolution, and the policy for those rules will be set to the overall resolution policy if an execution rule references the same resource.",
 	"executionRules":  "ExecutionRules determine whether the use of an image is allowed in an object with a pod spec. By default, these rules only apply to pods, but may be extended to other resource types. If all execution rules are negations, the default behavior is allow all. If any execution rule is an allow, the default behavior is to reject all.",
 }
 
@@ -55,6 +55,7 @@ func (ImagePolicyConfig) SwaggerDoc() map[string]string {
 
 var map_ImageResolutionPolicyRule = map[string]string{
 	"":               "ImageResolutionPolicyRule describes resolution rules based on resource.",
+	"policy":         "Policy controls whether resolution will happen if the rule doesn't match local names. This value overrides the global image policy for all target resources.",
 	"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.",
 }

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

@@ -14,7 +14,9 @@ type ImagePolicyConfig struct {
 
 	// ResolutionRules allows more specific image resolution rules to be applied per resource. If
 	// empty, it defaults to allowing local image stream lookups - "mysql" will map to the image stream
-	// tag "mysql:latest" in the current namespace if the stream supports it.
+	// tag "mysql:latest" in the current namespace if the stream supports it. The default for this
+	// field is all known types that support image resolution, and the policy for those rules will be
+	// set to the overall resolution policy if an execution rule references the same resource.
 	ResolutionRules []ImageResolutionPolicyRule `json:"resolutionRules"`
 
 	// ExecutionRules determine whether the use of an image is allowed in an object with a pod spec.
@@ -42,6 +44,9 @@ var (
 
 // ImageResolutionPolicyRule describes resolution rules based on resource.
 type ImageResolutionPolicyRule struct {
+	// Policy controls whether resolution will happen if the rule doesn't match local names. This value
+	// overrides the global image policy for all target resources.
+	Policy ImageResolutionType `json:"policy"`
 	// TargetResource is the identified group and resource. If Resource is *, this rule will apply
 	// to all resources in that group.
 	TargetResource GroupResource `json:"targetResource"`

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

@@ -28,6 +28,9 @@ func Validate(config *api.ImagePolicyConfig) field.ErrorList {
 	}
 
 	for i, rule := range config.ResolutionRules {
+		if len(rule.Policy) == 0 {
+			allErrs = append(allErrs, field.Required(field.NewPath(api.PluginName, "resolutionRules").Index(i).Child("policy"), "a policy must be specified for this resource"))
+		}
 		if len(rule.TargetResource.Resource) == 0 {
 			allErrs = append(allErrs, field.Required(field.NewPath(api.PluginName, "resolutionRules").Index(i).Child("targetResource", "resource"), "a target resource name or '*' must be provided"))
 		}

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

@@ -47,6 +47,33 @@ func TestValidation(t *testing.T) {
 		t.Fatal(errs)
 	}
 
+	if errs := Validate(&api.ImagePolicyConfig{
+		ResolveImages: api.DoNotAttempt,
+		ResolutionRules: []api.ImageResolutionPolicyRule{
+			{TargetResource: metav1.GroupResource{Resource: "test"}, Policy: api.Attempt},
+		},
+	}); len(errs) != 0 {
+		t.Fatal(errs)
+	}
+
+	if errs := Validate(&api.ImagePolicyConfig{
+		ResolveImages: api.DoNotAttempt,
+		ResolutionRules: []api.ImageResolutionPolicyRule{
+			{TargetResource: metav1.GroupResource{Resource: "test"}},
+		},
+	}); len(errs) == 0 {
+		t.Fatal(errs)
+	}
+
+	if errs := Validate(&api.ImagePolicyConfig{
+		ResolveImages: api.DoNotAttempt,
+		ResolutionRules: []api.ImageResolutionPolicyRule{
+			{Policy: api.Attempt},
+		},
+	}); len(errs) == 0 {
+		t.Fatal(errs)
+	}
+
 	if errs := Validate(&api.ImagePolicyConfig{
 		ResolveImages: api.DoNotAttempt,
 		ExecutionRules: []api.ImageExecutionPolicyRule{

pkg/image/admission/imagepolicy/api/validation/validation_test.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.Policy) {
+			return true
+		}
+		hasMatchingRule = true
 	}
-	return false
+	if hasMatchingRule {
+		return false
+	}
+	return api.RewriteImagePullSpec(config.config.ResolveImages)
 }
 
 // resolutionRuleCoversResource implements wildcard checking on Resource names