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

[TestProxy] Common Sanitizers #8038

Merged
merged 65 commits into from
May 9, 2024
Merged

[TestProxy] Common Sanitizers #8038

merged 65 commits into from
May 9, 2024

Conversation

scbedd
Copy link
Member

@scbedd scbedd commented Apr 9, 2024

This PR moves the test-proxy from a static append-only set of sanitizers, to an actual data structure with proper addition/removal of applied sanitizers.

  • Each recording or playback session now gets a modifiable list of sanitizers that are applied.
    • Session sanitizers are now cut at playback or record start. Any further session sanitizer addition will not directly affect the recording that has been started.
  • As a consequence of all these additional sanitizers, stored recordings will now be sanitized. The way this will work is
    • Client starts playback session, gets a recordingId back
    • Client uses recordingId to add a couple additional sanitizers
    • Client starts firing actual requests from the test
    • The proxy will sanitize the loaded recording according to which sanitizers are active when the first real request for the playback session occurs.
  • Upon calling /Admin/AddSanitizers or Admin/AddSanitizer, an object or list of objects representing the registered sanitizers is now returned to the calling client. These objects contain the base sanitizer properties + the id of the registered sanitizer.
  • Added route /Admin/RemoveSanitizers which can apply to either the session-level or an individual playback/record session.

Observers please add suggestions either in discussion or directly onto the branch.

@scbedd scbedd self-assigned this Apr 9, 2024
@scbedd scbedd changed the title Common Sanitizers PR [TestProxy] Common Sanitizers Apr 9, 2024
@scbedd scbedd mentioned this pull request Apr 10, 2024
2 tasks
@scbedd scbedd mentioned this pull request Apr 17, 2024
@chlowell
Copy link
Member

Does the proxy already have a way to deduplicate sanitizers so we don't run all of these twice? If not, I imagine it could compare two sanitizers with hashes of their string parameters

@scbedd
Copy link
Member Author

scbedd commented Apr 18, 2024

Does the proxy already have a way to deduplicate sanitizers so we don't run all of these twice? If not, I imagine it could compare two sanitizers with hashes of their string parameters

It does not at this time. We will pay the cost for now. I have another PR in the works that makes sanitizers set/get/remove from a single abstraction class. Doing this hash check there would make the most sense if we do it 👍

scbedd and others added 3 commits May 6, 2024 10:26
@scbedd
Copy link
Member Author

scbedd commented May 9, 2024

Thank you to all the reviewers on this PR: @pvaneck , @JoshLove-msft , @chlowell, @samvaity, and @benbp . Hugely appreciate all your feedback.

go, python, .net, and cpp have shifted onto a proxy version produced by this PR. Only java and js remain to migrate, and they are almost there. Given all that. I'm going to merge this PR, as I have further PRs to assist debugging that should include these changes.

@scbedd
Copy link
Member Author

scbedd commented May 9, 2024

/check-enforcer evaluate

@benbp benbp self-requested a review May 9, 2024 19:44
@scbedd scbedd merged commit 57382d5 into main May 9, 2024
14 checks passed
@scbedd scbedd deleted the add-common-sanitizers branch May 9, 2024 19:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

7 participants