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

Start QUIC RPC server in control plane speakers #2653

Merged
merged 4 commits into from
May 9, 2019
Merged

Start QUIC RPC server in control plane speakers #2653

merged 4 commits into from
May 9, 2019

Conversation

scrye
Copy link
Contributor

@scrye scrye commented May 8, 2019

Also includes topology generator support for QUIC and SVC resolution.


This change is Reviewable

@scrye scrye requested a review from oncilla May 8, 2019 12:30
Copy link
Contributor

@oncilla oncilla left a comment

Choose a reason for hiding this comment

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

Reviewed 13 of 13 files at r1.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @scrye)


go/lib/env/sample.go, line 76 at r1 (raw file):

# The address to start a QUIC server on (ip:port). If not set, a QUIC server is
# not started. (default "")
QUICListen = ""

We use line breaks to separate fields


go/lib/env/sample.go, line 78 at r1 (raw file):

QUICListen = ""
# Certificate file to use for authenticating QUIC connections.
QUICCertFile = "gen-certs/tls.pem"

We try to use paths that you would use in production.
Probably /etc/scion/quic/tls.pem or something


go/lib/infra/infraenv/infraenv.go, line 77 at r1 (raw file):

	// QUIC contains configuration details for QUIC servers. If nil, no QUIC
	// server is started.
	QUIC *QUIC

I don't think this works as you expect.
Servers will always create the QUIC object, even if the QUICAddr is not set.
Make this value type and decide based on QUIC.Address whether the server should be started


