-
Notifications
You must be signed in to change notification settings - Fork 50
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
Add crds.yaml-based defaulting functions to pkg/apis/templates/... #129
Add crds.yaml-based defaulting functions to pkg/apis/templates/... #129
Conversation
Signed-off-by: juliankatz <[email protected]>
Signed-off-by: juliankatz <[email protected]>
Signed-off-by: juliankatz <[email protected]>
functions Signed-off-by: juliankatz <[email protected]>
Signed-off-by: juliankatz <[email protected]>
Signed-off-by: juliankatz <[email protected]>
Signed-off-by: juliankatz <[email protected]>
Each ConstrainTemplate version (v1alpha1, v1beta1, v1) has a default value for the LegacySchema field. `true` for v1alpha1, v1beta1, and `false` for v1. These defaults are defined in the CRD yaml. For the ConstraintTemplate golang types to have the same functionality to have the same defaulting functionality. We are required to write custom defaulting functions for each defaulted field. While we could do this, we could easily see how this practice could become unsustainable. A forgotten defaulting function would yield unexpected bugs. To remedy this, this PR adds defaulting functions that default based on the content of deploy/crds.yaml, i.e. the ConstraintTemplate CRD generated by controller-gen. This provides us with a single defaulting source of truth: the constrainttemplate_types.go files, which contain annotations that define a field's default values. Fixes open-policy-agent#128 Signed-off-by: juliankatz <[email protected]>
Signed-off-by: juliankatz <[email protected]>
Codecov Report
@@ Coverage Diff @@
## master #129 +/- ##
==========================================
- Coverage 42.59% 42.39% -0.21%
==========================================
Files 35 49 +14
Lines 2775 2951 +176
==========================================
+ Hits 1182 1251 +69
- Misses 1228 1321 +93
- Partials 365 379 +14
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
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 with a couple nits
} | ||
|
||
unCRD := unstructured.Unstructured{} | ||
unCRD.UnmarshalJSON(crdJSON) |
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.
We can't unmarshal directly to constraintTemplateCRD
and save the middle step of converting to unstructured
?
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.
It doesn't look like CustomResourceDefinition
has an Unmarshal function.
apiextensions-apiserver/pkg/apis/apiextensions/v1/marshal.go
does have unmarshalling logic, but it only supports doing so into a JSONSchemaPropsOrArray
.
One idea:
I did the unmarshalling + FromUnstructured()
because it made it easier to dive into the versions:
and populate my ConstraintTemplateSchemas
map. I could probably simplify this logic by diving straight to versions
in the unstructured and unmarshalling those JSON blobs straight into apiextensionsv1.JSONSchemaProps
. I'd then convert to apiextensions.JSONSchemaProps
(apiextensions has no unmarshalling methods, only the versioned sub-packages) and create the Structural
type for the map.
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 tried this^ and it's more trouble than it's worth. B/c I use multiple properties of a version
in my loop, it pays to convert to a CustomResourceDefinition{}
.
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.
Actually, digging a bit deeper, it looks like we have these defaulting functions (which is good), but they're not being applied anywhere.
This means issues like open-policy-agent/gatekeeper#1458 will still occur.
Signed-off-by: juliankatz <[email protected]>
Signed-off-by: juliankatz <[email protected]>
Signed-off-by: juliankatz <[email protected]>
Signed-off-by: juliankatz <[email protected]>
Signed-off-by: juliankatz <[email protected]>
Signed-off-by: juliankatz <[email protected]>
Signed-off-by: juliankatz <[email protected]>
Signed-off-by: juliankatz <[email protected]>
Signed-off-by: juliankatz <[email protected]>
Signed-off-by: juliankatz <[email protected]>
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.
Looking good!
I think the last piece would be a "conversion" helper function that knows to apply defaults before converting. This could be leveraged by gator test
Is there an existing place where we'll use this function? That would help me know I'm making the right thing. |
Signed-off-by: juliankatz <[email protected]>
… crd-yaml-based-defaulting Signed-off-by: juliankatz <[email protected]>
Signed-off-by: juliankatz <[email protected]>
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, thanks for figuring this out!
Though it looks like there are linting errors, and we need to re-bake the CRD yaml into the golang constant |
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, missed some things
Signed-off-by: juliankatz <[email protected]>
Signed-off-by: juliankatz <[email protected]>
Signed-off-by: juliankatz <[email protected]>
Signed-off-by: juliankatz <[email protected]>
11b763b
to
73dcbcb
Compare
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
Signed-off-by: juliankatz <[email protected]>
239a1e9
to
7e84176
Compare
@@ -0,0 +1,6 @@ | |||
package templates | |||
|
|||
func init() { |
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.
It is valid to have multiple init() functions in the same Go package. You can rename both initializeScheme
and initializeCTSchemaMap
to init
and it will work.
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.
The problem wasn't the inability to have multiple init()
calls, but rather that initializeCTSchemaMap()
uses the Scheme
that's created in initializeScheme()
. As init functions are run according to the order of their containing file's name (crd_schema.go
before scheme.go
), this caused a null-pointer exception.
Rather than fix this through naming, I figured the best way to was to call them in an intentional order like I did in this init.go
file. I'll add a comment to init.go
explaining that choice.
Or, perhaps there's a pattern that would better suit this use-case? @willbeason
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.
Is there a reason the contents of those two methods can't be in the same function? initializeScheme
is rather short.
Signed-off-by: juliankatz <[email protected]>
Signed-off-by: juliankatz <[email protected]>
Signed-off-by: juliankatz <[email protected]>
c2c4af0
to
65f059b
Compare
… crd-yaml-based-defaulting Signed-off-by: juliankatz <[email protected]>
Each ConstrainTemplate version (v1alpha1, v1beta1, v1) has a default
value for the LegacySchema field.
true
for v1alpha1, v1beta1, andfalse
for v1. These defaults are defined in the CRD yaml.For the ConstraintTemplate golang types to have the same functionality
to have the same defaulting functionality. We are required to write
custom defaulting functions for each defaulted field.
While we could do this, we could easily see how this practice could
become unsustainable. A forgotten defaulting function would yield
unexpected bugs.
To remedy this, this PR adds defaulting functions that default based on
the content of deploy/crds.yaml, i.e. the ConstraintTemplate CRD
generated by controller-gen.
This provides us with a single defaulting source of truth: the
constrainttemplate_types.go files, which contain annotations that
define a field's default values.
Fixes #128
Signed-off-by: juliankatz [email protected]