-
Notifications
You must be signed in to change notification settings - Fork 84
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
feat: enable ALTS bound token (for DirectPath) in the grpc channel provider #2919
base: main
Are you sure you want to change the base?
Conversation
…o directpath-bound-token
I haven't got the e2e flow verified yet but confirmed this is the change we need to enable the bound token. So I will mark this ready for review to get feedback. I will update here once the e2e flow is verified. Would you PTAL this one-liner? Thanks! @BenWhitehead |
Just to give some update, I opened a nearly identical googleapis/java-spanner#3645 in Cloud Spanner and was able to verify the client logic was correct internally (where the monorepo made it easier to test) by seeing the access token request Url I've been trying to run the |
…orage into directpath-bound-token
When I run this branch with my dev logging I see the following request to the metadata service
Are there other dependencies that need updated in order to see the |
Thanks for testing this! Could you confirm that the gax-java dependency included googleapis/sdk-platform-java#3572? https://github.com/googleapis/sdk-platform-java/releases/tag/v2.53.0 should do. The underlying gax-grpc version should be 2.61.0. I use VS Code locally. If I click "Go to definition" of |
At the
So, from the maven perspective it's already updated. I ran the same code from #2919 (comment) and it produced the same request without Do any of the auth libraries need an update as well? |
This should be all we need because googleapis/sdk-platform-java#3572 invokes the new method from |
I think I found the problem. Unlike in java-spanner, here the InstantiatingGrpcChannelProvider is built with being given an |
Actually it's not immediately clear to me how the |
This is where we resolve the credentials instance from the base ServiceOptions java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcStorageOptions.java Lines 217 to 254 in 43553de
And here is where we pass it on to the gapic client builder: java-storage/google-cloud-storage/src/main/java/com/google/cloud/storage/GrpcStorageOptions.java Line 275 in 43553de
|
Thank you so much for your help so far, @BenWhitehead! Your maven knowledge saved me tons of time! Now I was able to confirm that with the current change, it will invoke the correct logic in The issue seems to be in how the hard bound credentials got created: https://github.com/googleapis/sdk-platform-java/blob/ad26cf98548e325c99edb263baf8fe1a7696e634/gax-java/gax-grpc/src/main/java/com/google/api/gax/grpc/InstantiatingGrpcChannelProvider.java#L1205-L1209. It converts the existing creds into a builder which actually copies over the In the CloudSpanner examples maybe the token hadn't been populated yet, so I was able to see the request with In summary, this PR itself should have no problem. @rmehta19 and I will work on fixing the logic in gax. I think this PR can wait till gax is fixed and the dependencies get updated. |
Sounds good @rockspore, please let me know when you want me to consider merging this. |
Hi @BenWhitehead to give you an update. Technically this PR is ready to be merged. But as I talked within the team, because GCS already has DirectPath as a GA feature, we would like to play safe here. Meanwhile, I also feel reluctant to add a new flag guard to toggle this because java-storage doesn't have known internal users and a default-off pretty much means no adoption. So my plan is to wait till we get some good signal from at least one of the following:
before we merge this PR. Let me know if you have a better idea. Thanks. |
I tend to agree. Holding off until we have signal from a different bundle I think makes sense. |
This will allow the gRPC channel provider to set up hard bound (compute engine) call credentials for the channel when DirectPath is compatible.
If the GCE/GKE metadata servers' support for such bound access tokens is not available yet, it will end up getting the normal unbound ones and nothing should fail.
No public storage API is changed.