go/lib/infra/infraenv/infraenv.go, line 93 at r1 (raw file):

	var quicConn net.PacketConn
	var quicAddress string
	if nc.QUIC != nil {

use nc.QUIC.Address for decision


go/lib/infra/infraenv/infraenv.go, line 139 at r1 (raw file):

		log.Root(),
	)
	if nc.QUIC != nil {

ditto


go/lib/infra/messenger/messenger.go, line 595 at r1 (raw file):

	var wg sync.WaitGroup
	if m.config.QUIC != nil {
		wg.Add(1)

This blocks forever, since there is no wg.Done()
IMO a done channel would be cleaner


go/lib/infra/messenger/messenger.go, line 609 at r1 (raw file):

	defer m.log.Info("Stopped listening QUIC")
	if err := m.quicServer.ListenAndServe(); err != nil {
		m.log.Error("Unable to start QUIC server", "err", err)

The error is also non-nil in case the network is closed.


python/topology/go.py, line 120 at r1 (raw file):

            'discovery': self._discovery_entry(),
            'metrics': self._metrics_entry(name, infra_elem, BS_PROM_PORT),
            'server': {

this can be put in a function to reduce code duplication.


python/topology/go.py, line 121 at r1 (raw file):

            'metrics': self._metrics_entry(name, infra_elem, BS_PROM_PORT),
            'server': {
                'QUICListen':  prom_addr_infra(self.args.docker, name, infra_elem, BS_QUIC_PORT),

The IP is available through get_pub_ip(infra_elem["Addrs"]) port with get_pub_ip(infra_elem["Addrs"]).


python/topology/go.py, line 125 at r1 (raw file):

                'QUICKeyFile': 'gen-certs/tls.key',
            },
            'client': {

ditto

Copy link
Contributor Author

@scrye scrye left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @oncilla)


go/lib/env/sample.go, line 76 at r1 (raw file):

Previously, Oncilla wrote…

We use line breaks to separate fields

Done.


go/lib/env/sample.go, line 78 at r1 (raw file):

Previously, Oncilla wrote…

We try to use paths that you would use in production.
Probably /etc/scion/quic/tls.pem or something

Done.


go/lib/infra/infraenv/infraenv.go, line 77 at r1 (raw file):

Previously, Oncilla wrote…

I don't think this works as you expect.
Servers will always create the QUIC object, even if the QUICAddr is not set.
Make this value type and decide based on QUIC.Address whether the server should be started

Done.


go/lib/infra/infraenv/infraenv.go, line 93 at r1 (raw file):

Previously, Oncilla wrote…

use nc.QUIC.Address for decision

Done.


go/lib/infra/infraenv/infraenv.go, line 139 at r1 (raw file):

Previously, Oncilla wrote…

ditto

Done.


go/lib/infra/messenger/messenger.go, line 595 at r1 (raw file):

Previously, Oncilla wrote…

This blocks forever, since there is no wg.Done()
IMO a done channel would be cleaner

Whoops, shows what not having proper tests can cause.

<-done


go/lib/infra/messenger/messenger.go, line 609 at r1 (raw file):

Previously, Oncilla wrote…

The error is also non-nil in case the network is closed.

If the network is closed, the fact will be included in the err? Not sure what the change here would be.


python/topology/go.py, line 120 at r1 (raw file):

Previously, Oncilla wrote…

this can be put in a function to reduce code duplication.

Done.


python/topology/go.py, line 121 at r1 (raw file):

Previously, Oncilla wrote…

The IP is available through get_pub_ip(infra_elem["Addrs"]) port with get_pub_ip(infra_elem["Addrs"]).

It was a bit more involved, but it worked eventually! Thanks!


python/topology/go.py, line 125 at r1 (raw file):

Previously, Oncilla wrote…

ditto

Done.

Copy link
Contributor

@oncilla oncilla left a comment

Choose a reason for hiding this comment

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

Reviewed 7 of 7 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @scrye)


python/topology/go.py, line 121 at r1 (raw file):

Previously, scrye (Sergiu Costea) wrote…

It was a bit more involved, but it worked eventually! Thanks!

Please apply the following patch and use get_pub_ip instead:

--- a/python/topology/common.py
+++ b/python/topology/common.py
@@ -16,6 +16,7 @@
 import os
 import subprocess
 import sys
+
 # SCION
 from lib.packet.scion_addr import ISD_AS
 from topology.net import AddressProxy
@@ -138,6 +139,14 @@ def get_pub(topo_addr):
     return topo_addr['IPv4']
 
 
+def get_pub_ip(topo_addr):
+    return get_pub(topo_addr)["Public"]["Addr"].ip
+
+
+def get_l4_port(topo_addr):
+    return get_pub(topo_addr)["Public"]["L4Port"]
+
+
 def srv_iter(topo_dicts, out_dir, common=False):
     for topo_id, as_topo in topo_dicts.items():
         base = topo_id.base_dir(out_dir)

scrye added 3 commits May 9, 2019 11:09
Also includes topology generator support for QUIC and SVC resolution.
Copy link
Contributor Author

@scrye scrye left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @oncilla)


python/topology/go.py, line 121 at r1 (raw file):

Previously, Oncilla wrote…

Please apply the following patch and use get_pub_ip instead:

--- a/python/topology/common.py
+++ b/python/topology/common.py
@@ -16,6 +16,7 @@
 import os
 import subprocess
 import sys
+
 # SCION
 from lib.packet.scion_addr import ISD_AS
 from topology.net import AddressProxy
@@ -138,6 +139,14 @@ def get_pub(topo_addr):
     return topo_addr['IPv4']
 
 
+def get_pub_ip(topo_addr):
+    return get_pub(topo_addr)["Public"]["Addr"].ip
+
+
+def get_l4_port(topo_addr):
+    return get_pub(topo_addr)["Public"]["L4Port"]
+
+
 def srv_iter(topo_dicts, out_dir, common=False):
     for topo_id, as_topo in topo_dicts.items():
         base = topo_id.base_dir(out_dir)

Done.

Copy link
Contributor

@oncilla oncilla left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 2 of 2 files at r3.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Copy link
Contributor

@oncilla oncilla left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 2 of 2 files at r4.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@scrye scrye merged commit 2800a2e into scionproto:master May 9, 2019
@scrye scrye deleted the pubpr-quic-config-new branch May 9, 2019 12:45
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.

2 participants