scmp/showpaths: remove local address flags #3691

matzf · 2020-03-23T15:28:49Z

scmp: determine the local address automatically

The local IA is obtained from the sciond connection (i.e. it is determined by the -sciond parameter).
The local IP address is determined by a routing table lookup (by creating a temporary, connected UDP socket), based on the next hop address.
The -local flag now allows to override this IP, to e.g. choose a specific network interface.

showpaths:

remove the redundant -srcIA parameter (was only used to check that it was entered correctly)
the local address for the health check is automatically determined, as described above. Unlike in the scmp case where the next hop of the selected path is used, here we determine the local IP based on a routing table lookup to the IP of address of an arbitrary infrastructure service, as this avoids restructuring the path prober too much (would have to create a separate connection for each path). This is not a fully general approach, but is intended as a "good enough" solution until snet supports wildcard addresses directly.
The -local flag now allows to override this IP, to e.g. choose a specific network interface.

Other changes:

fix binary integration tests setup; bin_wrapper.sh exit code was always 0, hiding failing test runs

This change is

oncilla

Reviewed 7 of 7 files at r1.
Reviewable status: 6 of 7 files reviewed, 11 unresolved discussions (waiting on @matzf and @oncilla)

a discussion (no related file):
Very nice work 💯
This is something that has been bugging us for some time now.

Thank you for creating the PR.

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

