Add support for validating XRD's openAPIV3Schema #145
Conversation
| github.com/docker/distribution => github.com/docker/distribution v0.0.0-20191216044856-a8371794149d | ||
| github.com/docker/docker => github.com/moby/moby v17.12.0-ce-rc1.0.20200618181300-9dc6525e6118+incompatible | ||
| github.com/golang/tools => ./internal/vendor/golang.org/x/tools | ||
| github.com/googleapis/gnostic => github.com/google/gnostic v0.5.5 |
There was a problem hiding this comment.
See note about needing to go to 1.17. This change was rooted in updating k8s.io/apiextensions-apiserver to v0.23.3.
- adds initial support for validating the openAPIV3Schema definition in an XRD using CRD's validation mechanisms - adds functionality to warn the end user if a validator cannot be found for a GVK - updates the required version of Go to 1.17 in order to resolve some dependencies in googleapis Signed-off-by: Taylor Thornton <thornton.tn@gmail.com>
tnthornton
force-pushed
the
xrd-openapi
branch
from
55f454d
to
746d9bd
Compare
There was a problem hiding this comment.
@tnthornton awesome to see this! A few nitpicks and questions, but mostly looks ready to go to me!
internal/xpkg/snapshot/validators.go
Outdated
| @@ -157,6 +161,15 @@ func validatorsFromV1Beta1XRD(x *xpextv1beta1.CompositeResourceDefinition, acc m | |||
| } | |||
|
|
|||
| func validatorsFromV1XRD(x *xpextv1.CompositeResourceDefinition, acc map[schema.GroupVersionKind]*validator.ObjectValidator) error { | |||
| errs := validateOpenAPIV3Schema(x) | |||
| if errs != nil { | |||
There was a problem hiding this comment.
| if errs != nil { | |
| if len(errs) != 0 { |
Current implementation panics on zero-length slice.
internal/xpkg/snapshot/xrd.go
Outdated
|
|
||
| // paths are return from CRD validations using map[key].field notation | ||
| func replaceMapKeys(fieldVal string) string { | ||
| sampleRegexp := regexp.MustCompile(`(\[([a-zA-Z]*)\])`) |
There was a problem hiding this comment.
I believe we can avoid calling this on every invocation of replaceMapKeys (i.e. compile once then use it where necessary)
internal/xpkg/snapshot/xrd.go
Outdated
|
|
||
| type cleaner func(string) string | ||
|
|
||
| // if the validations were all move to spec.validation, update the path |
There was a problem hiding this comment.
| // if the validations were all move to spec.validation, update the path | |
| // if the validations were all moved to spec.validation, update the path |
internal/xpkg/snapshot/xrd.go
Outdated
| return strings.Replace(fieldVal, "spec.validation", "spec.versions[0].schema", 1) | ||
| } | ||
|
|
||
| // paths are return from CRD validations using map[key].field notation |
There was a problem hiding this comment.
| // paths are return from CRD validations using map[key].field notation | |
| // paths are returned from CRD validations using map[key].field notation |
| } | ||
|
|
||
| func trimInvalidDollarSign(fieldVal string) string { | ||
| if idx := strings.Index(fieldVal, ".$"); idx != -1 { |
There was a problem hiding this comment.
Do we always expect .$ leading in the path if present? If so, I believe this is functionally equivalent to TrimPrefix, if not it looks like we are cutting off everything before the .$, which I believe would truncate our field path
There was a problem hiding this comment.
This should being doing the opposite of TrimPrefix. This handles the case where we have a path like so:
spec.versions[0].schema.openAPIV3Schema.$ref"
This function returns:
spec.versions[0].schema.openAPIV3Schema
The reason for that is the first string is not a valid yaml path. So I do what I can to get the error close to the problem point and highlight that they are using an invalid field.
FWIW, I also tried TrimSuffix, however the key with a .$ prefix is not guaranteed to be $ref. The end user can type whatever they want, which is why I ultimately went with the above approach.
There was a problem hiding this comment.
Ah! My apologies, I thought I saw fieldVal[idx:], which I would have caught if I looked closer at your unit test!
fix typos in doc strings Signed-off-by: Taylor Thornton <thornton.tn@gmail.com>
| } | ||
|
|
||
| func trimInvalidDollarSign(fieldVal string) string { | ||
| if idx := strings.Index(fieldVal, ".$"); idx != -1 { |
There was a problem hiding this comment.
Ah! My apologies, I thought I saw fieldVal[idx:], which I would have caught if I looked closer at your unit test!
Description of your changes
This change set adds the following new features to
xpls:In addition, the go mod minimum version requirement was bumped in order to account for repo name changes in github.com/googleapis/gnostic that transitive dependencies weren't able to handle very well.
Fixes #https://github.com/upbound/up-ls/issues/59
I have:
make reviewableto ensure this PR is ready for review.How has this code been tested
Beyond unit tests, general functional tests were performed to verify we saw the expected warns and errors. You can see in the screenshots below what that looks like:
openAPIV3Schemavalidation:warn end user if GVK is missing due to XRD's definition being broken: