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

TrustStore: Improve error context #3620

Merged
merged 4 commits into from
Jan 21, 2020

Conversation

oncilla
Copy link
Contributor

@oncilla oncilla commented Jan 20, 2020

Attach error context on wrapping:

Before:

2020-01-20 08:51:42.945524+0000 [EROR] Unable to get paths debug_id=7d9353b5 err=
>  unable to get latest TRC isd="3"
>      unable to get requested TRC
>      unable to resolve signed TRC from network
>      unable to resolve latest version
>      unable to resolve latest TRC
>      not found

After:

2020-01-20 15:20:40.198528+0000 [EROR] Unable to get paths debug_id=3b8c4bb6 err=
>  unable to get requested TRC isd="3" version="latest"
>      unable to fetch signed TRC from network addr="1-ff00:0:112,CS A (0x0002)"
>      error resolving latest TRC version number
>      rpc: error from remote: "not found"

Additionally, purge remains of trust v1

fixes #3618


This change is Reviewable

@oncilla oncilla force-pushed the pub-trust-improve-error-context branch from 3478697 to c1b015c Compare January 21, 2020 09:06
Copy link
Collaborator

@lukedirtwalker lukedirtwalker left a comment

Choose a reason for hiding this comment

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

Reviewed 18 of 18 files at r1.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @oncilla)


go/lib/infra/common.go, line 397 at r1 (raw file):

rpc error message

what about rpc: error from remote: to be a bit less ambiguous


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

		rawTRC, err := r.RPC.GetTRC(ctx, req, server)
		if err != nil {
			res <- resOrErr{Err: serrors.WrapStr("error requesting TRC", err,

Do we still need the additional context here, if we could remove it above in resolveLatestVersion ?


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

		decTRC, err := decoded.DecodeTRC(rawTRC)
		if err != nil {
			res <- resOrErr{Err: serrors.WrapStr("failed to parse TRC", err,

Do we still need the additional context here, if we could remove it above in resolveLatestVersion ?


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

	msg, err := r.RPC.GetCertChain(ctx, req, server)
	if err != nil {
		return decoded.Chain{}, serrors.WrapStr("rpc failed", err)

this is probably not needed?

@oncilla oncilla force-pushed the pub-trust-improve-error-context branch from c1b015c to a735e01 Compare January 21, 2020 13:40
Copy link
Contributor Author

@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.

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @lukedirtwalker)


go/lib/infra/common.go, line 397 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…
rpc error message

what about rpc: error from remote: to be a bit less ambiguous

Done.


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

Previously, lukedirtwalker (Lukas Vogel) wrote…

Do we still need the additional context here, if we could remove it above in resolveLatestVersion ?

yes. This is additional context tells us which link we failed to fetch.
Otherwise the information is lost when reading the results from the channel.


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

Previously, lukedirtwalker (Lukas Vogel) wrote…

Do we still need the additional context here, if we could remove it above in resolveLatestVersion ?

ditto


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

Previously, lukedirtwalker (Lukas Vogel) wrote…

this is probably not needed?

Done.

Copy link
Collaborator

@lukedirtwalker lukedirtwalker left a comment

Choose a reason for hiding this comment

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

:lgtm:

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

Attach error context on wrapping:

Before:
`
2020-01-20 08:51:42.945524+0000 [EROR] Unable to get paths debug_id=7d9353b5 err=
>  unable to get latest TRC isd="3"
>      unable to get requested TRC
>      unable to resolve signed TRC from network
>      unable to resolve latest version
>      unable to resolve latest TRC
>      not found
`

After:
`
2020-01-20 11:38:56.424738+0000 [EROR] Unable to get paths debug_id=351783d6 err=
>  unable to get latest TRC isd="3"
>      unable to get requested TRC isd="3" version="latest"
>      unable to resolve signed TRC from network addr="1-ff00:0:112,CS A (0x0002)"
>      error fetching latest TRC version number
>      ACK reject: not found
`

Additionally, remains of trust v1 purge

fixes scionproto#3618
@oncilla oncilla force-pushed the pub-trust-improve-error-context branch from a735e01 to 8fdecdc Compare January 21, 2020 13:44
@oncilla oncilla merged commit ecadd83 into scionproto:master Jan 21, 2020
@oncilla oncilla deleted the pub-trust-improve-error-context branch January 21, 2020 15:34
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.

Unhelpful crypto lookup error messages
2 participants