revised after feedback

go/hidden_path_srv/internal/hpsegreg/validator/BUILD.bazel → go/hidden_path_srv/internal/registration/BUILD.bazel

@@ -2,16 +2,23 @@ load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")
 
 go_library(
     name = "go_default_library",
-    srcs = ["validator.go"],
-    importpath = "github.com/scionproto/scion/go/hidden_path_srv/internal/hpsegreg/validator",
+    srcs = [
+        "handler.go",
+        "validator.go",
+    ],
+    importpath = "github.com/scionproto/scion/go/hidden_path_srv/internal/registration",
     visibility = ["//go/hidden_path_srv:__subpackages__"],
     deps = [
-        "//go/hidden_path_srv/internal/hpsegreg:go_default_library",
         "//go/lib/addr:go_default_library",
         "//go/lib/common:go_default_library",
         "//go/lib/ctrl/path_mgmt:go_default_library",
         "//go/lib/ctrl/seg:go_default_library",
         "//go/lib/hiddenpath:go_default_library",
+        "//go/lib/infra:go_default_library",
+        "//go/lib/infra/messenger:go_default_library",
+        "//go/lib/infra/modules/seghandler:go_default_library",
+        "//go/lib/log:go_default_library",
+        "//go/lib/snet:go_default_library",
         "//go/proto:go_default_library",
     ],
 )
@@ -33,5 +40,6 @@ go_test(
         "@com_github_golang_mock//gomock:go_default_library",
         "@com_github_stretchr_testify//assert:go_default_library",
         "@com_github_stretchr_testify//require:go_default_library",
+        "@org_golang_x_xerrors//:go_default_library",
     ],
 )

go/hidden_path_srv/internal/hpsegreg/handler.go → go/hidden_path_srv/internal/registration/handler.go

