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

[Recorder] Ensure redirected requests are passed to the recorder in Node #21355

Merged
merged 2 commits into from
Apr 14, 2022

Conversation

timovv
Copy link
Member

@timovv timovv commented Apr 12, 2022

Packages impacted by this PR

  • @azure-tools/test-recorder

Issues associated with this PR

Describe the problem that is addressed by this PR

When the service returns a redirect response, our test proxy policy would not properly rewrite the second request that is made by the pipeline due to the redirection. This meant that the second request would go to the service directly, instead of via the test proxy, which causes a number of issues.

Until recently, the test proxy has been following redirects before returning responses during record mode. This has been identified as incorrect behavior (see some discussion at Azure/azure-sdk-tools#2982), and a change is in the works to make the test proxy follow redirects by default (although the old behavior will still be configurable -- we'll need this for the browser scenario, since we don't control redirection behavior there). This meant that the issue this PR fixes couldn't be encountered until now, since the test proxy would not return a redirect response under any circumstance.

The fix involves changing the recorder HTTP policy slightly. Previously, we didn't rewrite the request if the X-Recording-Id header was present (suggesting we had already redirected the request). This logic falls through if the second time the request passes through the HTTP policy, the request hostname has been changed (e.g. due to a redirect). That case would mean we should rewrite the request to point to the test proxy a second time. I have updated the logic so that we instead check if the request URL is already pointing to the test proxy, and if so, then we don't need to rewrite the request. This check is more robust and works in the case of a cross-domain redirect.

Are there test cases added in this PR? (If not, why?)

Yes. Added a few cases:

  • Redirects: testing to ensure correct behavior during redirect, regardless of whether the redirect is a full URL or is just relative. The previous logic handled relative redirects correctly, so I added a test case for both cases to ensure that there was no regression.
  • Retrying: retrying was the original reason we had logic to not rewrite the request twice, so I have added a test case to make sure that retrying works OK.

@timovv timovv added the test-utils-recorder Label for the issues related to the common recorder label Apr 12, 2022
@timovv timovv requested a review from sadasant as a code owner April 12, 2022 00:06
@timovv timovv merged commit 597adef into Azure:main Apr 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test-utils-recorder Label for the issues related to the common recorder
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Unified recorder] Redirected requests not being sent to test proxy
2 participants