POC: use k8s validation-gen#389
Conversation
| ) | ||
|
|
||
| func (x *CreateActorRequest) Validate(ctx context.Context) error { | ||
| op := operation.Operation{Type: operation.Create} |
There was a problem hiding this comment.
I guess for updates we will apply the mutation to the resource (using the field mask) and then validate as a an Update operation?
We don't have an update method with field mask yet, but it sounds like something that would be good to prototype here as well. I'm sure things will come up (e.g. merge / validation semantics for non-scalar values, basically lists and maps).
There was a problem hiding this comment.
Yeah, that's how kube works - update-oriented validators get the "old" and "new" state of the resource, so they can consider the change. There's also an automatic rule that we never re-validate saved data. If it was allowed into the database, it is, by definition, valid. This means a) we save cycles re-validating known-good data and b) a user should never get a validation error for something they didn't change.
| tc := setupTest(t, ns) | ||
| defer tc.cleanup() | ||
|
|
||
| // FIXME: this should move next to the proto files and the validate method |
There was a problem hiding this comment.
I actually like having a test here for validation as it not only asserts that the validator works, but also that the gRPC server (as we whole) is propagating the Go error to the right gRPC status response.
There was a problem hiding this comment.
I ACK the value of complete testing, but also point out that doing rigorous-but-not-brittle testing requires things like something like "origin" which really shouldn't be part of the user-facing API but is fine for internal API.
There was a problem hiding this comment.
How about we move all the exhaustive test cases next to the proto / validate methods, and make it assert on the full (type, field, detail, origin) tuple, but then scope down ValidationError (or the AIP equivalent FieldViolation1) to what we actually want to put on the write (I assume origin, for example, is not meant to be part of the API contract). Then we have a thin layer of tests here just to make sure we properly map validation errors to gRPC status.
There was a problem hiding this comment.
That seems plausible. I added ValidationError because FieldValidation could not carry origin.
| func (x *CreateActorRequest) Validate(ctx context.Context) error { | ||
| op := operation.Operation{Type: operation.Create} | ||
| errs := Validate_CreateActorRequest(ctx, op, nil, x, nil) | ||
|
|
There was a problem hiding this comment.
qq, how do you envision more complex validations (e.g. cross-field) being implemented? Would that be a custom validation referenced from a tag in the field comment or just an extra call from here to the custom validator function?
There was a problem hiding this comment.
Declarative validation supports some cross-field things like one-of or contingent evaluation. It's incomplete, but they need to solve that anyway, so we're in the same boat. Finally there is an escape hatch for "this is weird, so we wrote some code by hand", and there MAY eventually be a "this is weird, here's some CEL"
Tim Hockin (thockin)
force-pushed
the
poc_use_k8s_validation-gen
branch
from
e031e2c to
3505e75
Compare
| if errors.As(err, &statusErr) { | ||
| st := statusErr.GRPCStatus() | ||
| return nil, status.Error(st.Code(), st.Message()) | ||
| //FIXME: investigate why we are doing this error deconstruction |
There was a problem hiding this comment.
Not 100% sure why this is here. It looks like it has the effect of sanitizing internal details from the response by unwrapping the error? But then it could just return the unwrapped error directly and avoid copying details.
Also, if that's the goal, then returning the raw error at the bottom still can leak internals.
Git blame shows Taahir Ahmed (@ahmedtd) as committer, adding him in case there is a Chesterton's Fence issue.
3 participants
This is a little hacky and it depends on some uncommitted changes to k8s tools (kubernetes/kubernetes#140204) , but it works!
Julian Gutierrez Oschmann (@juli4n) given the unreleased changes, this is too early to merge now, but what do you think directionally?
Max Smythe (@maxsmythe) can you speak to internal/ateinterceptors/ateinterceptors.go - we're disassembling an error and then reassembling it, losing the details. This copying works but is kind of yuck...
Joe Betz (@jpbetz) FYI