Skip to content
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 structural information to Constraint Kind CRDs #1249

Merged

Conversation

julianKatz
Copy link
Contributor

@julianKatz julianKatz commented Apr 14, 2021

Kubernetes v1 CRDs require structural schemas. This means that, where
possible, JSONSchemas should identify the type of a field. Fields
without this type information will require the key: value pair of
x-kubernetes-unknown-fields: true to be set. This signals the API
server to save the content found in that field to etcd without
validation.

This PR adds the remaining type information to the MatchSchema()
function, rendering its output structural. This func's output is what
populates the match section of the CRD for a Constraint kind.

It also imports Constraint Framework, including the updates made in
open-policy-agent/frameworks#114. This change transforms the schema
information provided by users in a v1beta1 ConstraintTemplate to be
structural.

Contributes to #550

Signed-off-by: juliankatz juliankatz@google.com

Copy link
Contributor

@maxsmythe maxsmythe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 comment, we'll need to explicitly add matchLabels as part of moving to v1 CRDs because we wont be able to rely on preserveUnknownFields

pkg/target/target.go Show resolved Hide resolved
@sozercan
Copy link
Member

sozercan commented Apr 15, 2021

e2e is failing with

Error from server ([spec.validation.openAPIV3Schema.properties[metadata].type: Invalid value: "": must be object, spec.validation.openAPIV3Schema.properties[metadata].type: Required value: must not be empty for specified object fields, spec.validation.openAPIV3Schema.properties[spec].properties[parameters].properties[labels].items: Required value: must be specified, spec.validation.openAPIV3Schema.properties[spec].properties[parameters].type: Required value: must not be empty for specified object fields, spec.validation.openAPIV3Schema.properties[spec].type: Required value: must not be empty for specified object fields, spec.validation.openAPIV3Schema.type: Required value: must not be empty at the root]): error when creating "test/bats/tests/templates/k8srequiredlabels_template.yaml": admission webhook "validation.gatekeeper.sh" denied the request: [spec.validation.openAPIV3Schema.properties[metadata].type: Invalid value: "": must be object, spec.validation.openAPIV3Schema.properties[metadata].type: Required value: must not be empty for specified object fields, spec.validation.openAPIV3Schema.properties[spec].properties[parameters].properties[labels].items: Required value: must be specified, spec.validation.openAPIV3Schema.properties[spec].properties[parameters].type: Required value: must not be empty for specified object fields, spec.validation.openAPIV3Schema.properties[spec].type: Required value: must not be empty for specified object fields, spec.validation.openAPIV3Schema.type: Required value: must not be empty at the root]

@julianKatz julianKatz force-pushed the 185143185-550-v1-CRD-support branch 3 times, most recently from ceed92a to c9e8dc8 Compare April 16, 2021 19:32
pkg/target/target.go Outdated Show resolved Hide resolved
pkg/target/target.go Outdated Show resolved Hide resolved
@julianKatz
Copy link
Contributor Author

@maxsmythe , I'm still figuring out the earlier e2e test failures. That's why I've got code commented out. I believe the failures were related to this new code. Will confirm now.

@julianKatz julianKatz force-pushed the 185143185-550-v1-CRD-support branch 2 times, most recently from e59c753 to 793ac20 Compare April 19, 2021 18:14
@julianKatz
Copy link
Contributor Author

I've confirmed that enabling the MatchLabels code (at least in its present form) triggers these errors in the e2e tests:

