Finalize segfetcher module

[lukedirtwalker]

This commit finalizes the segfetcher module so that it can be integrated into SD&PS as a next step.

Contributes #2454

---

This change is 

[oncilla]

Reviewed 21 of 21 files at r1.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @lukedirtwalker)

---

go/lib/infra/modules/segfetcher/fetcher.go, line 82 at r1 (raw file):

// FetchSegs fetches the required segments to build a path between src and dst
// of the request. Firs the request is validated and then depending on the

s/Firs/First

---

go/lib/infra/modules/segfetcher/fetcher.go, line 129 at r1 (raw file):

			continue
		}
		r := f.ReplyHandler.Handle(ctx, reply.Reply, reply.Peer, nil)

the nil channel here will cause a panic in SegReplyHandler

---

go/lib/infra/modules/segfetcher/fetcher.go, line 132 at r1 (raw file):

		select {
		case <-r.FullReplyProcessed():
			if err := r.Err(); err != nil {

This means one segment that fails to verify will break segment fetching, no?
(e.g. timeout when fetching a missing certificate)

---

go/lib/infra/modules/segfetcher/fetcher_test.go, line 117 at r1 (raw file):

			ExpectedSegs:   segfetcher.Segments{Up: seg.Segments{tg.seg130_111}},
		},
		// XXX(lukedirtwalker): testing the full loop is quite involved, not

hmm. Yeah.
TBH I don't think it is worth it, since Fetcher mostly just combines different components.
It's really nasty to test, and if it is buggy it will immediately show when sciond/PS are unable to provide paths.

---

go/lib/infra/modules/segfetcher/segreplyhandler.go, line 164 at r1 (raw file):

		full:  make(chan struct{}),
	}
	verifiedCh, units := h.Verifier.Verify(ctx, reply, server)

hm.
This changes the crypto lookup strategy.
Which is correct for the PS, but I remember some discussion of sciond going to CS instead.

[lukedirtwalker]

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

---

go/lib/infra/modules/segfetcher/fetcher.go, line 82 at r1 (raw file):

Previously, Oncilla wrote…

s/Firs/First

Done.

---

go/lib/infra/modules/segfetcher/fetcher.go, line 129 at r1 (raw file):

Previously, Oncilla wrote…

the nil channel here will cause a panic in SegReplyHandler

No? How? Selecting over a nil channel is valid, it will never be selected.

---

go/lib/infra/modules/segfetcher/fetcher.go, line 132 at r1 (raw file):

Previously, Oncilla wrote…

This means one segment that fails to verify will break segment fetching, no?
(e.g. timeout when fetching a missing certificate)

Yes. That is the current behavior as well. If we have multiple segments for the same type (e.g. multiple core segs) it could make sense to ignore the error and we might still get paths.
I don't want to overcomplicate things now.

---

go/lib/infra/modules/segfetcher/fetcher_test.go, line 117 at r1 (raw file):

Previously, Oncilla wrote…

hmm. Yeah.
TBH I don't think it is worth it, since Fetcher mostly just combines different components.
It's really nasty to test, and if it is buggy it will immediately show when sciond/PS are unable to provide paths.

Yeah. I will leave the comment so that it is clear why it isn't done.

---

go/lib/infra/modules/segfetcher/segreplyhandler.go, line 164 at r1 (raw file):

Previously, Oncilla wrote…

hm.
This changes the crypto lookup strategy.
Which is correct for the PS, but I remember some discussion of sciond going to CS instead.

Yeah I had to change it for the PS. But forgot to make it configurable for sciond. Done.

[oncilla]

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

---

go/lib/infra/modules/segfetcher/fetcher.go, line 129 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

No? How? Selecting over a nil channel is valid, it will never be selected.

oh, sorry I was looking at:

scion/go/lib/infra/modules/segfetcher/segreplyhandler.go

Line 160 in a1bbbb1

close(result.early)

and somehow thought that earlyTrigger is closed instead of result.eraly.

nevermind.

---

go/lib/infra/modules/segfetcher/fetcher.go, line 132 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

Yes. That is the current behavior as well. If we have multiple segments for the same type (e.g. multiple core segs) it could make sense to ignore the error and we might still get paths.
I don't want to overcomplicate things now.

ack