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

feat: Support suffix-based matching for resources #1796

Merged

Conversation

devholic
Copy link
Contributor

What this PR does / why we need it:

It would be useful to allow suffix-based matching for resource names, in addition to prefix-based matching.

This commit implements this feature by extending PrefixWildcard type instead of using regex since it might be problematic for large numbers of resources as @maxsmythe commented on the issue.

Which issue(s) this PR fixes:

Fixes #1571

Special notes for your reviewer:

None

@devholic devholic changed the title Support suffix-based matching for resources feat: Support suffix-based matching for resources Jan 18, 2022
Copy link
Contributor

@maxsmythe maxsmythe left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

Overall shape looks good, though I didn't have time to finish a close review. One nit that I can find so far.

pkg/controller/config/process/excluder.go Outdated Show resolved Hide resolved
devholic pushed a commit to devholic/gatekeeper that referenced this pull request Jan 19, 2022
open-policy-agent#1796 (comment)

Signed-off-by: Sunghoon Kang <hoon@linecorp.com>
@devholic devholic force-pushed the #1571-suffix-wildcard-support branch from ad8d595 to 17fa858 Compare January 19, 2022 03:27
pkg/target/target.go Outdated Show resolved Hide resolved
@sozercan
Copy link
Member

sozercan commented Jan 19, 2022

Can we update e2e to cover prefix (or suffix) match too? https://github.com/open-policy-agent/gatekeeper/blob/master/test/bats/tests/sync_with_exclusion.yaml#L9

Copy link
Contributor

@maxsmythe maxsmythe left a comment

Choose a reason for hiding this comment

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

LGTM!

devholic pushed a commit to devholic/gatekeeper that referenced this pull request Jan 20, 2022
open-policy-agent#1796 (comment)

Signed-off-by: Sunghoon Kang <hoon@linecorp.com>
devholic pushed a commit to devholic/gatekeeper that referenced this pull request Jan 20, 2022
open-policy-agent#1796 (comment)

Signed-off-by: Sunghoon Kang <hoon@linecorp.com>
Sunghoon Kang added 4 commits January 20, 2022 12:19
open-policy-agent#1571

It would be useful to allow suffix-based matching for resource names,
in addition to prefix-based matching.

This commit implements this feature by extending `PrefixWildcard` type
instead of using regex since it might be problematic for large numbers
of resources as @maxsmythe commented on the issue.

Signed-off-by: Sunghoon Kang <hoon@linecorp.com>
open-policy-agent#1796 (comment)

Signed-off-by: Sunghoon Kang <hoon@linecorp.com>
open-policy-agent#1796 (comment)

Signed-off-by: Sunghoon Kang <hoon@linecorp.com>
open-policy-agent#1796 (comment)

Signed-off-by: Sunghoon Kang <hoon@linecorp.com>
@devholic devholic force-pushed the #1571-suffix-wildcard-support branch from 71f7757 to 5af3a52 Compare January 20, 2022 03:20
@devholic
Copy link
Contributor Author

I rebased onto the latest master branch, and added the following patches:

  • Update pattern limitations in comment - efb931a
  • Add e2e tests - 5af3a52

@codecov-commenter
Copy link

Codecov Report

Merging #1796 (5af3a52) into master (8e580b6) will decrease coverage by 0.04%.
The diff coverage is 45.45%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1796      +/-   ##
==========================================
- Coverage   52.08%   52.03%   -0.05%     
==========================================
  Files          99       99              
  Lines        8821     8828       +7     
==========================================
  Hits         4594     4594              
- Misses       3865     3870       +5     
- Partials      362      364       +2     
Flag Coverage Δ
unittests 52.03% <45.45%> (-0.05%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/mutation/match/match.go 84.17% <ø> (ø)
pkg/mutation/match/zz_generated.deepcopy.go 0.00% <0.00%> (ø)
pkg/target/target.go 67.96% <0.00%> (-0.45%) ⬇️
pkg/watch/registrar.go 73.15% <0.00%> (-1.51%) ⬇️
pkg/controller/config/process/excluder.go 11.76% <16.66%> (ø)
pkg/util/wildcard.go 100.00% <100.00%> (ø)
pkg/watch/replay.go 78.97% <0.00%> (-2.28%) ⬇️
pkg/readiness/object_tracker.go 82.91% <0.00%> (-1.07%) ⬇️
pkg/readiness/ready_tracker.go 69.83% <0.00%> (-0.51%) ⬇️
...onstrainttemplate/constrainttemplate_controller.go 60.09% <0.00%> (+0.96%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c2d6b4f...5af3a52. Read the comment docs.

Copy link
Member

@sozercan sozercan left a comment

Choose a reason for hiding this comment

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

Thank you! LGTM

@sozercan sozercan merged commit 9d0bbc6 into open-policy-agent:master Jan 20, 2022
@devholic devholic deleted the #1571-suffix-wildcard-support branch January 20, 2022 05:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add suffix-based matching for namespaces and excludedNamespaces
4 participants