// Status indicates the state a path is in.
type Status struct {

I think this should now include the local address that was used for probing.
This will be useful information in showpaths.

We could now display which local address was used per path.

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

type Prober struct {
	DstIA    addr.IA
	Local    snet.UDPAddr

I think it is still useful to have a porber that works on a specific local address.

I think you should follow the same approach as scmp tool.
By default, resolve the addresses. If Local is set, use that instead.

To that end, make Local a pointer and only resolve addresses if p.Local == nil

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

	DstIA    addr.IA
	Local    snet.UDPAddr
	DispPath string

@sustrik Is this required?

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

type Prober struct {
	DstIA      addr.IA
	SciondConn sciond.Connector

I don't think this needs to be sciond aware.
All you need is the local IA + the paths. See bellow how to do the resolution.

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

	}

	localIA, err := findLocalIA(ctx, p.SciondConn)

I think that approach does not work in the general case.
You could have an AS where you can reach two distinct border routers using different interfaces.

What you want to do is to resolve per-path the local address.
I.e. for every path in paths: localAddr = findSrcIP(path.OverlayNextHop())

This will need a bit of restructuring of the code. Simplest would be to have one connection + go routine per path.
But I think it is worth the effort.

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

// findLocalIA gets the local IA from sciond
func findLocalIA(ctx context.Context, sciondConn sciond.Connector) (addr.IA, error) {

I don't think we should bake sciond support into this simple package.
I would rather have the caller configure the pathprober with the correct IA.

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

// findDefaultLocalIP returns _a_ IP of this host in the local AS.
func findDefaultLocalIP(ctx context.Context, sciondConn sciond.Connector) (net.IP, error) {

I don't think this is need (or the right thing to do). LocalIP might change based on the border router.

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

// findSrcIP returns the src IP used for traffic destined to dst
func findSrcIP(dst net.IP) (net.IP, error) {

You could make this a library function in snet/addrutil with the signature:
func ResolveLocal(dst *net.IPAddr) *net.IPAddr
or maybe for convenience func ResolveLocal(dst *net.UDPAddr) (*net.IPAddr, error)

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

// findAnyHostInLocalAS returns the IP address of some (infrastructure) host in the local AS.
func findAnyHostInLocalAS(ctx context.Context, sciondConn sciond.Connector) (net.IP, error) {

As mentioned above, this is not needed. You should take the first BR on the path, not any host.

go/tools/showpaths/paths.go, line 54 at r1 (raw file):

func init() {
	flag.Var(&local, "local", "Local address to use for health checks")

As mentioned in the pathprobe package, I think it will still be useful to have a tool where you can specify the exact local IP you want to use for show paths.

oncilla

Reviewed 1 of 1 files at r2.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @matzf)

sustrik

Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @matzf and @oncilla)

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

Previously, Oncilla wrote…

@sustrik Is this required?

I don't know, but it seems never to be set.

matzf

Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @matzf and @oncilla)

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

Previously, Oncilla wrote…

I think it is still useful to have a porber that works on a specific local address.

I think you should follow the same approach as scmp tool.
By default, resolve the addresses. If Local is set, use that instead.

To that end, make Local a pointer and only resolve addresses if p.Local == nil

Will do. Seems like this is required to make this work in this byzantine acceptance docker setup, anyway.

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

Previously, Oncilla wrote…

I think that approach does not work in the general case.
You could have an AS where you can reach two distinct border routers using different interfaces.

What you want to do is to resolve per-path the local address.
I.e. for every path in paths: localAddr = findSrcIP(path.OverlayNextHop())

This will need a bit of restructuring of the code. Simplest would be to have one connection + go routine per path.
But I think it is worth the effort.

I understand the limitations of the approach -- sorry for not documenting this in the PR.
I did not think it was worth the effort to refactor this code and fully generalize this, supporting setups that were not previously supported, because to me this is not more than a temporary workaround until snet properly supports wildcard addresses directly. Once this is available, the approach that you suggest would just look overcomplicated.
Personally, I'd prefer to keep this "good enough for now" solution (and maybe add some comments to indicate this), and put some effort into dusting off #3376.
What do you say?

matzf

Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @matzf and @oncilla)

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

Previously, Oncilla wrote…

You could make this a library function in snet/addrutil with the signature:
func ResolveLocal(dst *net.IPAddr) *net.IPAddr
or maybe for convenience func ResolveLocal(dst *net.UDPAddr) (*net.IPAddr, error)

Will do.

oncilla

Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @matzf and @oncilla)

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

Previously, matzf (Matthias Frei) wrote…

I understand the limitations of the approach -- sorry for not documenting this in the PR.
I did not think it was worth the effort to refactor this code and fully generalize this, supporting setups that were not previously supported, because to me this is not more than a temporary workaround until snet properly supports wildcard addresses directly. Once this is available, the approach that you suggest would just look overcomplicated.
Personally, I'd prefer to keep this "good enough for now" solution (and maybe add some comments to indicate this), and put some effort into dusting off #3376.
What do you say?

Tbh, I don't know what the plan for #3376 is, so I cannot really comment on that.
I get the "good enough" sentiment.

I think if you remove the sciond dependency + move findSrcIP to the library, the pathprobe is already in a mergable state.

matzf

Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @matzf and @oncilla)

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

Previously, Oncilla wrote…

I don't think we should bake sciond support into this simple package.
I would rather have the caller configure the pathprober with the correct IA.

Will do.

matzf

Reviewable status: 2 of 16 files reviewed, 11 unresolved discussions (waiting on @matzf and @oncilla)

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

Previously, matzf (Matthias Frei) wrote…

Will do. Seems like this is required to make this work in this byzantine acceptance docker setup, anyway.

Done.

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

Previously, Oncilla wrote…

I don't think this needs to be sciond aware.
All you need is the local IA + the paths. See bellow how to do the resolution.

Done.

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

Previously, Oncilla wrote…

Tbh, I don't know what the plan for #3376 is, so I cannot really comment on that.
I get the "good enough" sentiment.

I think if you remove the sciond dependency + move findSrcIP to the library, the pathprobe is already in a mergable state.

Done.

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

Previously, matzf (Matthias Frei) wrote…

Will do.

Done.

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

Previously, matzf (Matthias Frei) wrote…

Will do.

Done.

go/tools/showpaths/paths.go, line 54 at r1 (raw file):

Previously, Oncilla wrote…

As mentioned in the pathprobe package, I think it will still be useful to have a tool where you can specify the exact local IP you want to use for show paths.

Done.

oncilla

Reviewed 14 of 15 files at r3.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @matzf)

go/lib/sciond/sciond.go, line 84 at r3 (raw file):

