default resolution rule policy to the imagepolicy default

[bparees]

fixes #19775

[bparees]

/hold

@smarterclayton i'm sure you had a good reason for doing this as you did but since the godoc says:

	// 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

there seems to a a conflict between what as implemented (resolutionrule inherits the global default only if an execution rule covers it) and what the doc claims (resolutionrule inherits the global default).

So either we need to fix the doc or fix the behavior, can you help me understand the intent here?

[bparees]

actually there are two issues/questions w/ the original code:

1. the policy defaulting was only being applied if no resolution rules were explicitly defined by the user
2. the policy defaulting is only being applied to resolution rules that are covered by an execution rule

this PR changes both of those (defaults all resolution rules, does not look at execution rule coverage), but perhaps the right solution is to just fix (1) (apply defaulting to all resolution rules, but only if an execution rule covers them). @smarterclayton I need the reasoning behind the covering logic.

[bparees]

@GrahamDumpleton fyi

[liggitt]

I think the current behavior stemmed from this: #15868 (comment)

[smarterclayton]

This was to preserve back compat with 3.6. It is *very* possible any changes here could break running applications. I’m tempted to say we put a hold on changes to the api until we move this to an operator logic and then migrate all existing config to an api object, dropping support for the old config (or letting it be left in legacy mode). On May 19, 2018, at 11:39 AM, Ben Parees <[email protected]> wrote: actually there are two issues/questions w/ the original code: 1. the policy defaulting was only being applied if no resolution rules were explicitly defined by the user 2. the policy defaulting is only being applied to resolution rules that are covered by an execution rule this PR changes both of those (defaults all resolution rules, does not look at execution rule coverage), but perhaps the right solution is to just fix (1) (apply defaulting to all resolution rules, but only if an execution rule covers them). @smarterclayton <https://github.com/smarterclayton> I need the reasoning behind the covering logic. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#19766 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_p_hRACRNAvFlWAACRJtk6BU5wXfGks5t0DycgaJpZM4UEzC8> .

[bparees]

This was to preserve back compat with 3.6. It is very possible any changes here could break running applications.

Can we agree that if, after applying the existing defaulting logic (which is only applied to the default resolution rules), there is a resolution rule w/ no policy set(that would be one explicitly defined by the user), we should set that resolution rule's policy to the global default?

If we do not do that, the configuration is invalid and the cluster fails to start, so there should be no risk of changing existing behavior for a working cluster, it just ensures that someone who, in the future, writes a configuration that relies on the documented defaulting behavior, will get a working cluster and not a validation error.

[bparees]

(i've updated the PR to reflect that proposal)

[bparees]

@smarterclayton bump

[smarterclayton]

I can't see anything wrong with this but all of this is in a tape backup in my mind. Are you confident this can't accidentally regress deployed 3.9 clusters when they upgrade?

[bparees]

Are you confident this can't accidentally regress deployed 3.9 clusters when they upgrade?

From what i understand (which is worth sanity checking), if someone manually defined a resolution rule and did not define a policy for it, it will fail validation (policy is a required field). So any configurations this would affect are already invalid/failing to start. The intent here is just to help people who write a new config going forward.

[bparees]

@smarterclayton beginning of 3.11 feels like a good time to merge this...

[bparees]

/hold cancel

@smarterclayton @liggitt i'm feeling pretty good about this, have i convinced you?

[bparees]

/refresh

[bparees]

/retest all

[bparees]

/test all

[liggitt]

From what i understand (which is worth sanity checking), if someone manually defined a resolution rule and did not define a policy for it, it will fail validation (policy is a required field). So any configurations this would affect are already invalid/failing to start. The intent here is just to help people who write a new config going forward.

and we want to default the half they didn't specify? is that going to cause us problems if those defaults change in the future, and the changes don't mesh with the resolution rule they provided?

[bparees]

and we want to default the half they didn't specify? is that going to cause us problems if those defaults change in the future, and the changes don't mesh with the resolution rule they provided?

The api docs indicate that if the rule itself doesn't specify a value, the default value is pulled from the global policy setting(which is set by them). However the implementation didn't actually do that.
(instead you'd get an error indicating the value was required).

The sole purpose of this PR is to bring the behavior in line w/ the api doc. If we think the api doc behavior is not what we want, we can change the api docs and require that you always specify a policy for every rule, instead of allowing there to be a global policy that the rules inherit, but the global policy that is inherited by the rules by default seemed like a reasonable behavior to me.

I wouldn't think we'd be allowed to change the defaults (which in this case would be the default global policy, I guess).

[liggitt]

The api docs indicate that if the rule itself doesn't specify a value, the default value is pulled from the global policy setting(which is set by them). However the implementation didn't actually do that.
(instead you'd get an error indicating the value was required).

The sole purpose of this PR is to bring the behavior in line w/ the api doc.

Ok.

I wouldn't think we'd be allowed to change the defaults (which in this case would be the default global policy, I guess).

Clayton will have to refresh my memory… I thought new types could get default rules added for them

[bparees]

for reference this is the current api doc:

origin/pkg/image/admission/apis/imagepolicy/types.go

Lines 60 to 62 in dd68e3f

	// 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

[bparees]

technically i guess it doesn't say it inherits the global default if not specified, but the claim that it "overrides it" would imply that it's optional/uses the global value normally.

[bparees]

Clayton will have to refresh my memory… I thought new types could get default rules added for them

@liggitt new types of what? this change only affects resolutionrules (e.g. not executionrules and not necessarily a new type of rule if we introduced one).

[liggitt]

@liggitt new types of what? this change only affects resolutionrules (e.g. not executionrules and not necessarily a new type of rule if we introduced one).

I misread the defaulting... if they specify any resolution rules, we don't contribute any of the default ones, which might expand to include rules for new resource types in the future.

However the implementation didn't actually do that. (instead you'd get an error indicating the value was required).

does that mean that someone that doesn't specify any resolution rules gets an error? the defaults here don't set a policy value:

origin/pkg/image/admission/apis/imagepolicy/v1/defaults.go

Lines 24 to 34 in dab335c

	obj.ResolutionRules = []ImageResolutionPolicyRule{
	{TargetResource: GroupResource{Resource: "pods"}, LocalNames: true},
	{TargetResource: GroupResource{Group: "build.openshift.io", Resource: "builds"}, LocalNames: true},
	{TargetResource: GroupResource{Group: "batch", Resource: "jobs"}, 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},
	}

[bparees]

does that mean that someone that doesn't specify any resolution rules gets an error? the defaults here don't set a policy value:

yes they do:

origin/pkg/image/admission/apis/imagepolicy/v1/defaults.go

Lines 35 to 47 in dab335c

	// 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
	}
	}
	}

[bparees]

/retest

[liggitt]

/lgtm

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bparees, liggitt

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

The pull request process is described here

Needs approval from an approver in each of these files:

• pkg/cmd/OWNERS [bparees,liggitt]
• pkg/image/OWNERS [bparees,liggitt]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[bparees]

/retest

[openshift-bot]

/retest

Please review the full test history for this PR and help us cut down flakes.