Add endpoints and authentication details (including SigV4) as parameters to E2E test script #461

okhasawn · 2023-11-30T23:21:10Z

Description

This PR adds the ability to provide the endpoints and authentication details as parameters to the test script.
The end goal is to be able to provide either local or cloud based endpoints as parameters and the authentication details as parameters and have the E2E test script run successfully.
This PR also enables to the E2E test script to run on endpoints that use SigV4.

Issues Resolved

Migrations-1442

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.

Signed-off-by: Omar Khasawneh <[email protected]>

codecov · 2023-11-30T23:29:19Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c14529b) 77.08% compared to head (c7c1da3) 73.51%.

Additional details and impacted files

@@             Coverage Diff              @@
##               main     #461      +/-   ##
============================================
- Coverage     77.08%   73.51%   -3.58%     
+ Complexity     1183     1180       -3     
============================================
  Files           149      124      -25     
  Lines          6041     4886    -1155     
  Branches        561      439     -122     
============================================
- Hits           4657     3592    -1065     
+ Misses         1076      999      -77     
+ Partials        308      295      -13

Flag	Coverage Δ
unittests	`73.51% <ø> (-0.17%)`	⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…ing workflow Signed-off-by: Omar Khasawneh <[email protected]>

Signed-off-by: Omar Khasawneh <[email protected]>

lewijacn

As far as this structure for local runs I only have one comment that I added. If we want this PR to be just for local I can go ahead and approve.

As far as running this on AWS it seems we still need a couple things, but correct me if I'm wrong

Support for AWS SigV4 to verify clusters which have SigV4 enabled
A mechanism to only run certains tests depending on some input parameter e.g. I only want to run sigv4 tests on AWS.
Make sure existing test cases work for both local and AWS, I believe only test_0006_OSB needs to be tweaked but didn't check closely

lewijacn · 2023-12-14T18:55:54Z

.github/workflows/e2eTest.yml

@@ -44,4 +44,4 @@ jobs:
        run: |
          cd test
          chmod +x ./tests.py
-          pytest -n 3 tests.py
+          pytest tests.py --proxy_endpoint="https://localhost:9200" --source_endpoint="https://localhost:19200" --target_endpoint="https://localhost:29200" --source_auth_type="basic" --source_username="admin" --source_password="admin" --target_auth_type="basic" --target_username="admin" --target_password="admin" --source_verify_ssl=False --target_verify_ssl=False


Aren't these all defaults, do we need to specify?

Could you also add the --verbose flag to this command. Some of these tests take a while to run so you start to wonder if things are working with no output. This flag will show each test as it gets started running

You're right, I don't need to specify those in the workflow anymore. I had updated the defaults value after.
I'll update this to remove the values and add the verbose flag. Thank you!

lewijacn · 2023-12-14T22:19:07Z

test/tests.py

        self.assertEqual(response.status_code, HTTPStatus.METHOD_NOT_ALLOWED)

-    def test_0007_OSB(self):
+    def test_0006_OSB(self):


This seems like it will need to be tweaked to work for AWS as well. For AWS we will already be on the migration console box so we would just need to make the runTestBenchmarks.sh call from the right directory

You're right, this specific test won't work on AWS as of this moment. I imagined that this script will end up existing on the migration console and running from there. I can update this test in a future PR to support that.
As for the other tests; per my testing on our deployed AWS solution (w/o SigV4 whatsoever), they will pass.

okhasawn · 2023-12-14T22:48:54Z

Thank you for reviewing!
As for your comments:

SigV4 support will be added in a future PR.
Certain tests can be ran using pytest's method of running tests which is for example: pytest tests.py::E2ETests::test_0001_index tests.py::E2ETests::test_0002_document will get this to only run those two tests specified.
You're right, existing tests will work except for the one you mentioned. I'll add a flag to check if it's running on AWS in this PR to exclude this test for now, and update this test to work on AWS in a future PR as well.

Signed-off-by: Omar Khasawneh <[email protected]>

…tions into parametersTest

Signed-off-by: Omar Khasawneh <[email protected]>

okhasawn · 2024-01-03T15:11:37Z

Added option to run on SigV4 enabled endpoints.

lewijacn

Just to confirm we still have the one test that won't work for both AWS and local, seems like we need some pattern in place so that only proper tests will run when this test script is executed

lewijacn · 2024-01-05T16:19:33Z

test/operations.py



-def create_index(endpoint: str, index_name: str, auth: Optional[Tuple[str, str]] = None, verify_ssl: bool = False):
+def create_index(endpoint: str, index_name: str, auth, verify_ssl: bool = False, protocol: str = 'https'):


Where is the protocol argument being used in these methods? And why do only some methods have?

I'll remove this. Mistakenly added from something else I was testing.

lewijacn · 2024-01-05T16:21:45Z

test/requirements.txt

@@ -8,3 +8,5 @@ pytest==7.3.1
 pytest-xdist==3.3.1
 requests==2.31.0
 urllib3==2.0.7
+requests_aws4auth


Is this what FetchMigration uses as well?

Yup, although one has a "-" and the other has a "_", but they're the exact same.

lewijacn · 2024-01-05T16:25:52Z

test/operations.py



-def create_index(endpoint: str, index_name: str, auth: Optional[Tuple[str, str]] = None, verify_ssl: bool = False):
+def create_index(endpoint: str, index_name: str, auth, verify_ssl: bool = False, protocol: str = 'https'):
    response = requests.put(f'{endpoint}/{index_name}', auth=auth, verify=verify_ssl)


I assume the requests library is smart enough to see if it has basic or sigv4 auth passed to it and parse accordingly

