This makes validation gen more generally usable outside the confines of
k8s APIs. This does not eliminate 100% of direct calls but fixes all
the generated code.
We don't support this in k8s but it seems that validation-gen can be
used to generate code for other environments, like protobuf, which do
support maps of pointers (to structs).
Gemini helped with this one.
We don't support this in k8s but it seems that validation-gen can be
used to generate code for other environments, like protobuf, which do
support lists of pointers.
Gemini helped with this one.
Get rid of flag `--emit-register-funcs` and use a per-pkg tag.
Set `+k8s:validation-gen-scheme-registry=nil` to disable register
function generation for a pkg.
This relies on `+k8s:subfield` and validation cohorts. The
`k8s:optional` ensures that we don't run the name validation if name is
empty, because core apimachinery will already flag it as Required().
This demonstrates some of the DV value - docs and clients are now (in
theory) able to see what RC's name format is.
Co-Authored-by: Yongrui Lin <yongrlin@outlook.com>
This new function does 1 main thing (changes how name validation works),
and the rest is future-proofing. Specifically this changes the
signature of the name validation function.
Before: Name validation functions return `[]string` and apimachinery
logic wraps those in `field.Invalid()`.
Problem: This makes it hard to incrementally add declarative validation,
which needs access to potential name and generateName errors to set
origins properly.
After: New name validation functions return `field.ErrorList`, which can
be wrapped in trivial functions to do things like "mark as covered by
declarative" in just one place rather than for all types.
The "WithOpts" part of this gives us a new function name which isn't
horrible and is plausibly useful in the future.
Another notable change is that this no longer directly validates the
`generateName` field (but only on the new path -- existing code still
behaves the same). From the code:
```
+ // generateName is not directly validated here. Types can have
+ // different rules for name generation, and the nameFn is for validating
+ // the post-generation data, not the input. In the past we assumed that
+ // name generation was always "append 5 random characters", but that's not
+ // NECESSARILY true. Also, the nameFn should always be considering the max
+ // length of the name, and it doesn't know enough about the name generation
+ // to do that. Also, given a bad generateName, the user will get errors
+ // for both the generateName and name fields. We will focus validation on
+ // the name field, which should give a better UX overall.
```
This also beefs up tests a bit.
Given:
```
// +k8s:subfield(name)=+k8s:optional
// +k8s:subfield(name)=+k8s:format=dns-label
// +k8s:subfield(generateName)=+k8s:optional
// +k8s:subfield(generateName)=+k8s:format=dns-label
metav1.ObjectMeta `json:"metadata,omitempty" protobuf:"bytes,1,opt,name=metadata"`
```
...we get:
```
errs = append(errs,
func(fldPath *field.Path, obj, oldObj *metav1.ObjectMeta) (errs field.ErrorList) {
func() { // cohort name
if e := validate.Subfield(ctx, op, fldPath, obj, oldObj, "name", func(o *metav1.ObjectMeta) *string { return &o.Name }, validate.OptionalValue); len (e) != 0 {
return // do not proceed
}
errs = append(errs, validate.Subfield(ctx, op, fldPath, obj, oldObj, "name", func(o *metav1.ObjectMeta) *string { return &o.Name }, validate.DNSLabel)...)
}()
func() { // cohort generateName
if e := validate.Subfield(ctx, op, fldPath, obj, oldObj, "generateName", func(o *metav1.ObjectMeta) *string { return &o.GenerateName }, validate.OptionalValue); len(e) != 0
{
return // do not proceed
}
errs = append(errs, validate.Subfield(ctx, op, fldPath, obj, oldObj, "generateName", func(o *metav1.ObjectMeta) *string { return &o.GenerateName }, validate.DNSLabel)...)
}()
return
}(fldPath.Child("metadata"), &obj.ObjectMeta, safe.Field(oldObj, func(oldObj *corev1.ReplicationController) *metav1.ObjectMeta { return &oldObj.ObjectMeta }))...)
```
It is worth noting that this only works one level deep:
```
// +k8s:subfield(foo)=+k8s:subfield(bar)=+k8s:optional
// +k8s:subfield(foo)=+k8s:subfield(bar)=+k8s:format=dns-label
Field StructType
```
...will generate one cohort ("foo") with two distinct calls to Subfield,
so the optional will not short-circuit format. If we need deeper
cohorting we need to either make subfield take multiple field names
(e.g. `subfield(foo.bar)` or `subbfield(foo, bar)`) or we need to
accumulate "structure" in addition to validator calls. This is similar
to how we handle multiple `eachVal` tags - each one iterates the list.
On a given field or type, there may be multiple "cohorts" of validation
which need to be processed together. For example, a short-circuit
validation and a non-short-circuit for the same subfield.
The unnamed cohort (aka the default cohort) is first, followed by named
cohorts in order they are created. Named cohorts are emitted as inline
functions, so early-return can be used.
This allows tags like k8s:subfield or k8s:item to generate cohorts
(named after the field's JSON name or the selector string,
respectively).
Subsequent commits will add support to those tags.
Co-Authored-by: Yongrui Lin <yongrlin@outlook.com>
As discussed in a proposed PR, multi-match makes sense when origin is
specified and matched, but otherwise it feels "loose" to multi-match.
And in fact it was hiding bugs in declarative validation tests and real
bugs (in subsequent commits) in implementation.
Now we allow multi-match if and only if:
1) ErrorMatcher.ByOrigin() was specified
2) The origin string is not empty
3) The multiple "got" errors are not exactly identical
This last part is key -- if some change to DV codegens logic which
(errantly) calls the same function twice, we want to know about it.
Given the following:
```
type Struct type {
TypeMeta int
// +k8s:validateFalse="embedded"
OtherStruct
}
```
What should the field-path be for the error?
or:
```
type Struct type {
TypeMeta int
OtherStruct // embedded
}
// +k8s:validateFalse="otherstruct"
type OtherStruct {
I int
}
```
Conclusion: If we find an embedded field, we look for a nil fldPath, and
if so we set it to the type name. Embedded values in root types (e.g.
above `spec`) should not be a things we actually do.
Implementation: Add `safe.Value(*T, func() *T)`
Testing this exposed a bug -- we do this check for both the field AND
the type when the field is a typedef. Now fixed to only emit a
uniqueness check at the site where listmap keys are defined.
The path part of a list-item's fieldName is redundant with the path key
in the map. I don't love parsing paths, but it seems less awful that
overloading Context.Member.
We already have "Member" which is a way to "forward" information from
stem validators (each, subfield, item) to leaf validators. That works
for fields in a struct but not items in a list.
Now we have the list selection criteria.
This simplifies union a bit and gets rid of the need to regex parse the
field path.
KYAML is a strict subset of YAML, which is sort of halfway between YAML
and JSON. It has the following properties:
* Does not depend on whitespace (easier to text-patch and template).
* Always quotes value strings (no ambiguity aroud things like "no").
* Allows quoted keys, but does not require them, and only quotes them if
they are not obviously safe (e.g. "no" would always be quoted).
* Always uses {} for structs and maps (no more obscure errors about
mapping values).
* Always uses [] for lists (no more trying to figure out if a dash
changes the meaning).
* When printing, it includes a header which makes it clear this is YAML
and not ill-formed JSON.
* Allows trailing commas
* Allows comments,
* Tries to economize on vertical space by "cuddling" some kinds of
brackets together.
* Retains comments.
Examples:
A struct:
```yaml
metadata: {
creationTimestamp: "2024-12-11T00:10:11Z",
labels: {
app: "hostnames",
},
name: "hostnames",
namespace: "default",
resourceVersion: "15231643",
uid: "f64dbcba-9c58-40b0-bbe7-70495efb5202",
}
```
A list of primitves:
```yaml
ipFamilies: [
"IPv4",
"IPv6",
]
```
A list of structs:
```yaml
ports: [{
port: 80,
protocol: "TCP",
targetPort: 80,
}, {
port: 443,
protocol: "TCP",
targetPort: 443,
}]
```
A multi-document stream:
```yaml
---
{
foo: "bar",
}
---
{
qux: "zrb",
}
```
Notes:
* For types that define String() - should we prefer that or JSON?
* metav1.Time has a MarshalJSON() and inhereits a String() and they are
different
* Since validation runs on internal types, we still get some GoNames
instead of goNames.
Today, if the value passed is a struct, map, or list, we get Go's vative
rendering which is clunky.
This uses JSON (could be kyaml when that is ready) instead.
I hear it already: "But JSON is slow!". I benchmarked it -- for an
simple int or string field, JSON is only a little slower (~20%) than a
type assertion, but it IS slower, so I left the type assertion in.
Remember that this is only called when an API error has occurred.
The type assertions do not handle typedefs-to{string, int64, etc} so
those will fall back on JSON. Almost all of our errors go thru standard
functions which demand string or int64 anyway, so mostly pointless.
I also benchmarked using reflect to check `CanInt()` and that is almost
exactly as fast as type-switch but handles more cases, so we COULD
switch to that instead, if we wanted. I thought it wasn't worth the
complexity.
JSON is really there to handle composite types.
Given the following:
```
type Struct type {
TypeMeta int
// +k8s:validateFalse="embedded"
OtherStruct
}
```
What should the field-path be for the error?
or:
```
type Struct type {
TypeMeta int
OtherStruct // embedded
}
// +k8s:validateFalse="otherstruct"
type OtherStruct {
I int
}
```
Conclusion: If we find an embedded field, we look for a nil fldPath, and
if so we set it to the type name. Embedded values in root types (e.g.
above `spec`) should not be a things we actually do.
Implementation: Add `safe.Value(*T, func() *T)`
This isn't a significant functional bug in that the StreamDecoder is
still offset to account for it (which is why we never saw it before).
But it's a trap for future use-cases.
Also move one LOC to match a parallel-shaped block above.
Test case:
```
type T1 struct {
T2 T2 `json:"t2"`
T3 T3 `json:"t3"`
}
type T2 struct {
ST1 []T1 `json:"st1"`
}
// +k8s:validateFalse
type T3 struct {
T4 T4 `json:"t4"`
}
type T4 struct {
ST3 []T3 `json:"st3"`
}
```
Today, we do not detect T2 as having validations because T1 was
incomplete because T3 had not been finished. This is hard to resolve in
the current model where we look at `hasValidations()` during discovery.
We don't have the information we need at that point.
This commit changes the approach.
Instead of saying:
```
if hasValidation(slice-element-type) {
save the eachVal(type-function)
}
```
...we ALWAYS track the `eachVal(type-function)`, but in a different
field. Then at emit time we can check `hasValidations` and emit the
code to call the eachVal only if actually needed.
Additionally, `hasValidations` is changed to not cache answers. This was
the source of part of the original problem and, on measuring, the cache
added little performance, so I nixed it. As we add coverage across all
of k/k we might want to bring it back, but now we have good tests.
I converted `hasValidations` back to a method which is only accessible
in emit phase, not discovery. Not more temptation.
Lastly, I added test cases.
Tim Hockin