This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

Open questions:

• For types that define String() - should we prefer that or JSON? I chose JSON, but it changes some results. If we choose String() then do we put explicit quotes or leave it "naked"? One place (logs) renders int-type values as hex because of the default %#v, but other places like time render things that are clearly meant to be a string. We would not want to add MarshalJSON() to logs, since hex is not JSON. We could add ANOTHER optional method like MarshalLog() or MarshalErrorValue ? @pohly

• metav1.Time has a MarshalJSON() and inherits a String() (from embedded time.Time) and they are different - should we make them the same? @deads2k

• Since validation runs on internal types, we still get some GoNames instead of goNames, but this was true before.

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: thockin

The full list of commands accepted by this bot can be found here.

The pull request process is described here

(triage):
looks good from sig-auth side (credentialplugin changes)

For types that define String() - should we prefer that or JSON? I chose JSON, but it changes some results.

At first glance, this seems like a situation where a user-visible representation of a value is needed, which is what String is supposed to provide. Was JSON chosen because some of our values have String implementations which are fairly unreadable (the protobuf generated String implementations come to mind) or because we want a complete dump of the bad value (similar to how some test frameworks dump the entire error in addition to the "summary string" returned byError)?

If we choose String() then do we put explicit quotes or leave it "naked"?

As we use error-wrapping style (i.e. <prefix>: <details>) one can read from left to right and figure out what the value is without quoting it. I prefer leaving it "naked".

one can read from left to right and figure out what the value is without quoting it

Except that there is more text after the value:

path.to.field: Invalid value: "the value": the details

That invalidates my argument and quoting becomes necessary.

Or can we shuffle things around?

path.to.field: Invalid value, the details: the value

Apropos error rendering: should or shouldn't validation tests check for expected errors by comparing against full strings, check for more or less complete sub-strings, or against errors produced by calling the same error method as in the validation code?

For DRA, I chose the latter because I didn't want the test to depend on the implementation of those methods. As seen in this PR, some other validation tests use strings which then need to be updated when changing the implementation. Strings have the advantage that one can check the readability of the user-visible error message and more easily spot when the wrong method is used when the result makes no sense.

The Genesis of this PR (other than being a long-annoying thing) is declarative validation. We added support to auto-check listmap types for duplicates and throw errors. Testing those exposed the fact that we produce a "duplicate value" error, where the value is (for example) a whole container. It's clearly NOT a duplicate value, but the key is buried in there.

I thought "perhaps we can return a map[string]any with just the key field(s) set, but that still gets rendered with Go's "%#v", which is not super helpful. But if I run that through JSON (or KYAML) it is nicer.

As for readability, I don't think we intend the final error to be machine parseable or splittable, but I am eager to make it more useful to humans.

E.g. is something like "Invalid value ("the value"): the details" better? I think the quotes are useful to distinguish "true" from true (as in labels) or 4 from "4" (as in resources).

As for unit tests, I prefer they operate with the new Matcher logic, so they are less dependent on exact strings. For example, I want to make the error message better for dns-label, and it breaks hundreds of tests. This is why we are adding "origin".

where the value is (for example) a whole container

So that's exactly the case where ignoring the fmt.Stringer implementation in favor of some nicer rendering makes sense. I sometimes wish we wouldn't need those fmt.Stringer implementations (but protobuf needs them) or nicer output (let's replace with KYAM?!), but for now preferring JSON as proposed in this PR makes sense.

One can also argue that the API errors are meant to provide a data dump of the values, not just a user visible rendering, because one may have to inspect the entire value.

I don't think we intend the final error to be machine parseable or splittable

Agreed, that's why I thought it would be okay to not use quoting. That can make strings less readable and works fine as long as humans can "spot" where the value starts.

is something like "Invalid value ("the value"): the details" better

If we keep quoting the value, then path.to.field: Invalid value: "the value": the details is fine.

I prefer they operate with the new Matcher logic, so they are less dependent on exact strings

So produce expected errors and compare against the actual errors, which is what DRA does - except that it doesn't use the ErrorMatcher helper yet. Let me look into changing that now...

The advantage of matcher is that you can decide which criteria to match, often the field path + error type + detail substring is OK, but as we add more origin, the details string matters less and becomes the opposite of useful.

I want my error matching to be pretty complete, but the "origin instead of detail string" is nice. I converted pkg/apis/resource/validation, which included making some changes elsewhere - see #132577

AFAIK this is OK to review now.