Skip to content

Commit

Permalink
metadata: convert keys to lowercase in FromContext() (#4416)
Browse files Browse the repository at this point in the history
  • Loading branch information
@menghanl
menghanl committed Jun 3, 2021
1 parent c67c056 commit 32d5490
Showing 1 changed file with 49 additions and 20 deletions.
69 changes: 49 additions & 20 deletions metadata/metadata.go
View file
Expand Up @@ -93,12 +93,16 @@ func (md MD) Copy() MD {
}

// Get obtains the values for a given key.
//
// k is converted to lowercase before searching in md.
func (md MD) Get(k string) []string {
k = strings.ToLower(k)
return md[k]
}

// Set sets the value of a given key with a slice of values.
//
// k is converted to lowercase before storing in md.
func (md MD) Set(k string, vals ...string) {
if len(vals) == 0 {
return
Expand All @@ -107,7 +111,10 @@ func (md MD) Set(k string, vals ...string) {
md[k] = vals
}

// Append adds the values to key k, not overwriting what was already stored at that key.
// Append adds the values to key k, not overwriting what was already stored at
// that key.
//
// k is converted to lowercase before storing in md.
func (md MD) Append(k string, vals ...string) {
if len(vals) == 0 {
return
Expand All @@ -117,8 +124,9 @@ func (md MD) Append(k string, vals ...string) {
}

// Join joins any number of mds into a single MD.
// The order of values for each key is determined by the order in which
// the mds containing those values are presented to Join.
//
// The order of values for each key is determined by the order in which the mds
// containing those values are presented to Join.
func Join(mds ...MD) MD {
out := MD{}
for _, md := range mds {
Expand All @@ -145,8 +153,8 @@ func NewOutgoingContext(ctx context.Context, md MD) context.Context {
}

// AppendToOutgoingContext returns a new context with the provided kv merged
// with any existing metadata in the context. Please refer to the
// documentation of Pairs for a description of kv.
// with any existing metadata in the context. Please refer to the documentation
// of Pairs for a description of kv.
func AppendToOutgoingContext(ctx context.Context, kv ...string) context.Context {
if len(kv)%2 == 1 {
panic(fmt.Sprintf("metadata: AppendToOutgoingContext got an odd number of input pairs for metadata: %d", len(kv)))
Expand All @@ -159,20 +167,34 @@ func AppendToOutgoingContext(ctx context.Context, kv ...string) context.Context
return context.WithValue(ctx, mdOutgoingKey{}, rawMD{md: md.md, added: added})
}

// FromIncomingContext returns the incoming metadata in ctx if it exists. The
// returned MD should not be modified. Writing to it may cause races.
// Modification should be made to copies of the returned MD.
func FromIncomingContext(ctx context.Context) (md MD, ok bool) {
md, ok = ctx.Value(mdIncomingKey{}).(MD)
return
// FromIncomingContext returns the incoming metadata in ctx if it exists.
//
// All keys in the returned MD are lowercase.
func FromIncomingContext(ctx context.Context) (MD, bool) {
md, ok := ctx.Value(mdIncomingKey{}).(MD)
if !ok {
return nil, false
}
out := MD{}
for k, v := range md {
// We need to manually convert all keys to lower case, because MD is a
// map, and there's no guarantee that the MD attached to the context is
// created using our helper functions.
key := strings.ToLower(k)
out[key] = v
}
return out, true
}

// FromOutgoingContextRaw returns the un-merged, intermediary contents
// of rawMD. Remember to perform strings.ToLower on the keys. The returned
// MD should not be modified. Writing to it may cause races. Modification
// should be made to copies of the returned MD.
// FromOutgoingContextRaw returns the un-merged, intermediary contents of rawMD.
//
// Remember to perform strings.ToLower on the keys, for both the returned MD (MD
// is a map, there's no guarantee it's created using our helper functions) and
// the extra kv pairs (AppendToOutgoingContext doesn't turn them into
// lowercase).
//
// This is intended for gRPC-internal use ONLY.
// This is intended for gRPC-internal use ONLY. Users should use
// FromOutgoingContext instead.
func FromOutgoingContextRaw(ctx context.Context) (MD, [][]string, bool) {
raw, ok := ctx.Value(mdOutgoingKey{}).(rawMD)
if !ok {
Expand All @@ -182,16 +204,23 @@ func FromOutgoingContextRaw(ctx context.Context) (MD, [][]string, bool) {
return raw.md, raw.added, true
}

// FromOutgoingContext returns the outgoing metadata in ctx if it exists. The
// returned MD should not be modified. Writing to it may cause races.
// Modification should be made to copies of the returned MD.
// FromOutgoingContext returns the outgoing metadata in ctx if it exists.
//
// All keys in the returned MD are lowercase.
func FromOutgoingContext(ctx context.Context) (MD, bool) {
raw, ok := ctx.Value(mdOutgoingKey{}).(rawMD)
if !ok {
return nil, false
}

out := raw.md.Copy()
out := MD{}
for k, v := range raw.md {
// We need to manually convert all keys to lower case, because MD is a
// map, and there's no guarantee that the MD attached to the context is
// created using our helper functions.
key := strings.ToLower(k)
out[key] = v
}
for _, added := range raw.added {
if len(added)%2 == 1 {
panic(fmt.Sprintf("metadata: FromOutgoingContext got an odd number of input pairs for metadata: %d", len(added)))
Expand Down

5 comments on commit 32d5490

@royeo
Copy link

@royeo royeo commented on 32d5490 Dec 15, 2021

Choose a reason for hiding this comment

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

This change is sucks, FromIncomingContextdo breaking change.

@easwars
Copy link
Contributor

Choose a reason for hiding this comment

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

This change is sucks, FromIncomingContextdo breaking change.

This change was merged more than 6 months ago. If this breaks something for you, can you please be more specific about what broke and in what way.

@markuspeloquin
Copy link

@markuspeloquin markuspeloquin commented on 32d5490 Mar 22, 2022 •
edited

Choose a reason for hiding this comment

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

FromOutgoingContext used to create a copy of metadata, allocating new slices for each value.

Now it reuses those slices, appending values from raw.added each time FromOutgoingContext is called. If the slices have excess capacity, the original metadata map is updated.

Both FromOutgoingContext and FromIncomingContext are similarly affected. It's now possible to modify the metadata (using the return value) if the slices have excess capacity.

Update Okay, the double append doesn't seem like an issue, exactly. Calling append on a slice doesn't change the original slice; it returns a new slice that may point to the same array on the heap.

The other bug (modifying the result) is obviously easier to trigger. Though you could argue that you shouldn't manipulate the map directly:

md := metadata.MD{}
md.Set("x", "true")
ctx := metadata.NewOutgoingContext(context.Background(), md)
md, _ = metadata.FromOutgoingContext(ctx)
md["x"][0] = "false"
md, _ = metadata.FromOutgoingContext(ctx)
fmt.Printf("%v\n", md["x"]) // prints: false

@menghanl
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for commenting!
That was an unexpected behavior change. Just made a PR to fix it: #5267

@dfawley
Copy link
Member

Choose a reason for hiding this comment

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

Though you could argue that you shouldn't manipulate the map directly:

Note that before this change, the doc strings on these functions said explicitly not to make modifications to the map. So while there was a behavior change, nobody should have noticed if they were following the documentation. That said, we definitely messed up by forgetting to copy the slice when we removed that comment, hence the fix in #5267.

Please sign in to comment.