Trivariant LookupResult type to allow authorities to decline to respond to a query

[marcus0x62]

This PR contains changes to the lookup, search, and related authority functions that wrap the lookup in an Option. This is to allow authorities to use None to signal when a lookup was not found, but that searching should continue using other authorities configured in an authority chain.

The current use case is for the block list authority.

This also changes the lookup and build_response logic in catalog.rs to not call send_authoritative_response or send_forwarded_response if None is returned.

The use case for this is so that when an authority like the block list is consulted, we do not leak the request to other authorities for requests the user wants to block, and we do not incur extra latency by waiting for a timeout when the block list can immediately signal it does not have an answer for a particular query.

See discussion here and more generally in PR #2113

[djc]

In particular, context:

The most recent commit implements this -- this required changing all of the lookup and search call sites in the server to work, but take a look. Also keep in mind this uses None to signal "I didn't find a result, but keep looking" not "I didn't find a result." I'm not sure if that, intrinsically, will be clearer than signaling an error, but I'm fine either way.

@bluejekyll if you have some time it would be nice to get some feedback on these lookup call changes in particular. All of this stuff is based on your suggestion here to use an Option to represent the lookup failure in the blocklist. All of the lookup call sites had to change as a result, and I think using Option, compared to the LookupError::NotHandled solution, adds more complexity to the lookup call semantics than it adds clarity to the situation where an authority needs to signal that it didn't handle a lookup and the next authority in the chain should be consulted.

[djc]

I'm sympathetic to the idea that this isn't the best solution, but I also think LookupError::NotHandled would be confusing too -- NotHandled doesn't feel like an error case to me.

Some alternative suggestions:

• Should we transpose the Result<Option<_>, LookupError> to Option<Result<_, LookupError>> instead? I think this makes it a little more obvious that the authority can decline to provide a result, or provide a result or error?
• Maybe we need a custom tri-variant enum for this? enum { Found(_), Unhandled, Error(LookupError) }?

What do you think?

[marcus0x62]

I like the tri-variant idea -- it has the potential to be much more clear about what data is or is not being returned and how it should be acted upon. I'm going to spend some time today investigating this a bit more - there is a LookupResult type that is typed to Result<T, LookupError> and is only used it one place in the In Memory authority. I think I'll modify that to include a Bypass variant and see what the overall change looks like.

[bluejekyll]

That's for the discussion on this. I think I agree about the TriValue to make the intention clear. The only downside I foresee is that we might end up with some ergonomics that will be lost on Option and Result. That's probably ok, and maybe there's a way gain some of those back.

Anyway, I like that better as a path forward as the currently Option/Result combination seems unwieldy.

[marcus0x62]

I'll put together a separate branch with my changes related to the tri-variant..probably on Monday. I did get this working yesterday, without too much trouble, but the loss of option-/result-related methods is something to consider for sure.

[djc]

We can of course add some of our own helper methods to the enum definition, of course.

[marcus0x62]

I’ve been doing that, but so far only to the extent necessary to implement what is currently in use, which is: unwrap unwrap_err expect expect_err map map_err

…

On Mon, Mar 4, 2024 at 7:52 AM, Dirkjan Ochtman ***@***.***(mailto:On Mon, Mar 4, 2024 at 7:52 AM, Dirkjan Ochtman <<a href=)> wrote: We can of course add some of our own helper methods to the enum definition, of course. — Reply to this email directly, [view it on GitHub](#2160 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AE7LXV67LVDY6M5W6ZQ5OIDYWR4A3AVCNFSM6AAAAABEAQQMBGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNZWGYZDQMJQHE). You are receiving this because you authored the thread.Message ID: ***@***.***>

[marcus0x62]

This is ready for review. Changes for the trivariant implementation are here. Changes for the previous Result implementation are here.

[djc]

IMO this is looking pretty good. Probably still good to get a review from @bluejekyll, as well.

[djc]

Style nit: to reduce rightward drift, it would be nice to have early returns for the Err and Skip cases, and yield the lookup out of the match for further handling.

For a bunch of these matches, maybe use LookupResult::* just before the match to reduce the noise a bit?

[bluejekyll]

@djc, Thanks for the initial review here. I'm approving, but think that @djc's comments about the unrelated changes, would be good to reduce those if possible (or maybe they're needed because of the new type and just leave that comment).

[djc]

I resolved some of my comments -- @marcus0x62 have you looked at it? I think we could merge this if that was resolved.