Image policy is resolving images on replica sets by default by smarterclayton · Pull Request #15868 · openshift/origin

smarterclayton · 2017-08-18T22:16:22Z

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

smarterclayton · 2017-08-19T05:39:33Z

/retest On Aug 18, 2017, at 8:55 PM, OpenShift CI Robot <notifications@github.com> wrote: @smarterclayton <https://github.com/smarterclayton>: The following tests *failed*, say /retest to rerun them all: Test name Commit Details Rerun command ci/openshift-jenkins/extended_conformance_gce db9e95e <db9e95e> link <https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin_extended_conformance_gce/6268> /test extended_conformance_gce ci/openshift-jenkins/extended_conformance_install_update 33aef8c <33aef8c> link <https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin_extended_conformance_install_update/4433> /test extended_conformance_install_update Full PR test history <https://openshift-gce-devel.appspot.com/pr/15868>. Your PR dashboard <https://openshift-gce-devel.appspot.com/pr/smarterclayton>. Please help us cut down on flakes by linking to <https://github.com/kubernetes/community/blob/master/contributors/devel/flaky-tests.md#filing-issues-for-flaky-tests> an open issue <https://github.com/openshift/origin/issues?q=is:issue+is:open> when you hit one in your PR. Instructions for interacting with me using PR comments are available here <https://github.com/kubernetes/community/blob/master/contributors/devel/pull-requests.md>. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra <https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:> repository. I understand the commands that are listed here <https://github.com/kubernetes/test-infra/blob/master/commands.md>. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#15868 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_p1MmTn_3tNhXhrCwB81PmrKRRMmfks5sZjJygaJpZM4O8GSl> .

tnozicka

the other option would be to check if such object has a controllerRef set - https://github.com/kubernetes/community/blob/master/contributors/design-proposals/controller-ref.md

smarterclayton · 2017-08-19T17:09:04Z

We don't pipe through the full object today, but that is an option. I do think that initializers that mutate the target object will be important in the future - we may just not have thought through this enough upstream yet. Direct hash checking has problems - perhaps mutating admission controllers should have a way to signal that a deliberate mutation occurred and collision should be avoided. For now this is probably the simplest fix, and in 3.7 maybe you and I can start a discussion with @Kargakis and others about this On Aug 19, 2017, at 5:45 AM, Tomáš Nožička <notifications@github.com> wrote: *@tnozicka* commented on this pull request. the other option would be to check if such object has a controllerRef set - https://github.com/kubernetes/community/blob/master/contributors/design-proposals/controller-ref.md — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#15868 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_pxghrge53sZR7UsqKkrCdBLKSq8Aks5sZq7NgaJpZM4O8GSl> .

tnozicka · 2017-08-21T07:37:39Z

/test extended_conformance_install_update

tnozicka · 2017-08-21T08:47:35Z

extended_conformance_install_update is blocked on #15874

tnozicka

few comments; lgtm otherwise for short term

tnozicka · 2017-08-21T11:10:39Z

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

-			{TargetResource: GroupResource{Resource: "replicationcontrollers"}, LocalNames: true},
-			{TargetResource: GroupResource{Group: "extensions", Resource: "replicasets"}, LocalNames: true},
+			// pods are attempted for backwards compatibility.
+			{TargetResource: GroupResource{Resource: "pods"}, LocalNames: true, Type: Attempt},


@smarterclayton I have tried it with master and it behaved like AttemptRewrite notAttempt there. But maybe the backwards compatibility is w.r.t. something else?

With that said Attempt seems like more reasonable default option.

Master rewriting is a bug, and that's caused by local namespace resolution. If you look a few lines up the ResolveImage global (on the config) is defaulting to Attempt, and that's what we're controlling for here. What went into 3.6 was too aggressive.

tnozicka · 2017-08-21T12:05:25Z

pkg/image/admission/imagepolicy/imagepolicy_test.go

+				},
+			},
+		},
+		// does not rewrite the config because build has Attempt by default, which overrides global policy