spec.validation.openAPIV3Schema.properties[metadata].type: Invalid value: "": must be object
spec.validation.openAPIV3Schema.properties[metadata].type: Required value: must not be empty for specified object fields
spec.validation.openAPIV3Schema.properties[spec].properties[parameters].properties[labels].items: Required value: must be specified
spec.validation.openAPIV3Schema.properties[spec].properties[parameters].type: Required value: must not be empty for specified object fields
spec.validation.openAPIV3Schema.properties[spec].type: Required value: must not be empty for specified object fields
spec.validation.openAPIV3Schema.type: Required value: must not be empty at the root]): error when creating "test/bats/tests/templates/k8srequiredlabels_template.yaml": admission webhook "validation.gatekeeper.sh" denied the request: [spec.validation.openAPIV3Schema.properties[metadata].type: Invalid value: "": must be object
spec.validation.openAPIV3Schema.properties[metadata].type: Required value: must not be empty for specified object fields
spec.validation.openAPIV3Schema.properties[spec].properties[parameters].properties[labels].items: Required value: must be specified
spec.validation.openAPIV3Schema.properties[spec].properties[parameters].type: Required value: must not be empty for specified object fields
spec.validation.openAPIV3Schema.properties[spec].type: Required value: must not be empty for specified object fields
spec.validation.openAPIV3Schema.type: Required value: must not be empty at the root

@julianKatz
Copy link
Contributor Author

Tests are passing with MatchLabels turned on, but the x-kuberentes-preserve-unknown-fields line commented out. Trying again with it uncommented.

@julianKatz
Copy link
Contributor Author

I've confirmed that adding the x-kubernetes-preserve-unknown-fields: true key/value pair (or at least this line of go: XPreserveUnknownFields: &trueBool) to the matchLabels section triggers the e2e failure described earlier.

@maxsmythe
Copy link
Contributor

Weird that it's validating the entire resource using "v1" rules because of that. Let's put this PR on ice for a bit until the CF returns V1 CRDs, then we can revisit?

@julianKatz
Copy link
Contributor Author

This is blocked until open-policy-agent/frameworks#113 is merged

@julianKatz julianKatz force-pushed the 185143185-550-v1-CRD-support branch from 0fbb91d to 23ef3a3 Compare May 18, 2021 18:01
@julianKatz julianKatz changed the title Add type declarations to g8r MatchSchema() Add structural information to Constraint Kind CRDs May 18, 2021
@julianKatz julianKatz force-pushed the 185143185-550-v1-CRD-support branch from 23ef3a3 to be73c09 Compare May 18, 2021 18:31
@julianKatz julianKatz force-pushed the 185143185-550-v1-CRD-support branch from be73c09 to 5943f44 Compare May 18, 2021 20:25
@@ -347,9 +364,9 @@ func (r *ReconcileConstraintTemplate) Reconcile(ctx context.Context, request rec

proposedCRD := &apiextensionsv1beta1.CustomResourceDefinition{}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to move to apiextensionsv1 here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No reason not to, IMO

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It turns out that doing this upgrade (apiextensionsv1beta1 --> apiextensionsv1) actually causes a number of test breakages. As this PR is already large, I'm going to save those for a follow-up PR.

Does that work for you? @shomron

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are alot more places in GK that are still usingk8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1 so +1 on followup.

@julianKatz julianKatz force-pushed the 185143185-550-v1-CRD-support branch 6 times, most recently from c1b4911 to 6ed35da Compare May 19, 2021 04:33
Copy link
Contributor

@maxsmythe maxsmythe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Kubernetes v1 CRDs require structural schemas.  This means that, where
possible, JSONSchemas should identify the type of a field.  Fields
without this type information will require the key: value pair of
x-kubernetes-unknown-fields: true to be set.  This signals the API
server to save the content found in that field to etcd without
validation.

This PR adds the remaining type information to the MatchSchema()
function, rendering its output structural.  This func's output is what
populates the `match` section of the CRD for a Constraint kind.

It also imports Constraint Framework, including the updates made in
open-policy-agent/frameworks#114.  This change transforms the schema
information provided by users in a v1beta1 ConstraintTemplate to be
structural.

Contributes to open-policy-agent#550

Signed-off-by: juliankatz <juliankatz@google.com>
@julianKatz julianKatz force-pushed the 185143185-550-v1-CRD-support branch from 6ed35da to 2613b85 Compare May 20, 2021 15:45
@julianKatz julianKatz merged commit 92bf487 into open-policy-agent:master May 20, 2021
@julianKatz julianKatz deleted the 185143185-550-v1-CRD-support branch June 2, 2021 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants