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>
UniqueString rendered as {} in JSON/YAML, which isn't useful... Providing a
MarshalJSON which dumps the string is better.
This enables full object dumps in the ResourceSlice tracker. While at it:
- Consistently log the object content at level 6.
- Use past tense in log messages (recommended style).
- Rename "patch" -> "rule".
Some tests are flaking in this function, having a better logged
error will help to understand the cause.
Also modify the Handshake function to always write an http error,
otherwise we have inconsistencies in the codebase that is using the
function and since we are already passing the http writer it makes sense
to not just return an error and delegate the response to the caller.
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)`
