Never encode binary metadata within the metadata map (#1188)
This change ensures consistency for the user when accessing metadata values:
they are never encoded except when sent on the wire. Previously, they would
appear encoded to client code, but not to server code. As such, this
represents a behavior change, but one unlikely to affect user code, as it's
unusual to inspect the metadata after setting it.
diff --git a/metadata/metadata.go b/metadata/metadata.go
index 7ca4418..5395ca8 100644
--- a/metadata/metadata.go
+++ b/metadata/metadata.go
@@ -36,46 +36,15 @@
package metadata // import "google.golang.org/grpc/metadata"
import (
- "encoding/base64"
"fmt"
"strings"
"golang.org/x/net/context"
)
-const (
- binHdrSuffix = "-bin"
-)
-
-// encodeKeyValue encodes key and value qualified for transmission via gRPC.
-// Transmitting binary headers violates HTTP/2 spec.
-// TODO(zhaoq): Maybe check if k is ASCII also.
-func encodeKeyValue(k, v string) (string, string) {
- k = strings.ToLower(k)
- if strings.HasSuffix(k, binHdrSuffix) {
- val := base64.StdEncoding.EncodeToString([]byte(v))
- v = string(val)
- }
- return k, v
-}
-
-// DecodeKeyValue returns the original key and value corresponding to the
-// encoded data in k, v.
-// If k is a binary header and v contains comma, v is split on comma before decoded,
-// and the decoded v will be joined with comma before returned.
+// DecodeKeyValue returns k, v, nil. It is deprecated and should not be used.
func DecodeKeyValue(k, v string) (string, string, error) {
- if !strings.HasSuffix(k, binHdrSuffix) {
- return k, v, nil
- }
- vvs := strings.Split(v, ",")
- for i, vv := range vvs {
- val, err := base64.StdEncoding.DecodeString(vv)
- if err != nil {
- return "", "", err
- }
- vvs[i] = string(val)
- }
- return k, strings.Join(vvs, ","), nil
+ return k, v, nil
}
// MD is a mapping from metadata keys to values. Users should use the following
@@ -83,11 +52,11 @@
type MD map[string][]string
// New creates a MD from given key-value map.
-// Keys are automatically converted to lowercase. And for keys having "-bin" as suffix, their values will be applied Base64 encoding.
+// Keys are automatically converted to lowercase.
func New(m map[string]string) MD {
md := MD{}
- for k, v := range m {
- key, val := encodeKeyValue(k, v)
+ for k, val := range m {
+ key := strings.ToLower(k)
md[key] = append(md[key], val)
}
return md
@@ -95,20 +64,19 @@
// Pairs returns an MD formed by the mapping of key, value ...
// Pairs panics if len(kv) is odd.
-// Keys are automatically converted to lowercase. And for keys having "-bin" as suffix, their values will be appplied Base64 encoding.
+// Keys are automatically converted to lowercase.
func Pairs(kv ...string) MD {
if len(kv)%2 == 1 {
panic(fmt.Sprintf("metadata: Pairs got the odd number of input pairs for metadata: %d", len(kv)))
}
md := MD{}
- var k string
+ var key string
for i, s := range kv {
if i%2 == 0 {
- k = s
+ key = strings.ToLower(s)
continue
}
- key, val := encodeKeyValue(k, s)
- md[key] = append(md[key], val)
+ md[key] = append(md[key], s)
}
return md
}
diff --git a/metadata/metadata_test.go b/metadata/metadata_test.go
index fef0a0f..bd982cc 100644
--- a/metadata/metadata_test.go
+++ b/metadata/metadata_test.go
@@ -38,73 +38,20 @@
"testing"
)
-const binaryValue = string(128)
-
-func TestEncodeKeyValue(t *testing.T) {
- for _, test := range []struct {
- // input
- kin string
- vin string
- // output
- kout string
- vout string
- }{
- {"key", "abc", "key", "abc"},
- {"KEY", "abc", "key", "abc"},
- {"key-bin", "abc", "key-bin", "YWJj"},
- {"key-bin", binaryValue, "key-bin", "woA="},
- } {
- k, v := encodeKeyValue(test.kin, test.vin)
- if k != test.kout || !reflect.DeepEqual(v, test.vout) {
- t.Fatalf("encodeKeyValue(%q, %q) = %q, %q, want %q, %q", test.kin, test.vin, k, v, test.kout, test.vout)
- }
- }
-}
-
-func TestDecodeKeyValue(t *testing.T) {
- for _, test := range []struct {
- // input
- kin string
- vin string
- // output
- kout string
- vout string
- err error
- }{
- {"a", "abc", "a", "abc", nil},
- {"key-bin", "Zm9vAGJhcg==", "key-bin", "foo\x00bar", nil},
- {"key-bin", "woA=", "key-bin", binaryValue, nil},
- {"a", "abc,efg", "a", "abc,efg", nil},
- {"key-bin", "Zm9vAGJhcg==,Zm9vAGJhcg==", "key-bin", "foo\x00bar,foo\x00bar", nil},
- } {
- k, v, err := DecodeKeyValue(test.kin, test.vin)
- if k != test.kout || !reflect.DeepEqual(v, test.vout) || !reflect.DeepEqual(err, test.err) {
- t.Fatalf("DecodeKeyValue(%q, %q) = %q, %q, %v, want %q, %q, %v", test.kin, test.vin, k, v, err, test.kout, test.vout, test.err)
- }
- }
-}
-
func TestPairsMD(t *testing.T) {
for _, test := range []struct {
// input
kv []string
// output
- md MD
- size int
+ md MD
}{
- {[]string{}, MD{}, 0},
- {[]string{"k1", "v1", "k2-bin", binaryValue}, New(map[string]string{
- "k1": "v1",
- "k2-bin": binaryValue,
- }), 2},
+ {[]string{}, MD{}},
+ {[]string{"k1", "v1", "k1", "v2"}, MD{"k1": []string{"v1", "v2"}}},
} {
md := Pairs(test.kv...)
if !reflect.DeepEqual(md, test.md) {
t.Fatalf("Pairs(%v) = %v, want %v", test.kv, md, test.md)
}
- if md.Len() != test.size {
- t.Fatalf("Pairs(%v) generates md of size %d, want %d", test.kv, md.Len(), test.size)
- }
}
}
diff --git a/test/end2end_test.go b/test/end2end_test.go
index fd77cd7..6a36b68 100644
--- a/test/end2end_test.go
+++ b/test/end2end_test.go
@@ -75,8 +75,9 @@
var (
// For headers:
testMetadata = metadata.MD{
- "key1": []string{"value1"},
- "key2": []string{"value2"},
+ "key1": []string{"value1"},
+ "key2": []string{"value2"},
+ "key3-bin": []string{"binvalue1", string([]byte{1, 2, 3})},
}
testMetadata2 = metadata.MD{
"key1": []string{"value12"},
@@ -84,8 +85,9 @@
}
// For trailers:
testTrailerMetadata = metadata.MD{
- "tkey1": []string{"trailerValue1"},
- "tkey2": []string{"trailerValue2"},
+ "tkey1": []string{"trailerValue1"},
+ "tkey2": []string{"trailerValue2"},
+ "tkey3-bin": []string{"trailerbinvalue1", string([]byte{3, 2, 1})},
}
testTrailerMetadata2 = metadata.MD{
"tkey1": []string{"trailerValue12"},
diff --git a/transport/handler_server.go b/transport/handler_server.go
index 28c9ce0..24f306b 100644
--- a/transport/handler_server.go
+++ b/transport/handler_server.go
@@ -111,6 +111,10 @@
v = v[:i]
}
}
+ v, err := decodeMetadataHeader(k, v)
+ if err != nil {
+ return nil, streamErrorf(codes.InvalidArgument, "malformed binary metadata: %v", err)
+ }
metakv = append(metakv, k, v)
}
}
@@ -207,10 +211,9 @@
continue
}
for _, v := range vv {
- // http2 ResponseWriter mechanism to
- // send undeclared Trailers after the
- // headers have possibly been written.
- h.Add(http2.TrailerPrefix+k, v)
+ // http2 ResponseWriter mechanism to send undeclared Trailers after
+ // the headers have possibly been written.
+ h.Add(http2.TrailerPrefix+k, encodeMetadataHeader(k, v))
}
}
}
@@ -265,6 +268,7 @@
continue
}
for _, v := range vv {
+ v = encodeMetadataHeader(k, v)
h.Add(k, v)
}
}
diff --git a/transport/http2_client.go b/transport/http2_client.go
index 67a7945..380fff6 100644
--- a/transport/http2_client.go
+++ b/transport/http2_client.go
@@ -437,23 +437,23 @@
)
if md, ok := metadata.FromOutgoingContext(ctx); ok {
hasMD = true
- for k, v := range md {
+ for k, vv := range md {
// HTTP doesn't allow you to set pseudoheaders after non pseudoheaders were set.
if isReservedHeader(k) {
continue
}
- for _, entry := range v {
- t.hEnc.WriteField(hpack.HeaderField{Name: k, Value: entry})
+ for _, v := range vv {
+ t.hEnc.WriteField(hpack.HeaderField{Name: k, Value: encodeMetadataHeader(k, v)})
}
}
}
if md, ok := t.md.(*metadata.MD); ok {
- for k, v := range *md {
+ for k, vv := range *md {
if isReservedHeader(k) {
continue
}
- for _, entry := range v {
- t.hEnc.WriteField(hpack.HeaderField{Name: k, Value: entry})
+ for _, v := range vv {
+ t.hEnc.WriteField(hpack.HeaderField{Name: k, Value: encodeMetadataHeader(k, v)})
}
}
}
diff --git a/transport/http2_server.go b/transport/http2_server.go
index 31fefc7..14cd19c 100644
--- a/transport/http2_server.go
+++ b/transport/http2_server.go
@@ -644,13 +644,13 @@
if s.sendCompress != "" {
t.hEnc.WriteField(hpack.HeaderField{Name: "grpc-encoding", Value: s.sendCompress})
}
- for k, v := range md {
+ for k, vv := range md {
if isReservedHeader(k) {
// Clients don't tolerate reading restricted headers after some non restricted ones were sent.
continue
}
- for _, entry := range v {
- t.hEnc.WriteField(hpack.HeaderField{Name: k, Value: entry})
+ for _, v := range vv {
+ t.hEnc.WriteField(hpack.HeaderField{Name: k, Value: encodeMetadataHeader(k, v)})
}
}
bufLen := t.hBuf.Len()
@@ -713,21 +713,17 @@
panic(err)
}
- for k, v := range metadata.New(map[string]string{"grpc-status-details-bin": (string)(stBytes)}) {
- for _, entry := range v {
- t.hEnc.WriteField(hpack.HeaderField{Name: k, Value: entry})
- }
- }
+ t.hEnc.WriteField(hpack.HeaderField{Name: "grpc-status-details-bin", Value: encodeBinHeader(stBytes)})
}
// Attach the trailer metadata.
- for k, v := range s.trailer {
+ for k, vv := range s.trailer {
// Clients don't tolerate reading restricted headers after some non restricted ones were sent.
if isReservedHeader(k) {
continue
}
- for _, entry := range v {
- t.hEnc.WriteField(hpack.HeaderField{Name: k, Value: entry})
+ for _, v := range vv {
+ t.hEnc.WriteField(hpack.HeaderField{Name: k, Value: encodeMetadataHeader(k, v)})
}
}
bufLen := t.hBuf.Len()
diff --git a/transport/http_util.go b/transport/http_util.go
index 89c1525..e5120eb 100644
--- a/transport/http_util.go
+++ b/transport/http_util.go
@@ -36,6 +36,7 @@
import (
"bufio"
"bytes"
+ "encoding/base64"
"fmt"
"io"
"net"
@@ -50,7 +51,6 @@
spb "google.golang.org/genproto/googleapis/rpc/status"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/grpclog"
- "google.golang.org/grpc/metadata"
"google.golang.org/grpc/status"
)
@@ -164,6 +164,31 @@
return d.statusGen
}
+const binHdrSuffix = "-bin"
+
+func encodeBinHeader(v []byte) string {
+ return base64.StdEncoding.EncodeToString(v)
+}
+
+func decodeBinHeader(v string) ([]byte, error) {
+ return base64.StdEncoding.DecodeString(v)
+}
+
+func encodeMetadataHeader(k, v string) string {
+ if strings.HasSuffix(k, binHdrSuffix) {
+ return encodeBinHeader(([]byte)(v))
+ }
+ return v
+}
+
+func decodeMetadataHeader(k, v string) (string, error) {
+ if strings.HasSuffix(k, binHdrSuffix) {
+ b, err := decodeBinHeader(v)
+ return string(b), err
+ }
+ return v, nil
+}
+
func (d *decodeState) processHeaderField(f hpack.HeaderField) error {
switch f.Name {
case "content-type":
@@ -181,12 +206,12 @@
case "grpc-message":
d.rawStatusMsg = decodeGrpcMessage(f.Value)
case "grpc-status-details-bin":
- _, v, err := metadata.DecodeKeyValue("grpc-status-details-bin", f.Value)
+ v, err := decodeBinHeader(f.Value)
if err != nil {
return streamErrorf(codes.Internal, "transport: malformed grpc-status-details-bin: %v", err)
}
s := &spb.Status{}
- if err := proto.Unmarshal([]byte(v), s); err != nil {
+ if err := proto.Unmarshal(v, s); err != nil {
return streamErrorf(codes.Internal, "transport: malformed grpc-status-details-bin: %v", err)
}
d.statusGen = status.FromProto(s)
@@ -203,12 +228,12 @@
if d.mdata == nil {
d.mdata = make(map[string][]string)
}
- k, v, err := metadata.DecodeKeyValue(f.Name, f.Value)
+ v, err := decodeMetadataHeader(f.Name, f.Value)
if err != nil {
grpclog.Printf("Failed to decode (%q, %q): %v", f.Name, f.Value, err)
return nil
}
- d.mdata[k] = append(d.mdata[k], v)
+ d.mdata[f.Name] = append(d.mdata[f.Name], v)
}
}
return nil
diff --git a/transport/http_util_test.go b/transport/http_util_test.go
index a5f8a85..8e2c488 100644
--- a/transport/http_util_test.go
+++ b/transport/http_util_test.go
@@ -35,6 +35,7 @@
import (
"fmt"
+ "reflect"
"testing"
"time"
)
@@ -143,3 +144,46 @@
}
}
}
+
+const binaryValue = string(128)
+
+func TestEncodeMetadataHeader(t *testing.T) {
+ for _, test := range []struct {
+ // input
+ kin string
+ vin string
+ // output
+ vout string
+ }{
+ {"key", "abc", "abc"},
+ {"KEY", "abc", "abc"},
+ {"key-bin", "abc", "YWJj"},
+ {"key-bin", binaryValue, "woA="},
+ } {
+ v := encodeMetadataHeader(test.kin, test.vin)
+ if !reflect.DeepEqual(v, test.vout) {
+ t.Fatalf("encodeMetadataHeader(%q, %q) = %q, want %q", test.kin, test.vin, v, test.vout)
+ }
+ }
+}
+
+func TestDecodeMetadataHeader(t *testing.T) {
+ for _, test := range []struct {
+ // input
+ kin string
+ vin string
+ // output
+ vout string
+ err error
+ }{
+ {"a", "abc", "abc", nil},
+ {"key-bin", "Zm9vAGJhcg==", "foo\x00bar", nil},
+ {"key-bin", "woA=", binaryValue, nil},
+ {"a", "abc,efg", "abc,efg", nil},
+ } {
+ v, err := decodeMetadataHeader(test.kin, test.vin)
+ if !reflect.DeepEqual(v, test.vout) || !reflect.DeepEqual(err, test.err) {
+ t.Fatalf("decodeMetadataHeader(%q, %q) = %q, %v, want %q, %v", test.kin, test.vin, v, err, test.vout, test.err)
+ }
+ }
+}
