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

Sig4v release 0.1 #273

Conversation

gregschohn
Copy link
Collaborator

Description

Release 0.1 merge of #267

Issues Resolved

https://opensearch.atlassian.net/browse/MIGRATIONS-1287

Is this a backport? Yes - see
#267
#272

Testing

See #267

Check List

  • New functionality includes testing
    • All tests pass, including unit test, integration test and doctest
  • New functionality has been documented
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

okhasawn and others added 14 commits August 11, 2023 16:58
Signed-off-by: Omar Khasawneh <[email protected]>
Signed-off-by: Omar Khasawneh <[email protected]>
Signed-off-by: Omar Khasawneh <[email protected]>
Signed-off-by: Omar Khasawneh <[email protected]>
These patches introduce some new interfaces and rename some existing JSON transformers to make new transformation types more distinguishable.
I've also moved around some fields between classes as thinking about how to handle auth headers has started to create some new types of patterns.

Signed-off-by: Greg Schohn <[email protected]>
…igning all the way back to the command line.

The command line now takes a service and region as opposed to just a zero arity boolean flag as both are required for SigV4 calculations.  These values should refer to the region and service that the replayer is making calls to (e.g., the 'target' server).
Notice that an AuthTransformerFactory is passed around that returns an AuthTransformer for a specific http message.
There could be different strategies dependent upon the original message.  For example, unauthorized users might simply be mapped to no transformation, meaning that we don't need to go through the added effort of buffering and signing the messages.
The old style http header transformation via jolt is also gone and has been replaced with an (HeaderOnly)AuthTransformer class that manipulates headers.  I'm not happy with the care that I had to put into the  NettyDecodedHttpRequestHandler class (now NettyDecodedHttpRequestPreliminaryConvertHandler) to do the auth transformation in two spots while also passing more complicated auth transformations into the pipeline construction helper methods.
I've also converted the SigV4Signer to be an AuthTransformer and have tied it into the netty handler to host it, though none of that has been tested in this commit.

Unrelated changes include converting the packets of an HttpMessage(AndTimestamp) from an ArrayList to a named extension (ie. typedef).  I've never been comfortable with the vagueness of the ArrayList.  I had also been working on edits to take the response into the AuthTransformer so that 403s could be ignored - but decided that would be a bad idea.  1) Not every request will have a response and 2) it will allow future requests that should have been blocked to never be tested and the time to test them will be within the target server.

Signed-off-by: Greg Schohn <[email protected]>
…of the replayer.

I've got a unit test to verify that the contents of the signature match a mock key and mock date.  To put that together, I've extracted another helper function from a similar test and placed it into TestUtils.
There was one other bugfix to remove a spurious "toString()" that followed a headers.put call for the content-length.  The string wasn't used and put() will return null when the header wasn't previously present, creating an NPE situation.

Signed-off-by: Greg Schohn <[email protected]>
…ayload streams.

A couple other minor improvements and cleanups were thrown in here too, most notably, tracking the shadow request packets that are sent, which is very helpful to debug signing problems.

