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

[Backport 2.x] Integration tests framework #3388

Merged
merged 20 commits into from
Sep 29, 2023

Conversation

peternied
Copy link
Member

@peternied peternied commented Sep 21, 2023

Description

Test framework that allows for declaring within the test class all
related users, roles, and other security primitives. Emphasis on
hamcrest Matchers for straight forward to read, author, and
troubleshoot test cases.

Special Note

This is a giant pull request - I would highly recommend using the github.dev [view] for reviewing and providing feedback. I'll also provide comments for the initial reviewers where I call out any deviations I've taken from what is in main

Issues Resolved

Check List

  • New functionality includes testing
  • 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.

Test framework that allows for declaring within the test class all
related users, roles, and other security primatives.  Empahsis on
hamcrest assertMatchers for straight forward to read, author, and
troubleshoot test cases.

Signed-off-by: Peter Nied <[email protected]>
@peternied
Copy link
Member Author

peternied commented Sep 21, 2023

Round 1, tests with failures:

  • org.opensearch.security.DefaultConfigurationTests.shouldLoadDefaultConfiguration
  • org.opensearch.security.PointInTimeOperationTest.listPitSegmentsCreatedWithIndexAlias_positive
  • org.opensearch.security.PointInTimeOperationTest.listPitSegments_negative
  • org.opensearch.security.PointInTimeOperationTest.listPitSegments_positive
  • org.opensearch.security.PointInTimeOperationTest.listPitSegmentsCreatedWithIndexAlias_negative
  • org.opensearch.security.api.DashboardsInfoTest.testDashboardsInfoValidationMessage
  • org.opensearch.security.api.DashboardsInfoWithSettingsTest.testDashboardsInfoValidationMessageWithCustomMessage
  • org.opensearch.security.rest.AuthZinRestLayerTests.testAccessDeniedForUserWithNoPermissions
  • org.opensearch.security.rest.AuthZinRestLayerTests.testShouldAllowAtRestAndTransport
  • org.opensearch.security.rest.AuthZinRestLayerTests.testBackwardsCompatibility
  • org.opensearch.security.rest.AuthZinRestLayerTests.testShouldAllowAtRestAndBlockAtTransport
  • org.opensearch.security.rest.AuthZinRestLayerTests.testShouldBlockAccessToEndpointForWhichUserHasNoPermission
  • org.opensearch.security.rest.WhoAmITests.testWhoAmIWithGetPermissionsLegacy
  • org.opensearch.security.rest.WhoAmITests.testWhoAmIWithoutGetPermissions
  • org.opensearch.security.rest.WhoAmITests.testWhoAmIWithGetPermissions
  • org.opensearch.security.rest.WhoAmITests.testAuditLogSimilarityWithTransportLayer
  • org.opensearch.security.PointInTimeOperationTest.listPitSegmentsCreatedWithIndexAlias_positive
  • org.opensearch.security.SearchOperationTest.shouldUpdateDocumentsInBulk_partiallyPositive
  • org.opensearch.security.SearchOperationTest.shouldRestoreSnapshot_failureOperationForbidden
  • org.opensearch.security.SearchOperationTest.shouldDeleteDocumentInBulk_partiallyPositive

Resolved, submitting another run

@codecov
Copy link

codecov bot commented Sep 21, 2023

Codecov Report

Merging #3388 (ba3a701) into 2.x (a2daf9f) will increase coverage by 0.16%.
Report is 7 commits behind head on 2.x.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##                2.x    #3388      +/-   ##
============================================
+ Coverage     64.11%   64.28%   +0.16%     
- Complexity     3367     3458      +91     
============================================
  Files           256      256              
  Lines         19515    19515              
  Branches       3297     3297              
============================================
+ Hits          12512    12545      +33     
+ Misses         5360     5355       -5     
+ Partials       1643     1615      -28     
Files Coverage Δ
...curity/dlic/rest/validation/EndpointValidator.java 94.20% <ø> (ø)

... and 15 files with indirect coverage changes

build.gradle Show resolved Hide resolved
@peternied peternied marked this pull request as draft September 22, 2023 01:54
@peternied
Copy link
Member Author

Note; Moving this to draft while I'm making sure these tests are stable, I've added a bunch of debug only changes that should be reverted before merging, I'll pull this out of draft when its mergeable - but feel free to review it as it stands

@peternied
Copy link
Member Author

peternied commented Sep 22, 2023

Ok here is the actual numbers that are telling:

$ ls | grep "integration.*txt" | grep ubuntu | xargs -I {} tail -n 300 {} | grep "\- org" | awk -F' - ' '{print $2}' | sort | uniq -c
      1 org.opensearch.security.PointInTimeOperationTest.listAllPits_positive
      1 org.opensearch.security.SearchOperationTest.shouldCreateIndexTemplate_positive
      2 org.opensearch.security.SearchOperationTest.shouldDeleteDocumentInBulk_partiallyPositive
      3 org.opensearch.security.SearchOperationTest.shouldIndexDocumentInBulkRequest_partiallyPositive
      2 org.opensearch.security.SearchOperationTest.shouldReindexDocuments_positive
      1 org.opensearch.security.SearchOperationTest.shouldRestoreSnapshot_failureForbiddenIndex
      1 org.opensearch.security.SearchOperationTest.shouldRestoreSnapshot_failureOperationForbidden
      4 org.opensearch.security.SearchOperationTest.shouldRestoreSnapshot_positive
      1 org.opensearch.security.SearchOperationTest.shouldUpdateDocumentsInBulk_partiallyPositive
      2 org.opensearch.security.SearchOperationTest.shouldUpdateDocumentsInBulk_positive
      3 org.opensearch.security.SearchOperationTest.shouldUpdateTemplate_positive

