VA: Make performRemoteValidation more generic #7847
Conversation
beautifulentropy
commented
Nov 26, 2024
•
beautifulentropy
commented
- Make performRemoteValidation a more generic function that returns a new remoteResult interface
- Modify the return value of IsCAAValid and PerformValidation to satisfy the remoteResult interface
- Include compile time checks and tests that pass an arbitrary operation
beautifulentropy
force-pushed
the
generic-perform-remote-validation
branch
from
c6d1a3d to
80fa2a3
Compare
beautifulentropy
force-pushed
the
generic-perform-remote-validation
branch
from
80fa2a3 to
b305e5e
Compare
| } else { | ||
| va.log.Errf("Remote VA %q.PerformValidation failed: %s", resp.addr, resp.err) | ||
| currProb = probs.ServerInternal("Secondary domain validation RPC failed") | ||
| va.log.Errf("Operation on remote VA (%s) failed: %s", resp.addr, resp.err) |
There was a problem hiding this comment.
If there's a clean way to get the human-readable name of the operation interpolated into this log message (and the similar one on line 526), that'd be really nice. But I suspect that doing so ends up being pretty ugly, so feel free to ignore me.
There was a problem hiding this comment.
I'm pretty sure it does wind up being ugly. We could pass the name of the operation into performRemoteValidation, but passing in a string just for the purpose of returning an error is ugly indeed.
Though that makes me realize: that's exactly what error wrapping is for. The caller (PerformValidation or IsCAAValid) knows which operation it was asking for, and can add that information by wrapping a returned error.
There was a problem hiding this comment.
I tried to add this but unfortunately it wasn’t very testable. The only way I can think to introduce a failure would be to replace the PerformValidation impl but that’s the same impl we’re going to call the operation on inside of performRemoteOperation. I would only be testing that my own test impl wraps errors 😅. I also don’t think it’ll be terribly useful. Inside of PerformValidation we never return the internal error to the caller (except for some of the first few checks), instead we log them and then pass it through detailedError() which is going to unwrap to a specific error and return it as a problem that’s relevant to the requester.
