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

Test proxy & storage tests improvements #4241

Merged
merged 21 commits into from
Feb 22, 2023
Merged

Conversation

Jinming-Hu
Copy link
Member

@Jinming-Hu Jinming-Hu commented Jan 12, 2023

In this PR, we:

  1. Removed unused definitions in sdk/core/azure-core-test/inc/azure/core/test/network_models.hpp.
  2. Removed unused functions TestBase::ValidateSkippingTest() and TestBase::IsValidTime().
  3. Added an overload of function TestBase::InitClientOptions().
  4. Removed unused functions TestProxyManager::SetStopRecordMode(), TestProxyManager::SetStopPlaybackMode().
  5. Rewrote proxy matcher setting logic.
    1. Use json object rather than hard-coded json string, for better readability and maintenance.
    2. Exclude Expect and Connection headers so that recording made by libcurl can be used by WinHTTP and also the other way around
    3. Removed some unnecessary ignored headers
    4. Added some ignored query parameters
  6. Fixed an issue in libcurl transport layer. Closes Bug: Host header is not correctly set for non-default port #4213
  7. Fixed an issue where libcurl transport layer adds Content-Length header for HEAD/GET/DELETE requests, which caused a behavior misalignment between libcurl and WinHTTP, making the requests recorded by one of the transport layer unusable by the other.
    1. Also removed Content-Length header for GET requests from existing recordings.
  8. Fixed a bug where test proxy sometimes discarded request body.
  9. Fixed a bug where requests were not properly redirected if Host was set in the original request *
  10. Enabled some storage test cases that were disabled in Test proxy #4118. Closes LIVEONLY tests in Storage  #4147
  11. Removed CHECK_SKIP_TEST macro. We check if test cases is live-only in SetUpTestBase() function. With this change, the only thing we need to do to mark a test case live-only is to add _LIVEONLY_ suffix in case name. There's no need to call CHECK_SKIP_TEST macro anymore.

* A little more explanation of this bug:
Transport layer accepts a request with a URL and request headers in it. If Host header is not in request headers, libcurl transport will extract host from URL and set Host header. If Host is already in request, libcurl transport does nothing.
Test-proxy policy modifies host in URL to localhost:5001, but it doesn't modify Host header (I think it's a negligence).
For most of the requests we don't set Host header. Test-proxy modifies host in the URL to localhost:5001, libcurl extracts localhost:5001 from URL and set Host header. Everything works perfectly.
For very few requests, Host header is already set. Test-proxy only modifies host in the URL, libcurl doesn't set Host header because there's one already. Now the host in Host header and URL don't match. These requests cannot be recorded & played back.

Pull Request Checklist

Please leverage this checklist as a reminder to address commonly occurring feedback when submitting a pull request to make sure your PR can be reviewed quickly:

See the detailed list in the contributing guide.

  • C++ Guidelines
  • Doxygen docs
  • Unit tests
  • No unwanted commits/changes
  • Descriptive title/description
    • PR is single purpose
    • Related issue listed
  • Comments in source
  • No typos
  • Update changelog
  • Not work-in-progress
  • External references or docs updated
  • Self review of PR done
  • Any breaking changes?

@Jinming-Hu Jinming-Hu added Client This issue points to a problem in the data-plane of the library. Storage Storage Service (Queues, Blobs, Files) test-enhancement labels Jan 12, 2023
@Jinming-Hu Jinming-Hu self-assigned this Jan 12, 2023
Copy link
Member

@antkmsft antkmsft left a comment

Choose a reason for hiding this comment

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

You probably want George's review, but as for me, changes look good.
You are touching a lot of tests and as a result, code coverage may improve. I suggest to grab code coverage numbers from a successful run closer to the end of code review, and update the numbers in ci.ymls.

@Jinming-Hu
Copy link
Member Author

Jinming-Hu commented Jan 12, 2023

Thanks @antkmsft !

@gearama Can you please help review this PR? and also take a look at the sanitizer issue #4147?

@gearama
Copy link
Member

gearama commented Jan 12, 2023

/azp run cpp - keyvault - ci

@azure-pipelines
Copy link

Azure Pipelines could not run because the pipeline triggers exclude this branch/path.

@gearama
Copy link
Member

gearama commented Jan 12, 2023

/azp run cpp - attestation - ci

@azure-pipelines
Copy link

Azure Pipelines could not run because the pipeline triggers exclude this branch/path.

@gearama
Copy link
Member

gearama commented Jan 12, 2023

need to make sure all the other piplines run , make a change in the read me in the folder of the services to kick in the test runs
image

@Jinming-Hu
Copy link
Member Author

need to make sure all the other piplines run , make a change in the read me in the folder of the services to kick in the test runs
image

@gearama What change do I need to make to enable the tests?

@antkmsft
Copy link
Member

@Jinming-Hu, I think @gearama meant that you can touch some files in each of the library, so the CI would trigger a run of cpp - (library) - ci. The other way to do this is probably by manually kicking off these pipelines.

@antkmsft
Copy link
Member

@Jinming-Hu, you can trigger CI pipelines manually, as I just did for identity pipeline, but the runs won't be visible here.

