Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

QUIC/DoQ support using quic-go #1377

Closed
wants to merge 1 commit into from
Closed

QUIC/DoQ support using quic-go #1377

wants to merge 1 commit into from

Conversation

jelu
Copy link

@jelu jelu commented May 25, 2022

Work In Progress !

Draft implementation of DoQ support using quic-go, this PR serves as discussion forum for if this is the right way forward as the dependencies are quite large.

TODO:

  • Should there be a ListenAndServeDoQ()?
  • Add _test code
  • Discuss usage of response.tcp when using QUIC

+ all TODO's in the code

Example:

cert, _ := tls.LoadX509KeyPair("<cert>", "<key>")
config := tls.Config{
    Certificates: []tls.Certificate{cert},
}
server := &dns.Server{Addr: "<addr>", Net: "doq", TLSConfig: &config, Handler: <handle>})

ref #1370

Draft implementation of DoQ support using quic-go, example:

  cert, _ := tls.LoadX509KeyPair("<cert>", "<key>")
  config := tls.Config{
      Certificates: []tls.Certificate{cert},
  }
  server := &dns.Server{Addr: "<addr>", Net: "doq", TLSConfig: &config, Handler: <handle>})
@jelu jelu requested review from miekg and tmthrgd as code owners May 25, 2022 08:44
@jelu
Copy link
Author

jelu commented May 25, 2022

@miekg @tmthrgd No hurry to review, think I'll spend some time just looking at the server.go code. Thinking maybe a interface for all protocols is better, so any protocol can just be plugged in.

@miekg
Copy link
Owner

miekg commented May 26, 2022

thanks for doing this. I'll go over the code and try to ask stupid questions.

Note: I have no intention to merge this, because I'm waiting what Go's std lib will do here. Also net.Conn is the defacto std interface we're looking for, so quic should (somehow) adapt to that, instead of the other way around

@@ -74,6 +81,7 @@ type response struct {
tsigProvider TsigProvider
udp net.PacketConn // i/o connection if UDP was used
tcp net.Conn // i/o connection if TCP was used
doq quic.Stream // i/o connection if QUIC was used
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can't we check if it's a quick stream when we accept a udp conn? And then do a type assertion on the net.PacketConn we have if it implements Stream() - or whatever the interface is?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A quic.Stream is not a net.PacketConn, and a quic.Connection can have many quic.Streams (each stream will carry one query/response)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, but https://github.com/lucas-clemente/quic-go/blob/master/example/echo/echo.go#L39 shows it is a conn first and after that some quic stuff happens, I wonder if we could do the same to avoid much code duplication (this may be impossible)

server.go Show resolved Hide resolved
server.go Show resolved Hide resolved
server.go Show resolved Hide resolved
@jelu
Copy link
Author

jelu commented May 27, 2022

Note: I have no intention to merge this, because I'm waiting what Go's std lib will do here. Also net.Conn is the defacto std interface we're looking for, so quic should (somehow) adapt to that, instead of the other way around

Ah OK.

So are you willing to change the internal of dns.Server to allow for a more pluggable protocol design?

If so, then I can spend next week hammering out an suggestion.

If not, then I'll solved all of this in another way.

Please let me know ASAP, thanks!

@miekg
Copy link
Owner

miekg commented May 27, 2022

honestly don't know - when we added grpc in coredns nothing needed changing this lib, so I don't see yet why quic is somehow special here (yes I understand the streams are fundamentally different than anything before)

Also once we merged we can't easily break folks because of backwards compat and we might also design it towards quic-go while noone knows how the final impl. looks that will hopefully end up in the std lib.

@miekg
Copy link
Owner

miekg commented May 27, 2022

ref golang/go#32204

@jelu
Copy link
Author

jelu commented May 27, 2022

The suggestion, if any, would be made in a way that has zero affect on the current API and interfaces. I'm thinking just internal stuff to add more protocol via dns.Server.Net in a pluggable way so that I can make (and manage) a "dns-quic-go" module that people can use seamlessly.

Basically use-case would be:

import (
  "github.com/miekg/dns"
  "github.com/DNS-OARC/dns-quic-go"
)

func () {
  srv := &dns.Server{Addr: "...", Net: "quic-go", Handler: ...}
  srv.ListenAndServe()
}

Of course if there is no interest at all in having such pluggable design then I'll solve it in another way and we can close this PR.

@miekg
Copy link
Owner

miekg commented May 27, 2022 via email

@jelu
Copy link
Author

jelu commented May 30, 2022

After looking over the code I can't really see a simple way to make protocols pluggable, they are too tied into a lot of things such as the Reader interface which has ReadTCP(), ReadUDP() etc and none of that is applicable to DoQ.

The design is also around having one listener listening for one connection that delivers DNS messages where QUIC is conn listener -> stream listener -> streams. Sure, that could be solved with channels etc but then it's workarounds which makes the code clunky and slow.

I think a better approach is to make my own module that mimics dns.Server as much as possible.

Thanks for the reviews and discussion!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants