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: cache namespaces in targethandler #1908

Merged
merged 31 commits into from
Mar 16, 2022

Conversation

davis-haba
Copy link
Contributor

@davis-haba davis-haba commented Mar 10, 2022

Signed-off-by: davis-haba davishaba@google.com

What this PR does / why we need it:

This introduces functionality to cache namespaces in the TargetHandler, which now implements the handler.Cache interface from frameworks.

Which issue(s) this PR fixes (optional, using fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when the PR gets merged):
Fixes #1849

Special notes for your reviewer:

@davis-haba davis-haba self-assigned this Mar 10, 2022
@davis-haba davis-haba changed the title Cache namespaces in targethandler feat: cache namespaces in targethandler Mar 10, 2022
@codecov-commenter
Copy link

codecov-commenter commented Mar 10, 2022

Codecov Report

Merging #1908 (650b65a) into master (80d9993) will increase coverage by 0.11%.
The diff coverage is 94.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1908      +/-   ##
==========================================
+ Coverage   52.13%   52.24%   +0.11%     
==========================================
  Files         100      100              
  Lines        8958     8992      +34     
==========================================
+ Hits         4670     4698      +28     
- Misses       3912     3916       +4     
- Partials      376      378       +2     
Flag Coverage Δ
unittests 52.24% <94.00%> (+0.11%) ⬆️

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

Impacted Files Coverage Δ
pkg/target/target.go 73.03% <94.00%> (+2.45%) ⬆️
...onstrainttemplate/constrainttemplate_controller.go 57.21% <0.00%> (-1.45%) ⬇️

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 80d9993...650b65a. Read the comment docs.

@davis-haba
Copy link
Contributor Author

@willbeason PTAL

@davis-haba davis-haba requested a review from willbeason March 10, 2022 19:46
@davis-haba davis-haba requested a review from willbeason March 10, 2022 23:46
davis-haba and others added 19 commits March 14, 2022 08:43
Signed-off-by: davis-haba <davishaba@google.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: davis-haba <davishaba@google.com>
Signed-off-by: Avinash Desireddy <avinashr.desireddy@gmail.com>
Signed-off-by: davis-haba <davishaba@google.com>
Signed-off-by: Manuel Rüger <manuel@rueg.eu>

Co-authored-by: Sertaç Özercan <852750+sozercan@users.noreply.github.com>
Signed-off-by: davis-haba <davishaba@google.com>
…website (open-policy-agent#1892)

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: davis-haba <davishaba@google.com>
…ta.17 in /website (open-policy-agent#1893)

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: davis-haba <davishaba@google.com>
…1896)

Co-authored-by: Sertaç Özercan <852750+sozercan@users.noreply.github.com>
Signed-off-by: davis-haba <davishaba@google.com>
…ent#1899)

Signed-off-by: juliankatz <juliankatz@google.com>
Signed-off-by: davis-haba <davishaba@google.com>
…agent#1897)

Co-authored-by: Sertaç Özercan <852750+sozercan@users.noreply.github.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>
This reverts commit 95bab77.

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>
davis-haba and others added 8 commits March 14, 2022 08:43
Signed-off-by: davis-haba <davishaba@google.com>
Signed-off-by: davis-haba <davishaba@google.com>
Signed-off-by: davis-haba <davishaba@google.com>
…e instead of interface. Properly wrap caching errors

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: Zhimin Xiang <zhiminx@google.com>
Signed-off-by: davis-haba <davishaba@google.com>
Signed-off-by: Michael Grosser <michael@grosser.it>

Co-authored-by: Sertaç Özercan <852750+sozercan@users.noreply.github.com>
Co-authored-by: Rita Zhang <rita.z.zhang@gmail.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.

LGTM with 1 nit and 1 clarifying question.

}

func (m *Matcher) Match(review interface{}) (bool, error) {
var gkReq *gkReview
Copy link
Contributor

Choose a reason for hiding this comment

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

@willbeason Will your follow-on PR make it so HandleReview() always returns a gkReview?

Side note... with these changes, we will also be able to drop that Unstable field when passing to Rego, since Rego no longer has need for an injected namespace

Copy link
Member

Choose a reason for hiding this comment

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

On the first note: sure, I'll make sure of that. On the second, it's an optimization we can look at later.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

Signed-off-by: davis-haba <davishaba@google.com>
cache *nsCache
}

type nsCache struct {
Copy link
Member

@sozercan sozercan Mar 15, 2022

Choose a reason for hiding this comment

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

do we want to make this generic and replace

type nsCache struct {

Copy link
Contributor

Choose a reason for hiding this comment

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

What do you mean by "make this generic"? Indicate that we should be able to cache other objects?

If so, we could, but we probably don't want to do that currently since we'd be duplicating OPA's cache. It's something we may want to consider as possible future work if we needed wider access to the cache, though.

Copy link
Member

Choose a reason for hiding this comment

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

I think Sertaç is talking about that we have a different nsCache in audit/manager.go. I had thought about that initially when looking at Davis's PR, but I think that deduplication would be better suited in a different PR. Audit's nsCache has slightly different uses, so changing that in this PR would make this change a bit larger (e.g. we'd have to figure out where to export the type so it's available from both locations). It's work that is easy to have as a follow-up

Copy link
Member

Choose a reason for hiding this comment

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

*follow-up = after merging in the frameworks changes

Copy link
Member

Choose a reason for hiding this comment

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

SGTM as a follow-up

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.

LGTM

@willbeason willbeason merged commit f6e7926 into open-policy-agent:master Mar 16, 2022
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.

Cache Namespaces in k8s Handler cache