That's correct!

Signed-off-by: Omar Khasawneh <[email protected]>

lewijacn · 2024-01-09T15:10:54Z

test/tests.py

-            cmd_exec = f"docker exec {container_id} ./runTestBenchmarks.sh"
-            logger.warning(f"Running command: {cmd_exec}")
+        if self.deployment_type == "cloud":
+            cmd_exec = f"./runTestBenchmarks --unique-id {self.index}"


The path here for runTestBenchmarks will be in a different directory. It's probably easiest to use /root/runTestBenchmarks.sh

Could we also use the self.unique_id instead of self.index here? It seems like it may be clearer

Good idea, I'll update in the next commit.

lewijacn · 2024-01-09T15:15:33Z

test/tests.py

@@ -103,7 +104,7 @@ def setup_authentication(self, auth_type, username, password):
        return None

    def set_common_values(self):
-        self.index = f"my_index_{uuid.uuid4()}"
+        self.index = self.unique_id


It seems like it would be nice to keep the previous format like: f"test_index_{self.unique_id}" to be able to clearly tell what our test index is

I imagine our unique id for the integ tests may just be the name of the test and the pipeline run number like full_run_21

Signed-off-by: Omar Khasawneh <[email protected]>

Add endpoints and authentication details as parameters

edcda15

Signed-off-by: Omar Khasawneh <[email protected]>

okhasawn added 3 commits December 1, 2023 11:41

Add separate authentication details for both endpoints + update exist…

79b0b84

…ing workflow Signed-off-by: Omar Khasawneh <[email protected]>

Add more authentication parameters to support more scenarios

bd3ab8e

Signed-off-by: Omar Khasawneh <[email protected]>

Fix failing workflow

e16999a

Signed-off-by: Omar Khasawneh <[email protected]>

okhasawn marked this pull request as ready for review December 4, 2023 18:34

okhasawn requested review from chelma, gregschohn, kartg, lewijacn, mikaylathompson and sumobrian as code owners December 4, 2023 18:34

okhasawn changed the title ~~[WIP - Do Not Merge] Add endpoints and authentication details as parameters to E2E test script~~ Add endpoints and authentication details as parameters to E2E test script Dec 4, 2023

okhasawn added 2 commits December 4, 2023 12:35

Merge branch 'main' into parametersTest

6d59979

Add default values for parameters + Update documentation

c009947

Signed-off-by: Omar Khasawneh <[email protected]>

lewijacn reviewed Dec 14, 2023

View reviewed changes

okhasawn added 4 commits January 2, 2024 17:23

Merge branch 'main' into parametersTest

2930a32

No longer need to specify parameters as there are default values now

f721399

Signed-off-by: Omar Khasawneh <[email protected]>

Merge branch 'parametersTest' of github.com:okhasawn/opensearch-migra…

ae0e482

…tions into parametersTest

Add SigV4 Support

8e2a3c6

Signed-off-by: Omar Khasawneh <[email protected]>

okhasawn changed the title ~~Add endpoints and authentication details as parameters to E2E test script~~ Add endpoints and authentication details (including SigV4) as parameters to E2E test script Jan 3, 2024

lewijacn reviewed Jan 5, 2024

View reviewed changes

Add unique index parameter + accomodate cloud deployment for OSB test

cb13a85

Signed-off-by: Omar Khasawneh <[email protected]>

lewijacn reviewed Jan 9, 2024

View reviewed changes

fix path + update index name

c7c1da3

Signed-off-by: Omar Khasawneh <[email protected]>

lewijacn approved these changes Jan 11, 2024

View reviewed changes

okhasawn merged commit 75ff259 into opensearch-project:main Jan 11, 2024
7 of 8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add endpoints and authentication details (including SigV4) as parameters to E2E test script #461

Add endpoints and authentication details (including SigV4) as parameters to E2E test script #461

okhasawn commented Nov 30, 2023 •

edited

Loading

codecov bot commented Nov 30, 2023 •

edited

Loading

lewijacn left a comment

lewijacn Dec 14, 2023

lewijacn Dec 14, 2023

okhasawn Dec 14, 2023

lewijacn Dec 14, 2023

okhasawn Dec 14, 2023

okhasawn commented Dec 14, 2023

okhasawn commented Jan 3, 2024

lewijacn left a comment

lewijacn Jan 5, 2024

okhasawn Jan 9, 2024

lewijacn Jan 5, 2024

okhasawn Jan 9, 2024

lewijacn Jan 5, 2024

okhasawn Jan 9, 2024

lewijacn Jan 9, 2024

lewijacn Jan 9, 2024

okhasawn Jan 9, 2024

lewijacn Jan 9, 2024

lewijacn Jan 9, 2024



		def create_index(endpoint: str, index_name: str, auth: Optional[Tuple[str, str]] = None, verify_ssl: bool = False):
		def create_index(endpoint: str, index_name: str, auth, verify_ssl: bool = False, protocol: str = 'https'):

Add endpoints and authentication details (including SigV4) as parameters to E2E test script #461

Add endpoints and authentication details (including SigV4) as parameters to E2E test script #461

Conversation

okhasawn commented Nov 30, 2023 • edited Loading

Description

Issues Resolved

Check List

codecov bot commented Nov 30, 2023 • edited Loading

Codecov Report

lewijacn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

okhasawn commented Dec 14, 2023

okhasawn commented Jan 3, 2024

lewijacn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

okhasawn commented Nov 30, 2023 •

edited

Loading

codecov bot commented Nov 30, 2023 •

edited

Loading