Skip to content

chore: use original API request context for auth token requests#847

Merged
ctreatma merged 7 commits into
mainfrom
shared-auth
Jul 1, 2025
Merged

chore: use original API request context for auth token requests#847
ctreatma merged 7 commits into
mainfrom
shared-auth

Conversation

@ctreatma
Copy link
Copy Markdown
Contributor

@ctreatma ctreatma commented Jan 29, 2025

#844 updated the Config object to create a new auth client every time a new Fabric API client was created for SDK resources. If we stuck with this pattern, we would have to duplicate that behavior for every Equinix API client except for Metal. In addition, this pattern means that the provider will make a separate auth token request per resource rather than reusing valid tokens across all resources that use the same provider (and therefore auth) config.

This reverts the auth client change by adopting a custom HTTP transport and OAuth token source that enables passing the original request context for an API request to the auth token client. As a result, we no longer need to use equinix/oauth2-go.

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jan 29, 2025

Codecov Report

Attention: Patch coverage is 96.77419% with 1 line in your changes missing coverage. Please review.

Project coverage is 48.19%. Comparing base (f3d9263) to head (fd0f6c5).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
internal/config/config.go 96.77% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main     #847       +/-   ##
===========================================
+ Coverage   27.94%   48.19%   +20.24%     
===========================================
  Files         240      240               
  Lines       31715    31712        -3     
===========================================
+ Hits         8864    15283     +6419     
+ Misses      22696    15733     -6963     
- Partials      155      696      +541     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ctreatma ctreatma force-pushed the shared-auth branch 3 times, most recently from f78ef0c to 2e16ea4 Compare February 4, 2025 14:58
@ctreatma ctreatma marked this pull request as ready for review February 4, 2025 15:06
@ctreatma ctreatma requested a review from a team as a code owner February 4, 2025 15:06
@ctreatma ctreatma changed the title chore: [WIP] investigating options for sharing authClient across resources & services chore: use original API request context for auth token requests Feb 4, 2025
thogarty
thogarty previously approved these changes Feb 5, 2025
Copy link
Copy Markdown
Contributor

@thogarty thogarty left a comment

Choose a reason for hiding this comment

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

Awesome, @ctreatma !

I'm still working through the lower level details but your implementation looks solid from a decent high level understanding.

I'm curious if this removes the API Request/Response from the log output though with a different round tripper. If it does then I'll add it back in with a follow up after looking into it.

@thogarty
Copy link
Copy Markdown
Contributor

thogarty commented Feb 5, 2025

Awesome, @ctreatma !

I'm still working through the lower level details but your implementation looks solid from a decent high level understanding.

I'm curious if this removes the API Request/Response from the log output though with a different round tripper. If it does then I'll add it back in with a follow up after looking into it.

Answered my own question. It doesn't because we still wrap this in logging.NewTransport which handles that for us with an additional wrapper. Good to go 👍

@ctreatma
Copy link
Copy Markdown
Contributor Author

@displague also interested to get your take on this one.

Comment thread internal/authtoken/oauth2.go Outdated
@displague displague requested a review from Copilot February 20, 2025 17:48
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.

Files not reviewed (1)
  • go.mod: Language not supported
Comments suppressed due to low confidence (2)

internal/authtoken/oauth2.go:90

  • The error message 'oauth2: server response missing access token' could be more descriptive. Consider including additional context about the server response.
return nil, fmt.Errorf("oauth2: server response missing access token")

internal/config/config.go:83

  • The new behavior introduced in 'newAuthClient' function is not covered by tests. Ensure that this function is properly tested.
c.authClient = c.newAuthClient()

Comment thread internal/authtoken/transport.go Outdated
Comment thread go.mod
Comment thread internal/authtoken/transport.go Outdated
@thogarty
Copy link
Copy Markdown
Contributor

@ctreatma + @displague , what are your thoughts on this one?

We've got an open Goalie ticket for auth related issues on fabric_service_profile data source. My theory is that they're hitting a rate limiting issue trying to get so many auth tokens for each call.

This should fix that issue.

@ctreatma
Copy link
Copy Markdown
Contributor Author

ctreatma commented Mar 18, 2025

I think we should be a bit more certain that the requests are being rate-limited for the customer in question before merging/releasing a change to auth logic for all customers. While this should be safe for everyone, it's nonetheless a core change and comes with some risk.

In terms of pure mechanics, releasing this will require review/merge/release of the corresponding SDK changes, and I don't recall how hacked-in those changes are.

SDK draft PR used for this feature: equinix/equinix-sdk-go#102