image

Maybe it is better to just add a space into each library's inc/library.hpp header (undoing it after the PR is done).

I wonder if running non-CI pipelines would give us the same level of verification (although taking longer). These ones you can run via /azp run cpp - (library).

Next week I'll bring this up, I think we should allow the /azp command to trigger runs for CI pipelines, even if none of the files from that library was modified in the PR. Because 1. a responsible human triggers the command knowing best what he wants; 2. This PR demonstrates it could be useful/needed sometimes.

@Jinming-Hu
Copy link
Member Author

@antkmsft Really? There's no way to trigger the pipeline without actually making changes? Is that how pipeline works? It's kind of inconvenient.

So we'll need to add an extra empty line or something in the README.md to trigger the pipeline. If all the tests pass, we revert the changes. It that correct?

Copy link
Member

@LarryOsterman LarryOsterman left a comment

Choose a reason for hiding this comment

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

It looks good to me, don't forget to handle #4372 at some point :).

@Jinming-Hu Jinming-Hu merged commit 395e9a0 into Azure:main Feb 22, 2023
@Jinming-Hu Jinming-Hu deleted the fix_proxy branch February 22, 2023 01:37
microzchang added a commit that referenced this pull request Mar 6, 2023
* update comment (#4364)

* update comment

* jghjg

* update broken link

* Update sdk/keyvault/tools/cleanup/src/cleanup.cpp

Co-authored-by: Anton Kolesnyk <[email protected]>

---------

Co-authored-by: Anton Kolesnyk <[email protected]>

* Docker comment (#4375)

* update comment

* add comment about vcpkg

* dsfs

* Trigger`keyvault` on proxy changes (#4343)

* autotrigger the keyvault and storage CI when a testproxy file is changed

* Generate API review for C++ using new parser (#4302)

* Add vmImage back to common perf.yml - Fixes #5466 - Partially reverts #5456 (#4376)

Co-authored-by: Mike Harder <[email protected]>

* Fix pipelines path (#4358)

* test path

* qwq

* dsda

* asas

* dsada

* sdsds

* Update sdk/keyvault/azure-security-keyvault-certificates/perf-tests.yml

Co-authored-by: Mike Harder <[email protected]>

* Update sdk/keyvault/azure-security-keyvault-certificates/perf-tests.yml

Co-authored-by: Mike Harder <[email protected]>

* remove warmup

---------

Co-authored-by: Mike Harder <[email protected]>

* Test proxy & storage tests improvements (#4241)

* Temporarily pin Node 18 to 18.13.0 - Fixes #5536 (#4378)

Co-authored-by: Mike Harder <[email protected]>

* Storage tests improvement (#4382)

* Update CODEOWNERS (#4380)

* show headers and query parameters in storage CI pipeline (#4379)

* Sync eng/common directory with azure-sdk-tools for PR 5431 #2501

Co-authored-by: Konrad Jamrozik <[email protected]>

* Sync eng/common directory with azure-sdk-tools for PR 5562 (#4384)

* Add todos to update packages to pick up the newest CODEOWNERS interpreter

* update

---------

Co-authored-by: Konrad Jamrozik <[email protected]>

* Fix Share Client failure #4377 (#4381)

* Fix Share Client failure #4377

* decrease request count to avoid throttling errors for storage tests (#4385)

* Sync eng/common directory with azure-sdk-tools for PR 5568 (#4387)

* we encourage folks to place their assets.jsons at the package level

* update generate-assets-json.ps1 to only include src/**/session-records so as to avoid picking up the duplicated 'target' sessionrecords

---------

Co-authored-by: scbedd <[email protected]>

* Add support to ignore invalid cert common name (#4361)

* update to version 7.4 for admin. update tests (#4388)

* update proxy version to include Info/Active against individual sessions + allow delayed response (#4391)

Co-authored-by: scbedd <[email protected]>

* Sync eng/common directory with azure-sdk-tools for PR 5540 (#4396)

* add parameter to set cadl emitter options

* remove emitter name in the additional parameter

---------

Co-authored-by: chunyu3 <[email protected]>

* Follow-up to update changelog to reflect community contribution (#4393)

* Follow-up to update changelog to reflect community contribution

* Upgrade cspell version from 0.1 to 0.2

* logging api post request body (#4404)

Co-authored-by: Albert Cheng <[email protected]>

* Sync eng/common directory with azure-sdk-tools for PR 5595 (#4400)

* Use "npm ci" to install cspell and respect package-lock.json

* Review feedback

* Pipe npm ci output to Write-Host

---------

Co-authored-by: Daniel Jurek <[email protected]>

---------

Co-authored-by: George Arama <[email protected]>
Co-authored-by: Anton Kolesnyk <[email protected]>
Co-authored-by: Scott Beddall <[email protected]>
Co-authored-by: Praven Kuttappan <[email protected]>
Co-authored-by: Azure SDK Bot <[email protected]>
Co-authored-by: Mike Harder <[email protected]>
Co-authored-by: JinmingHu <[email protected]>
Co-authored-by: Konrad Jamrozik <[email protected]>
Co-authored-by: chunyu3 <[email protected]>
Co-authored-by: Ahson Khan <[email protected]>
Co-authored-by: Albert Cheng <[email protected]>
Co-authored-by: Daniel Jurek <[email protected]>
microzchang added a commit to microzchang/azure-sdk-for-cpp that referenced this pull request Apr 4, 2023
* update comment (Azure#4364)

* update comment

* jghjg

* update broken link

* Update sdk/keyvault/tools/cleanup/src/cleanup.cpp

Co-authored-by: Anton Kolesnyk <[email protected]>

---------

Co-authored-by: Anton Kolesnyk <[email protected]>

* Docker comment (Azure#4375)

* update comment

* add comment about vcpkg

* dsfs

* Trigger`keyvault` on proxy changes (Azure#4343)

* autotrigger the keyvault and storage CI when a testproxy file is changed

* Generate API review for C++ using new parser (Azure#4302)

* Add vmImage back to common perf.yml - Fixes Azure#5466 - Partially reverts Azure#5456 (Azure#4376)

Co-authored-by: Mike Harder <[email protected]>

* Fix pipelines path (Azure#4358)

* test path

* qwq

* dsda

* asas

* dsada

* sdsds

* Update sdk/keyvault/azure-security-keyvault-certificates/perf-tests.yml

Co-authored-by: Mike Harder <[email protected]>

* Update sdk/keyvault/azure-security-keyvault-certificates/perf-tests.yml

Co-authored-by: Mike Harder <[email protected]>

* remove warmup

---------

Co-authored-by: Mike Harder <[email protected]>

* Test proxy & storage tests improvements (Azure#4241)

* Temporarily pin Node 18 to 18.13.0 - Fixes Azure#5536 (Azure#4378)

Co-authored-by: Mike Harder <[email protected]>

* Storage tests improvement (Azure#4382)

* Update CODEOWNERS (Azure#4380)

* show headers and query parameters in storage CI pipeline (Azure#4379)

* Sync eng/common directory with azure-sdk-tools for PR 5431 Azure#2501

Co-authored-by: Konrad Jamrozik <[email protected]>

* Sync eng/common directory with azure-sdk-tools for PR 5562 (Azure#4384)

* Add todos to update packages to pick up the newest CODEOWNERS interpreter

* update

---------

Co-authored-by: Konrad Jamrozik <[email protected]>

* Fix Share Client failure Azure#4377 (Azure#4381)

* Fix Share Client failure Azure#4377

* decrease request count to avoid throttling errors for storage tests (Azure#4385)

* Sync eng/common directory with azure-sdk-tools for PR 5568 (Azure#4387)

* we encourage folks to place their assets.jsons at the package level

* update generate-assets-json.ps1 to only include src/**/session-records so as to avoid picking up the duplicated 'target' sessionrecords

---------

Co-authored-by: scbedd <[email protected]>

* Add support to ignore invalid cert common name (Azure#4361)

* update to version 7.4 for admin. update tests (Azure#4388)

* update proxy version to include Info/Active against individual sessions + allow delayed response (Azure#4391)

Co-authored-by: scbedd <[email protected]>

* Sync eng/common directory with azure-sdk-tools for PR 5540 (Azure#4396)

* add parameter to set cadl emitter options

* remove emitter name in the additional parameter

---------

Co-authored-by: chunyu3 <[email protected]>

* Follow-up to update changelog to reflect community contribution (Azure#4393)

* Follow-up to update changelog to reflect community contribution

* Upgrade cspell version from 0.1 to 0.2

* logging api post request body (Azure#4404)

Co-authored-by: Albert Cheng <[email protected]>

* Sync eng/common directory with azure-sdk-tools for PR 5595 (Azure#4400)

* Use "npm ci" to install cspell and respect package-lock.json

* Review feedback

* Pipe npm ci output to Write-Host

---------

Co-authored-by: Daniel Jurek <[email protected]>

---------

Co-authored-by: George Arama <[email protected]>
Co-authored-by: Anton Kolesnyk <[email protected]>
Co-authored-by: Scott Beddall <[email protected]>
Co-authored-by: Praven Kuttappan <[email protected]>
Co-authored-by: Azure SDK Bot <[email protected]>
Co-authored-by: Mike Harder <[email protected]>
Co-authored-by: JinmingHu <[email protected]>
Co-authored-by: Konrad Jamrozik <[email protected]>
Co-authored-by: chunyu3 <[email protected]>
Co-authored-by: Ahson Khan <[email protected]>
Co-authored-by: Albert Cheng <[email protected]>
Co-authored-by: Daniel Jurek <[email protected]>
antkmsft pushed a commit that referenced this pull request Apr 5, 2023
# Conflicts:
#	sdk/core/azure-core/CHANGELOG.md
antkmsft pushed a commit that referenced this pull request Apr 5, 2023
# Conflicts:
#	sdk/core/azure-core/CHANGELOG.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Client This issue points to a problem in the data-plane of the library. Storage Storage Service (Queues, Blobs, Files) test-enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: Host header is not correctly set for non-default port LIVEONLY tests in Storage
6 participants