Not by default, but explicitly set Attempt. (Default is AttemptRewrite.)

I'm going to change the build, that was wrong.

tnozicka · 2017-08-21T12:40:25Z

In future if this plugin would mutate only top level objects which can be found by not having ownerRef (which all controllers set) this kind of issues would go away I think.

Like it would resolve the image only in Deployment itself and left RS and Pod with controllerRef set alone.

Unless there is a use case for mutating controlled objects.

smarterclayton · 2017-08-21T14:05:31Z

I think there is a use case for mutating controlled objects - I explicitly want to be able to default replication controllers at time of snapshot

smarterclayton · 2017-08-21T14:12:23Z

/retest

tnozicka · 2017-08-21T14:39:54Z

In that case we need to work on supporting that upstream as you said and back it up with an use case / example.

tnozicka · 2017-08-21T14:40:58Z

networking/GitHub failed
/test extended_conformance_install_update

tnozicka · 2017-08-22T08:04:47Z

new day; let's give it another shot
/test extended_conformance_install_update

tnozicka · 2017-08-22T09:46:44Z

@openshift/api-review needs API review :)

smarterclayton · 2017-08-22T15:51:00Z

Updated to make builds Attempt instead of AttemptRewrite

smarterclayton · 2017-08-22T16:02:10Z

@liggitt need you to do quick API review. 3.6 is broken, in that rewrite should not happen unless requested. Now, resolution rules override / more specific than the default resolution.

tnozicka

nits in comments; lgtm otherwise

tnozicka · 2017-08-22T15:58:36Z

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

-			{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.


I don't mean to bikeshed on this but I don't get the meaning of rewrite attempted when the line bellow says Attempt, not AttemptRewrite?

This is for resolution (do we look up this image value for possible application of rules).

tnozicka · 2017-08-22T16:05:37Z

pkg/image/admission/imagepolicy/imagepolicy_test.go

+				},
+			},
+		},
+		// does not rewrite the config because build has DoNotAttempt by default, which overrides global policy


actually build has Attempt by default; the rest holds true

smarterclayton · 2017-08-22T22:04:59Z

Updated with feedback from @liggitt

Rename Type to Policy. When defaulting ResolutionRules, default Policy to whatever the value of the top level ResolveImages value is, IF an execution rule is defined that references that resource (backwards compatible).

Makes Policy required so that users can't accidentally footgun themselves.

smarterclayton · 2017-08-22T23:57:19Z

/retest Prometheus flake fixed (or reduced) in another PR

liggitt · 2017-08-24T02:53:01Z

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

+// 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 == "*") {


can group be *?

No, no compelling reason. apps could legitimately have a resource wildcard, but everything on the server seemed a stretch.

liggitt · 2017-08-24T02:53:18Z

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

