pkg/snet: support URI style UDPAddr encoding #4254
Conversation
There was a problem hiding this comment.
Thanks @bunert for this contribution.
Is there a specific reason to change this now? Yes, I would also have preferred this format, but it's also not a proper URI and it's rejected by any URI parser except golang's (which seems to be somewhat more lenient than others). If we're just adding this as new variant because it's nicer, to me that feels like it's only making things more complicated.
FWIW, in cases where round-tripping a SCION address through golang's web stack is required, we've "just" been mangling the address into some internal form that doesn't trip up the parser.
If we decide to change the address format, we should perhaps discuss whether there are further changes to the address format that would simplify processing. In particular, should also get rid of the comma (e.g. replace it with a second dash)?
Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @bunert)
pkg/snet/udpaddr.go line 30 at r1 (raw file):
var addrRegexpLegacy = regexp.MustCompile(`^(?P<ia>\d+-[\d:A-Fa-f]+),(?P<host>.+)$`) var addrRegexp = regexp.MustCompile(`^[\[](?P<ia>\d+-[\d:A-Fa-f]+),(?P<host>.+)[\]]:(?P<port>.+)$`)
Simplify single-character-character-lists ([\[] === \[)
Suggestion:
^\[(?P<ia>\d+-[\d:A-Fa-f]+),(?P<host>.+)\]:(?P<port>.+)$pkg/snet/udpaddr.go line 67 at r1 (raw file):
} udp, err := net.ResolveUDPAddr("udp", rawHost)
Use netip.ParseAddr to parse IP addresses or netip.ParseAddrPort to parse host:port.
pkg/snet/udpaddr.go line 188 at r1 (raw file):
} left, right := strings.Count(s, "["), strings.Count(s, "]") if left != right {
Both should be equal to 1, the brackets are no longer optional.
pkg/snet/udpaddr.go line 191 at r1 (raw file):
return "", "", serrors.New("invalid address: bracket count mismatch", "addr", s) } var rawHost string = "[" + match[2] + "]:" + match[3]
Just return the port string separately (as string or int)
There was a problem hiding this comment.
Reviewable status: 1 of 2 files reviewed, 4 unresolved discussions (waiting on @bunert and @matzf)
pkg/snet/udpaddr.go line 30 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
Simplify single-character-character-lists (
[\[]===\[)
Done.
pkg/snet/udpaddr.go line 67 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
Use
netip.ParseAddrto parse IP addresses ornetip.ParseAddrPortto parse host:port.
May I ask why? snet.UDPAddr contains a *net.UDPAddr, so it seemed more convenient to use the net pkg for parsing...
pkg/snet/udpaddr.go line 188 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
Both should be equal to 1, the brackets are no longer optional.
Done.
There was a problem hiding this comment.
Is there a specific reason to change this now?
This was an open "good first issue" for a long time, and we finally wanted to address it. There is no particular reason to do it right now, other than preparing for cleaning up the parsing mess that is snet.UDPAddr.
Yes, I would also have preferred this format, but it's also not a proper URI and it's rejected by any URI parser except golang's (which seems to be somewhat more lenient than others).
While this is true, the parsers we have checked when writing https://scion.docs.anapaya.net/en/latest/uri.html (python/go) were both happy with it. The two alternatives to make it URI compatible are also listed in that document with the reasoning why they were not chosen. Of course we can reopen the discussion there.
If we're just adding this as new variant because it's nicer, to me that feels like it's only making things more complicated.
To me the end goal is to get rid of all the variants that are not inline with https://scion.docs.anapaya.net/en/latest/uri.html.
The current code is a mess, I agree. If we change to the proposed encoding, there is exactly one way to specify the UDP address.
From
// - isd-as,ipv4:port (e.g., 1-ff00:0:300,192.168.1.1:8080)
// - isd-as,[ipv6]:port (e.g., 1-ff00:0:300,[f00d::1337]:8080)
// - isd-as,[ipv6%zone]:port (e.g., 1-ff00:0:300,[f00d::1337%zone]:8080)
// - isd-as,[ipv4]:port (e.g., 1-ff00:0:300,[192.168.1.1]:8080)
// - isd-as,[ipv4] (e.g., 1-ff00:0:300,[192.168.1.1])
// - isd-as,[ipv6] (e.g., 1-ff00:0:300,[f00d::1337])
// - isd-as,[ipv6%zone] (e.g., 1-ff00:0:300,[f00d::1337%zone])
// - isd-as,ipv4 (e.g., 1-ff00:0:300,192.168.1.1)
// - isd-as,ipv6 (e.g., 1-ff00:0:300,f00d::1337)
// - isd-as,ipv6%zone (e.g., 1-ff00:0:300,f00d::1337%zone)To
// - [isd-as,ipv4]:port (e.g., [1-ff00:0:300,192.168.1.1]:8080)
// - [isd-as,ipv6]:port (e.g., [1-ff00:0:300,f00d::1337]:8080)
// - [isd-as,ipv6%zone]:port (e.g., [1-ff00:0:300,f00d::1337%zone]:8080)For me, that is a big win. Especially when using tools like scion ping/traceroute.
If we decide to change the address format, we should perhaps discuss whether there are further changes to the address format that would simplify processing. In particular, should also get rid of the comma (e.g. replace it with a second dash)?
Sure, we can discuss. But let's do that outside of this PR. I guess until this is decided we will need to freeze this PR.
Reviewable status: 1 of 2 files reviewed, 4 unresolved discussions (waiting on @bunert and @matzf)
There was a problem hiding this comment.
[ ... ] preparing for cleaning up the parsing mess that is snet.UDPAddr.
Ok if that's the plan than I'm more than happy with this! 👍
While this is true, the parsers we have checked when writing https://scion.docs.anapaya.net/en/latest/uri.html (python/go) were both happy with it
That's good, I was not aware that this works in python 👍
For me, that is a big win. Especially when using tools like scion ping/traceroute.
These tools require host addresses only, not host:port UDP addresses. For this case, not including the brackets should still be possible, as described in the proposal document (the definition of SCIONAddress does not include the brackets).
To support the different use cases properly, I'd propose to add parsing functionality for snet.SCIONAddress, accepting the SCIONAddress format (i.e. no brackets, no port) (*). This functionality is adopted eg. for ping/traceroute. Parsing a UDP address in the URI format then becomes net.SplitHostPort(**), followed by parsing the host part as SCIONAddress and creating a snet.UDPAddr by something like host.WithPort(port).
If you think this deviates too much from what you had in mind, we should perhaps discuss the details in an issue.
(*) This is a functionality that I've been missing from snet anyway. Our pan library re-implements address parsing for contexts where the host:port format is illegal, https://github.com/netsec-ethz/scion-apps/blob/ef972274f4190de5ac702cc7eff579d936c6650d/pkg/pan/addr.go#L141-L189
(**) Thankfully, net.SplitHostPort works with the address format proposed here 🎉
Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @bunert)
pkg/snet/udpaddr.go line 67 at r1 (raw file):
Previously, bunert wrote…
May I ask why?
snet.UDPAddrcontains a*net.UDPAddr, so it seemed more convenient to use thenetpkg for parsing...
Ah sure, yes sorry for not giving any justification. In my view, netip.ParseAddrPort is preferable here because it only parses addresses and never resolves names. Resolving names is not necessary (or even desirable) here and introduces additional failure modes.
In the longer run, we'll want to change our snet.UDPAddr to internally use netip.Addr, not only for performance reasons but also because the usability of the API is just far better. And btw., netip's "predecessor" package, inet.af/netaddr is already used in this file.
There was a problem hiding this comment.
These tools require host addresses only, not host:port UDP addresses. For this case, not including the brackets should still be possible, as described in the proposal document (the definition of SCIONAddress does not include the brackets).
Yes, you are absolutely right. scion ping was the wrong example to take here. I meant cases where you actually need a SCION UDP address.
To support the different use cases properly, I'd propose to add parsing functionality for snet.SCIONAddress, accepting the SCIONAddress format (i.e. no brackets, no port) (*). This functionality is adopted eg. for ping/traceroute. Parsing a UDP address in the URI format then becomes net.SplitHostPort(**), followed by parsing the host part as SCIONAddress and creating a snet.UDPAddr by something like host.WithPort(port).
This is exactly what I had in mind. Eventually, snet.UDPAddr should no longer be able to parse addresses without a port.
(It is quite odd that we had this in the first place)
I want a strict separation between SCION UDP, SCION SVC and SCION Address parsing.
If you think this deviates too much from what you had in mind, we should perhaps discuss the details in an issue.
I think we agree on the end state of what should be done. I wanted to go a non-breaking route where we still are able to parse the "legacy" way until other tools have been adapted. However, if you feel we can do a breaking change here, we can already clean this up now.
(**) Thankfully, net.SplitHostPort works with the address format proposed here 🎉
It was written with net.SplitHostPort and net.JoinHostPort in mind. Glad that it still works after all this time :)
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @bunert)
There was a problem hiding this comment.
Excellent, that sounds like a good plan. 👍
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @bunert)
pkg/snet/udpaddr.go line 77 at r2 (raw file):
} // The supported legacy formats are:
Nit, perhaps elaborate a bit what "legacy" means here and what the plan is for phasing out these formats.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @bunert and @matzf)
pkg/snet/udpaddr.go line 67 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
Ah sure, yes sorry for not giving any justification. In my view,
netip.ParseAddrPortis preferable here because it only parses addresses and never resolves names. Resolving names is not necessary (or even desirable) here and introduces additional failure modes.In the longer run, we'll want to change our
snet.UDPAddrto internally usenetip.Addr, not only for performance reasons but also because the usability of the API is just far better. And btw.,netip's "predecessor" package,inet.af/netaddris already used in this file.
+1 for using netip when necessary.
However, I think parsing is a bit convoluted. Parsing the address can be boiled down to these steps:
- net.SplitHostPort
- parse port
- split host at comma
- parse IA
- parse IP address
The beautiful thing is that this does not even require a regex.
outline https://go.dev/play/p/KCGrcLzUo2M
pkg/snet/udpaddr.go line 42 at r2 (raw file):
// ParseUDPAddr converts an address string to a SCION address. func ParseUDPAddr(s string) (*UDPAddr, error) { addr, err := parseUDPAddr(s)
To reduce cognitive load when reading this code, you can make the exit conditions more unambiguous.
Suggestion:
addr, err := parseUDPAddr(s)
if err != nil {
return parseUDPAddrLegacy(s)
}
return addr, nilpkg/snet/udpaddr.go line 55 at r2 (raw file):
// Examples: // - [isd-as,ipv4]:port (e.g., [1-ff00:0:110,192.0.2.1]:80) // - [isd-as,ipv6%zone]:port (e.g., [1-ff00:0:110,2001:DB8::1]:80)
The example does not match. No zone vs zone.
Code quote:
// - [isd-as,ipv6%zone]:port (e.g., [1-ff00:0:110,2001:DB8::1]:80)
There was a problem hiding this comment.
Reviewed 1 of 2 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @bunert and @matzf)
There was a problem hiding this comment.
I adjusted the requested changes. Should the parsing functionality for the snet.SCIONAddress be part of this PR or was it just a general discussion?
Reviewable status: 0 of 2 files reviewed, 4 unresolved discussions (waiting on @matzf and @oncilla)
pkg/snet/udpaddr.go line 67 at r1 (raw file):
Previously, oncilla (Dominik Roos) wrote…
+1 for using netip when necessary.
However, I think parsing is a bit convoluted. Parsing the address can be boiled down to these steps:
- net.SplitHostPort
- parse port
- split host at comma
- parse IA
- parse IP address
The beautiful thing is that this does not even require a regex.
Done.
pkg/snet/udpaddr.go line 191 at r1 (raw file):
Previously, matzf (Matthias Frei) wrote…
Just return the port string separately (as string or int)
removed this part.
pkg/snet/udpaddr.go line 42 at r2 (raw file):
Previously, oncilla (Dominik Roos) wrote…
To reduce cognitive load when reading this code, you can make the exit conditions more unambiguous.
Done.
pkg/snet/udpaddr.go line 55 at r2 (raw file):
Previously, oncilla (Dominik Roos) wrote…
The example does not match. No zone vs zone.
Done.
There was a problem hiding this comment.
Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @bunert and @matzf)
pkg/snet/udpaddr.go line 42 at r2 (raw file):
Previously, bunert wrote…
Done.
Only half. Always explicitly return nil if there is no error.
pkg/snet/udpaddr.go line 63 at r3 (raw file):
} parts := strings.Split(host, ",") if len(parts) != 2 {
This is a new error. The wrapped error is nil here.
Code quote:
if len(parts) != 2 {
return nil, serrors.WrapStr("invalid address: host parts invalid", err, "host", host)
}pkg/snet/udpaddr.go line 71 at r3 (raw file):
} ip, err := netip.ParseAddr(parts[1]) if len(parts) != 2 {
Suggestion:
if err != nil {pkg/snet/udpaddr_test.go line 260 at r3 (raw file):
assert.NoError(t, err) assert.Equal(t, test.ia, a.IA.String()) ipLegacy := net.ParseIP(test.host)
I think you will appreciate https://pkg.go.dev/net#IP.Equal
Also, try to figure out why it failed in the first place here.
Hint: IPv6 vs IPv4 form https://pkg.go.dev/net#IP
Code quote:
ipLegacy := net.ParseIP(test.host)
ip, err := netip.ParseAddr(test.host)
assert.NoError(t, err)
assert.Contains(t, []net.IP{ipLegacy, ip.AsSlice()}, a.Host.IP)
There was a problem hiding this comment.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @matzf and @oncilla)
pkg/snet/udpaddr.go line 42 at r2 (raw file):
Previously, oncilla (Dominik Roos) wrote…
Only half. Always explicitly return nil if there is no error.
Done.
pkg/snet/udpaddr.go line 63 at r3 (raw file):
Previously, oncilla (Dominik Roos) wrote…
This is a new error. The wrapped error is nil here.
Done.
pkg/snet/udpaddr.go line 71 at r3 (raw file):
} ip, err := netip.ParseAddr(parts[1]) if len(parts) != 2 {
Done.
pkg/snet/udpaddr_test.go line 260 at r3 (raw file):
Previously, oncilla (Dominik Roos) wrote…
I think you will appreciate https://pkg.go.dev/net#IP.Equal
Also, try to figure out why it failed in the first place here.
Hint: IPv6 vs IPv4 form https://pkg.go.dev/net#IP
Done.
There was a problem hiding this comment.
This should be part of a separate PR. Here we focus on the UDPAddr only.
Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @matzf)
oncilla
changed the title
There was a problem hiding this comment.
Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @bunert)
|
The linter is failing here because we use a relatively old version of golangci-lint, which does not support go-1.18. Without investigating this thoroughly, I suspect that this now fails because we're using |
|
The lint issue is fixed by #4269. |
There was a problem hiding this comment.
Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status:
Support the URI style encoding of a SCION UDP address during parsing as described here: [proposals/uri](https://docs.scion.org/en/latest/uri.html#extensions-to-rfc-3986) The following encodings of an UDP address are now supported: - `[1-ff00:0:110,127.0.0.1]:3000` - `[1-ff00:0:110,::1]:3000` - `[1-ff00:0:110,fe::1%eth1]:3000` The other encodings are deprecated and will be removed eventually.
Support the URI style encoding of a SCION UDP address during parsing as described here: proposals/uri
The following encodings of an UDP address are now supported:
[1-ff00:0:110,127.0.0.1]:3000[1-ff00:0:110,::1]:3000[1-ff00:0:110,fe::1%eth1]:3000The other encodings are deprecated and will be removed eventually.
This change is