Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Strictly match Group in ResourceTypeMatcher: K8s core group #1390

Merged
merged 2 commits into from
Apr 19, 2022

Conversation

ewhamilton
Copy link
Contributor

@ewhamilton ewhamilton commented Apr 16, 2022

Change Overview

This PR supports strict matching of Group in ResourceTypeMatcher.

This allows use of K8s core resources (group == "") in filters without
accidentally matching resources with same name in other groups. Maintains
wildcard behavior for "" in version and resource so that matches can be
done for entire groups and all versions of a specific group:resource.

Note: the filter package has been contributed to Kanister in the hope that it
may be useful. It does not appear that the Matches() method that is modified
here is used by anything in Kanister other than test code. This package is
used by K10 and an additional unit test also exercises this change.

Pull request type

Please check the type of change your PR introduces:

  • 🐛 Bugfix

Issues

  • #XXX

Test Plan

  • 💪 Manual
  • ⚡ Unit test
  • 💚 E2E

Copy link
Contributor

@ihcsim ihcsim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not familiar enough with the pkg/discovery/discovery.go code to tell how this change will affect it:

 ✗ grep -rnI "\.Any(" *                                                                                                
pkg/discovery/discovery.go:63:          if !exclude.Any(gvr) {

Any() eventually calls into Matches().

Otherwise, I agree with this change since upstream uses the same value.

@@ -18,7 +18,7 @@ import (
"testing"

. "gopkg.in/check.v1"
"k8s.io/api/core/v1"
v1 "k8s.io/api/core/v1"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VSCode likely added it automatically. I don't think it is needed. OTOH, 72 of the 82 imports of "k8s.io/api/core/v1" in kanister code use it. So I'd do it for consistency.

@ewhamilton
Copy link
Contributor Author

ewhamilton commented Apr 18, 2022

I am not familiar enough with the pkg/discovery/discovery.go code to tell how this change will affect it: exclude.Any() uses it.

Within the Kanister package, the two uses of ignoreGroupErrors are just above, AllGVRsIgnoreGroupErrs() and NamespacedGVRsIgnoreGroupErrs(). Neither are currently called from within Kanister.

NamespacedGVRsIgnoreGroupErrs() is used by K10 where it passes in

		filter.ResourceTypeMatcher{
			filter.ResourceTypeRequirement{Group: "metrics.k8s.io"},
			filter.ResourceTypeRequirement{Group: "external.metrics.k8s.io"},
			filter.ResourceTypeRequirement{Group: "custom.metrics.k8s.io"},
		}

Since the Group values here are not "", there is no effect. If for some reason we wanted to exclude errors for the core group, we wouldn't want it to match other groups accidentally either, so the change would be desirable. If we wanted to ignore errors from all GVRs, an exclude filter of "" would do so.

@mergify mergify bot merged commit a5f36d8 into master Apr 19, 2022
@mergify mergify bot deleted the strict-group-match branch April 19, 2022 20:13
@shuguet shuguet added this to In Progress in Kanister via automation Apr 21, 2022
@shuguet shuguet moved this from In Progress to Done in Kanister Apr 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants