feat: Include oldObject and admission request metadata in expansion #3341
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
This commit updates `pkg/expansion` and `pkg/webhook` to include additional information from the `AdmissionRequest` object when expanding workload resources. The two main pieces of now-propagated information are `OldObject` and `UserInfo`, which allow for policies that apply to updated resources, and that depend on the user / system that is performing the creation/updating of the resource. Signed-off-by: David Symons <[email protected]>
maxsmythe
requested changes
Mar 28, 2024
There was a problem hiding this comment.
Thanks for the PR!
Because this change is super specific to the admission webhook, I think we can restrict our changes to only that file (webhook/policy.go). Here are my rough thoughts:
- rename
createReviewForResultant()createResultantReviews(), refactor to accept a review object and return review objects for the expanded resources - as part of the above refactoring, call expansion twice: once for object and once for oldObject (where non-nil)
- As far as testing goes, we will only need to write unit tests for
createResultantReviews()
| ) | ||
|
|
||
| // Expandable represents an expandable object and its metadata. | ||
| type Expandable struct { |
There was a problem hiding this comment.
Because object, oldObject, etc. only exist for webhook admission (not audit, gator, or any other enforcement point), we probably should not make these part of the interface. We can probably achieve the same result by modifying how the admission webhook invokes expansion.
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.
What this PR does / why we need it: Update the expansion logic to propagate the
oldObjectanduserInfofields from the admission request (along with a few other fields)I've been trying to set up a policy that allows users to edit fields of a specific workload resource (eg. a
Deployment) but prevents them from modifying thePodtemplate within that resource, as that could allow them to alter the workload and lead to security issues (eg. modifying the container image that is executed)Currently, the expansion logic only expands the
objectfield, meaning it's not possibly to detect if a policy is operating on aCREATEor anUPDATErequest, and act accordingly. Additionally, because it doesn't propagate theuserInfofield, it's not possible to make policies that depend on which user made the changeSpecial notes for your reviewer: I've tried to add tests to
pkg/expansion, but wasn't sure how to go about setting up a test inpkg/webhookto confirm all fields from the admission request are propagated as expected