fix: rework ns check, refactor: bubble up match err for mut #2812

acpana · 2023-06-03T05:14:00Z

This patch relaxes the namespace check in the expansion system to support
expansion that happens when no namespace selector is specified.

It also has a small refactor to propagate any matching errors from the mutation system's Matches() calls.

Fixes #2575

Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>

codecov-commenter · 2023-06-03T05:21:03Z

Codecov Report

Patch coverage: 38.70% and project coverage change: +0.04% 🎉

Comparison is base (1e68843) 53.07% compared to head (ba1d4a0) 53.12%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2812      +/-   ##
==========================================
+ Coverage   53.07%   53.12%   +0.04%     
==========================================
  Files         135      135              
  Lines       11790    11806      +16     
==========================================
+ Hits         6258     6272      +14     
- Misses       5046     5048       +2     
  Partials      486      486

Flag	Coverage Δ
unittests	`53.12% <38.70%> (+0.04%)`	⬆️

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

Files Changed	Coverage Δ
...tation/mutators/assignimage/assignimage_mutator.go	`38.26% <0.00%> (ø)`
...mutation/mutators/assignmeta/assignmeta_mutator.go	`35.92% <0.00%> (+0.34%)`	⬆️
.../mutation/mutators/modifyset/modify_set_mutator.go	`52.34% <0.00%> (ø)`
pkg/mutation/types/mutator.go	`33.33% <ø> (ø)`
pkg/mutation/system.go	`77.30% <16.66%> (-4.94%)`	⬇️
pkg/expansion/system.go	`71.25% <70.00%> (-0.80%)`	⬇️
pkg/mutation/mutators/assign/assign_mutator.go	`29.49% <100.00%> (ø)`
pkg/target/matcher.go	`89.09% <100.00%> (ø)`

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

maxsmythe

To support cluster-scoped objects and maintain simplicity, I'd probably just drop the requirement that namespaces exist and assume the resource is cluster-scoped if NS is nil.

@davis-haba Any reason that approach would be breaking?

pkg/gator/expand/expand.go

pkg/expansion/system.go

Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>

pkg/expansion/system.go

Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>

Signed-off-by: alex <8968914+acpana@users.noreply.github.com>

Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>

acpana · 2023-08-02T01:03:16Z

@maxsmythe @davis-haba per our offline chat, I pushed a commit to propagate the error from the mutation interface on Matches(). Let me know what y'all think

test/gator/expand/test.bats

test/gator/test/test.bats

Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>

davis-haba · 2023-08-03T23:51:03Z

Great job!

Commented with 1 question about the fail message for constraints, but otherwise LGTM.

pkg/expansion/system.go

test/gator/test/test.bats

maxsmythe

LGTM

acpana · 2023-08-04T20:22:28Z

@sozercan / @ritazh PTAL when you have a chance! 🙏🏼

pkg/expansion/system.go

pkg/expansion/system_test.go

Co-authored-by: Rita Zhang <rita.z.zhang@gmail.com> Signed-off-by: alex <8968914+acpana@users.noreply.github.com>

pkg/expansion/system_test.go

Signed-off-by: alex <8968914+acpana@users.noreply.github.com>

Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>

ritazh

LGTM

…icy-agent#2812) Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com> Signed-off-by: alex <8968914+acpana@users.noreply.github.com> Co-authored-by: Davis Haba <52938648+davis-haba@users.noreply.github.com> Co-authored-by: Rita Zhang <rita.z.zhang@gmail.com>

feat: opt to relax ns check

bb6fa5d

Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>

acpana requested review from davis-haba, maxsmythe and sozercan June 3, 2023 05:14

maxsmythe requested changes Jun 5, 2023

View reviewed changes

pkg/gator/expand/expand.go Outdated Show resolved Hide resolved

pkg/expansion/system.go Outdated Show resolved Hide resolved

fixme: partially revert ns relax option

ebcc98e

Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>

acpana requested a review from maxsmythe July 18, 2023 19:35

acpana changed the title ~~feat: opt to relax ns check~~ fix: rework ns check Jul 27, 2023

set resultant namespace from parent

9bd37bd

Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>

JaydipGabani reviewed Jul 31, 2023

View reviewed changes

pkg/expansion/system.go Outdated Show resolved Hide resolved

maxsmythe reviewed Jul 31, 2023

View reviewed changes

pkg/expansion/system.go Outdated Show resolved Hide resolved

acpana added 5 commits August 1, 2023 19:08

fix: if not found, dont set ns

c678c48

Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>

refactor: propagate mut err on match

258f335

Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>

test mut match err in exp

a506270

Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>

Merge branch 'master' into acpana/fix-ns-exp

762bd24

Signed-off-by: alex <8968914+acpana@users.noreply.github.com>

test: bats match the right substring

9bdc716

Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>

acpana requested a review from maxsmythe August 2, 2023 01:02

acpana changed the title ~~fix: rework ns check~~ fix: rework ns check, refactor: bubble up match err for mut Aug 2, 2023

maxsmythe reviewed Aug 2, 2023

View reviewed changes

test/gator/expand/test.bats Outdated Show resolved Hide resolved

test/gator/test/test.bats Outdated Show resolved Hide resolved

acpana added 2 commits August 2, 2023 17:56

have the matcher print obj name

698a5cf

Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>

review: comments, err msg

9145f82

Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>

acpana requested review from maxsmythe and JaydipGabani August 2, 2023 17:58

acpana added this to the v3.13.0 milestone Aug 3, 2023

Merge branch 'master' into acpana/fix-ns-exp

8b93fef

davis-haba approved these changes Aug 3, 2023

View reviewed changes

pkg/expansion/system.go Show resolved Hide resolved

test/gator/test/test.bats Outdated Show resolved Hide resolved

maxsmythe approved these changes Aug 4, 2023

View reviewed changes

maxsmythe requested a review from ritazh August 4, 2023 19:59

ritazh reviewed Aug 4, 2023

View reviewed changes

pkg/expansion/system.go Outdated Show resolved Hide resolved

ritazh reviewed Aug 4, 2023

View reviewed changes

pkg/expansion/system_test.go Show resolved Hide resolved

ritazh reviewed Aug 4, 2023

View reviewed changes

pkg/expansion/system_test.go Outdated Show resolved Hide resolved

ritazh reviewed Aug 4, 2023

View reviewed changes

pkg/expansion/system_test.go Show resolved Hide resolved

Update pkg/expansion/system.go

4286321

Co-authored-by: Rita Zhang <rita.z.zhang@gmail.com> Signed-off-by: alex <8968914+acpana@users.noreply.github.com>

acpana commented Aug 7, 2023

View reviewed changes

pkg/expansion/system_test.go Outdated Show resolved Hide resolved

acpana commented Aug 7, 2023

View reviewed changes

pkg/expansion/system_test.go Outdated Show resolved Hide resolved

acpana added 2 commits August 7, 2023 10:34

Apply suggestions from code review

343646f

Signed-off-by: alex <8968914+acpana@users.noreply.github.com>

Merge branch 'master' into acpana/fix-ns-exp

be4e194

acpana requested a review from ritazh August 7, 2023 17:35

acpana added 3 commits August 7, 2023 22:20

test: update ns selector test

9e71853

Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>

Merge branch 'master' into acpana/fix-ns-exp

9572968

review: rename test namespace

ba1d4a0

Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>

ritazh approved these changes Aug 8, 2023

View reviewed changes

acpana merged commit 5c49294 into open-policy-agent:master Aug 8, 2023
15 checks passed

acpana mentioned this pull request Aug 9, 2023

docs, gator: include example of using expansion with custom namespace #2936

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: rework ns check, refactor: bubble up match err for mut #2812

fix: rework ns check, refactor: bubble up match err for mut #2812

acpana commented Jun 3, 2023 •

edited

codecov-commenter commented Jun 3, 2023 •

edited

maxsmythe left a comment

acpana commented Aug 2, 2023

davis-haba commented Aug 3, 2023

maxsmythe left a comment

acpana commented Aug 4, 2023

ritazh left a comment

fix: rework ns check, refactor: bubble up match err for mut #2812

fix: rework ns check, refactor: bubble up match err for mut #2812

Conversation

acpana commented Jun 3, 2023 • edited

codecov-commenter commented Jun 3, 2023 • edited

Codecov Report

maxsmythe left a comment

Choose a reason for hiding this comment

acpana commented Aug 2, 2023

davis-haba commented Aug 3, 2023

maxsmythe left a comment

Choose a reason for hiding this comment

acpana commented Aug 4, 2023

ritazh left a comment

Choose a reason for hiding this comment

acpana commented Jun 3, 2023 •

edited

codecov-commenter commented Jun 3, 2023 •

edited