Bump scionproto and rains dependencies

[martenwallewein]

Hey guys, following this issue #244

to have scion-apps aligned with the newest scionproto and also quic-go version I started to migrate this (or at least to give it a try ^^). It currently waits for this PR netsec-ethz/rains#242 to be merged beforehand, but overall the changes look doable.

Will keep you in loop!

---

This change is 

[marcfrei]

Thanks for starting this @martenwallewein. Some first comments below.

Reviewed 23 of 25 files at r1, all commit messages.
Reviewable status: 23 of 25 files reviewed, 5 unresolved discussions / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained

---

go.mod line 24 at r1 (raw file):

)

// replace github.com/netsec-ethz/rains => ../rains/

Remember to remove this again before finalizing the PR.

---

pkg/pan/path.go line 32 at r1 (raw file):

type Path struct {
	source         IA
	destination    IA

What's the reason for this change?

Code quote:

	source         IA
	destination    IA

---

pkg/pan/policy.go line 188 at r1 (raw file):

func (p snetPathWrapper) Destination() addr.IA          { panic("not implemented") }

// TODO: I'm not sure if it's fine to panic here...

Remember to clean this up again before finalizing the PR.

---

pkg/pan/quic_listen.go line 61 at r1 (raw file):

	//	Listener: listener,
	//	conn:     conn,
	//}, nil

Remove this old code, history will be preserved by Git.

---

pkg/pan/raw.go line 110 at r1 (raw file):

			Source: snet.SCIONAddress{
				IA:   addr.IA(src.IA),
				Host: addr.MustParseHost(src.IP.IPAddr().IP.String()),

It's good to have PRs that focus on single issue but here I think the correct way to handle this is to go ahead and replace inet.af/netaddr by net/netip like in scionproto/scion@c43f942

[martenwallewein]

Hey @marcfrei thanks for the helpful comments! Although it's an early stage for this PR, it's great to already have this kind of feedback! Especially the hint for replacing inet.af/netaddr by net/netip and the link to the commit are really helpful, I wasn't aware that similar changes were done in scionproto!

Will keep you in loop when I've tackled the comments.

[sarcasticadmin]

@martenwallewein building the latest on this branch works for me and the binaries basic functionality (--help) seems good. Nice job!

Consider updating the _examples/go.{mod,sum} as well: https://github.com/netsec-ethz/scion-apps/blob/28b48f6c4b4aa806d85ba0051ddab00317a1e3ac/_examples/go.mod

[matzf]

@martenwallewein I took the liberty to push to your branch; I've updated the _examples, fixed the lint errors and updated the SCION setup for the integration tests. I think this is ready now (except for Marc's comments, that is).

[matzf]

Reviewed 1 of 25 files at r1.
Reviewable status: 6 of 63 files reviewed, 5 unresolved discussions / 0 of 1 LGTMs obtained / 0 of 1 approvals obtained

---

go.mod line 24 at r1 (raw file):

Previously, marcfrei (Marc Frei) wrote…

Remember to remove this again before finalizing the PR.

Done, by Marten

---

pkg/pan/path.go line 32 at r1 (raw file):

Previously, marcfrei (Marc Frei) wrote…

What's the reason for this change?

Done, reverted. Was probably a change due to a somewhat confusing wrapper to use pan.Path as snet.Path.

---

pkg/pan/policy.go line 188 at r1 (raw file):

Previously, marcfrei (Marc Frei) wrote…

Remember to clean this up again before finalizing the PR.

Done, by Marten.

---

pkg/pan/quic_listen.go line 61 at r1 (raw file):

Previously, marcfrei (Marc Frei) wrote…

Remove this old code, history will be preserved by Git.

Done, by Marten.

---

pkg/pan/raw.go line 110 at r1 (raw file):

Previously, marcfrei (Marc Frei) wrote…

It's good to have PRs that focus on single issue but here I think the correct way to handle this is to go ahead and replace inet.af/netaddr by net/netip like in scionproto/scion@c43f942

Done. This is a relatively big change, please review!

[sarcasticadmin]

@matzf thanks for updating _examples. Im able to build and basic functionality looks good on my end.

My build environment:

$ go version
go version go1.21.3 linux/amd64

A few tests of the tools inside SCiONLabs:

bwtestserver:

$ ./result/bin/scion-bwtestserver --listen=:30100
Received request: N 18-ffaa:1:10bf,127.0.0.1:35319
Received request: R 18-ffaa:1:10bf,127.0.0.1:35319

bwtestclient:

 $ ./result/bin/scion-bwtestclient -s 18-ffaa:1:10a0,127.0.0.1:30100 -cs 10,1000,1250,1Mbps -sc 10,1000,12500,10Mbps

Test parameters:
client->server: 10 seconds, 1000 bytes, 1250 packets
server->client: 10 seconds, 1000 bytes, 12500 packets

S->C results
Attempted bandwidth: 10000000 bps / 10.00 Mbps
Achieved bandwidth: 9975200 bps / 9.98 Mbps
Loss rate: 0.2%
Interarrival time min/avg/max/mdev = 0.002/0.802/46.580/6.766 ms

C->S results
Attempted bandwidth: 1000000 bps / 1.00 Mbps
Achieved bandwidth: 1000000 bps / 1.00 Mbps
Loss rate: 0.0%
Interarrival time min/avg/max/mdev = 0.005/8.007/60.863/7.270 ms

---

helloquic server:

$ ./result/bin/example-helloquic -listen 127.0.0.1:9000
18-ffaa:1:10bf,127.0.0.1:9000
New session 18-ffaa:1:10a0,127.0.0.1:34776
hi dude, 0

helloquic client:

$ ./result/bin/example-helloquic -remote  18-ffaa:1:10bf,127.0.0.1
:9000
gotcha: hi dude, 0

[martenwallewein]

@martenwallewein I took the liberty to push to your branch; I've updated the _examples, fixed the lint errors and updated the SCION setup for the integration tests. I think this is ready now (except for Marc's comments, that is).

