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: recursive expansion #2679

Merged
merged 13 commits into from May 10, 2023

Conversation

davis-haba
Copy link
Contributor

@davis-haba davis-haba commented Apr 5, 2023

Implements recursive expansion.

Example use case: one could configure Cronjobs to expand into a Jobs, which then expands into Pods.

If a cycle is detected which would result in infinite expansion, every template that belongs to that cycle is disabled.

This is to prevent a "first past the post" race condition where different pods may have different templates enabled depending on the order their controllers reconciled the templates.

By disabling all templates belonging to a cycle, we can guarantee that every controller is enforcing the same templates in the steady state (after controllers have finished reconciling all updates).

Fixes: #2511

@davis-haba davis-haba changed the title Expansion recursive feat: recursive expansion Apr 5, 2023
@codecov-commenter
Copy link

codecov-commenter commented Apr 5, 2023

Codecov Report

Patch coverage: 77.18% and project coverage change: +0.28 🎉

Comparison is base (80540bd) 52.79% compared to head (beb4481) 53.07%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2679      +/-   ##
==========================================
+ Coverage   52.79%   53.07%   +0.28%     
==========================================
  Files         123      124       +1     
  Lines       10941    11182     +241     
==========================================
+ Hits         5776     5935     +159     
- Misses       4710     4772      +62     
- Partials      455      475      +20     
Flag Coverage Δ
unittests 53.07% <77.18%> (+0.28%) ⬆️

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

Impacted Files Coverage Δ
pkg/controller/expansion/expansion_controller.go 52.57% <48.83%> (-2.91%) ⬇️
pkg/expansion/system.go 72.04% <79.54%> (-9.91%) ⬇️
pkg/expansion/db.go 83.42% <83.42%> (ø)
pkg/readiness/ready_tracker.go 69.19% <100.00%> (ø)

... and 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

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.

Nice job!

Biggest missing piece is the need to figure out conflicting templates.

pkg/expansion/graph.go Outdated Show resolved Hide resolved
pkg/expansion/system.go Outdated Show resolved Hide resolved
pkg/expansion/system.go Show resolved Hide resolved
Copy link
Contributor

@acpana acpana left a comment

Choose a reason for hiding this comment

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

this is looking great 🥇 ; main question about the graph impl

pkg/controller/expansion/expansion_controller.go Outdated Show resolved Hide resolved
pkg/expansion/graph.go Outdated Show resolved Hide resolved
pkg/expansion/graph.go Outdated Show resolved Hide resolved
pkg/expansion/system.go Show resolved Hide resolved
pkg/readiness/ready_tracker.go Show resolved Hide resolved
pkg/expansion/system_test.go Outdated Show resolved Hide resolved
Signed-off-by: davis-haba <davishaba@google.com>
Signed-off-by: davis-haba <davishaba@google.com>
Signed-off-by: davis-haba <davishaba@google.com>
Signed-off-by: davis-haba <davishaba@google.com>
Signed-off-by: davis-haba <davishaba@google.com>
Signed-off-by: davis-haba <davishaba@google.com>
Signed-off-by: davis-haba <davishaba@google.com>
Signed-off-by: davis-haba <davishaba@google.com>
Signed-off-by: davis-haba <davishaba@google.com>
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.

Code LGTM, haven't looked at tests yet

pkg/controller/expansion/expansion_controller.go Outdated Show resolved Hide resolved
pkg/controller/expansion/expansion_controller.go Outdated Show resolved Hide resolved
pkg/expansion/db.go Show resolved Hide resolved
pkg/expansion/db.go Show resolved Hide resolved
pkg/expansion/system.go Show resolved Hide resolved
@maxsmythe
Copy link
Contributor

LGTM with nits, I mean

Signed-off-by: davis-haba <davishaba@google.com>
Copy link
Contributor

@acpana acpana 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 addressing comments made earlier 🥇 ; I don't have anything else blocking on my end

Signed-off-by: davis-haba <davishaba@google.com>
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, thanks!

@davis-haba
Copy link
Contributor Author

@ritazh @sozercan PTAL

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.

Sorry for the delay. LGTM

@davis-haba davis-haba merged commit bd89540 into open-policy-agent:master May 10, 2023
15 checks passed
anlandu pushed a commit to anlandu/gatekeeper that referenced this pull request May 19, 2023
Implements recursive expansion

Signed-off-by: Anlan Du <adu47249@gmail.com>
@ritazh ritazh added this to the v3.13.0 milestone Jul 13, 2023
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.

GVKs creating GVKs
6 participants