fix: properly include peering links into segments #4890
Conversation
tzaeschke
left a comment
There was a problem hiding this comment.
This is quite a big PR. I thinnk my main concern is that we appear to break some central SCION assumptions:
- we have at most one of each of UP, CORE, DOWN (e.g. UP is never followed by UP)
- Between CORE ASes we have only CORE segments, no UP or DOWN.
Also, in some places we seem to have UP and DOWN for core ASes, in other only UP.
This code is new to me, and I haven't implemented peering myself yet, so I assume I misunderstand many things, maybe they could be clarified?
|
|
||
| // validNextSeg returns whether nextSeg is a valid next segment in a path from the given currSeg. | ||
| // A path can only contain at most 1 up, 1 core, and 1 down segment. | ||
| // Exception: one-hop segments (for core peering) can follow up segments. |
There was a problem hiding this comment.
Is this really an exception? Isn't the one-hop segment also a core segment?
There was a problem hiding this comment.
After reading the rest of the PR, this seems to be correct, one-hop segments in the core don't appear to be core segments....
|
|
||
| for _, b := range beacons[beacon.DefaultGroup] { | ||
| if w.Intfs.Get(b.InIfID) == nil { | ||
| // TODO: less hacky? |
There was a problem hiding this comment.
Maybe clarify what that TODO means?
| // registered remotely), which is what we need for one-hop segments. | ||
| writers := []*periodic.Runner{t.segmentWriter(beacon.RegPolicyTypeCore)} | ||
| if hasPeeringInterfaces(t.AllInterfaces) { | ||
| writers = append(writers, t.segmentWriter(beacon.RegPolicyTypeUp)) |
There was a problem hiding this comment.
This feels like a hack to me. The user of this function requests a CORE segment and instead gets an UP segment?
This relates to the comment in graph.go, where a UP segment can now be followed by another UP segment. Is there no other way of doing this?
| } | ||
| return up, nil | ||
| case seg.TypeUp: | ||
| // Up segment requests are normally resolved locally. The exception is |
There was a problem hiding this comment.
As mentioned elsewhere, I find it a bit confusing that we have UP segments here. As I understand, they are only used when peering between cores?
Why are there UP segments between cores? Is there no other way?
| toRegister = append(toRegister, &seg.Meta{Type: r.Type, Segment: b.Segment}) | ||
| logBeacons[b.Segment.GetLoggingID()] = b | ||
|
|
||
| // For one-hop segments (single AS entry with 0/0 hop field), store as both |
There was a problem hiding this comment.
If I understand correctly, here it says that we have UP and DOWN segments for one-hop core segments. In other places we have to have only UP segments. Why do we have sometimes only UP and sometimes UP and DOWN?
|
|
||
| // SegmentsToRegister returns a GroupedBeacons to register at the time of the call. | ||
| // The selection is based on the configured policy for the requested segment type. | ||
| // For TypeDown or TypeUp, returns an empty map (core ASes don't have up/down beacons, |
There was a problem hiding this comment.
Actually, in other places it says that we now do have UP and DOWN (on-hop-)segments for core ASes....?
| switch currSeg.Type { | ||
| case proto.PathSegType_up: | ||
| // Allow transitioning to one-hop up segments for core peering | ||
| if nextSeg.Type == proto.PathSegType_up && isOneHopSegment(nextSeg) { |
There was a problem hiding this comment.
Is there no other way than allowing UP after UP segments? This breaks a very fundamental assumption about SCION, i.e. having maximum one of each UP/CORE/DOWN.
I would be a bit concerned that many other places, including communication with endhosts or other control server implementations, may be affected by this. This assumption is probably also manifested in many places in scionproto, it feels to me like it should be documented prominently for future developers that this assumption no longer holds....
Maybe I misunderstand something fundamentally here?.
There was a problem hiding this comment.
I realized that I developed a tunnel vision with this approach, so I ended up asking AI if it's possible to find another way, without UP after UP segments. It produced a solution that seems rather elegant. In it, one-hop segments are stored only as Down segments, so they are destination-side only. The source-side peering edges are instead extracted directly from the last ASEntry's PeerEntries in core segments. I made a separate pull request to make comparison in the Github UI somewhat simpler: #4905 What do you think?
Also, while writing that code, AI was able to find a minor bug in the tests in this PR, which I pushed here too.
There was a problem hiding this comment.
Thanks for the effort, that looks better to me.
Unfortunately, I still don't quite understand how core peering works. Is there a document that describes that?
There was a problem hiding this comment.
Unfortunately, I still don't quite understand how core peering works. Is there a document that describes that?
To be clear, this is not something I necessarily expect from this PR, it is just a general question.
There was a problem hiding this comment.
Do you have any concrete questions regarding #4905, or is it about the algorithm overall?
Sorry, I didn't see your answer. The question is concerning the algorithm overall. Maybe I should raise a issue.
That said, I seem to remember you discussed this with Anapaya? Wasn't there a document involved?
There was a problem hiding this comment.
There was a document, mostly about approaches to fix the problem: https://docs.google.com/document/d/1MYvrlT162CAA4kjZ6l8rNH71S5MWMzan9Pz_f09hfV8/edit?tab=t.0#heading=h.b9uh5g8baxne
To be honest I do not really share the concern. An up and down segment are the same thing. There is no difference between them other than we call them "up" or "down" depending on the context that they are being used in. FWIW, SCION would have worked just as well with "core" and "non-core" segments. Up just means "non-core" segment that is traversed against construction direction, Down just means "non-core" segment that is traversed in construction direction. This PR does not change anything in that regard, it also does not change the SCION invariants regarding segment traversal. Now, I don't have the whole code in mind anymore, and I need to read through it again. There might be an alternative way such that we do not confuse "up" and "down" segments in terminology. But I think the approach itself is sound. Attaching peering links to core segments is strictly worse to me. Here we change the SCION invariants because we now are allowed to use peering links between core and non core segments. This will require additional dataplane checks that we do not traverse the core segment before going into a peering link. An abuse scenario which we do not even need to think about with the current approach. (I have not give an in-depth read on the other PR, so take that statement with a grain of salt.) |
|
A bit more background why I was concerned about up+up. I agree, they are all just segments and differ only in the construction direction. However:
@oncilla Now I am confused, as far as I understand that invariant is already broken because it is already allowed to use peering links between core and non core segments (the IETF spec now specifically allows peering between any nodes, including cores). This PR only fixes a problem with the code that breaks this invariant...? Sorry if I misunderstand what you are saying. Also, could you briefly explain what the problem is if we traverse a core link before going into a peering link? If that is a problem, it would eliminate more options of what is possible (e.g. there could not be any peering between any of (B),(C),(D) or (E) with each other or with (Y1) or (Y2) in example 5 here, because they inevitably already have UP (X2->A) and CORE (A->B). I realize I am hijacking this thread for a general discussion, so please let me know if you think it's easier if I wait for the document to be available. I am happy to wait :) |
I agree with you. The second segment should be referred to as "down". I will need to read through the code and see how we can amend this.
We will not end up with 4 segments no. In fact if we traverse a peering link we will always only end up with exactly 2 segments.
I think this is what leads to the confusion. The fact that a core AS is involved in the peering link should not change what type of segments are traversed. Even in that case, we traverse a "up" and "down" segment. (With at least one of them being a single-hop segment)
It is important to distinguish between core ASes and core segments.
The goal of peering links is to offer connectivity between the two "cones" of downstreams that are spawned by the two ASes that peer together. It is an explicit non-goal to offer the cone transit to the SCION internet. Stitching a core segment with a down segment, or up segment with a core segment would however do exactly that. |
|
@oncilla Thanks, that is really helpful, it clarifies some thing for me. For context, SCION IETF CP draft states (Section 1.3): This appears to be incorrect. So as I understand it there are now two scenarios, the (simple) main scenario and the more complex edge case with up+core / core+down / core. Main scenario:Peering links go from any AS on an UP segment to any AS on a DOWN segment (these ASes may be CORE ASes). Edge cases:There are up to three scenarios outside the main scenario:
So my open questions are:
EDIT:
|
Should we update the IETF drafts to clarify this? I was about to send the draft for publication later today.. I could try to package it as part of the final review clarifications. If we do, we need to be really sure about it, as we are really at the last step |
I might misunderstand what a CORE segment here means, but here're my 2 cents: They can in certain situations. For example, see the test in this PR: both 120 and 410 are core ASes, and the peering link exists. Also, see the test for 410 and 123: we can use the peering link between 410 and 120 to further go down to 123 from 120. If these two examples are forbidden, then there's no sense in peering links in a way they already exist in the world. Peering links exist for the commercial contracts between two ASes in my understanding. Direct traffic should always flow between them. We sometimes restrict if it's the traffic that involves any other ASes. |
|
@nicorusti This also affects DP: (also, the last sentence seems garbled) The following is clearer, but contradicts what was discussed in the slack discussion which seem to state that up+core and core+down can also have peering links.
This is correct, however, there is the "loophole" (the edge cases I mention above) where a up+core/core+down/core is very much allowed as long as the core segment is replaced by a 1-AS up or down segment.
As my questions above indicate, the point of allowing up+core and core+down is unclear to me and I am not sure how it is supposed to work. How about rephrasing the spec to something like: CP draft (Section 1.3): "Peering links exist between ASes with a peering relationship (settlement-free or paid). DP Section 1.1: |
|
Allowing core+down and up+core (and core only?) can be done in the next iteration, but currently I think these would require more explanation than just rearranging a sentence. |
|
@katyatitkova Thanks, could you paste a diagram of the topology? This basically is question 2. and 3. here: |
|
@tzaeschke The diagram is here: https://github.com/scionproto/scion/pull/4890/changes#diff-2761d4ea424d2914a49640a110ec5574629756311f937b5d24306ebcd71097ba. The tests are for combinator only, but it's possible to add an integration test too if needed (it will be rather trivial, but they'll require longer times to set up and run compared to unit tests). Since it's unit tests only, we provide the segments to the test, with a helper for one-hop segments |
|
@katyatitkova Thanks. I meant more like a graphical diagram, but it's not important. |
|
@tzaeschke In my opinion, the peering link itself is allowed to exist between any two ASes. The reason why it exists is more about their internal agreements and contracts, and it doesn't make sense to restrict creation of the peering links. The usage might be restricted, but I think there are already enough mechanisms in the draft that explain those restrictions. The paths must be valley-free, which already disallows to use a certain peering link in a certain scenario (but it's still valid to use it in another scenario). Having multiple peering links in a path is already disallowed, or connecting two segments of the same type with a peering link. So, in my opinion, there's no need to modify the drafts. What do you think? |
I do not follow the conclusion. The draft is talking about peering relationships and AS types. It does not talk about segment types.
No they do not. And this is exactly why we introduce these one-hop segments.
No, peering links should not be advertised in core segments.
How did you try it? With this PR?
These edge cases do not exist. The main scenario allows to cross ISD borders.
|
|
@katyatitkova Thanks you. I don't think peering between any two ASes is possible, counterexamples:
This third option gives me headaches. From the design perspective, I see several problems:
For reference, I created two PRs: |
|
@oncilla Thanks again.
Exactly. It just says "any two ASes", which includes ASes on CORE segment, which, as I understand, is not allowed? It is more obvious here where it talks about segment but does that it cannot be CORE segments (SCION DP 1.1):
Okay, to clarify my question: they do exist as input (it is what a an endhost receives from a path service) but not as output (the core is replaced with up or down), right? In case you mean that they don't even exist as input, could you give an example where we need the 1-AS segment?
Yes I did try, but not with this PR. I assumed it would be working in the production network.
Thanks again for all the input @oncilla and @katyatitkova |
I might misunderstand your point, but core ASes can be on core segments and on non-core segments.
I don't read that sentence as contradicting to anything I said above. Could you point me to exactly what you mean.
They do neither exist in input, nor in output. All the peering information comes from non-core segments.
As mentioned above. They are used if a core AS (not core segment) is involved in the peering link. Example 1: Involved segments:
Example 2: Involved segments:
Peering links are not implemented on the Anapaya appliance, and support in the open-source is also not finished. Checking against production will not show you anything. |
I still don't quite understand this, so maybe we need to go through this step by step. E.g. for example 1:
I assumed the following: If I understand you correctly, this is wrong because there is a core segment used in 3) and 4) and this is wrong.
It talks about two partial segments that can be combined, but it does not specify that they cannot be core segments, instead they must be one up and one down segment (possibly 1-AS segments). |
E1 notices that it is in a core AS (through the TRC), hence it sends the following requests:
E1 then combines the 1-AS up segment it found, with the C2 -> N1 down segment it found. Not utilizing the core segment. Note: Here, core segments are used during the segment lookup (exactly the same as they were before). But they are not used during path construction (neither input nor output) when finding a path that traverse the peer link.
Your sentence is contradicting: "two partial segments that can be combined, but it does not specify that they cannot be core segments" A core segment is neither an up nor a down segment. Hence it is not allowed to be used to peer.
Only peering links can be used for peering. Core links cannot be used as peering links |
Thanks, that makes sense. I guess what confused me was the slack thread which says "I guess you'd need to actually request a down-segment to a down-stream AS".
Exactly, that is why I put the word "instead" between the two statements. To be more clear: the spec simply specifies that segment can be combined, but fails to say that only up and down segments can be combined. As in implementer this would be very confusing because if I read that I would try to implement it for all types of segments, including core segments.
Yes.
Apologies, I meant to write " core segments cannot be used in peering?". I think the answer is "yes". |
|
The part regarding a peering link connecting up and down segment is in the draft already: https://datatracker.ietf.org/doc/html/draft-dekater-scion-dataplane#section-4.2.2 The quoted part of the same draft is intentionally high-level |
The path combination algorithm was written when peering links with a core AS on either end were not allowed. Now they are allowed, and sometimes the valid paths that include such peering links are not being found. This PR fixes it by introducing one-hop segments: single AS entries (ConsIngress=0, ConsEgress=0) with peer entries for each peering link. They are stored as both Up and Down types.
src == dstis used to request such one-hop segments. One-hop segments can follow an up segment.Big thanks to @oncilla, who helped me to debug the issue and proposed an idea of one-hop segments together with @shitz.
There were a lot of corner cases, that's why
peering_test.gois so big. I tried to cover every possible combination of types of ASes (core to core, core to non-core and so on). All the tests are using the graph which picture is included into this PR. Please tell me if I missed any combination either in the tests or in the graph.Fixes #4828