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.

Sign up for GitHub

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

adding a proxy setup to base conftest. just need to disable it when p… #25532

Closed
wants to merge 4 commits into from
Closed

adding a proxy setup to base conftest. just need to disable it when p… #25532

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8b83dbd
adding a proxy setup to base conftest. just need to disable it when p…
scbedd Aug 3, 2022
95c5b9e
removing default settings from conftest. adding to reset. sending res…
scbedd Aug 3, 2022
0daf913
Merge branch 'main' into hotfix/additional-sanitization
mccoyp Dec 30, 2022
f4e57aa
Add other defaults, use string sanitizer
mccoyp Dec 30, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion sdk/conftest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,13 @@
import os
import pytest

from devtools_testutils import environment_variables, recorded_test, test_proxy, variable_recorder
from devtools_testutils import (
add_general_string_sanitizer,
environment_variables,
recorded_test,
test_proxy,
variable_recorder,
)


def pytest_configure(config):
Expand Down
9 changes: 3 additions & 6 deletions tools/azure-sdk-tools/devtools_testutils/proxy_startup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@

from .config import PROXY_URL
from .helpers import is_live_and_not_recording
from .sanitizers import add_remove_header_sanitizer, set_custom_default_matcher
from .sanitizers import add_remove_header_sanitizer, set_custom_default_matcher, add_general_regex_sanitizer, _send_reset_request


_LOGGER = logging.getLogger()
Expand Down Expand Up @@ -180,11 +180,8 @@ def start_test_proxy(request) -> None:

# Wait for the proxy server to become available
check_proxy_availability()
# remove headers from recordings if we don't need them, and ignore them if present
# Authorization, for example, can contain sensitive info and can cause matching failures during challenge auth
headers_to_ignore = "Authorization, x-ms-client-request-id, x-ms-request-id"
add_remove_header_sanitizer(headers=headers_to_ignore)
set_custom_default_matcher(excluded_headers=headers_to_ignore)
# Call reset to ensure default matcher and sanitizers are set
_send_reset_request({})


def stop_test_proxy() -> None:
Expand Down
14 changes: 14 additions & 0 deletions tools/azure-sdk-tools/devtools_testutils/sanitizers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
from typing import TYPE_CHECKING
from urllib.error import HTTPError
import requests
import os

from .config import PROXY_URL
from .helpers import get_recording_id, is_live, is_live_and_not_recording
Expand Down Expand Up @@ -590,6 +591,19 @@ def _send_reset_request(headers: dict) -> None:
response = requests.post(f"{PROXY_URL}/Admin/Reset", headers=headers)
response.raise_for_status()

headers_to_ignore = "Authorization, x-ms-client-request-id, x-ms-request-id"
Copy link
Member Author

Choose a reason for hiding this comment

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

Not certain it's intended. But when we start up the test-proxy, we set a couple default custom items.

Then after reset, we don't actually resend those. I've moved all of the common default to _send_reset and then just called that during proxy startup.

One place, all the defaults. I could be missing reasoning here though 👍

Copy link
Member

Choose a reason for hiding this comment

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

Only setting the header rules once was actually intentional, albeit for rare edge cases. There are some teams that need request ID headers for tests, so being able to reset everything to a clean slate seemed like a good solution at the time.

I'd like to do something similar to your suggestion, so that we have a persistent default baseline of things to ignore -- otherwise, "resetting to default" isn't really giving people the default they're expecting.

To solve both those problems, it seems like the solution might be to provide methods for removing headers from this exclusion list 🤔

add_remove_header_sanitizer(headers=headers_to_ignore)
set_custom_default_matcher(excluded_headers=headers_to_ignore)

client_id = os.environ.get("AZURE_CLIENT_ID", "00000000-0000-0000-0000-000000000000")
client_secret = os.environ.get("AZURE_CLIENT_SECRET", "00000000-0000-0000-0000-000000000000")
tenant_id = os.environ.get("AZURE_TENANT_ID", "00000000-0000-0000-0000-000000000000")
sub_id = os.environ.get("AZURE_SUBSCRIPTION_ID", "00000000-0000-0000-0000-000000000000")
add_general_string_sanitizer(target=client_id, value="00000000-0000-0000-0000-000000000000")
add_general_string_sanitizer(target=client_secret, value="00000000-0000-0000-0000-000000000000")
add_general_string_sanitizer(target=tenant_id, value="00000000-0000-0000-0000-000000000000")
add_general_string_sanitizer(target=sub_id, value="00000000-0000-0000-0000-000000000000")


def _send_sanitizer_request(sanitizer: str, parameters: dict) -> None:
"""Sends a POST request to the test proxy endpoint to register the specified sanitizer.
Expand Down