Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

trust: start using snet.UDPAddr #3464

Merged
merged 2 commits into from
Dec 6, 2019

Conversation

karampok
Copy link
Contributor

@karampok karampok commented Dec 6, 2019

which replaces the snet.Addr

Contributes: #3344


This change is Reviewable

which replaces the snet.Addr

Contributes: scionproto#3344
@karampok karampok requested a review from oncilla December 6, 2019 10:04
Copy link
Contributor

@oncilla oncilla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 7 of 7 files at r1.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @karampok)


go/lib/infra/modules/trust/handlers.go, line 50 at r1 (raw file):

	logger := log.FromCtx(h.request.Context())
	l := metrics.HandlerLabels{
		Client:  addrLocation(h.request.Peer, h.store.ia),

should we have an "unknown" value for this?


go/lib/infra/modules/trust/handlers.go, line 59 at r1 (raw file):

	peer, err := redirectToUDP(h.request.Peer)
	if err != nil {
		logger.Error("[TrustStore:trcReqHandler] wrong address type, err", err)

s/, err/", err/


go/lib/infra/modules/trust/handlers.go, line 61 at r1 (raw file):

		logger.Error("[TrustStore:trcReqHandler] wrong address type, err", err)
		metrics.Handler.Request(l).Inc()
		return infra.MetricsErrInternal

Not sure if this is an ErrInternal or ErrInvalid.

ErrInternal because it gets this far. ErrInvalid because its an invalid request packet


go/lib/infra/modules/trust/handlers.go, line 106 at r1 (raw file):

		trcObj, err = h.store.getTRC(subCtx, trcReq.ISD,
			scrypto.Version(trcReq.Version), opts, peer)

if you do getTRC(subCtx, trcReq.ISD, trcReq.Version, opts, peer), this should fit on one line


go/lib/infra/modules/trust/handlers.go, line 157 at r1 (raw file):

	peer, err := redirectToUDP(h.request.Peer)
	if err != nil {
		logger.Error("[TrustStore:trcReqHandler] wrong address type, err", err)

ditto


go/lib/infra/modules/trust/handlers.go, line 250 at r1 (raw file):

	peer, err := redirectToUDP(h.request.Peer)
	if err != nil {
		logger.Error("[TrustStore:trcReqHandler] wrong address type, err", err)

ditto


go/lib/infra/modules/trust/handlers.go, line 329 at r1 (raw file):

	peer, err := redirectToUDP(h.request.Peer)
	if err != nil {
		logger.Error("[TrustStore:trcReqHandler] wrong address type, err", err)

ditto


go/lib/infra/modules/trust/handlers.go, line 394 at r1 (raw file):

	switch p := address.(type) {
	case *snet.Addr:
		return addrLocation(p.ToXAddr(), localIA)

can we ever get into that branch?

Copy link
Contributor Author

@karampok karampok left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 6 of 7 files reviewed, 8 unresolved discussions (waiting on @karampok and @oncilla)


go/lib/infra/modules/trust/handlers.go, line 50 at r1 (raw file):

Previously, Oncilla wrote…

should we have an "unknown" value for this?

to my opinion no, but no strong preference


go/lib/infra/modules/trust/handlers.go, line 59 at r1 (raw file):

Previously, Oncilla wrote…

s/, err/", err/

Done.


go/lib/infra/modules/trust/handlers.go, line 61 at r1 (raw file):

Previously, Oncilla wrote…

Not sure if this is an ErrInternal or ErrInvalid.

ErrInternal because it gets this far. ErrInvalid because its an invalid request packet

some lines bellow we do the same stuff for message type, so I would say the same as there.
(but no strong preference, pick your favorite)


go/lib/infra/modules/trust/handlers.go, line 157 at r1 (raw file):

Previously, Oncilla wrote…

ditto

Done.


go/lib/infra/modules/trust/handlers.go, line 250 at r1 (raw file):

Previously, Oncilla wrote…

ditto

Done.


go/lib/infra/modules/trust/handlers.go, line 329 at r1 (raw file):

Previously, Oncilla wrote…

ditto

Done.


go/lib/infra/modules/trust/handlers.go, line 394 at r1 (raw file):

Previously, Oncilla wrote…

can we ever get into that branch?

yes, there is somewhere a store.getTRClabels where you enter the server not the client, and that is snet.Addr.

Copy link
Contributor Author

@karampok karampok left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 6 of 7 files reviewed, 8 unresolved discussions (waiting on @oncilla)


go/lib/infra/modules/trust/handlers.go, line 106 at r1 (raw file):

Previously, Oncilla wrote…

if you do getTRC(subCtx, trcReq.ISD, trcReq.Version, opts, peer), this should fit on one line

Done.

Copy link
Contributor

@oncilla oncilla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved


go/lib/infra/modules/trust/handlers.go, line 50 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

to my opinion no, but no strong preference

🤷‍♀️


go/lib/infra/modules/trust/handlers.go, line 394 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

yes, there is somewhere a store.getTRClabels where you enter the server not the client, and that is snet.Addr.

alright 👍

Copy link
Contributor

@oncilla oncilla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@karampok karampok merged commit 57d28c5 into scionproto:master Dec 6, 2019
@karampok karampok deleted the pub-more-appaddr branch December 11, 2019 09:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants