(bugfix): make k8s 1.25 validation logic check api group before issuing a warning

[everettraven]

Description of Change

This PR updates the k8s 1.25 validation logic and the associated test cases to verify if any of the apiGroups defined in an RBAC PolicyRule has a potential deprecation. If it finds one it will check a combination of the apiGroup and all resources defined in the PolicyRule (i.e apiGroup/resource) to see if any of these combinations results in a deprecated API - if so it returns a warning.

Motivation for Change

There was a bug in the validation logic where it was issuing a warning even if the API group the resource belonged to had not been deprecated.

[everettraven]

/cc @joelanford @oceanc80 @grokspawn

[oceanc80]

I think this should be !ok to early out if we don't find the deprecated api in the set of CRs

[joelanford]

I think that is actually a check that’s saying “if this resource is already covered by one that’s listed in the CSV as something the operator uses, then that’s a more reliable check, so skip this check”

This might be something we want to add a comment for since it isn't immediately obvious why we'd be bailing out when we find an RBAC rule for a resource within a deprecated API.

[everettraven]

Taking a look at this, I think it could really go either way. The intention behind this section is exactly what @joelanford mentioned.

I think we could either add a comment or adopt the recommendation by @oceanc80 and remove the continue statement within the block.

i.e go from:

for _, res := range rule.Resources {
	if _, ok := resInCsvCrds[res]; ok {
		continue
	}
       warnIfDeprecated(schema.GroupResource{Group: apiGroup, Resource: res}, fmt.Sprintf("ClusterServiceVersion.Spec.InstallStrategy.StrategySpec.%s[%d].Rules[%d]", permField, i, j))
}

to:

for _, res := range rule.Resources {
	if _, ok := resInCsvCrds[res]; !ok {
            warnIfDeprecated(schema.GroupResource{Group: apiGroup, Resource: res}, fmt.Sprintf("ClusterServiceVersion.Spec.InstallStrategy.StrategySpec.%s[%d].Rules[%d]", permField, i, j))
	}
}

Technically saves us a line of code and if it feels more readable that way I am happy to change it

[joelanford]

Same question here. Can we make this a map[string]struct{} (or use sets.String from k8s.io/apimachinery)?

[everettraven]

After adding maps to resolve your previous comment, I ended up removing this function definition as the map used by warnIfDeprecated has the schema.GroupResource as a key. If the given schema.GroupResource doesn't match any of the keys it doesn't warn.

I think the only downside to having removed this function is that now it always runs the loop to create a schema.GroupResource and verify it instead of only if the group has a deprecated api. If this is something we are concerned about I can re-implement this function.

@joelanford @oceanc80 WDYT?

[theishshah]

/lgtm

[theishshah]

/approve

[kevinrizza]

/approve

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: everettraven, kevinrizza, theishshah

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [kevinrizza]

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