Hey @matzf, thanks a lot for picking this up! I was on vacation until today and didn't had a chance to look at this since my last commit. Since I was quite lost with some changes, especially to PAN, I really appreciate that you tackled these issues! Looking forward to have scion-apps running the latest scion version! 🥳

[sarcasticadmin]

Is there anything left to do before getting this merged?

[matzf]

From my side, this is complete, pending @marcfrei's review.

I left a "BUG" comment on the pan.ListenQUIC API, which is now somewhat broken, because it can no longer properly close the underlying socket. The issue is that the quic-go library changed the listener from an interface type to a struct, which cannot be wrapped as easily. This is probably not a huge practical problem as our demo applications only open one listener for the duration of the program.
The API should be cleaned up eventually though, perhaps ideally just getting rid of the ListenQUIC helper and leaving this up to the applications.

[marcfrei]

@martenwallewein: what do you think, should I merge the PR?

Reviewed 17 of 25 files at r3, 1 of 1 files at r4, 39 of 39 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

[martenwallewein]

@martenwallewein: what do you think, should I merge the PR?

Reviewed 17 of 25 files at r3, 1 of 1 files at r4, 39 of 39 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

Yeah sure, if you reviewed @matzf and my changes and everything looks good, then we can go ahead from my side. Looking at the issue #244 we agreed to merge this in a dev branch or something like that, which I forgot when opening the PR against master, since the changes may break stuff in drkey. With respect to the open issue of the QUIC listener @matzf mentioned, this looks like another argument to merge it into a dev branch.

To summarize it: Yeah it can be merged, in a dev branch if you prefer this, otherwise to master. Thanks all for your help and suggestions!

[matzf]

The only use of drkey in the scion-apps was the _examples/hellodrkey, which I've deleted here, as it is now maintained in scionproto/scion as demo/drkey.
I don't want to disregard the discussion in #244, but I think with this example code gone, there is no problem to merge this directly to master.

[martenwallewein]

The only use of drkey in the scion-apps was the _examples/hellodrkey, which I've deleted here, as it is now maintained in scionproto/scion as demo/drkey. I don't want to disregard the discussion in #244, but I think with this example code gone, there is no problem to merge this directly to master.

Perfect, so no objections from me to merge into master 👍

[marcfrei]

Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved