-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
fix(auth): make sure quota option takes precedence over env/file #10797
Conversation
I can confirm that this fixes the issue I raised (with the reproducing example I have). |
How does this fix work? Why does the capitalization matter? |
The reason I'm asking is that header field names MUST be converted to lowercase prior to their encoding in HTTP/2. But the changed header key in this PR retains the uppercase |
@quartzmo The capitalization only really matters here for the gRPC case, but I changed both for consistency. It matters so that there is a collision in the map sent by the bridging code I linked to in the PR desc. |
@@ -38,7 +38,7 @@ const ( | |||
// Check env to decide if using google-c2p resolver for DirectPath traffic. | |||
enableDirectPathXdsEnvVar = "GOOGLE_CLOUD_ENABLE_DIRECT_PATH_XDS" | |||
|
|||
quotaProjectHeaderKey = "X-Goog-User-Project" | |||
quotaProjectHeaderKey = "X-goog-user-project" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it's OK to leave X
capitalized here in grpc metadata?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, because that is what it is here: https://github.com/googleapis/google-api-go-client/blob/21f10edb1d9d6a3d3baa5b1edcef6588da99def8/transport/grpc/dial.go#L212
Also align header keys set from old library. Example: https://github.com/googleapis/google-api-go-client/blob/21f10edb1d9d6a3d3baa5b1edcef6588da99def8/transport/grpc/dial.go#L212
Eventually we should consider pushing all quota project resolution down the the credentials package, but for now this will work.
Fixes: #10795