@@ -12,10 +12,9 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-package hpsegreg
+package registration
 
 import (
-	"github.com/scionproto/scion/go/hidden_path_srv/internal/helper"
 	"github.com/scionproto/scion/go/lib/addr"
 	"github.com/scionproto/scion/go/lib/common"
 	"github.com/scionproto/scion/go/lib/ctrl/path_mgmt"
@@ -81,7 +80,8 @@ func (h *hpSegRegHandler) handle(logger log.Logger) (*infra.HandlerResult, error
 		sendAck(proto.Ack_ErrCode_reject, messenger.AckRejectFailedToParse)
 		return infra.MetricsErrInvalid, nil
 	}
-	helper.LogHPSegRecs(logger, "[hpSegRegHandler]", h.request.Peer, hpSegReg.HPSegRecs)
+	logger.Debug("[hpSegRegHandler] Received HPSegRecs", "src",
+		h.request.Peer, "data", hpSegReg.HPSegRecs)
 
 	snetPeer := h.request.Peer.(*snet.Addr)
 	peerPath, err := snetPeer.GetPath()
@@ -109,6 +109,7 @@ func (h *hpSegRegHandler) handle(logger log.Logger) (*infra.HandlerResult, error
 	// wait until processing is done.
 	<-res.FullReplyProcessed()
 	if err := res.Err(); err != nil {
+		logger.Error("[hpSegRegHandler] Failed to handle path segments", "err", err)
 		sendAck(proto.Ack_ErrCode_reject, err.Error())
 		return infra.MetricsErrInvalid, nil
 	}

go/hidden_path_srv/internal/hpsegreg/validator/validator.go → go/hidden_path_srv/internal/registration/validator.go

@@ -12,10 +12,9 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-package validator
+package registration
 
 import (
-	"github.com/scionproto/scion/go/hidden_path_srv/internal/hpsegreg"
 	"github.com/scionproto/scion/go/lib/addr"
 	"github.com/scionproto/scion/go/lib/common"
 	"github.com/scionproto/scion/go/lib/ctrl/path_mgmt"
@@ -34,9 +33,9 @@ const (
 	ErrNotReader    common.ErrMsg = "peer is not a reader of this group"
 )
 
-var _ hpsegreg.Validator = (*DefaultValidator)(nil)
+var _ Validator = (*DefaultValidator)(nil)
 
-// DefaultValidator validates
+// DefaultValidator validates hidden path registrations based on hidden path group configurations
 type DefaultValidator struct {
 	localIA addr.IA
 	groups  map[hiddenpath.GroupId]*hiddenpath.Group
@@ -58,8 +57,10 @@ func (v *DefaultValidator) Validate(hpSegReg *path_mgmt.HPSegReg, peer addr.IA)
 	if err := v.checkGroupPermissions(id, peer); err != nil {
 		return common.NewBasicError("Group configuration error", err, "group", id)
 	}
-	err := v.checkSegments(hpSegReg.Recs)
-	return common.NewBasicError("Invalid hidden segment", err)
+	if err := v.checkSegments(hpSegReg.Recs); err != nil {
+		return common.NewBasicError("Invalid hidden segment", err)
+	}
+	return nil
 }
 
 func (v *DefaultValidator) checkGroupPermissions(groupId hiddenpath.GroupId, peer addr.IA) error {
@@ -98,12 +99,3 @@ func checkHiddenSegExtn(s *seg.Meta) bool {
 	}
 	return lastASEntry.Exts.HiddenPathSeg.Set
 }
-
-// NullValidator validates all input
-var NullValidator = nullValidator{}
-
-type nullValidator struct{}
-
-func (v nullValidator) Validate(hpSegReg *path_mgmt.HPSegReg, peer addr.IA) error {
-	return nil
-}

go/hidden_path_srv/internal/hpsegreg/validator/validator_test.go → go/hidden_path_srv/internal/registration/validator_test.go

@@ -12,16 +12,17 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-package validator_test
+package registration_test
 
 import (
 	"testing"
 
 	"github.com/golang/mock/gomock"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"golang.org/x/xerrors"
 
-	"github.com/scionproto/scion/go/hidden_path_srv/internal/hpsegreg/validator"
+	"github.com/scionproto/scion/go/hidden_path_srv/internal/registration"
 	"github.com/scionproto/scion/go/lib/addr"
 	"github.com/scionproto/scion/go/lib/common"
 	"github.com/scionproto/scion/go/lib/ctrl/path_mgmt"
@@ -133,41 +134,41 @@ func TestValidator(t *testing.T) {
 			peer:    ia110,
 			groupId: wrongId,
 			segs:    []*seg.Meta{seg110_133},
-			Err:     validator.ErrUnknownGroup,
+			Err:     registration.ErrUnknownGroup,
 		},
 		"wrong registry": {
 			hpsIA:   ia113,
 			peer:    ia112,
 			groupId: group.Id,
 			segs:    []*seg.Meta{seg110_133},
-			Err:     validator.ErrNotRegistry,
+			Err:     registration.ErrNotRegistry,
 		},
 		"not a writer": {
 			hpsIA:   ia110,
 			peer:    ia113,
 			groupId: group.Id,
 			segs:    []*seg.Meta{seg110_133},
-			Err:     validator.ErrNotWriter,
+			Err:     registration.ErrNotWriter,
 		},
 		"missing extension": {
 			hpsIA:   ia115,
 			peer:    ia110,
 			groupId: group.Id,
 			segs:    []*seg.Meta{seg210_212},
-			Err:     validator.ErrMissingExtn,
+			Err:     registration.ErrMissingExtn,
 		},
 		"wrong seg type": {
 			hpsIA:   ia115,
 			peer:    ia110,
 			groupId: group.Id,
 			segs:    []*seg.Meta{seg110_120},
-			Err:     validator.ErrWrongSegType,
+			Err:     registration.ErrWrongSegType,
 		},
 	}
 
 	for name, test := range tests {
 		t.Run(name, func(t *testing.T) {
-			validator := validator.NewDefaultValidator(
+			validator := registration.NewDefaultValidator(
 				test.hpsIA,
 				map[hiddenpath.GroupId]*hiddenpath.Group{
 					group.Id: group,
@@ -179,8 +180,14 @@ func TestValidator(t *testing.T) {
 					Recs:    test.segs,
 				},
 			}
-			err := validator.Validate(msg, test.peer).(common.BasicError)
-			assert.Equal(t, test.Err, err.GetErr())
+			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))
+			}
+
 		})
 	}
 }

go/lib/hiddenpath/group.go

@@ -179,7 +179,7 @@ func (g *Group) HasRegistry(ia addr.IA) bool {
 }
 
 // ToMsg returns h as Cerializable message suitable to be sent via messenger
-func (g Group) ToMsg() *path_mgmt.HPCfg {
+func (g *Group) ToMsg() *path_mgmt.HPCfg {
 	return &path_mgmt.HPCfg{
 		GroupId:    g.Id.ToMsg(),
 		Version:    uint32(g.Version),

go/lib/infra/modules/seghandler/seghandler.go

@@ -44,7 +44,7 @@ type Handler struct {
 	Storage  Storage
 }
 
-// Handle handles verifies and stores a set of segments.
+// Handle verifies and stores a set of segments.
 func (h *Handler) Handle(ctx context.Context, recs Segments, server net.Addr,
 	earlyTrigger <-chan struct{}) *ProcessedResult {