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.

Sign up for GitHub

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

tls: append h2 to tlsconfig.NextProtos #2744

Merged
merged 1 commit into from
Apr 8, 2019
Merged

tls: append h2 to tlsconfig.NextProtos #2744

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 14 additions & 4 deletions credentials/credentials.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,6 @@ import (
"google.golang.org/grpc/credentials/internal"
)

// alpnProtoStr are the specified application level protocols for gRPC.
var alpnProtoStr = []string{"h2"}

// PerRPCCredentials defines the common interface for the credentials which need to
// attach security information to every RPC (e.g., oauth2).
type PerRPCCredentials interface {
Expand Down Expand Up @@ -208,10 +205,23 @@ func (c *tlsCreds) OverrideServerName(serverNameOverride string) error {
return nil
}

const alpnProtoStrH2 = "h2"

func appendH2ToNextProtos(ps []string) []string {
for _, p := range ps {
if p == alpnProtoStrH2 {
return ps
}
}
ret := make([]string, 0, len(ps)+1)
dfawley marked this conversation as resolved.
Show resolved Hide resolved
Copy link

Choose a reason for hiding this comment

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

This seems like an unnecessary optimisation since the underlying array would at most end up as 4-10 elements in usual cases if the capacity was spent before an append. And this only happens when creating a new transport credential. Out of curiosity: is there something I'm missing?

Copy link
Member

Choose a reason for hiding this comment

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

We can't modify the underlying array, as it's owned by the user, so it needs to be copied first. Since we know the final size of it, there's no reason not to allocate the right amount of memory up front.

Copy link

Choose a reason for hiding this comment

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

Thanks for the explanation dfawley, that makes a lot of sense!

ret = append(ret, ps...)
return append(ret, alpnProtoStrH2)
}

// NewTLS uses c to construct a TransportCredentials based on TLS.
func NewTLS(c *tls.Config) TransportCredentials {
tc := &tlsCreds{cloneTLSConfig(c)}
tc.config.NextProtos = alpnProtoStr
tc.config.NextProtos = appendH2ToNextProtos(tc.config.NextProtos)
return tc
}

Expand Down
37 changes: 37 additions & 0 deletions credentials/credentials_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"context"
"crypto/tls"
"net"
"reflect"
"testing"

"google.golang.org/grpc/testdata"
Expand Down Expand Up @@ -204,3 +205,39 @@ func tlsClientHandshake(conn net.Conn, _ string) (AuthInfo, error) {
}
return TLSInfo{State: clientConn.ConnectionState()}, nil
}

func TestAppendH2ToNextProtos(t *testing.T) {
tests := []struct {
name string
ps []string
want []string
}{
{
name: "empty",
ps: nil,
want: []string{"h2"},
},
{
name: "only h2",
ps: []string{"h2"},
want: []string{"h2"},
},
{
name: "with h2",
ps: []string{"alpn", "h2"},
want: []string{"alpn", "h2"},
},
{
name: "no h2",
ps: []string{"alpn"},
want: []string{"alpn", "h2"},
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
if got := appendH2ToNextProtos(tt.ps); !reflect.DeepEqual(got, tt.want) {
t.Errorf("appendH2ToNextProtos() = %v, want %v", got, tt.want)
}
})
}
}