-
Notifications
You must be signed in to change notification settings - Fork 49
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: Add ValidatingAdmissionPolicy generation function #376
feat: Add ValidatingAdmissionPolicy generation function #376
Conversation
Signed-off-by: Max Smythe <smythe@google.com>
b410a0f
to
5e8e843
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #376 +/- ##
==========================================
+ Coverage 54.06% 54.38% +0.31%
==========================================
Files 67 71 +4
Lines 4796 5176 +380
==========================================
+ Hits 2593 2815 +222
- Misses 1923 2064 +141
- Partials 280 297 +17
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: Max Smythe <smythe@google.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
excited to see this happening 🎉
left some minor, non blocking comments
|
||
func NewWrapper(req *admissionv1.AdmissionRequest) (*RequestWrapper, error) { | ||
var object runtime.Object | ||
if len(req.Object.Raw) != 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we return error early for all the len == 0 cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
len == 0 is acceptable in some cases (e.g. object
on delete, oldObject
on create)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(in these cases, expected value is nil)
Expression string | ||
Message string | ||
MessageExpression string | ||
// A CEL expression. Maps to ValidationAdmissionPolicy's spec.validations |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this comment is meant for the Validation
type so might be good to move it before the type definition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Putting the comment here is what would generate the docstring for Open API schema.
For example, this field:
Turns into this schema:
I was aiming for consistency, but happy to move things if you like. LMK what you prefer.
Expression string `json:"expression"` | ||
} | ||
|
||
type Variable struct { | ||
// A CEL variable definition. Maps to ValidationAdmissionPolicy's spec.variables |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same, move comment before the type
Signed-off-by: Max Smythe <smythe@google.com>
fa9c08d
to
c171f27
Compare
Name = "K8sNativeValidation" | ||
// ReservedPrefix signifies a prefix that no user-defined value (variable, matcher, etc.) is allowed to have. | ||
// This gives us the ability to add new variables in the future without worrying about breaking pre-existing templates. | ||
ReservedPrefix = "g8r_internal_" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Should we rename this to gatekeeper_...
? Users/contributors might not know what g8r means. That made me think maybe we should rename "K8s" to "Kubernetes" too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBH, I'm mostly concerned about brevity/uniqueness. Since this is more about reserving a namespace for us, ideally users never collide with the naming and therefore never need to know the details.
I'm okay with either, but would rather not rename things more than one more time in this PR.
@ritazh preferred name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm ok with either as well. +1 on "gatekeeper_" since g8r is less well known.
return nil, err | ||
} | ||
|
||
matchConditions, err := source.GetV1Alpha1MatchConditions() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we only convert to the alpha one? backward compat? when should we consider beta and v1?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say we'd switch once its possible to generate v1beta1 (or greater) VAP resources.
If we wanted to avoid future breaking changes, I could rename this to TemplateToV1Alpha1PolicyDefinition()
if you prefer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm ok keeping it as is.
name: "Simple match", | ||
matcher: nsMatcher{"somename"}, | ||
namespace: ptr.To[string]("somename"), | ||
shouldMatch: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is shouldMatch false for this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
b/c its testing excluded namespaces
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, for some reason my comments were listed as "pending review", so some of these are older responses (but probably still relevant)
Expression string | ||
Message string | ||
MessageExpression string | ||
// A CEL expression. Maps to ValidationAdmissionPolicy's spec.validations |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Putting the comment here is what would generate the docstring for Open API schema.
For example, this field:
Turns into this schema:
I was aiming for consistency, but happy to move things if you like. LMK what you prefer.
|
||
func NewWrapper(req *admissionv1.AdmissionRequest) (*RequestWrapper, error) { | ||
var object runtime.Object | ||
if len(req.Object.Raw) != 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
len == 0 is acceptable in some cases (e.g. object
on delete, oldObject
on create)
|
||
func NewWrapper(req *admissionv1.AdmissionRequest) (*RequestWrapper, error) { | ||
var object runtime.Object | ||
if len(req.Object.Raw) != 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(in these cases, expected value is nil)
Name = "K8sNativeValidation" | ||
// ReservedPrefix signifies a prefix that no user-defined value (variable, matcher, etc.) is allowed to have. | ||
// This gives us the ability to add new variables in the future without worrying about breaking pre-existing templates. | ||
ReservedPrefix = "g8r_internal_" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBH, I'm mostly concerned about brevity/uniqueness. Since this is more about reserving a namespace for us, ideally users never collide with the naming and therefore never need to know the details.
I'm okay with either, but would rather not rename things more than one more time in this PR.
@ritazh preferred name?
return nil, err | ||
} | ||
|
||
matchConditions, err := source.GetV1Alpha1MatchConditions() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say we'd switch once its possible to generate v1beta1 (or greater) VAP resources.
If we wanted to avoid future breaking changes, I could rename this to TemplateToV1Alpha1PolicyDefinition()
if you prefer?
name: "Simple match", | ||
matcher: nsMatcher{"somename"}, | ||
namespace: ptr.To[string]("somename"), | ||
shouldMatch: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
b/c its testing excluded namespaces
Signed-off-by: Max Smythe <smythe@google.com>
Signed-off-by: Max Smythe <smythe@google.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Excited to integrate this!
Adds the ability to generate a VAP policy definition from a constraint template using the K8sNative engine.
This is an incremental step towards implementing the Gatekeeper-to-VAP User Flow