// either an error occurs, or the method successfully returns.
type Connector interface {
	// LocalIA is a convenience function to query the local IA, using ASInfo

This is an interface. The internals do not matter.

// LocalIA requests form SCIOND the local ISD-AS number.

go/lib/sciond/sciond.go, line 86 at r3 (raw file):

	// LocalIA is a convenience function to query the local IA, using ASInfo
	LocalIA(ctx context.Context) (addr.IA, error)
	// Paths requests from SCIOND a set of end to end paths between src and

While already here, could you change this to // Path requests from SCIOND a set of end to end paths between the source and destination.

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

Previously, Oncilla wrote…

I think this should now include the local address that was used for probing.
This will be useful information in showpaths.

We could now display which local address was used per path.

Never mind, the approach does not resolve per path.

go/lib/snet/addrutil/addrutil.go, line 55 at r3 (raw file):

}

// ResolveLocal returns the local IP address used for traffic destined to dst

period at the end.

go/tools/scmp/main.go, line 151 at r3 (raw file):

}

// setLocalASInfo queries the local AS information from SCIOND; sets cmn.LocalIA and localMtu

period at the end.

unrelated side-note: that global state in this app makes me a :sad-panda:

go/tools/showpaths/paths.go, line 142 at r3 (raw file):

// possibility to have the same functionality, i.e. refresh, fetch all paths.
// https://github.com/scionproto/scion/issues/3348
func getPaths(sdConn sciond.Connector, ctx context.Context) ([]snet.Path, error) {

sdConn no longer needed

go/tools/showpaths/Readme.md, line 18 at r3 (raw file):

.. between the local AS and 2-ff00:0:222 ..

matzf

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

go/lib/sciond/sciond.go, line 84 at r3 (raw file):

Previously, Oncilla wrote…

This is an interface. The internals do not matter.

// LocalIA requests form SCIOND the local ISD-AS number.

Done.

go/lib/sciond/sciond.go, line 86 at r3 (raw file):

Previously, Oncilla wrote…

While already here, could you change this to // Path requests from SCIOND a set of end to end paths between the source and destination.

Done.

go/lib/snet/addrutil/addrutil.go, line 55 at r3 (raw file):

Previously, Oncilla wrote…

period at the end.

Done.

go/tools/scmp/main.go, line 151 at r3 (raw file):

Previously, Oncilla wrote…

period at the end.

unrelated side-note: that global state in this app makes me a :sad-panda:

Done.

Ah, yes, I agree, it's not awesome.

go/tools/showpaths/paths.go, line 142 at r3 (raw file):

Previously, Oncilla wrote…

sdConn no longer needed

It is still needed, unless I misunderstand what you mean. Maybe you were thinking of the variable of the same name in scmp/main.go (this is showpaths)?

go/tools/showpaths/Readme.md, line 18 at r3 (raw file):

Previously, Oncilla wrote…

.. between the local AS and 2-ff00:0:222 ..

Done.

Determine the local IA and local IP automatically: - The local IA is very clear and can be directly obtained from the sciond connection (i.e. it is determined by the -sciond parameter). - The local IP address is determined by a routing table lookup (by creating a temporary, connected UDP socket), based on the next hop address. The -local flag now allows to override this IP, to e.g. choose a specific network interface.

Remove the -local flag and automatically determine a sensible local IP address based on a routing table lookup to the IP of address of a infrastructure service.

Status code of tested binary was ignored due to the subsequent successful echo; always returned success.

Not sure if this is the same confusing setup as with the showpaths tests, where -local needs to be provided (as the dispatcher is running in a different container with a different IP address, if i understand correctly).

oncilla

Reviewed 4 of 4 files at r4.
Reviewable status: 15 of 16 files reviewed, all discussions resolved (waiting on @oncilla)

go/tools/showpaths/paths.go, line 142 at r3 (raw file):

Previously, matzf (Matthias Frei) wrote…

It is still needed, unless I misunderstand what you mean. Maybe you were thinking of the variable of the same name in scmp/main.go (this is showpaths)?

Never mind, I was derping hard 🙈

oncilla

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

scmp: determine the local address automatically - The local IA is obtained from the sciond connection (i.e. it is determined by the `-sciond` parameter). - The local IP address is determined by a routing table lookup (by creating a temporary, connected UDP socket), based on the next hop address. The `-local` flag now allows to override this IP, to e.g. choose a specific network interface. showpaths: - remove the redundant `-srcIA` parameter (was only used to check that it was entered correctly) - the local address for the health check is automatically determined, as described above. Unlike in the `scmp` case where the next hop of the selected path is used, here we determine the local IP based on a routing table lookup to the IP of address of an arbitrary infrastructure service, as this avoids restructuring the path prober too much (would have to create a separate connection for each path). This is not a fully general approach, but is intended as a "good enough" solution until `snet` supports wildcard addresses directly. The `-local` flag now allows to override this IP, to e.g. choose a specific network interface. Other changes: * fix binary integration tests setup; `bin_wrapper.sh` exit code was always 0, hiding failing test runs

matzf mentioned this pull request Mar 24, 2020

Extend scmp tool allow using hostnames netsec-ethz/scion-apps#133

Open

3 tasks

matzf force-pushed the scmp-local-addr branch from 38179ca to e22a38f Compare March 24, 2020 06:41

oncilla requested changes Mar 24, 2020

View reviewed changes

oncilla reviewed Mar 24, 2020

View reviewed changes

sustrik reviewed Mar 24, 2020

View reviewed changes

matzf commented Mar 24, 2020

View reviewed changes

oncilla requested changes Mar 24, 2020

View reviewed changes

matzf commented Mar 24, 2020

View reviewed changes

oncilla requested changes Mar 24, 2020

View reviewed changes

matzf commented Mar 24, 2020

View reviewed changes

matzf added 18 commits March 24, 2020 16:59

showpaths: remove unnecessary -srcIA flag

0302e2a

showpaths: automatically determine -local

783d836

Remove the -local flag and automatically determine a sensible local IP address based on a routing table lookup to the IP of address of a infrastructure service.

Fix lint

40b281f

Adapt showpath invocation in acceptance test

347986e

Fix lint, fix contexts

00ee18a

Remove leftover -local flag

ec283e2

Remove leftover import

c470f53

Add -local back, move helper functions to lib

e03077e

Extend doc string, update README

b5cf81f

Update sciond.Connector mock

3ab3022

Update scmp README

67f6fe8

Temporarily set debug loglevel for integration tests

ed1f7d0

Fix bin_wrapper exit code

ca453b4

Status code of tested binary was ignored due to the subsequent successful echo; always returned success.

Revert log level for integration tests to warn

3352976

Fix now actually failing scmp integration

4c07636

Not sure if this is the same confusing setup as with the showpaths tests, where -local needs to be provided (as the dispatcher is running in a different container with a different IP address, if i understand correctly).

Pass missing -sciond in scmp_integration

40fd8d3

scmp tests need -local in docker setup

a979a2f

Various doc cleanup

a9688a7

matzf force-pushed the scmp-local-addr branch from 0e48735 to a9688a7 Compare March 24, 2020 15:59

oncilla approved these changes Mar 24, 2020

View reviewed changes

oncilla merged commit 64e5ff9 into scionproto:master Mar 24, 2020

matzf deleted the scmp-local-addr branch March 24, 2020 17:53

oncilla mentioned this pull request Apr 11, 2020

Let scmp infer local address #3390

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

scmp/showpaths: remove local address flags #3691

scmp/showpaths: remove local address flags #3691

matzf commented Mar 23, 2020 •

edited

Loading

oncilla left a comment

oncilla left a comment

sustrik left a comment

matzf left a comment

matzf left a comment

oncilla left a comment

matzf left a comment

matzf left a comment

oncilla left a comment

matzf left a comment

oncilla left a comment

oncilla left a comment

scmp/showpaths: remove local address flags #3691

scmp/showpaths: remove local address flags #3691

Conversation

matzf commented Mar 23, 2020 • edited Loading

oncilla left a comment

Choose a reason for hiding this comment

oncilla left a comment

Choose a reason for hiding this comment

sustrik left a comment

Choose a reason for hiding this comment

matzf left a comment

Choose a reason for hiding this comment

matzf left a comment

Choose a reason for hiding this comment

oncilla left a comment

Choose a reason for hiding this comment

matzf left a comment

Choose a reason for hiding this comment

matzf left a comment

Choose a reason for hiding this comment

oncilla left a comment

Choose a reason for hiding this comment

matzf left a comment

Choose a reason for hiding this comment

oncilla left a comment

Choose a reason for hiding this comment

oncilla left a comment

Choose a reason for hiding this comment

matzf commented Mar 23, 2020 •

edited

Loading