Filter duplicate paths in path combinator

[FR4NK-W]

Combine should not return dulicate paths,
sciond does not filter separately.

---

This change is 

[scrye]

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @FR4NK-W and @scrye)

a discussion (no related file):
I think the combinator is working as intended. While the forwarding interfaces are the same, from a crypto point of view the paths are different. This means that one path can be valid for forwarding, while the other would fail. If SCIOND chooses only one of the paths, and it is the wrong one, the application has no way of obtaining the other one.

For completeness, I'm also pinning here the unit test you linked on chat: https://github.com/scionproto/scion/blob/master/go/lib/infra/modules/combinator/testdata/00_loops.txt . The first two paths are an example of same forwarding, but different crypto due to different segments being used.

I discussed this a bit with @kormat, and it's probably better to have applications be aware that two different paths can have the same interface representation. Can you please include here a detailed description of how this negatively impacts the SIG? We can then define the use case we want, and later fix it in the SIG directly.

---

[scrye]

Reviewed 1 of 1 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @FR4NK-W)

[FR4NK-W]

@scrye
This impacts the ability of the SIG to fail over to a working path after a link failure.
The SIG tries repeatedly to use a path that is identical at the interfaces level as the failed one.
The SIG does not get a new path that is different in the interfaces used.

E.g. There are 4 paths from A to B that are identical at the interface level as the one in use and 1 with different interfaces.
A link fails on the path currently in use. The SIG tries to get a new path with getNewPath, different from the current one and will use a path with the with different HFs but with the same interfaces.
The failure count on the paths is not sufficient to recover from this state.

In the case shown below, where the ASes involved in the cryptographic verification are the same, is there still a reason for keeping multiple ways of representing/verifying the same forwarding path?
Would the pathmgr be the right place to handle this ambiguity between forwarding path and cryptographic verification of the path (so that not each application has to be explicitly aware / implement its own handling)?
E.g.:

         C                                              Path #0:
      12| |13                                            Fields:
        | |                                                  IF CS. ISD=1
      21| |31                                                 HF .V InIF=0 OutIF=12
         B                                                    HF .. InIF=21 OutIF=14
       14|                                                    HF .. InIF=41 OutIF=0
         |                                                Interfaces:
       41|                                                  B#14
         A                                                  A#41


                                                         
                                                         Path #1:
                                                           Fields:
                                                             IF CS. ISD=1
                                                               HF .V InIF=0 OutIF=13
                                                               HF .. InIF=31 OutIF=14
                                                               HF .. InIF=41 OutIF=0
                                                           Interfaces:
                                                             B#14
                                                             A#41

[juagargi]

In the near future applications may want to define their own particular way to compare paths, but I agree that most of them will not want to have the same interface-identical paths repeated. For this, I would also keep a call not filtering paths with identical interfaces (see note in the implementation).

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @FR4NK-W)

---

go/lib/infra/modules/combinator/combinator.go, line 54 at r1 (raw file):

		pathSlice = append(pathSlice, path.GetFwdPathMetadata())
	}
	shortPaths := FilterLongPaths(pathSlice)

I would also leave an implementation of the current no-filtered behavior, with an explicit name (something like CombineAllowSameIfaces but better named).

[scrye]

@FR4NK-W:

The failure count on the paths is not sufficient to recover from this state.

The SIG should recover. If it doesn't, it's a bug (if path 1 is bad, and path 2 has the same interfaces and is also bad, path 3 should be tried, and so on and so forth). Also, if one of the forwarding interfaces is down, the revocation should flush all the paths from the app. What is the exact behavior you're seeing when the SIG attempts to recover?

@juagargi:

but I agree that most of them will not want to have the same interface-identical paths repeated.

Having identical paths should not affect application logic, because applications should always look at the raw forwarding path as a black box. If the interfaces are identical, apps should just make the assumption that the raw path is somehow different, and should in effect track health separately.

What we could do is have SCIOND have a bias towards non-duplicates, meaning that if an app asks for K paths, SCIOND will give lower scores to duplicates, so apps get maximum interface diversity out of the partial answer. Would something like this fix the issue you're seeing?

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @FR4NK-W)

---

go/lib/infra/modules/combinator/combinator.go, line 54 at r1 (raw file):

Previously, juagargi (Juan A. García Pardo Giménez de los Galanes) wrote…

I would also leave an implementation of the current no-filtered behavior, with an explicit name (something like CombineAllowSameIfaces but better named).

The SCIOND IPC API would need to change then, s.t. clients can select between the two. It's a breaking change, but we can do it if there are no other solutions. I'm wondering if the scoring suggestion above works though, because that would mean we don't need any API changes.

[scrye]

After discussing this internally for a bit, we agree that SCIOND should be able to delete duplicates, but this should be configurable and not the default. This means the combinator is not the place to do the filtering (the combinator should always build everything), and it should be done instead on the Combine call site (where callers can freely process the path set).

It might be the case that for Anapaya deployments we will also enable this in the near future.

To merge this, the following changes are needed:

• duplicate paths removal logic moved out of package combinator, and into SCIOND code;
• duplicate paths removal logic should be hidden behind a feature flag (see Feature flags #3030 for feature flag support);
• the default flag setting (false) should be to not filter duplicates (so the default should be the current logic on master);
• the duplicate removal function should have unit tests.

[scrye]

With #3930 (and due to the new data-plane path format), duplicate paths are now filtered. Closing this.