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

Jaeger query doesn't propagate auth token on dependency endpoint #2369

Closed
yoave23 opened this issue Aug 3, 2020 · 5 comments · Fixed by #2434
Closed

Jaeger query doesn't propagate auth token on dependency endpoint #2369

yoave23 opened this issue Aug 3, 2020 · 5 comments · Fixed by #2434
Labels

Comments

@yoave23
Copy link
Contributor

yoave23 commented Aug 3, 2020

Describe the bug
Jaeger query doesn't propagate auth token on the /dependencies endpoint. Works fine others.
To Reproduce
Steps to reproduce the behavior:

  1. Start jaeger-query with QUERY_BEARER_TOKEN_PROPAGATION=true
  2. make a GET request to /dependencies?endTs=?????????&lookback=??????? with an authorization token
  3. inspect jaeger-query request headers

Expected behavior
The dependency endpoint should propagate the auth token the same way it does on other endpoints (/operations, /traces, etc.)

Version (please complete the following information):

  • OS: [macOS]
  • Jaeger version: [1.8]
  • Deployment: [Docker]

Additional context
We're using ES as storage although it seems that it affects all storage types
looks like the issue (for ES storage) is located here: plugin/storage/es/dependencystore/storage.go:84 where it uses the DependencyStore's context (Background context) instead of the original one

@yoave23 yoave23 added the bug label Aug 3, 2020
@ghost ghost added the needs-triage label Aug 3, 2020
@objectiser
Copy link
Contributor

@yoave23 Thanks for reporting this - would you be interested in submitting a PR?

@yoave23
Copy link
Contributor Author

yoave23 commented Aug 3, 2020

@objectiser sure, on it

@yoave23
Copy link
Contributor Author

yoave23 commented Aug 3, 2020

@objectiser it seems like we need to change the GetDependencies in storage/dependencystore/interface.go:32 in order to be able to use the real context. Does this sound like a valid approach? if not, would you mind recommending a different approach?

@objectiser
Copy link
Contributor

@yoave23 Yes that is the right approach.

@objectiser
Copy link
Contributor

@yoave23 Sorry should have realised earlier, but might be worth waiting for #2364 to merge (hopefully this week) - and then add your change on top?

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

Successfully merging a pull request may close this issue.

2 participants