+				obj.ResolutionRules[i].Policy = DoNotAttempt
+				for _, rule := range obj.ExecutionRules {
+					if executionRuleCoversResource(rule, obj.ResolutionRules[i].TargetResource) {
+						obj.ResolutionRules[i].Policy = obj.ResolveImages


mfojtik · 2017-08-24T10:34:53Z

@smarterclayton this will require ansible update to set the resolutionRules ?

liggitt · 2017-08-24T16:37:08Z

API looks fine, just nits. make sure there's a release note about the new required field

smarterclayton · 2017-08-24T16:44:33Z

@mfojtik no we rely on defaulting today

Will fix the nits and repost in a short time.

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

smarterclayton · 2017-08-24T18:52:55Z

denitted

mfojtik · 2017-08-24T18:53:48Z

/lgtm

openshift-merge-robot · 2017-08-24T18:54:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mfojtik, smarterclayton

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these OWNERS Files:

~~pkg/image/OWNERS~~ [mfojtik,smarterclayton]
~~pkg/oc/bootstrap/OWNERS~~ [mfojtik,smarterclayton]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

tnozicka · 2017-08-24T21:04:09Z

/retest

tnozicka · 2017-08-25T07:02:02Z

/test extended_conformance_install_update

tnozicka · 2017-08-25T07:20:15Z

flake #15977
/test extended_conformance_install_update

openshift-merge-robot · 2017-08-25T09:53:58Z

/test all [submit-queue is verifying that this PR is safe to merge]

openshift-merge-robot · 2017-08-25T11:45:19Z

Automatic merge from submit-queue

openshift-merge-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 18, 2017

openshift-merge-robot assigned miminar and legionus Aug 18, 2017

openshift-merge-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. needs-api-review labels Aug 18, 2017

smarterclayton assigned tnozicka Aug 18, 2017

smarterclayton force-pushed the fix_imagepolicy_master branch 2 times, most recently from db9e95e to 33aef8c Compare August 19, 2017 00:02

tnozicka reviewed Aug 19, 2017

View reviewed changes

tnozicka mentioned this pull request Aug 21, 2017

Install dnsmasq configuration fails during conformance runs #15874

Closed

tnozicka reviewed Aug 21, 2017

View reviewed changes

tnozicka added priority/P0 queue/critical-fix labels Aug 22, 2017

smarterclayton force-pushed the fix_imagepolicy_master branch from 33aef8c to 209468e Compare August 22, 2017 15:49

tnozicka reviewed Aug 22, 2017

View reviewed changes

smarterclayton force-pushed the fix_imagepolicy_master branch from 209468e to 962153e Compare August 22, 2017 21:57

liggitt reviewed Aug 24, 2017

View reviewed changes

liggitt added api-approved and removed needs-api-review labels Aug 24, 2017

openshift-merge-robot added the needs-api-review label Aug 24, 2017

smarterclayton force-pushed the fix_imagepolicy_master branch from 962153e to 32f53e3 Compare August 24, 2017 17:55

openshift-ci-robot assigned mfojtik Aug 24, 2017

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 24, 2017

openshift-merge-robot merged commit 02eb0a3 into openshift:master Aug 25, 2017

tnozicka mentioned this pull request Aug 25, 2017

Image policy is resolving images on replica sets by default #15867

Merged

praveenkumar mentioned this pull request Dec 15, 2017

"oc cluster up --version v3.6.1" is failing with oc v3.6.0 binary #17821

Closed

liggitt mentioned this pull request May 19, 2018

default resolution rule policy to the imagepolicy default #19766

Merged

Conversation

smarterclayton commented Aug 18, 2017

Uh oh!

smarterclayton commented Aug 19, 2017 via email

Uh oh!

tnozicka left a comment

Choose a reason for hiding this comment

Uh oh!

smarterclayton commented Aug 19, 2017 via email

Uh oh!

tnozicka commented Aug 21, 2017

Uh oh!

tnozicka commented Aug 21, 2017

Uh oh!

tnozicka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnozicka commented Aug 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smarterclayton commented Aug 21, 2017

Uh oh!

smarterclayton commented Aug 21, 2017

Uh oh!

tnozicka commented Aug 21, 2017

Uh oh!

tnozicka commented Aug 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tnozicka commented Aug 22, 2017

Uh oh!

tnozicka commented Aug 22, 2017

Uh oh!

smarterclayton commented Aug 22, 2017

Uh oh!

smarterclayton commented Aug 22, 2017

Uh oh!

tnozicka left a comment

Choose a reason for hiding this comment

Uh oh!

tnozicka Aug 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smarterclayton commented Aug 22, 2017

Uh oh!

smarterclayton commented Aug 22, 2017 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mfojtik commented Aug 24, 2017

Uh oh!

liggitt commented Aug 24, 2017

Uh oh!

smarterclayton commented Aug 24, 2017

Uh oh!

smarterclayton commented Aug 24, 2017

Uh oh!

mfojtik commented Aug 24, 2017

Uh oh!

openshift-merge-robot commented Aug 24, 2017

Uh oh!

tnozicka commented Aug 24, 2017

Uh oh!

tnozicka commented Aug 21, 2017 •

edited

Loading

tnozicka commented Aug 21, 2017 •

edited

Loading

tnozicka Aug 22, 2017 •

edited

Loading