huitema / dnsoquic

DNS over QUIC
10 stars 7 forks source link

Rober Wilton's IESG ballot comments #150

Open huitema opened 2 years ago

huitema commented 2 years ago

Robert Wilton has entered the following ballot position for draft-ietf-dprive-dnsoquic-10: No Objection

When responding, please keep the subject line intact and reply to all email addresses included in the To and CC lines. (Feel free to cut this introductory paragraph, however.)

Please refer to https://www.ietf.org/about/groups/iesg/statements/handling-ballot-positions/ for more information about how to handle DISCUSS and COMMENT positions.

The document, along with other ballot positions, can be found here: https://datatracker.ietf.org/doc/draft-ietf-dprive-dnsoquic/


COMMENT:

Hi,

Thanks for this doc. It looks like DNS and QUIC could be a good fit. Just a few minor comments/questions.

In QUIC, sending STOP_SENDING requests that a peer cease transmission on a stream. If a DoQ client wishes to cancel an outstanding request, it MUST issue a QUIC STOP_SENDING with error code DOQ_REQUEST_CANCELLED. This may be sent at any time but will be ignored if the server response has already been acknowledged. The corresponding DNS transaction MUST be abandoned.

I presume that there is no requirement that the DNS transaction be immediately abandoned. E.g., if a server already has a reply queued for sending, then it is still reasonable to send that response?

Because new error codes can be defined without negotiation, use of an error code in an unexpected context or receipt of an unknown error code MUST be treated as equivalent to DOQ_NO_ERROR.

I find DOQ_NO_ERROR to be a strange name for an error code because I would naturally assume that DOQ_NO_ERROR is equivalent to "success", but that doesn't seem to be the intention here. In particular, I find it strange to treat an unknown error equivalently to DOQ_NO_ERROR. I'm not saying that the behaviour is wrong, only that the naming is slightly strange/confusing.

In theory, padding at the QUIC level could result in better performance for the equivalent protection

As a nit, I didn't find "QUIC level" to be particularly clear, and hence I was wondering whether this could be clarified. E.g., is this at the QUIC protocol level, or QUIC packet level.

10.4. DNS over QUIC Error Codes Registry Registrations in this registry MUST include the following fields:

This lists various fields that MUST be included, but doesn't specify values for the initially assigned values in the table.

Regards, Rob

saradickinson commented 2 years ago

To the first point, I think this paragraph just refers to the client behaviour. We could add a sentence to the second paragraph to clarify the server behaviour but the question of queue is interesting (queued in the QUIC layer or the application layer)? 'Servers should abandon the DNS transaction and send no further responses than those that might already be queued."?

Regarding the error code I'm minded to push back because no-one else has raised this point and I think the behaviour in the draft is reasonable. I can only think of changing the name to DOQ_CONN_CLOSE?

On the third point, I've changed s/QUIC level/QUIC packet level/ in the editorial changes PR https://github.com/huitema/dnsoquic/pull/163 (did this before generating the PR below....)

I've created https://github.com/huitema/dnsoquic/pull/164 to try to address this.

huitema commented 2 years ago

STill need a PR to address the "stop sending" issue.