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

a discussion (no related file):
I think it would be nice to have a package segreg and in there have the type Handler

go/hidden_path_srv/internal/handlers/common.go, line 25 at r1 (raw file):

	"github.com/scionproto/scion/go/hidden_path_srv/internal/hiddenpathdb"
	"github.com/scionproto/scion/go/hidden_path_srv/internal/hpsegreq"

Wait, I just realized that importing hpsegreq in hiddenpathdb will be problematic once we want to implement hpsegreq.Handler in the hpsegreq package.
We should really move GroupIdSet into hiddenpath package.

go/hidden_path_srv/internal/handlers/common.go, line 39 at r1 (raw file):

const (
	HandlerTimeout = 30 * time.Second

Actually this isn't needed. (and is wrong in the PS; created #3077). The context that is given in the request already contains a timeout.

go/hidden_path_srv/internal/handlers/common.go, line 43 at r1 (raw file):

const (
	NoSegmentsErr   common.ErrMsg = "No segments"

All those variables should have a prefix Err not postfix. In this case ErrNoSegments. See also: https://github.com/golang/go/wiki/Errors#naming

go/hidden_path_srv/internal/handlers/common.go, line 43 at r1 (raw file):

const (
	NoSegmentsErr   common.ErrMsg = "No segments"

Error messages should start with lower case. "no segments"

go/hidden_path_srv/internal/handlers/common.go, line 78 at r1 (raw file):

}

func (h *baseHandler) verifyAndStore(ctx context.Context, src net.Addr,

hm I don't like this. In the long term we should integrate this into SegReplyHandler in go/lib/infra/modules/segfetcher/segreplyhandler.go But I will do this (create #3076 to track it)

go/hidden_path_srv/internal/handlers/common.go, line 79 at r1 (raw file):

func (h *baseHandler) verifyAndStore(ctx context.Context, src net.Addr,
	hpSegReg *path_mgmt.HPSegReg) error {

also having this in common feels a bit off since it explicitly uses HPSegReg

go/hidden_path_srv/internal/handlers/hpsegreg.go, line 66 at r1 (raw file):

		return infra.MetricsErrInternal, nil
	}
	subCtx, cancelF := context.WithTimeout(h.request.Context(), HandlerTimeout)

as mentioned above, not needed.

go/hidden_path_srv/internal/handlers/hpsegreg.go, line 83 at r1 (raw file):

		return infra.MetricsErrInvalid, nil
	}
	svcToQuery := &snet.Addr{

I think you can just use peer below, or is there any reason why we need this?

go/hidden_path_srv/internal/handlers/hpsegreg_test.go, line 234 at r1 (raw file):

			},
		},
	}

When looking at the code it looks like there are 2 parts: Validation and the "Verifying and Storing" (it's actually 3 but the latter is kind of tightly coupled). So #3076 will really make this distinction stronger. I wonder if it would make sense to already design the split now. Basically have a validator with all the tests that are here, but the tests will be simpler,because we don't have to mock all the DB and messenger interactions. The handler will then basically have a Validator and RegistrationHandler interface so testing interaction would be really easy.

I imagine something like this:

type Validator interface {
	Validate(*path_mgmt.HPSegReg) error
}

type SegProcessor interface {
	ProcessSegs(SegRecords) error
}

type SegRecords struct {
	Segs      []*seg.Meta
	SRevInfos []*path_mgmt.SignedRevInfo
	HPGroupID hiddenpath.GroupId
}

and have a DefaultValidator that does the checks you do now. Then SegProcessor would verify and store and in the long term it would just be an adaptor for segfetcher.SegReplyHandler (which I will refactor to handle SegRecords structs.)

go/hidden_path_srv/internal/handlers/hpsegreg_test.go, line 273 at r1 (raw file):

}

func getMocks(ctrl *gomock.Controller) *mocks {

createMocks

go/hidden_path_srv/internal/handlers/hpsegreg_test.go, line 299 at r1 (raw file):

}

type ackMatcher struct {

use xtest.AckMsg{Ack: ...} instead. (see go/lib/xtest/matchers.go)

go/lib/hiddenpath/group.go, line 136 at r1 (raw file):

// HasWriter returns true if ia is a Writer of h
func (h Group) HasWriter(ia addr.IA) bool {

All receivers should be pointer. If one is pointer, all should be. See also: https://golang.org/doc/faq#methods_on_values_or_pointers (paragraph about consistency)

Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @chaehni)

go/hidden_path_srv/internal/handlers/hpsegreg.go, line 83 at r1 (raw file):

Nevermind me, this is actually needed for QUIC. Because QUIC resolution only happens for SVC addresses.

Reviewable status: 3 of 19 files reviewed, 12 unresolved discussions (waiting on @chaehni and @lukedirtwalker)

go/hidden_path_srv/internal/handlers/common.go, line 25 at r1 (raw file):

Done.

go/hidden_path_srv/internal/handlers/common.go, line 39 at r1 (raw file):

Done.

go/hidden_path_srv/internal/handlers/common.go, line 43 at r1 (raw file):

Done.

go/hidden_path_srv/internal/handlers/common.go, line 43 at r1 (raw file):

Done.

go/hidden_path_srv/internal/handlers/common.go, line 78 at r1 (raw file):

Done.

go/hidden_path_srv/internal/handlers/common.go, line 79 at r1 (raw file):

Done.

go/lib/hiddenpath/group.go, line 136 at r1 (raw file):

Done.

go/hidden_path_srv/internal/handlers/hpsegreg.go, line 66 at r1 (raw file):

Done.

go/hidden_path_srv/internal/handlers/hpsegreg_test.go, line 234 at r1 (raw file):

Done.

go/hidden_path_srv/internal/handlers/hpsegreg_test.go, line 273 at r1 (raw file):

Done.

go/hidden_path_srv/internal/handlers/hpsegreg_test.go, line 299 at r1 (raw file):

Done.

Reviewed 17 of 17 files at r2.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @chaehni)

go/hidden_path_srv/internal/hpsegreg/validator/validator.go, line 1 at r2 (raw file):

// Copyright 2019 ETH Zurich

I think there was a slight misunderstanding, sorry. This should be in the same package as hpsegreg
i.e. go/hidden_path_src/internal/hpsegreg/validator.go

go/hidden_path_srv/internal/hpsegreg/validator/validator.go, line 1 at r2 (raw file):

// Copyright 2019 ETH Zurich

Also now that i'm thinking about the package name we could also use registration

go/hidden_path_srv/internal/hpsegreg/validator/validator.go, line 39 at r2 (raw file):

var _ hpsegreg.Validator = (*DefaultValidator)(nil)

// DefaultValidator validates

validates what?

go/hidden_path_srv/internal/hpsegreg/validator/validator.go, line 62 at r2 (raw file):

	}
	err := v.checkSegments(hpSegReg.Recs)
	return common.NewBasicError("Invalid hidden segment", err)

this always returns an error, I don't think that is what we want.

go/hidden_path_srv/internal/hpsegreg/validator/validator.go, line 104 at r2 (raw file):

// NullValidator validates all input
var NullValidator = nullValidator{}

No used?

go/hidden_path_srv/internal/hpsegreg/handler.go, line 84 at r2 (raw file):

		return infra.MetricsErrInvalid, nil
	}
	helper.LogHPSegRecs(logger, "[hpSegRegHandler]", h.request.Peer, hpSegReg.HPSegRecs)

can we just inline this code? just call logger.Debug() directly here. It will make it easier to debug where this line was printed.

go/hidden_path_srv/internal/hpsegreg/handler.go, line 112 at r2 (raw file):

	<-res.FullReplyProcessed()
	if err := res.Err(); err != nil {
		sendAck(proto.Ack_ErrCode_reject, err.Error())

here we should also log the error.

go/hidden_path_srv/internal/hpsegreg/validator/validator_test.go, line 182 at r2 (raw file):

				},
			}
			err := validator.Validate(msg, test.peer).(common.BasicError)

this should be:

			err := validator.Validate(msg, test.peer)
			if test.Err == nil {
				assert.NoError(t, err)
			} else {
				require.Error(t, err)
				assert.True(t, xerrors.Is(err, test.Err))
			}

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