I noticed when validating a custom resource using the apiextensions NewSchemaValidator (which is using the SchemaValidator in this repo) that when a validation error occurs within a slice, the index of the slice is omitted from the fieldpath returned in the error.
YAML snippet:
spec:
dependsOn:
- provider: registry.upbound.io/crossplane/provider-aws
version: 1
Output:
spec.dependsOn.version in body must be of type string: "integer"
I looked into why this is happening by traversing through the recursive validation that is performed against the schema. When we get to a the sliceValidator for spec.dependsOn, it's Applies() call returns true, causing it to be run.
Looking good so far. When the validation actually runs, we get the length for the slice and construct a new SchemaValidator that we will reuse to validate each item (just one item in this case).
When we first construct this nested SchemaValidator we set the path to match the parent validator (spec.dependsOn). This is not accurate though and needs to be updated on each iteration, so we call SetPath(). After setting the path, we can see that the validator has the proper index appended (spec.dependsOn.0).
Still looking good (though the format is a bit strange, more on this below). Unfortunately, the initial call to NewSchemaValidator() is when each of its child validators are instantiated (e.g. formatValidator, objectValidator, etc.), and the subsequent call to SetPath() updates the SchemaValidator path, but none of the child validator paths (so now the SchemaValidator has path spec.dependsOn.0, but all child validators have path spec.dependsOn). This can be observed when we iterate through each of the child validators and compare their path with the SchemaValidator:
Unfortunately this means that if an error occurs anywhere in any descendant validator, the index of the slice is omitted from the field path (as seen at beginning of the ticket). I have also verified that this is happening in the API server validation (that consumes this repo):
Note: this example is using a different resource.
$ kubectl apply -f definition.yaml --validate=false
The CompositeResourceDefinition "compositepostgresqlinstances.database.example.org" is invalid: spec.versions.referenceable: Invalid value: "string": spec.versions.referenceable in body must be of type boolean: "string"
I imagine most users are not catching this because the client-side validation provides a more accurate error:
$ kubectl apply -f definition.yaml
error: error validating "definition.yaml": error validating data: ValidationError(CompositeResourceDefinition.spec.versions[0].referenceable): invalid type for io.crossplane.apiextensions.v1.CompositeResourceDefinition.spec.versions.referenceable: got "string", expected "boolean"; if you choose to ignore these errors, turn validation off with --validate=false
Proposal
I would like to propose two changes, and I am happy to implement them:
- Set path for child validators in
SchemaValidator SetPath()
The simplest fix is to set the path for all child validators when the path is set on SchemaValidator. Though I believe there could be some more elegant handling of paths while performing this traversal, this does address the issue and is a lightweight change to fix this in the immediate term.
// SetPath sets the path for this schema validator
func (s *SchemaValidator) SetPath(path string) {
s.Path = path
for _, v := range s.validators {
v.SetPath(path)
}
}
I have verified that this works correctly:
spec.dependsOn.0.version in body must be of type string: "integer"
- Use JSONPath syntax for indices
To be more aligned with client-side validation, and for a more generally accepted format, it would be great to represent that index of a slice as dependsOn[0] instead of dependsOn.0.
If both of these fixes sound reasonable to folks, I'll go ahead and open PRs to address.
/assign
/bug
React
Typescript
Django
Laravel
Facebook
Microsoft
Google
Alibaba
D3
Tencent