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

all: Application Default Creds not included in context or tracing #4487

Closed
tam7t opened this issue Jul 22, 2021 · 3 comments
Closed

all: Application Default Creds not included in context or tracing #4487

tam7t opened this issue Jul 22, 2021 · 3 comments
Assignees
Labels
type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.

Comments

@tam7t
Copy link

tam7t commented Jul 22, 2021

Is your feature request related to a problem? Please describe.

When ADC is used with instance-metadata

  • resulting requests to instance metadata will not have RPC deadlines
  • open-telemetry tracing of RPC calls wont include spans for retrieving metadata auth tokens (at least when using go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc)

This is due to:

Describe the solution you'd like

  • I'd expect request deadlines to propagate to all downstream RPCs from the initial method call
  • A way to enable tracing that separates out time spent talking to metadata server from time spent talking to the API

Describe alternatives you've considered

Additional context

@tam7t tam7t added the triage me I really want to be triaged. label Jul 22, 2021
@codyoss
Copy link
Member

codyoss commented Jul 22, 2021

Hey @tam7t thanks for the request. I am not familiar with otelgrpc but I believe any requests that are made for ADC will all be HTTP based. To see this supported I agree that golang/oauth2#262 would need to be implemented first. Although that is an interface so we can't make changes to it, a new interface would need to be created and a migration path figured out for the many libraries using TokenSource today.

I don't think this request is really related to #4483 as that package just makes HTTP requests to a known endpoint, there is no auth involved.

This is something that is probably worth exploring and talking through but I think the changes would need to start in golang/oauth2 and then propagate to other packages. Would you mind opening this issue over on the oauth2 repo or adding some more context to golang/oauth2#262 why that change is important.

cc @broady

@codyoss codyoss added type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. and removed triage me I really want to be triaged. labels Jul 22, 2021
@codyoss codyoss self-assigned this Jul 22, 2021
@tam7t
Copy link
Author

tam7t commented Jul 22, 2021

Yeah, I only tagged #4483 because the oauth2 package depends on it in order to fetch the auth token from instance metadata. I will comment on the golang/oauth2 package with context.

@codyoss
Copy link
Member

codyoss commented Apr 16, 2024

With our new auth module starting to roll out token calls will now accept a context from the call they originate from. Also the compute/metadata package was likewise recently updated.

@codyoss codyoss closed this as completed Apr 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
None yet
Development

No branches or pull requests

2 participants