feat: add ErrPeerIDMismatch error type to replace ad-hoc errors#2451
feat: add ErrPeerIDMismatch error type to replace ad-hoc errors#2451marten-seemann merged 4 commits intomasterfrom
Conversation
Would this be resolved by quic-go/quic-go#3730 (comment) (i.e. by returning / wrapping the error returned by crypto/tls)? |
marten-seemann
left a comment
marten-seemann
left a comment
There was a problem hiding this comment.
This looks very clean.
Before merging I'd like to:
- Figure out what to do with QUIC.
- Add a transport integration test for this.
aschmahmann
force-pushed
the
feat/err-peerid-mismatch
branch
2 times, most recently
from
a6d4520 to
69498e7
Compare
|
Thanks @aschmahmann! This is a nice addition |
aschmahmann
force-pushed
the
feat/err-peerid-mismatch
branch
from
c8707b9 to
fcd3351
Compare
|
Seems like the quic-go branch from the linked PR doesn't work with Go 1.21 and that when that's updated things should pass here. |
Yay, that's the last-minute breaking change Go 1.21 introduced again: golang/go@a915b999c9. I rebased the quic-go PR and updated the dependency here, and now tests are passing. I'll take this as confirmation that quic-go/quic-go#4015 is the right way to go and will go ahead and merge that PR. Will ship in the v0.38 release later this month. |
MarcoPolo
left a comment
MarcoPolo
left a comment
There was a problem hiding this comment.
lgtm, minus one little thing
| // Try connecting with the bogus peer ID | ||
| if err := testHost.Connect(ctx, *ai); err != nil { | ||
| // Extract the actual peer ID from the error | ||
| newPeerId, err := extractPeerIDFromError(err) |
There was a problem hiding this comment.
Can we assert the error returned has the correct values for expected and actual. I can see this being missed by accident silently.
marten-seemann
force-pushed
the
feat/err-peerid-mismatch
branch
from
14ac002 to
f0ca926
Compare
|
#2506 was merged. I'll be taking over this PR to make sure we can include it in the v0.31 release. Hope you don't mind. |
Definitely don't mind. Thanks for pushing this over the finish line (and doing the associated quic-go PR)🙏. |
3 participants
partially resolves #2449.
This PR mostly works. However, it doesn't work with QUIC because QUIC encapsulates the error string instead of exposing it via unwrapping in https://github.com/quic-go/quic-go/blob/f3a0ce1599732cb56b958c2f0903074b62a7e9bf/internal/handshake/crypto_setup.go#L643.
Could add additional tests (or move over the ones from vole) if we're happy with this PR.
@marten-seemann is making a change to quic-go to allow inspection of the inner error inside the transport error reasonable?