Signed-off-by: Greg Schohn <[email protected]>
* main: (27 commits)
  Switch to size-based rollover policy
  MIGRATIONS-1200 - Update python test cluster_migration_core path
  MIGRATIONS-1200 - Fix github workflow actions
  MIGRATIONS-1200 - Fix python requirement paths
  MIGRATIONS-1200 - Fix flake8 line too long error
  MIGRATIONS-1200 update python requirements files with new path
  MIGRATIONS-1200 - Move cluster_traffic_capture to experimental
  MIGRATIONS-1200 - Move cluster_migration_core to experimental
  MIGRATIONS-1200 - Move knowledge_base to experimental
  MIGRATIONS-1200 - Create experimental subdirectory and place upgrades within experimental
  Revert "MIGRATIONS-1200 - Remove historical capture CDK stack"
  Update to hourly, fix filename
  Add logging config so output tuples go to shared filesystem
  Add option to use AWS secret in auth header for Replayer requests (opensearch-project#265)
  [Fetch Migration] Added total document count to report (opensearch-project#261)
  MIGRATIONS-1200 - Rename 'upgrades' directory to 'experimental' and replace references
  MIGRATIONS-1200 - Remove deprecated docker files
  MIGRATIONS-1200 - Remove historical capture CDK stack
  MIGRATIONS-1200 - Remove unused TrafficReplayer directory
  Soften assertions for aspects of the test that are independent of the strict contract that the ExpiringSubstitutableItemPool defines.
  ...

Signed-off-by: Greg Schohn <[email protected]>
…e isn't already one present.

This pushes the success rate for signatures to 100% in the first 120 messages from the opensearch benchmark workload when rerun against a cluster that uses sigv4.

Signed-off-by: Greg Schohn <[email protected]>
* capture-and-replay-v0.1.0:
  Update runTestBenchmarks with params and default values
  Update README.md
  MIGRATIONS-1200 - Remove plugins
  MIGRATIONS-1200 - Remove FetchMigration
  MIGRATIONS-1200 - Remove datastash
  MIGRATIONS-1200 - Capture and Replay v.0.1.0 branch

Signed-off-by: Greg Schohn <[email protected]>
@codecov
Copy link

codecov bot commented Aug 19, 2023

Codecov Report

Merging #273 (1be6a9d) into capture-and-replay-v0.1.0 (3d2e3bd) will increase coverage by 0.62%.
Report is 1 commits behind head on capture-and-replay-v0.1.0.
The diff coverage is 71.28%.

❗ Current head 1be6a9d differs from pull request most recent head 2b6ba46. Consider uploading reports for the commit 2b6ba46 to get more accurate results

@@                       Coverage Diff                       @@
##             capture-and-replay-v0.1.0     #273      +/-   ##
===============================================================
+ Coverage                        59.04%   59.66%   +0.62%     
- Complexity                         584      608      +24     
===============================================================
  Files                               74       82       +8     
  Lines                             2991     3129     +138     
  Branches                           279      292      +13     
===============================================================
+ Hits                              1766     1867     +101     
- Misses                            1057     1085      +28     
- Partials                           168      177       +9     
Flag Coverage Δ
unittests 59.66% <71.28%> (+0.62%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
...n/java/org/opensearch/migrations/replay/Utils.java 40.00% <ø> (ø)
.../datahandlers/http/NettyJsonContentCompressor.java 88.37% <ø> (ø)
...ions/transform/RemovingAuthTransformerFactory.java 0.00% <0.00%> (ø)
.../opensearch/migrations/replay/TrafficReplayer.java 7.17% <12.90%> (-1.40%) ⬇️
...nsearch/migrations/transform/IAuthTransformer.java 28.57% <28.57%> (ø)
...replay/PacketToTransformingHttpHandlerFactory.java 63.63% <75.00%> (+3.63%) ⬆️
.../org/opensearch/migrations/replay/SigV4Signer.java 77.58% <77.58%> (ø)
.../datahandlers/http/NettyJsonContentAuthSigner.java 86.36% <86.36%> (ø)
...datahandlers/http/RequestPipelineOrchestrator.java 96.36% <92.30%> (-1.60%) ⬇️
...tyDecodedHttpRequestPreliminaryConvertHandler.java 94.50% <96.00%> (ø)
... and 16 more

... and 1 file with indirect coverage changes

@mikaylathompson
Copy link
Collaborator

This history looks weird -- there are unrelated commits in there, some of which are already merged to the release branch. What's going on there?

@sumobrian sumobrian changed the base branch from main to capture-and-replay-v0.1.0 August 21, 2023 03:01
gregschohn and others added 4 commits August 20, 2023 23:03
* sigv4excision_andsigner:
  PR feedback around command line arguments

Signed-off-by: Greg Schohn <[email protected]>
* capture-and-replay-v0.1.0:
  Add script to transform tuples to human readable format

Signed-off-by: Greg Schohn <[email protected]>
@sumobrian sumobrian merged commit 0bfd95f into opensearch-project:capture-and-replay-v0.1.0 Aug 21, 2023
3 checks passed
@gregschohn gregschohn deleted the sig4v_release_0.1 branch August 22, 2023 03:44
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.

4 participants