@ctreatma ctreatma force-pushed the shared-auth branch 2 times, most recently from fc38b02 to 29f9e9f Compare June 16, 2025 21:31
@ctreatma ctreatma marked this pull request as ready for review June 16, 2025 21:56
@ctreatma ctreatma marked this pull request as draft June 17, 2025 14:47
ctreatma added a commit to equinix/equinix-sdk-go that referenced this pull request Jun 18, 2025
This adds a service covering the `/oauth2/v1` API endpoints. One oddity
here is that the service is called `accesstokenv1`, which matches its
name according to the API catalog, but per the recommendation from `make
onboard-service`, it _should_ be called `oauth2v1`.

This service is used by equinix/terraform-provider-equinix#847. In the
Terraform provider we need to be able to reuse the same token across
multiple request contexts to avoid making unnecessary duplicate requests
to the token service. That means we need to introduce some custom types
based on `golang.org/x/oauth2` because [the `oauth2` library does not
accept a context at the time a token is
obtained](golang/oauth2#262) (in the core
library, the context can only be specified when a token source is
instantiated).
@ctreatma ctreatma marked this pull request as ready for review June 18, 2025 14:31
ctreatma pushed a commit that referenced this pull request Jun 30, 2025
…#904)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
|
[github.com/equinix/equinix-sdk-go](https://github.com/equinix/equinix-sdk-go)
| `v0.52.0` -> `v0.53.0` |
[![age](https://developer.mend.io/api/mc/badges/age/go/github.com%2fequinix%2fequinix-sdk-go/v0.53.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/go/github.com%2fequinix%2fequinix-sdk-go/v0.53.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/go/github.com%2fequinix%2fequinix-sdk-go/v0.52.0/v0.53.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/go/github.com%2fequinix%2fequinix-sdk-go/v0.52.0/v0.53.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>equinix/equinix-sdk-go
(github.com/equinix/equinix-sdk-go)</summary>

###
[`v0.53.0`](https://github.com/equinix/equinix-sdk-go/releases/tag/v0.53.0)

[Compare
Source](https://github.com/equinix/equinix-sdk-go/compare/v0.52.0...v0.53.0)

##### Features

- onboard accesstokenv1 service
([#&#8203;102](https://github.com/equinix/equinix-sdk-go/issues/102))
([c607127](https://github.com/equinix/equinix-sdk-go/commit/c607127e1bbd8e488f17a4dcc12f6c5a861c4abd)),
closes
[#847](https://github.com/equinix/terraform-provider-equinix/issues/847)

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
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.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

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

---

This PR was generated by [Mend Renovate](https://mend.io/renovate/).
View the [repository job
log](https://developer.mend.io/github/equinix/terraform-provider-equinix).

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

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
@ctreatma
Copy link
Copy Markdown
Contributor Author

@displague @thogarty The SDK has been updated with accesstokenv1 support and the corresponding context-aware TokenSource, so this PR is ready for final review.

The Fabric test workflow was recently changed such that this PR did not trigger the workflow; rather than address that here I ran the test workflow manually against this branch: https://github.com/equinix/terraform-provider-equinix/actions/runs/15980969812

Copy link
Copy Markdown
Member

@displague displague left a comment

Choose a reason for hiding this comment

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

In the test failures, I see evidence that #912 is needed (to add the missing shell script used for processing test output) and I see this equinix_fabric_route_aggregation sweep failed (@thogarty):

2025/06/30 18:45:55 [DEBUG*** Sweeping Route Aggregations
2025/06/30 18:45:55 [DEBUG*** POST https://uatapi.equinix.com/fabric/v4/routeAggregations/search
2025/06/30 18:45:55 [DEBUG*** Completed Sweeper (equinix_fabric_route_aggregation) in region (all) in 408.432994ms
2025/06/30 18:45:55 [ERROR*** Error running Sweeper (equinix_fabric_route_aggregation) in region (all): error getting streams list for sweeping fabric route aggregations: 400 Bad Request Code: EQ-3044002, Message: Invalid argument value passed., Details: Invalid argument value passed., AdditionalInfo: [***Property: /values, Invalid argument value passed.***

Aside from several other Fabric 400s due to resources not being available to the user, I see no signs that this optimization to use a generated SDK for auth token processing negatively affected authentication/authorization.

@ctreatma ctreatma merged commit 91ca477 into main Jul 1, 2025
12 of 15 checks passed
@ctreatma ctreatma deleted the shared-auth branch July 1, 2025 15:47
@github-actions
Copy link
Copy Markdown

This PR is included in version 3.11.1 🎉

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.

5 participants