PS: Add new lookup logic (not enabled yet) #2978
Conversation
There was a problem hiding this comment.
Reviewed 12 of 12 files at r1.
Reviewable status: all files reviewed, 18 unresolved discussions (waiting on @lukedirtwalker)
go/path_srv/internal/handlers/common.go, line 55 at r1 (raw file):
IA addr.IA TopoProvider topology.Provider Messenger infra.Messenger
segfetcher.API?
go/path_srv/internal/segreq/db.go, line 36 at r1 (raw file):
// PSPathDB is a wrapper around the path db that handles retries and changes // GetNextQuery behavior for usage in segfetcher. type PSPathDB struct {
drop PS
go/path_srv/internal/segreq/db.go, line 51 at r1 (raw file):
case <-time.After(db.RetrySleep): } res, err = db.PathDB.Get(ctx, params)
maybe add a comment why you expect that the PathDB will contain that info at some point.
(I for one do not get why it would)
go/path_srv/internal/segreq/db.go, line 76 at r1 (raw file):
// core segments or a local down segment. func (i *CoreLocalInfo) IsSegLocal(ctx context.Context, src, dst addr.IA) (bool, error) { isCore, err := i.CoreChecker.IsCore(ctx, dst)
I think the following would be easier to follow:
// All local core and down segments.
if dst.I == i.LocalIA.i {
return true, nil
}
// All core segments.
isCore, err := i.CoreChecker.IsCore(ctx, dst)
if err != nil {
return err
}
return isCore, nilgo/path_srv/internal/segreq/db.go, line 85 at r1 (raw file):
// IsParamsLocal returns whether params is a core segment request. func (i *CoreLocalInfo) IsParamsLocal(params *query.Params) bool { return len(params.SegTypes) == 1 && params.SegTypes[0] == proto.PathSegType_core
why are down segments not local?
go/path_srv/internal/segreq/db.go, line 98 at r1 (raw file):
func (i *NonCoreLocalInfo) IsSegLocal(ctx context.Context, src, dst addr.IA) (bool, error) { // Check if it is an up segment request. if src.Equal(i.LocalIA) && dst.I == i.LocalIA.I {
Do we not support requests of the form ( src==someCore -> dst == localIA)?
Or do we enforce somewhere that upseg -> src==localIA and if downseg -> dst != localIA
go/path_srv/internal/segreq/db.go, line 99 at r1 (raw file):
// Check if it is an up segment request. if src.Equal(i.LocalIA) && dst.I == i.LocalIA.I { return i.CoreChecker.IsCore(ctx, dst)
Why do we still need to check whether IsCore.
We are on one end of the segment anyway.
go/path_srv/internal/segreq/db_test.go, line 184 at r1 (raw file):
func TestCoreLocaInfo(t *testing.T) {
wut?
go/path_srv/internal/segreq/doc.go, line 16 at r1 (raw file):
// Package segreq contains everything that is needed to handle segment requests // in the path server. It relieas on the segfetcher module and therefore has
s/relieas/relies
go/path_srv/internal/segreq/handler.go, line 48 at r1 (raw file):
PathDB: args.PathDB, RevCache: args.RevCache, Messenger: args.Messenger,
this should be segfetcher.API
go/path_srv/internal/segreq/helpers.go, line 77 at r1 (raw file):
} // XXX(roosd): Dirty hack to avoid exceeding jumbo frames until quic is implemented.
Will this still be necessary? 😞
go/path_srv/internal/segreq/provider.go, line 81 at r1 (raw file):
// for a core segment request we have to request the segments at the // given start point (core PS). return p.coreSvcAddr(ctx, addr.SvcPS, []addr.IA{req.Src})
what if src is localIA? (or does this implicitly assume that we will never get here because of the IsLocal check?)
go/path_srv/internal/segreq/splitter.go, line 28 at r1 (raw file):
// PsSplitter splits requests for the PS. type PsSplitter struct {
just Splitter?
It's internal to the PS anyway.
go/path_srv/internal/segreq/splitter.go, line 44 at r1 (raw file):
} switch { case !srcCore && dstCore:
should this check whether they are in the same ISD?
go/path_srv/internal/segreq/splitter.go, line 49 at r1 (raw file):
return segfetcher.RequestSet{Cores: segfetcher.Requests{r}}, nil case srcCore && !dstCore: return segfetcher.RequestSet{Down: r}, nil
ditto
go/path_srv/internal/segreq/splitter.go, line 55 at r1 (raw file):
} func (s *PsSplitter) isCore(ctx context.Context, dst addr.IA) (bool, error) {
s/dst/ia
go/path_srv/internal/segreq/splitter.go, line 59 at r1 (raw file):
return false, common.NewBasicError(segfetcher.InvalidRequest, nil, "reason", "empty ia") } if s.isWildCard(dst) {
ia.IsWildcard()
go/path_srv/internal/segreq/splitter_test.go, line 33 at r1 (raw file):
) func TestSplitter(t *testing.T) {
Wildcard tests missing.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 18 unresolved discussions (waiting on @lukedirtwalker and @oncilla)
go/path_srv/internal/handlers/common.go, line 55 at r1 (raw file):
Previously, Oncilla wrote…
segfetcher.API?
Done.
go/path_srv/internal/segreq/db.go, line 36 at r1 (raw file):
Previously, Oncilla wrote…
drop PS
Done.
go/path_srv/internal/segreq/db.go, line 51 at r1 (raw file):
Previously, Oncilla wrote…
maybe add a comment why you expect that the PathDB will contain that info at some point.
(I for one do not get why it would)
Done. (on the method doc string)
go/path_srv/internal/segreq/db.go, line 76 at r1 (raw file):
Previously, Oncilla wrote…
I think the following would be easier to follow:
// All local core and down segments. if dst.I == i.LocalIA.i { return true, nil } // All core segments. isCore, err := i.CoreChecker.IsCore(ctx, dst) if err != nil { return err } return isCore, nil
Done.
go/path_srv/internal/segreq/db.go, line 85 at r1 (raw file):
Previously, Oncilla wrote…
why are down segments not local?
some of them are. Done.
go/path_srv/internal/segreq/db.go, line 98 at r1 (raw file):
Previously, Oncilla wrote…
Do we not support requests of the form ( src==someCore -> dst == localIA)?
Or do we enforce somewhere that
upseg -> src==localIAandif downseg -> dst != localIA
Right we only need to check the source, the validator should do the rest.
go/path_srv/internal/segreq/db.go, line 99 at r1 (raw file):
Previously, Oncilla wrote…
Why do we still need to check whether
IsCore.
We are on one end of the segment anyway.
Done.
go/path_srv/internal/segreq/db_test.go, line 184 at r1 (raw file):
Previously, Oncilla wrote…
wut?
Done.
go/path_srv/internal/segreq/doc.go, line 16 at r1 (raw file):
Previously, Oncilla wrote…
s/relieas/relies
Done.
go/path_srv/internal/segreq/handler.go, line 48 at r1 (raw file):
Previously, Oncilla wrote…
this should be
segfetcher.API
Done.
go/path_srv/internal/segreq/helpers.go, line 77 at r1 (raw file):
Previously, Oncilla wrote…
Will this still be necessary? 😞
not sure TBH. We should send only one type of segment per request so it might be less. But I'd leave it until we have quic enabled.
go/path_srv/internal/segreq/splitter.go, line 28 at r1 (raw file):
Previously, Oncilla wrote…
just
Splitter?
It's internal to the PS anyway.
Done.
go/path_srv/internal/segreq/splitter.go, line 44 at r1 (raw file):
Previously, Oncilla wrote…
should this check whether they are in the same ISD?
No, but we should have a validator that makes sure that this can never happen.
go/path_srv/internal/segreq/splitter.go, line 49 at r1 (raw file):
Previously, Oncilla wrote…
ditto
No, but we should have a validator that makes sure that this can never happen.
go/path_srv/internal/segreq/splitter.go, line 55 at r1 (raw file):
Previously, Oncilla wrote…
s/dst/ia
Done.
go/path_srv/internal/segreq/splitter.go, line 59 at r1 (raw file):
Previously, Oncilla wrote…
ia.IsWildcard()
Done.
go/path_srv/internal/segreq/splitter_test.go, line 33 at r1 (raw file):
Previously, Oncilla wrote…
Wildcard tests missing.
No the non-core to core is specific IA to wildcard. wildcard-wildcard is not realisitic (should be catched by the validator).
There was a problem hiding this comment.
Reviewed 18 of 18 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @lukedirtwalker)
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @oncilla)
go/path_srv/internal/segreq/provider.go, line 81 at r1 (raw file):
Previously, Oncilla wrote…
what if src is localIA? (or does this implicitly assume that we will never get here because of the IsLocal check?)
yeah this assume that we never get here with an up segment request.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @lukedirtwalker)
go/path_srv/internal/segreq/provider.go, line 81 at r1 (raw file):
Previously, lukedirtwalker (Lukas Vogel) wrote…
yeah this assume that we never get here with an up segment request.
at least mention it in the doc string.
(also, I'm worried this might bite us in the future. Maybe still add an error case or panic just to be safe)
Add all the bits needed to use the new lookup logic via the segfetcher module in the PS. Contributes scionproto#2454
lukedirtwalker
force-pushed
the
pubPSNewLookupHandler
branch
from
519d532
to
a15d54e
Compare
There was a problem hiding this comment.
Reviewable status: 21 of 22 files reviewed, 1 unresolved discussion (waiting on @oncilla)
go/path_srv/internal/segreq/provider.go, line 81 at r1 (raw file):
Previously, Oncilla wrote…
at least mention it in the doc string.
(also, I'm worried this might bite us in the future. Maybe still add an error case or panic just to be safe)
Done
There was a problem hiding this comment.
Reviewed 1 of 1 files at r3.
Reviewable status:complete! all files reviewed, all discussions resolved
There was a problem hiding this comment.
Reviewable status:
Add all the bits needed to use the new lookup logic via the segfetcher module in the PS.
Contributes #2454
This change is