-
Notifications
You must be signed in to change notification settings - Fork 51
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
Inject empty parameters
field for legacy schemas
#130
Inject empty parameters
field for legacy schemas
#130
Conversation
Codecov Report
@@ Coverage Diff @@
## master #130 +/- ##
==========================================
+ Coverage 42.19% 42.59% +0.40%
==========================================
Files 35 35
Lines 2766 2775 +9
==========================================
+ Hits 1167 1182 +15
+ Misses 1234 1228 -6
Partials 365 365
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
1832705
to
55cb826
Compare
To handle legacy (pre-v1) CRDs, we inject x-kubernetes-preserve-unknown-fields in order to preserve the legacy behavior that unknown fields were not pruned by the API server. Unfortunately, we are not handling the case where no parameters are provided, which currently means that parameters will be stripped from users' CRs. This fixes that. Fixes: open-policy-agent/gatekeeper#1468 Signed-off-by: Max Smythe <smythe@google.com>
55cb826
to
53c3ea9
Compare
@@ -34,7 +34,8 @@ type CRD struct { | |||
} | |||
|
|||
type CRDSpec struct { | |||
Names Names `json:"names,omitempty"` | |||
Names Names `json:"names,omitempty"` | |||
// +kubebuilder:default={legacySchema: 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.
This strikes me as the case of Validation
being a nil
pointer, which then should be defaulted to:
{ legacySchema: false }
But that seems different than the test cases that were added in constrainttemplate_types_test.go
, which are about Validation.OpenAPIV3Schema
being nil
.
Can you elaborate?
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.
If validation
is nil, then K8s defaulting doesn't go further, which means the default value for legacySchema
is never applied.
The test validates that legacySchema
behaves properly when openAPIV3Schema
is nil. The extra defaulting makes it so that that logic actually gets invoked.
inSchema := in.OpenAPIV3Schema | ||
// legacySchema should allow for users to provide arbitrary parameters, regardless of whether the user specified them | ||
if in.LegacySchema && inSchema == nil { | ||
inSchema = &apiextensionsv1.JSONSchemaProps{} |
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.
This strikes me as a defaulting action, rather than a conversion action. I think we'd be better off linking those two things together somehow.
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.
This is a conversion action.
In the case where legacySchema == false
, the schema remains nil and the expectation is that no parameters are provided.
When legacySchema == true
, users could provide any value to parameters and it would be accepted, this preserves that.
Essentially we are unpacking the implicit x-preserves
embedded in legacySchema
, no new intent is being injected (which defaulting would do)
@@ -147,6 +141,8 @@ spec: | |||
type: array | |||
type: object | |||
validation: | |||
default: | |||
legacySchema: true |
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.
Thanks for working on this PR @maxsmythe! With this change, users would still need to update their CRDs right? (ie. we need to force update the GK CT CRD via helm)
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.
You're welcome! :)
Users still need to update their CRDs. In general, it seems dangerous to allow CRDs to go stale.
I talk a bit more about the CRD thing in Oren's issue:
open-policy-agent/gatekeeper#1458
Basically saying that we could paper over having an old CRD, but that would eventually break anyway once we moved to defaulting to v1 CRDs.
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.
Also, if we do want to paper over having an out-of-date CRD manifest, we'd have to do that in the Gatekeeper repo.
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
To handle legacy (pre-v1) CRDs, we inject
x-kubernetes-preserve-unknown-fields
in order to preserve the legacy behavior
that unknown fields were not pruned by the API
server.
Unfortunately, we are not handling the case
where no parameters are provided, which currently
means that parameters will be stripped from users' CRs.
This fixes that.
Fixes: open-policy-agent/gatekeeper#1468
Signed-off-by: Max Smythe smythe@google.com