showpaths: Fix probing

[lukedirtwalker]

Also abstract the probing into its own library so that other modules can also use it.
Also introduce an acceptance test for showpaths probing.

Fixes #3126

This change is 

[oncilla]

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

---

acceptance/showpaths_status_acceptance/test, line 48 at r1 (raw file):

            self.tools_dc('start', 'tester*')
            self.docker_status()

should be 2 lines between definitions. Damn linter does not lint the test files :sad-panda:

---

go/lib/sciond/pathprobe/paths.go, line 50 at r1 (raw file):

// Predefined path status
var (
	Unknown = PathStatus{status: statusUnknown}

Why did you choose to make this public and not the constants above.
As a client, this package will be ~hard to use. E.g. there is no easy way to check which status was returned.

E.g. to check whether the status is SCMP you need to do:
status != pathprobe.Unknown && status != pathprobe.Timeout && status != pathprobe.Alive

Plus the check is very brittle, e.g. additional status or some change that adds additional info to the response for Alive/Timeout.

I suggest you make the constants public and make:

type PathStatus struct {
  Status string
  AdditionalInfo string
}

the vars, you can still use privately.

---

go/lib/sciond/pathprobe/paths.go, line 63 at r1 (raw file):

// PathProber can be used to get the status of a path.
type PathProber struct {

name is stutter-y pathprobe.PathProber maybe just Prober

---

go/lib/sciond/pathprobe/paths.go, line 83 at r1 (raw file):

	// the path is alive.
	pathStatuses := make(map[string]PathStatus, len(paths))
	scmpH := scmpHandler{mtx: &sync.Mutex{}, statuses: pathStatuses}

slightly awkward initialization, because scmpHandler has a value receiver.

---

go/lib/sciond/pathprobe/paths.go, line 86 at r1 (raw file):

	network := snet.NewCustomNetworkWithPR(p.Local.IA,
		&snet.DefaultPacketDispatcherService{
			Dispatcher:  reliable.NewDispatcherService(""),

the path should probably be configurable.

---

go/lib/sciond/pathprobe/paths.go, line 91 at r1 (raw file):

		nil,
	)
	if err := snet.InitWithNetwork(network); err != nil {

This will break if application already has initialized the network. Why do you need to set the network? You could just use network.ListenSCION below.

Also, tbh. I think network initialization should probably not be part of this library, let the caller take care of it.

---

go/lib/sciond/pathprobe/paths.go, line 104 at r1 (raw file):

	}
	for _, path := range paths {
		scmpH.setStatus(string(path.Path.FwdPath), Timeout)

It would be good to have a public function PathKey(fwdPath []byte) string such that package clients can easily get the path keys.
Reading the doc string, It is not obvious to me that it is simply string(fwdPath). Plus, in case we want to change it -> simple refactor.

---

go/lib/sciond/pathprobe/paths.go, line 105 at r1 (raw file):

	for _, path := range paths {
		scmpH.setStatus(string(path.Path.FwdPath), Timeout)
		p.sendTestPacket(scionConn, path)

this ignores the errors, is that by design?
e.g. If a test packet fails to be sent, it appears to have timed out.

---

go/lib/sciond/pathprobe/paths.go, line 108 at r1 (raw file):

	}
	for i := len(scmpH.statuses); i > 0; i-- {
		err := p.receiveTestReply(scionConn)

I think here would be a good place for common.MultiError (maybe also the send errors above)

---

go/lib/sciond/pathprobe/paths.go, line 116 at r1 (raw file):

}

func (p PathProber) sendTestPacket(scionConn *snet.SCIONConn, path sciond.PathReplyEntry) error {

just send?

---

go/lib/sciond/pathprobe/paths.go, line 142 at r1 (raw file):

}

func (p PathProber) receiveTestReply(scionConn *snet.SCIONConn) error {

just receive?

---

go/lib/sciond/pathprobe/paths.go, line 149 at r1 (raw file):

		return nil
	}
	if xerrors.Is(err, errBadHost) || xerrors.Is(err, errSCMP) {

💯

[lukedirtwalker]

Reviewable status: 3 of 6 files reviewed, 11 unresolved discussions (waiting on @oncilla)

---

acceptance/showpaths_status_acceptance/test, line 48 at r1 (raw file):

Previously, Oncilla wrote…

should be 2 lines between definitions. Damn linter does not lint the test files :sad-panda:

Done.

---

go/lib/sciond/pathprobe/paths.go, line 50 at r1 (raw file):

Previously, Oncilla wrote…

Why did you choose to make this public and not the constants above.
As a client, this package will be ~hard to use. E.g. there is no easy way to check which status was returned.

E.g. to check whether the status is SCMP you need to do:
status != pathprobe.Unknown && status != pathprobe.Timeout && status != pathprobe.Alive

Plus the check is very brittle, e.g. additional status or some change that adds additional info to the response for Alive/Timeout.

I suggest you make the constants public and make:

type PathStatus struct {
  Status string
  AdditionalInfo string
}

the vars, you can still use privately.

Done.

---

go/lib/sciond/pathprobe/paths.go, line 63 at r1 (raw file):

Previously, Oncilla wrote…

name is stutter-y pathprobe.PathProber maybe just Prober

Done.

---

go/lib/sciond/pathprobe/paths.go, line 83 at r1 (raw file):

Previously, Oncilla wrote…

slightly awkward initialization, because scmpHandler has a value receiver.

Done.

---

go/lib/sciond/pathprobe/paths.go, line 86 at r1 (raw file):

Previously, Oncilla wrote…

the path should probably be configurable.

Done.

---

go/lib/sciond/pathprobe/paths.go, line 91 at r1 (raw file):

Previously, Oncilla wrote…

This will break if application already has initialized the network. Why do you need to set the network? You could just use network.ListenSCION below.

Also, tbh. I think network initialization should probably not be part of this library, let the caller take care of it.

Done.

---

go/lib/sciond/pathprobe/paths.go, line 104 at r1 (raw file):

Previously, Oncilla wrote…

It would be good to have a public function PathKey(fwdPath []byte) string such that package clients can easily get the path keys.
Reading the doc string, It is not obvious to me that it is simply string(fwdPath). Plus, in case we want to change it -> simple refactor.

Done.

---

go/lib/sciond/pathprobe/paths.go, line 105 at r1 (raw file):

Previously, Oncilla wrote…

this ignores the errors, is that by design?
e.g. If a test packet fails to be sent, it appears to have timed out.

Done.

---

go/lib/sciond/pathprobe/paths.go, line 108 at r1 (raw file):

Previously, Oncilla wrote…

I think here would be a good place for common.MultiError (maybe also the send errors above)

Done.

---

go/lib/sciond/pathprobe/paths.go, line 116 at r1 (raw file):

Previously, Oncilla wrote…

just send?

Done.

---

go/lib/sciond/pathprobe/paths.go, line 142 at r1 (raw file):

Previously, Oncilla wrote…

just receive?

Done.

[oncilla]

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

---

go/lib/sciond/pathprobe/paths.go, line 78 at r2 (raw file):

// Prober can be used to get the status of a path.
type Prober struct {
	SrcIA    addr.IA

Do we actually need SrcIA?
It should be probably already set in Local, and if not, it will be set to the local IA, no?

[lukedirtwalker]

Reviewable status: 4 of 6 files reviewed, 1 unresolved discussion (waiting on @oncilla)

---

go/lib/sciond/pathprobe/paths.go, line 78 at r2 (raw file):

Previously, Oncilla wrote…

Do we actually need SrcIA?
It should be probably already set in Local, and if not, it will be set to the local IA, no?

Done.

[oncilla]

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

[oncilla]

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