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.

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

Code that relies on gRPC metadata formatting should be fixed #18303

Closed
4 tasks done
dfawley opened this issue Jul 8, 2024 · 11 comments
Closed
4 tasks done

Code that relies on gRPC metadata formatting should be fixed #18303

dfawley opened this issue Jul 8, 2024 · 11 comments

Comments

@dfawley
Copy link

dfawley commented Jul 8, 2024

Bug report criteria

What happened?

This code:

etcd/client/v3/watch.go

Lines 1038 to 1043 in 21e5876

func streamKeyFromCtx(ctx context.Context) string {
if md, ok := metadata.FromOutgoingContext(ctx); ok {
return fmt.Sprintf("%+v", md)
}
return ""
}

expects consistent ordering from gRPC as well as expecting all metadata to be included.

However, metadata can contain PII, so we would like to change gRPC's behavior to not print any potential PII. grpc/grpc-go#7395

What did you expect to happen?

Code correctness to not rely upon gRPC's metadata.MD.String method.

How can we reproduce it (as minimally and precisely as possible)?

Look at the code.

Anything else we need to know?

No response

Etcd version (please run commands below)

master

Etcd configuration (command line flags or environment variables)

No response

Etcd debug information (please run commands below, feel free to obfuscate the IP address or FQDN in the output)

No response

Relevant log output

No response

@dfawley
Copy link
Author

dfawley commented Jul 8, 2024

One trivial fix would be forcing the Go builtin formatting for the raw, underlying map type:

return fmt.Sprint(map[string][]string(md))

@ahrtr
Copy link
Member

ahrtr commented Jul 9, 2024

Thanks @dfawley for raising this ticket. We should raise it long time ago.

Please read clarification below In case anyone gets confused.

  • fmt.Sprintf("%+v", md) will call (md MD) String(), which iterates over a map, but the iteration order over maps is not specified and is not guaranteed to be the same from one iteration to the next.
  • Golang fmt package guarantees that Maps are printed in a consistent order, sorted by the values of the keys.

@ahrtr
Copy link
Member

ahrtr commented Jul 9, 2024

  • Golang fmt package guarantees that Maps are printed in a consistent order, sorted by the values of the keys.

It's exactly the reason why #18303 (comment) can resolve this issue.

@JabulaniNgcobo
Copy link

#18308

@JabulaniNgcobo
Copy link

#18289

@ahrtr
Copy link
Member

ahrtr commented Jul 13, 2024

@mohamedawnallah Please update changelog for both 3.5 and 3.4. thx

@mohamedawnallah
Copy link
Contributor

@ahrtr Sounds good! Updated the change log for releases 3.4 and 3.5 in the following PRs:

@ahrtr
Copy link
Member

ahrtr commented Jul 14, 2024

@ahrtr Sounds good! Updated the change log for releases 3.4 and 3.5 in the following PRs:

Thanks. Actually one PR should be good, there is no need to raise two separate PRs to update changelog. But not a big problem, so I won't insist on it.

@mohamedawnallah
Copy link
Contributor

Thanks for the clarification! Combining into a single PR makes sense. I'll follow that approach in the future. Thanks for the flexibility on this one!

@ahrtr
Copy link
Member

ahrtr commented Jul 14, 2024

All done!

Thanks @mohamedawnallah and @dfawley

@ahrtr ahrtr closed this as completed Jul 14, 2024
@dfawley
Copy link
Author

dfawley commented Jul 16, 2024

Thank you for the quick fixes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

4 participants