linux failures ^

$ ls | grep "integration.*txt" | grep win | xargs -I {} tail -n 300 {} | grep "\- org" | awk -F' - ' '{print $2}' | sort | uniq -c
     12 org.opensearch.security.DlsIntegrationTests.classMethod
      2 org.opensearch.security.DlsIntegrationTests.testSearchForAllDocumentsWithIndexPattern
      3 org.opensearch.security.DlsIntegrationTests.testShouldSearchAll
     12 org.opensearch.security.DoNotFailOnForbiddenTests.classMethod
     12 org.opensearch.security.FlsAndFieldMaskingTests.classMethod
     12 org.opensearch.security.PointInTimeOperationTest.classMethod
     12 org.opensearch.security.SearchOperationTest.classMethod
      1 org.opensearch.security.SearchOperationTest.shouldDeleteDocumentInBulk_positive
      1 org.opensearch.security.SearchOperationTest.shouldIndexDocumentInBulkRequest_partiallyPositive
     12 org.opensearch.security.SecurityConfigurationTests.classMethod
     12 org.opensearch.security.SecurityRolesTests.classMethod
     12 org.opensearch.security.SslOnlyTests.classMethod
     12 org.opensearch.security.TlsTests.classMethod
     12 org.opensearch.security.api.CreateResetPasswordTest.classMethod
     12 org.opensearch.security.api.DashboardsInfoTest.classMethod
     12 org.opensearch.security.api.DashboardsInfoWithSettingsTest.classMethod
     12 org.opensearch.security.http.CertificateAuthenticationTest.classMethod
     12 org.opensearch.security.privileges.PrivilegesEvaluatorTest.classMethod

windows failures ^

@peternied peternied marked this pull request as ready for review September 25, 2023 22:10
@peternied
Copy link
Member Author

OK - I'm pretty sure this is ready to merge, or near as ready as I'm going to try to push it, I've made a number of annotations to ignore flaky tests, we should revisit these and I'll add several tracking issues, along with disabling windows due to the network trouble.

Proof that this test collection is stable - 10x runs on JDK 11 / 17

Copy link
Collaborator

@willyborankin willyborankin left a comment

Choose a reason for hiding this comment

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

LGTM

@peternied
Copy link
Member Author

@RyanL1997 @DarshitChanpura could I get another review?

Copy link
Member

@DarshitChanpura DarshitChanpura left a comment

Choose a reason for hiding this comment

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

LGTM. I left one question out of curiosity.

Nit: There are some new files being introduced with floraGunn license headers (there is one more license header as well in a couple of files). We should clean this up at some point. Not necessarily in this PR, as these need to be fixed in main as well.

build.gradle Show resolved Hide resolved
Copy link
Collaborator

@RyanL1997 RyanL1997 left a comment

Choose a reason for hiding this comment

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

Hi @peternied, thanks for taking this on and in generally this is looking good to me. I just left a minor comment for the httpcomponents dependencies.

build.gradle Show resolved Hide resolved
@peternied peternied merged commit 19c6a9d into opensearch-project:2.x Sep 29, 2023
56 checks passed
@peternied peternied deleted the 2.x-intergration-tests branch September 29, 2023 19:00
@peternied
Copy link
Member Author

@DarshitChanpura & @RyanL1997 see [1] for the fast follow up items.

peternied added a commit to peternied/security that referenced this pull request Oct 7, 2023
Test framework that allows for declaring within the test class all
related users, roles, and other security primitives.  Emphasis on
hamcrest Matchers for straight forward to read, author, and
troubleshoot test cases.

Signed-off-by: Peter Nied <[email protected]>
Signed-off-by: Peter Nied <[email protected]>
Signed-off-by: Peter Nied <[email protected]>
@peternied peternied mentioned this pull request Oct 10, 2023
3 tasks
DarshitChanpura pushed a commit that referenced this pull request Oct 16, 2023
- Remove an unused JVM setting for integration tests
- Fixed an issue where you couldn't run non-resource tests unless you
had CI_ENVIRONMENT set to normal on your machine (Already fixed in main)
- Fixed an issue where code coverage was always run after tests
completed (Already fixed in main)

- Related #3388

Signed-off-by: Peter Nied <[email protected]>
cwperks pushed a commit to cwperks/security that referenced this pull request Nov 20, 2023
- Remove an unused JVM setting for integration tests
- Fixed an issue where you couldn't run non-resource tests unless you
had CI_ENVIRONMENT set to normal on your machine (Already fixed in main)
- Fixed an issue where code coverage was always run after tests
completed (Already fixed in main)

- Related opensearch-project#3388

Signed-off-by: Peter Nied <[email protected]>
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.

5 participants