-
Notifications
You must be signed in to change notification settings - Fork 27
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
Sync devDeploy with recent Replayer auth changes #279
Sync devDeploy with recent Replayer auth changes #279
Conversation
Signed-off-by: Tanner Lewis <[email protected]>
Codecov Report
@@ Coverage Diff @@
## main #279 +/- ##
=========================================
Coverage 61.93% 61.93%
Complexity 617 617
=========================================
Files 82 82
Lines 3129 3129
Branches 292 292
=========================================
Hits 1938 1938
Misses 1014 1014
Partials 177 177
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please look at the comment on 'extra args'. Approving this since it should unbreak my previous changes.
deployment/copilot/README.md
Outdated
There is a level of precedence that will determine which or if any Auth header should be added to outgoing Replayer requests, which is listed below. | ||
1. If the user provides an explicit auth header option to the Replayer, such as providing a static value auth header or using a user and secret arn pair, this mechanism will be used for the auth header of outgoing requests. The options can be found as Parameters [here](../../TrafficCapture/trafficReplayer/src/main/java/org/opensearch/migrations/replay/TrafficReplayer.java) | ||
2. If the devDeploy script is used and its Replayer command has not been altered (as in the case of 1.) and CDK deploys a target cluster with a configured FGAC user (see `fineGrainedManagerUserName` and `fineGrainedManagerUserSecretManagerKeyARN` CDK context options [here](../cdk/opensearch-service-migration/README.md)) or is running in demo mode (see `enableDemoAdmin` CDK context option), this user and secret arn pair will be provided in the Replay command for the Auth header | ||
3. If the user provides no auth header option and incoming captured requests have an auth header, this auth header will be reused for outgoing requests | ||
4. If the user provides no auth header option and incoming captured requests have no auth header, then no auth header will be used for outgoing requests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably moved to the Replayer's README and linked from here. That doesn't need to be done now though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have went ahead and moved and referenced here
--auth-header-value) | ||
REPLAY_AUTH_HEADER="$2" | ||
shift # past argument | ||
shift # past value | ||
;; | ||
--aws-auth-header-user) | ||
REPLAY_AWS_AUTH_HEADER_USER="$2" | ||
shift # past argument | ||
shift # past value | ||
;; | ||
--aws-auth-header-secret) | ||
REPLAY_AWS_AUTH_HEADER_SECRET="$2" | ||
shift # past argument | ||
shift # past value | ||
;; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you have the dev-deploy script take in the arguments to use for the replayer? Something like "--extraFlags" and then let the developer tack on any auth flags or anything else that might be necessary. As it is, it will be painful for a dev to play around w/ different options.
And I would say that none of the arguments are checked - which should be fine if this is a "developer" script
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am a little conflicted with what we should do here. I don't want the additional tax of actually knowing what arguments the Replayer takes which makes something like "--extraFlags" seem preferrable, however this also becomes a balancing act of does my default command already have an argument that is being passed in with "--extraFlags", it seems it would be easier for the user to just update the command in the script to what they need.
What might be slightly convenient is to allow a user to specify all the arguments for the Replayer (not sure how useful this is) in the devDeploy script, or do something like "--replayerAuthFlags" which allows specifying auth flags and defaults to our normal setting but can be overriden with whatever other auth flags
Lmk your thoughts here
deployment/copilot/devDeploy.sh
Outdated
# Gather CDK output which includes export commands needed by Copilot, and make them available to the environment | ||
found_exports=$(grep -o "export [a-zA-Z0-9_]*=[^\\;\"]*" cdkOutput.json) | ||
eval "$(grep -o "export [a-zA-Z0-9_]*=[^\\;\"]*" cdkOutput.json)" | ||
# Collect export commands from CDK output, which are needed by Copilot, wrap the values in quotes and make them available to the environment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit - are you finding/gathering commands or values?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adjusted
Signed-off-by: Tanner Lewis <[email protected]>
The backport to
To backport manually, run these commands in your terminal: # Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/backport-capture-and-replay-v0.1.0 capture-and-replay-v0.1.0
# Navigate to the new working tree
pushd ../.worktrees/backport-capture-and-replay-v0.1.0
# Create a new branch
git switch --create backport/backport-279-to-capture-and-replay-v0.1.0
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 4165dc48496deb0670d370d7c478fabc69b5c702
# Push it to GitHub
git push --set-upstream origin backport/backport-279-to-capture-and-replay-v0.1.0
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/backport-capture-and-replay-v0.1.0 Then, create a pull request where the |
) * Sync devDeploy with recent Replayer auth changes Signed-off-by: Tanner Lewis <[email protected]> (cherry picked from commit 4165dc4)
* Sync devDeploy with recent Replayer auth changes Signed-off-by: Tanner Lewis <[email protected]> (cherry picked from commit 4165dc4)
* main: (27 commits) Align capture proxy for docker and Copilot and add cat indices convenience script MIGRATIONS-1289: Add end of segment message to capture serializer (opensearch-project#276) Sync devDeploy with recent Replayer auth changes (opensearch-project#279) Update documentation for auth header Sync devDeploy with recent Replayer auth changes Fix linter error Handle chunked transfer encoding Make path search case-insensitive-ish and don't output bytes to a json PR feedback around command line arguments Address review comments Bugfix to apply the default signer class's content checksum when there 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. Add error handling, progress bar, cleanup Bugfixes to get SigV4 working in at least simple cases w/ and w/out payload 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. Add documentation and some cleanup/todos Add script to transform tuples to human readable format Review comment and basic documentation Update script with params and defaults for copilot vs docker Cleanup after reviewing the changes on the branch in aggregate. Cleanup and bugfixes to get SigV4 more correctly wired with the rest of the replayer. Further changes to unify the replayer's auth story and to tie sigv4 signing all the way back to the command line. ... Signed-off-by: Greg Schohn <[email protected]> # Conflicts: # TrafficCapture/trafficReplayer/src/main/java/org/opensearch/migrations/replay/PacketToTransformingHttpHandlerFactory.java # TrafficCapture/trafficReplayer/src/main/java/org/opensearch/migrations/replay/TrafficReplayer.java # TrafficCapture/trafficReplayer/src/main/java/org/opensearch/migrations/replay/Utils.java # TrafficCapture/trafficReplayer/src/main/java/org/opensearch/migrations/replay/datahandlers/NettyPacketToHttpConsumer.java # TrafficCapture/trafficReplayer/src/test/java/org/opensearch/migrations/replay/PayloadRepackingTest.java
Description
Adjust devDeploy.sh with recent Replayer auth changes. Also remove additional "orchestration" logic from devDeploy surrounding setting the auth header, which will be taxing to keep up to date and probably doesn't belong there.
Issues Resolved
None
Testing
Manual deployment testing
Check List
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.