Add type for matching Constraints #193
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 5 commits
January 27, 2022 10:25
Targets sometimes need to maintain state about the system. For example, the Gatekeeper target needs to track the set of current Namespaces on the cluster in order to properly match objects to Constraints when Audit is called. This commit adds a Cache interface which Targets may choose to implement. If they implement this interface, Client attempts to add and remove objects from the Target cache just as it does for Driver caches. These operations are not atomic, so it is possible for systems to get into an inconsistent state. There isn't a good solution to this now - I've opened open-policy-agent#189 to solve this in the future. The implications are quite complex and there's a lot of edge cases. This commit also modifies the test target handler matchers - they now access the test target's cache in order to function. These matchers aren't called yet - we don't want to break Gatekeeper since Gatekeeper Golang matchers are not yet implemented. Signed-off-by: Will Beason <[email protected]>
Signed-off-by: Will Beason <[email protected]>
Otherwise it is easy to get into inconsistent cache states. There's lots of edge cases that can cause unpredictable behaviors that we don't want to allow. Signed-off-by: Will Beason <[email protected]>
Since adding data can fail in the target cache, remove data from the driver cache. Note that addition/deletion occur in opposite orders for AddData and RemoveData - this is because we want to prioritize reversible over potentially-irreversible operations. Removing data from the handler cache can't fail, so it is safe to add it first. Signed-off-by: Will Beason <[email protected]>
Otherwise we can easily end up in very annoying inconsistent states. If deleteion really, really needs to fail then the application should panic rather than allow things to get in an inconsistent state. Per discussion Signed-off-by: Will Beason <[email protected]>
|
My reason for the current implementation as maps-of-maps-of-maps is that this messiness is contained to the package - the type is fully contained in the |
willbeason
force-pushed
the
constraint-matcher
branch
from
9ac0fd3 to
1cd8587
Compare
|
Fixes #176 |
maxsmythe
reviewed
Feb 2, 2022
willbeason
force-pushed
the
constraint-matcher
branch
from
1cd8587 to
b022569
Compare
Codecov Report
@@ Coverage Diff @@
## master #193 +/- ##
==========================================
+ Coverage 47.88% 48.90% +1.01%
==========================================
Files 59 60 +1
Lines 2865 2926 +61
==========================================
+ Hits 1372 1431 +59
- Misses 1243 1244 +1
- Partials 250 251 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
maxsmythe
reviewed
Feb 4, 2022
maxsmythe
reviewed
Feb 8, 2022
added 5 commits
February 8, 2022 07:20
Prep work for compiler sharding. This adds a store of the matchers corresponding to Constraints by-target and by-kind. We could do constriant matching per-target, but it's better for us to fail fast rather than partially review objects when we're just going to return errors. Thus, ConstraintsFor iterates over all targets itself rather than requiring its caller to. Signed-off-by: Will Beason <[email protected]>
Each Handler potentially has its own review type to be matched against, so this makes requests for matching happen per-target. Signed-off-by: Will Beason <[email protected]>
Signed-off-by: Will Beason <[email protected]>
Signed-off-by: Will Beason <[email protected]>
Signed-off-by: Will Beason <[email protected]>
willbeason
force-pushed
the
constraint-matcher
branch
from
0020fa6 to
544a387
Compare
added 2 commits
February 8, 2022 07:32
Signed-off-by: Will Beason <[email protected]>
Signed-off-by: Will Beason <[email protected]>
added 2 commits
February 10, 2022 10:36
Signed-off-by: Will Beason <[email protected]>
Signed-off-by: Will Beason <[email protected]>
|
Fixes #176 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Prep work for compiler sharding. This adds a store of the matchers
corresponding to Constraints by-target and by-kind.
We could do constraint matching per-target, but it's better for us to
fail fast rather than partially review objects when we're just going to
return errors. Thus, ConstraintsFor iterates over all targets itself
rather than requiring its caller to.
The triple-nested maps are ugly - I would not need much convincing
to replace them with types.
Signed-off-by: Will Beason [email protected]