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

fix(googleclientauth/idtoken): use metadata server when credential json is not available #863

Conversation

rinx
Copy link
Contributor

@rinx rinx commented Jun 20, 2024

googleclientauth extension with id_token mode works correctly when credential.json is specified.
However, the current implementation doesn't work correctly with the metadata server.

This PR fixes its behavior.
I confirmed that it works correctly on Cloud Run.

@rinx rinx requested a review from a team as a code owner June 20, 2024 12:02
Copy link

codecov bot commented Jun 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 62.88%. Comparing base (4caace7) to head (b492386).
Report is 24 commits behind head on main.

Current head b492386 differs from pull request most recent head 70b42d3

Please upload reports for the commit 70b42d3 to get more accurate results.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #863      +/-   ##
==========================================
+ Coverage   61.03%   62.88%   +1.85%     
==========================================
  Files          56       57       +1     
  Lines        5903     4958     -945     
==========================================
- Hits         3603     3118     -485     
+ Misses       2143     1680     -463     
- Partials      157      160       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@damemi damemi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reading the code for NewTokenSource, it looks like idtoken.WithCredentialsJSON returns an option function that just copies the JSON (creds.json) to a field.

NewTokenSource then calls internal.Creds() which calls baseCreds() which reads from the JSON if it's not nil. Otherwise, newTokenSource() falls back to the metadata server by default.

I'm guessing that the bug here comes from the current line always setting idtoken.WithCredentialsJSON(creds.JSON) even if creds.JSON is empty.

My confusion though, is that I would expect the option function I linked above to make an empty array, but I guess that's different from a nil array? And that's the bug, because baseCreds() is checking for creds != nil when maybe it should be checking for len(creds) > 0.

Either way, @rinx is there any way we can add a test for this? Verifying manually is good but we should have something reproducible

@damemi
Copy link
Contributor

damemi commented Jun 20, 2024

/gcbrun

@rinx rinx force-pushed the extension-googleclientauth-fix-idtoken-metadata-server branch from 252747b to 3a73d30 Compare June 21, 2024 13:13
@rinx rinx force-pushed the extension-googleclientauth-fix-idtoken-metadata-server branch from 3a73d30 to 70b42d3 Compare June 21, 2024 13:18
@rinx
Copy link
Contributor Author

rinx commented Jun 21, 2024

@damemi Your point is correct.
I've added several test cases to verify the changes are correct.

@rinx rinx requested a review from damemi June 25, 2024 01:41
@damemi
Copy link
Contributor

damemi commented Jun 25, 2024

/gcbrun

