Skip to content

Commit

Permalink
Fix breakage resulting from error string matching
Browse files Browse the repository at this point in the history
Fixes an issue resulting from string matching the error result of grpc
methods. Adds comments documenting the fix, as well as explaining how to
avoid this issue in the future.

Signed-off-by: Drew Erny <drew.erny@docker.com>
  • Loading branch information
dperny committed May 14, 2018
1 parent 2a14d4a commit 8dc087b
Show file tree
Hide file tree
Showing 2 changed files with 49 additions and 5 deletions.
47 changes: 42 additions & 5 deletions node/node.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ import (
"google.golang.org/grpc"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/credentials"
"google.golang.org/grpc/status"
)

const (
Expand Down Expand Up @@ -1291,13 +1292,49 @@ func (fs *firstSessionErrorTracker) SessionError(err error) {
fs.mu.Unlock()
}

// SessionClosed returns an error if we haven't yet established a session, and
// we get a gprc error as a result of an X509 failure.
func (fs *firstSessionErrorTracker) SessionClosed() error {
fs.mu.Lock()
defer fs.mu.Unlock()
// unfortunately grpc connection errors are type grpc.rpcError, which are not exposed, and we can't get at the underlying error type
if !fs.pastFirstSession && grpc.Code(fs.err) == codes.Internal &&
strings.HasPrefix(grpc.ErrorDesc(fs.err), "connection error") && strings.Contains(grpc.ErrorDesc(fs.err), "transport: x509:") {
return fs.err

// if we've successfully established at least 1 session, never return
// errors
if fs.pastFirstSession {
return nil
}
return nil

// get the GRPC status from the error, because we only care about GRPC
// errors
grpcStatus, ok := status.FromError(fs.err)
// if this isn't a GRPC error, it's not an error we return from this method
if !ok {
return nil
}

// additionally, the error we're looking for is an internal error
if grpcStatus.Code() != codes.Internal {
return nil
}

// NOTE(dperny): grpc does not expose the error type, which means we have
// to string matching to figure out if it's an x509 error.
//
// the error we're looking for starts with "connection error:", then says
// "transport:" and finally has "x509:"
// specifically, it reads:
//
// transport: authentication handshake failed: x509: certificate signed by unknown authority
//
// this string matching has caused trouble in the past. specifically, at
// some point between grpc versions 1.3.0 and 1.7.5, the string we were
// matching changed from "transport: x509" to "transport: authentication
// handshake failed: x509", which was an issue because we were matching for
// string "transport: x509:".
if !strings.HasPrefix(grpcStatus.Message(), "connection error") &&
!strings.Contains(grpcStatus.Message(), "transport:") &&
!strings.Contains(grpcStatus.Message(), "x509:") {
return nil
}
return fs.err
}
7 changes: 7 additions & 0 deletions vendor.conf
Original file line number Diff line number Diff line change
@@ -1,4 +1,11 @@
# grpc and protobuf
#
# NOTE(dperny): there is some error handling, found in the
# (*firstSessionErrorTracker).SessionClosed method in node/node.go, which
# relies on string matching to handle x509 errors. between grpc versions 1.3.0
# and 1.7.5, the error string we were matching changed, breaking swarmkit.
# after updating GRPC, if integration test failures occur, verify that the
# string matching there is correct.
google.golang.org/grpc v1.7.5
github.com/gogo/protobuf v0.5
github.com/golang/protobuf 1e59b77b52bf8e4b449a57e6f79f21226d571845
Expand Down

0 comments on commit 8dc087b

Please sign in to comment.