feat(transport): Log DirectPath misconfiguration #2225
Conversation
transport/grpc/dial.go
Outdated
| } else { | ||
| // Case 2: credential is not correctly set | ||
| if !isTokenSourceDirectPathCompatible(ts, o) { | ||
| log.Print("WARNING: DirectPath is misconfigured. Please make sure the token source is fetched from GCE metadata server and the default service account is used") |
There was a problem hiding this comment.
How does a user do this correctly?
There was a problem hiding this comment.
GCS client should set the AllowNonDefaultServiceAccount option to true, https://github.com/googleapis/google-api-go-client/blob/main/transport/grpc/dial.go#L269, and customers do not need to worry about this.
|
If we are going to start logging about this I think we need a nice section in the package docs on how to configure this all correctly. Also logging is not super ideal today because the way this is being done a user could shut off all logging that uses the standard library or keep it all on. Let me think about this. Are we sure we don't want to return an error for misconfiguration? What state will there app be in if this is not working correctly? |
I am not sure about what are the "package docs" you pointed to. My understanding is that each DirectPath service (GCS, CBT, CS) will publish its own doc to its customers.
The app will use CloudPath if DirectPath misconfiguration happens. The purpose here is to give a hint to the customers who want to use DirectPath but traffic is actually CloudPath. |
|
We are pending approval from @quartzmo |
Can I get some help about how to limit the log? Is |
Yes, I believe that is a good solution. I have not yet personally used it in production however. |
|
I added the rate.Sometimes to limit the log. PTAL. Thanks! |
|
@mohanli-ml IIUC you have to do this because Go doesn't have a similar option to what you did with Java in the constructor? I would think you could check this once without having to do it for every channel instantiation. |
The credential code for Java and Go is a bit different. In Java, the credential is passed to the builder, so the builder can do the check. In Go the credential is created during channel construction, so builder does not have this information. Anyways the code will not do the check for every channel instantiation, it is limited to every second. |
| @@ -296,6 +304,24 @@ func checkDirectPathEndPoint(endpoint string) bool { | |||
| return true | |||
| } | |||
|
|
|||
| func logDirectPathMisconfig(endpoint string, ts oauth2.TokenSource, o *internal.DialSettings) { | |||
There was a problem hiding this comment.
Please add tests for this function in case changes are made to this code there's expectation on what's expected for this code.
There was a problem hiding this comment.
Thanks @mohanli-ml one more approval from @quartzmo and let's ship it.
|
I do not have the access to merge. @quartzmo Chris, can you help? Thanks! |
noahdietz
added
the
automerge
I've added the |
gcf-merge-on-green
bot
removed
the
automerge
|
FYI @mohanli-ml this will be in v0.150.0 #2244 |
🤖 I have created a release *beep* *boop* --- ## [0.150.0](https://github.com/googleapis/google-api-go-client/compare/v0.149.0...v0.150.0) (2023-11-06) ### Features * **all:** Auto-regenerate discovery clients ([#2243](https://github.com/googleapis/google-api-go-client/issues/2243)) ([2ce2d2d](https://github.com/googleapis/google-api-go-client/commit/2ce2d2d63343cae224e0f44f37d0426e9c7acbaa)) * **all:** Auto-regenerate discovery clients ([#2245](https://github.com/googleapis/google-api-go-client/issues/2245)) ([5693997](https://github.com/googleapis/google-api-go-client/commit/56939974844fd102839077f56ca84454458749f2)) * **all:** Auto-regenerate discovery clients ([#2246](https://github.com/googleapis/google-api-go-client/issues/2246)) ([8bfbeac](https://github.com/googleapis/google-api-go-client/commit/8bfbeaca4cb80ffe972d56762c832ed31785e8ca)) * **all:** Auto-regenerate discovery clients ([#2249](https://github.com/googleapis/google-api-go-client/issues/2249)) ([b56da3d](https://github.com/googleapis/google-api-go-client/commit/b56da3d6d7c0ad083d8835dead8f681de263fa39)) * **all:** Auto-regenerate discovery clients ([#2250](https://github.com/googleapis/google-api-go-client/issues/2250)) ([c08d405](https://github.com/googleapis/google-api-go-client/commit/c08d405c38ecd133175b932232211e046c4aa07c)) * **all:** Auto-regenerate discovery clients ([#2252](https://github.com/googleapis/google-api-go-client/issues/2252)) ([7529003](https://github.com/googleapis/google-api-go-client/commit/752900316914347301220b5f5dc2eb5eea2a8325)) * **transport:** Log DirectPath misconfiguration ([#2225](https://github.com/googleapis/google-api-go-client/issues/2225)) ([85e85ad](https://github.com/googleapis/google-api-go-client/commit/85e85ad04e28b434dd731c3b537d0fc35ff3639a)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
We met issues where customers wanted to use DirectPath, but DirectPath was misconfigured and it took 2 weeks for debugging. So we want to add WARNING logs if DirectPath flag/option is set but DirectPath is not used. Issue: googleapis/sdk-platform-java#2164.
Java PR: googleapis/sdk-platform-java#2105.
Note that the log is added when a channel is created because we need to check the OAuth token, which means it will be logged every channel. This is a bad behavior if customer has a large channel pool size. Ideally I want to use something like LOG_EVERY_N_SEC, and I found
rate.Sometimescan do this. Cody, do you have suggestions here?@codyoss @frankyn