@damemi damemi merged commit 648a71c into GoogleCloudPlatform:main Jul 8, 2024
25 of 26 checks passed
mx-psi referenced this pull request in open-telemetry/opentelemetry-collector-contrib Jul 30, 2024
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
|
[github.com/GoogleCloudPlatform/opentelemetry-operations-go/detectors/gcp](https://togithub.com/GoogleCloudPlatform/opentelemetry-operations-go)
| `v1.24.0` -> `v1.24.1` |
[![age](https://developer.mend.io/api/mc/badges/age/go/github.com%2fGoogleCloudPlatform%2fopentelemetry-operations-go%2fdetectors%2fgcp/v1.24.1?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/go/github.com%2fGoogleCloudPlatform%2fopentelemetry-operations-go%2fdetectors%2fgcp/v1.24.1?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/go/github.com%2fGoogleCloudPlatform%2fopentelemetry-operations-go%2fdetectors%2fgcp/v1.24.0/v1.24.1?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/go/github.com%2fGoogleCloudPlatform%2fopentelemetry-operations-go%2fdetectors%2fgcp/v1.24.0/v1.24.1?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
|
[github.com/GoogleCloudPlatform/opentelemetry-operations-go/extension/googleclientauthextension](https://togithub.com/GoogleCloudPlatform/opentelemetry-operations-go)
| `v0.48.1-0.20240618202726-8ffe2564d48b` -> `v0.48.1` |
[![age](https://developer.mend.io/api/mc/badges/age/go/github.com%2fGoogleCloudPlatform%2fopentelemetry-operations-go%2fextension%2fgoogleclientauthextension/v0.48.1?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/go/github.com%2fGoogleCloudPlatform%2fopentelemetry-operations-go%2fextension%2fgoogleclientauthextension/v0.48.1?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/go/github.com%2fGoogleCloudPlatform%2fopentelemetry-operations-go%2fextension%2fgoogleclientauthextension/v0.48.1-0.20240618202726-8ffe2564d48b/v0.48.1?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/go/github.com%2fGoogleCloudPlatform%2fopentelemetry-operations-go%2fextension%2fgoogleclientauthextension/v0.48.1-0.20240618202726-8ffe2564d48b/v0.48.1?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

> [!WARNING]
> Some dependencies could not be looked up. Check the Dependency
Dashboard for more information.

---

### Release Notes

<details>
<summary>GoogleCloudPlatform/opentelemetry-operations-go
(github.com/GoogleCloudPlatform/opentelemetry-operations-go/extension/googleclientauthextension)</summary>

###
[`v0.48.1`](https://togithub.com/GoogleCloudPlatform/opentelemetry-operations-go/releases/tag/v0.48.1):
v1.24.1 and v0.48.1

[Compare
Source](https://togithub.com/GoogleCloudPlatform/opentelemetry-operations-go/compare/v0.48.0...v0.48.1)

#### What's Changed

- \[chore] update deprecated use of CreateSettings by
[@&#8203;codeboten](https://togithub.com/codeboten) in
[https://github.com/GoogleCloudPlatform/opentelemetry-operations-go/pull/862](https://togithub.com/GoogleCloudPlatform/opentelemetry-operations-go/pull/862)
- fix(googleclientauth/idtoken): use metadata server when credential
json is not available by [@&#8203;rinx](https://togithub.com/rinx) in
[https://github.com/GoogleCloudPlatform/opentelemetry-operations-go/pull/863](https://togithub.com/GoogleCloudPlatform/opentelemetry-operations-go/pull/863)
- Update cloud.google.com/go and OTel dependencies by
[@&#8203;dashpole](https://togithub.com/dashpole) in
[https://github.com/GoogleCloudPlatform/opentelemetry-operations-go/pull/867](https://togithub.com/GoogleCloudPlatform/opentelemetry-operations-go/pull/867)
- Update to otel-go codecov presubmit by
[@&#8203;dashpole](https://togithub.com/dashpole) in
[https://github.com/GoogleCloudPlatform/opentelemetry-operations-go/pull/868](https://togithub.com/GoogleCloudPlatform/opentelemetry-operations-go/pull/868)
- Fix typo in intToDoubleFeatureGate by
[@&#8203;chingis-fiskil](https://togithub.com/chingis-fiskil) in
[https://github.com/GoogleCloudPlatform/opentelemetry-operations-go/pull/870](https://togithub.com/GoogleCloudPlatform/opentelemetry-operations-go/pull/870)
- Prerelease 1.24.1/v0.48.1 by
[@&#8203;damemi](https://togithub.com/damemi) in
[https://github.com/GoogleCloudPlatform/opentelemetry-operations-go/pull/872](https://togithub.com/GoogleCloudPlatform/opentelemetry-operations-go/pull/872)

#### New Contributors

- [@&#8203;chingis-fiskil](https://togithub.com/chingis-fiskil) made
their first contribution in
[https://github.com/GoogleCloudPlatform/opentelemetry-operations-go/pull/870](https://togithub.com/GoogleCloudPlatform/opentelemetry-operations-go/pull/870)

**Full Changelog**:
GoogleCloudPlatform/opentelemetry-operations-go@v0.48.0...v0.48.1

</details>

---

### Configuration

📅 **Schedule**: Branch creation - "on tuesday" (UTC), Automerge - At any
time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

👻 **Immortal**: This PR will be recreated if closed unmerged. Get
[config help](https://togithub.com/renovatebot/renovate/discussions) if
that's undesired.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR was generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View the
[repository job
log](https://developer.mend.io/github/open-telemetry/opentelemetry-collector-contrib).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy40NDAuNyIsInVwZGF0ZWRJblZlciI6IjM3LjQ0MC43IiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6WyJkZXBlbmRlbmNpZXMiLCJyZW5vdmF0ZWJvdCJdfQ==-->

---------

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: opentelemetrybot <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants