New test framework by nibix · Pull Request #1967 · opensearch-project/security

nibix · 2022-07-26T15:29:05Z

Signed-Off-By: Nils Bandener nils.bandener@eliatra.com

Description

This is the first iteration of a new framework for integration tests. The main focus is on conciseness and self-contained test. The framework offers:

A programmatic way of defining the cluster setup
A programmatic way of defining users and roles
A programmatic way of defining indices
A REST client with methods tailored for testing

This is now all part of the test class, so you do not need to jump around in various configuration files to get an overview on the test setup. There are of course still a lot of features missing, like setting up test data. I propose to add these features in increments.

The new framework and tests live in a new source folder called "newTest" so they do not interfere with the already existing tests

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.

peternied

Some comments after an initial pass, I'll circle back with a complete review

build.gradle

src/newTest/java/org/opensearch/node/PluginAwareNode.java

build.gradle

src/newTest/java/org/opensearch/test/AbstractIntegrationTest.java

src/newTest/java/org/opensearch/test/GenericIntegrationTest.java

src/newTest/java/org/opensearch/test/SecurityRolesTests.java

src/newTest/java/org/opensearch/test/framework/certificate/Certificates.java

src/newTest/java/org/opensearch/test/framework/cluster/ClusterConfiguration.java

src/newTest/java/org/opensearch/test/framework/cluster/ContextHeaderDecoratorClient.java

src/newTest/java/org/opensearch/test/framework/cluster/PortAllocator.java

src/newTest/java/org/opensearch/test/framework/cluster/SocketUtils.java

src/newTest/java/org/opensearch/test/framework/cluster/TestRestClient.java

peternied · 2022-07-28T19:42:54Z

Thanks for the contribution, this is a great step forward in how tests are authored in this codebase. I left a lot of feedback, lets work down that list and as we resolve some of those conversations we can checkpoint where this change is at.

davidlago · 2022-08-09T14:27:40Z

@jochenkressin @lukasz-soszynski-eliatra please amend your commits adding DCO to them, the check is failing. Thanks!

peternied · 2022-08-12T18:59:21Z

I've reviewed the outstanding comments and resolved a bunch, thanks!

lukasz-soszynski-eliatra · 2022-08-17T11:17:07Z

@davidlago @peternied
IIRC you mentioned in one of our calls that the CI build has to be run manually. Could you run CI build for me, please?

peternied · 2022-08-17T12:00:35Z

@lukasz-soszynski-eliatra the build has run and it looked like it failed https://github.com/opensearch-project/security/runs/7877581013?check_suite_focus=true I believe it will continue to trigger automatically on your fork as well as on this pull request as I have approved it. Let us know if you are seeing any 'waiting for approval' gates.

lukasz-soszynski-eliatra · 2022-08-18T08:31:40Z

I rebased branch eliatra:integration-test-framework into newest opensearch-project:main yesterday. Could you run build one again in order to check if I solve the last problem, please?

peternied

I've resolved a bunch of the previous comments that don't need any updates, great progress. The list of changes is going down, but the inclusive name of cluster manager is still a bigger lingering presence in the code.

build.gradle

src/integrationTest/java/org/opensearch/test/framework/TestIndex.java

src/integrationTest/java/org/opensearch/test/framework/certificate/Certificates.java

src/newTest/java/org/opensearch/test/framework/cluster/NestedValueMap.java

src/newTest/java/org/opensearch/test/framework/cluster/OpenSearchClientProvider.java

src/newTest/java/org/opensearch/test/framework/cluster/SocketUtils.java

build.gradle

codecov-commenter · 2022-08-19T12:09:16Z

Codecov Report

Merging #1967 (67b5446) into main (7f992eb) will decrease coverage by 0.02%.
The diff coverage is 33.33%.

❗ Current head 67b5446 differs from pull request most recent head 6e5e885. Consider uploading reports for the commit 6e5e885 to get more accurate results

@@             Coverage Diff              @@
##               main    #1967      +/-   ##
============================================
- Coverage     60.99%   60.96%   -0.03%     
+ Complexity     3226     3225       -1     
============================================
  Files           256      256              
  Lines         18075    18072       -3     
  Branches       3225     3224       -1     
============================================
- Hits          11024    11017       -7     
- Misses         5472     5476       +4     
  Partials       1579     1579

Impacted Files	Coverage Δ
.../org/opensearch/security/support/PemKeyReader.java	`73.38% <20.00%> (-3.38%)`	⬇️
...ch/security/securityconf/DynamicConfigFactory.java	`56.05% <100.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

peternied

I've created some pull requests for specific updates, which you can take or leave, but there seems to be some kind of warnings that are shown when the tests run that seem like they might indicate there is a problem

src/integrationTest/java/org/opensearch/test/framework/cluster/ClusterManager.java

peternied · 2022-09-08T16:04:27Z

Retriggered the CI - I think we are super close!

Signed-off-by: Kacper Trochimiak <kacper.trochimiak@eliatra.com>

Signed-Off-By: Nils Bandener <git@bndnr.de>

Removes the following messages from the output: 2022-08-25 12:51:21 SUITE-SecurityRolesTests-seed#[BB25A03A663EB2F7] WARN Salt:48 - If you plan to use field masking pls configure compliance salt e1ukloTsQlOgPquJ to be a random string of 16 chars length identical on all nodes 2022-08-25 12:51:21 SUITE-SecurityRolesTests-seed#[BB25A03A663EB2F7] ERROR SinkProvider:64 - Default endpoint could not be created, auditlog will not work properly. 2022-08-25 12:51:21 SUITE-SecurityRolesTests-seed#[BB25A03A663EB2F7] WARN AuditMessageRouter:61 - No default storage available, audit log may not work properly. Please check configuration. Signed-off-by: Peter Nied <petern@amazon.com>

Signed-off-by: Peter Nied <petern@amazon.com>

Signed-off-by: Lukasz Soszynski <lukasz.soszynski@eliatra.com>

Signed-Off-By: Nils Bandener <git@bndnr.de>

…y class NodeAndClusterIdConverter during tests execution Signed-off-by: Lukasz Soszynski <lukasz.soszynski@eliatra.com>

peternied

Last item before we merge is the copyright headers in the test cases files - could you do a quick pass over any files that can remove this since they are net new for this project?

Please do not modify these in any files that have been copied from other codebases.

src/integrationTest/java/org/opensearch/security/SecurityRolesTests.java

src/integrationTest/java/org/opensearch/test/framework/cluster/ClusterManager.java

Signed-Off-By: Nils Bandener <git@bndnr.de>

peternied · 2022-09-13T16:40:34Z

@opensearch-project/security Could we have another reviewer take a look?

cwperks · 2022-09-15T15:33:19Z

build.gradle

+    integrationTestImplementation 'junit:junit:4.13.2'
+    integrationTestImplementation "org.opensearch.plugin:reindex-client:${opensearch_version}"
+    integrationTestImplementation "org.opensearch.plugin:percolator-client:${opensearch_version}"
+    integrationTestImplementation 'commons-io:commons-io:2.11.0'


I'm curious, why do dependencies need to be repeated for integrationTestImplementation if we already have an entry for testImplementation?

As the goal of the project is the replacement of large parts of the old legacy tests, we want to keep the test definitions as separate as possible.

DarshitChanpura

Ty so much for this PR!

DarshitChanpura · 2022-09-15T15:18:04Z

src/integrationTest/java/org/opensearch/node/PluginAwareNode.java

+* distributed under the License is distributed on an "AS IS" BASIS,
+* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+* See the License for the specific language governing permissions and
+* limitations under the License.


Does this file come from floragunn repo or is it a new file ? If it is a new file we should remove the floragunn license section

This should also apply to any other files in this PR

This is based on code from floragunn

This is based on floragunn code.

@lukasz-soszynski-eliatra @nibix Can you please provide the link to the source repo where this is based off of? This would be super helpful to add it to merge-commit

DarshitChanpura · 2022-09-15T15:24:13Z

src/integrationTest/java/org/opensearch/security/privileges/PrivilegesEvaluatorTest.java

+	protected final static TestSecurityConfig.User NEGATED_REGEX = new TestSecurityConfig.User("negated_regex_user")
+			.roles(new Role("negated_regex_role").indexPermissions("read").on("/^[a-z].*/")
+					.clusterPermissions("cluster_composite_ops"));
+


Ty for adding this! Can you please add a line or two comment over here explaining the idea behind these tests? I feel it would be helpful to understand as it took me a while

This could be addressed in a separate PR as it isn't a blocking change

src/integrationTest/java/org/opensearch/test/framework/TestSecurityConfig.java

src/integrationTest/java/org/opensearch/test/framework/certificate/Certificates.java

...integrationTest/java/org/opensearch/test/framework/cluster/ContextHeaderDecoratorClient.java

src/integrationTest/java/org/opensearch/test/framework/cluster/LocalOpenSearchCluster.java

...nTest/java/org/opensearch/test/framework/cluster/MinimumSecuritySettingsSupplierFactory.java

src/integrationTest/java/org/opensearch/test/framework/cluster/SocketUtils.java

src/integrationTest/java/org/opensearch/test/framework/cluster/SocketUtilsTests.java

…dded. Signed-off-by: Lukasz Soszynski <lukasz.soszynski@eliatra.com>

DarshitChanpura

Thank you! Can you please respond to couple of comments above and we will get this merged

peternied · 2022-09-20T14:47:56Z

This is a giant PR and it has two approvals so I am merging. I know there are some outstanding comments - we can handle outstanding comments asynchronously.

@DarshitChanpura if there are any you are feeling passionate about please create issues for follow up or pull requests to resolve.

This is the first iteration of a new framework for integration tests. The main focus is on conciseness and self-contained test. The framework offers: A programmatic way of defining the cluster setup A programmatic way of defining users and roles A programmatic way of defining indices A REST client with methods tailored for testing This is now all part of the test class, so you do not need to jump around in various configuration files to get an overview on the test setup. There are of course still a lot of features missing, like setting up test data. I propose to add these features in increments. Signed-off-by: Nils Bandener <nils.bandener@eliatra.com> Signed-off-by: Jochen Kressin <jkressin@kressin.info> Signed-off-by: Lukasz Soszynski <lukasz.soszynski@eliatra.com> Signed-off-by: Kacper Trochimiak <kacper.trochimiak@eliatra.com> Signed-off-by: Nils Bandener <git@bndnr.de> Signed-off-by: Peter Nied <petern@amazon.com> Co-authored-by: Jochen Kressin <jkressin@kressin.info> Co-authored-by: Lukasz Soszynski <lukasz.soszynski@eliatra.com> Co-authored-by: Kacper Trochimiak <kacper.trochimiak@eliatra.com> Co-authored-by: Peter Nied <petern@amazon.com>

This is the first iteration of a new framework for integration tests. The main focus is on conciseness and self-contained test. The framework offers: A programmatic way of defining the cluster setup A programmatic way of defining users and roles A programmatic way of defining indices A REST client with methods tailored for testing This is now all part of the test class, so you do not need to jump around in various configuration files to get an overview on the test setup. There are of course still a lot of features missing, like setting up test data. I propose to add these features in increments. Signed-off-by: Nils Bandener <nils.bandener@eliatra.com> Signed-off-by: Jochen Kressin <jkressin@kressin.info> Signed-off-by: Lukasz Soszynski <lukasz.soszynski@eliatra.com> Signed-off-by: Kacper Trochimiak <kacper.trochimiak@eliatra.com> Signed-off-by: Nils Bandener <git@bndnr.de> Signed-off-by: Peter Nied <petern@amazon.com> Co-authored-by: Jochen Kressin <jkressin@kressin.info> Co-authored-by: Lukasz Soszynski <lukasz.soszynski@eliatra.com> Co-authored-by: Kacper Trochimiak <kacper.trochimiak@eliatra.com> Co-authored-by: Peter Nied <petern@amazon.com> Signed-off-by: Stephen Crawford <steecraw@amazon.com>

nibix requested a review from a team July 26, 2022 15:29

nibix force-pushed the integration-test-framework branch from 59f136c to 484d72d Compare July 26, 2022 16:23

peternied reviewed Jul 27, 2022

View reviewed changes

peternied reviewed Jul 28, 2022

View reviewed changes

src/newTest/java/org/opensearch/test/framework/certificate/Certificates.java Outdated Show resolved Hide resolved

peternied suggested changes Jul 28, 2022

View reviewed changes

peternied self-assigned this Jul 28, 2022

kt-eliatra force-pushed the integration-test-framework branch 6 times, most recently from ca1793e to 3995774 Compare August 11, 2022 13:38

lukasz-soszynski-eliatra force-pushed the integration-test-framework branch 2 times, most recently from 987fdb0 to 88394ec Compare August 17, 2022 09:50

lukasz-soszynski-eliatra force-pushed the integration-test-framework branch from 88394ec to 77080b0 Compare August 17, 2022 16:02

peternied suggested changes Aug 18, 2022

View reviewed changes

nibix force-pushed the integration-test-framework branch 2 times, most recently from 412f050 to f7e0738 Compare August 25, 2022 10:04

peternied suggested changes Aug 25, 2022

View reviewed changes

src/integrationTest/java/org/opensearch/test/framework/cluster/ClusterManager.java Show resolved Hide resolved

This was referenced Aug 30, 2022

It is possible to disable NodeAndClusterIdConverter plugin. opensearch-project/OpenSearch#4322

Closed

[FEATURE] Integration tests should generate X.509 certificates in the course of tests execution #2057

Closed

lukasz-soszynski-eliatra force-pushed the integration-test-framework branch from e4c0f63 to 26405c4 Compare September 5, 2022 11:37

kt-eliatra and others added 12 commits September 13, 2022 13:28

add tests for SocketUtils class

5340d9a

Signed-off-by: Kacper Trochimiak <kacper.trochimiak@eliatra.com>

add new line to SocketUtilsTests class

e2c66c9

Signed-off-by: Kacper Trochimiak <kacper.trochimiak@eliatra.com>

add separate spotless configuration for integrationTest

777096e

Signed-off-by: Kacper Trochimiak <kacper.trochimiak@eliatra.com>

integrationTest - apply spotless formatting

652af5b

Signed-off-by: Kacper Trochimiak <kacper.trochimiak@eliatra.com>

CI/CD - run integrationTest task only when test task is executed

df0da3c

Signed-off-by: Kacper Trochimiak <kacper.trochimiak@eliatra.com>

Removed uses of NestedValueMap and replaced by ToXContent

9f4d715

Signed-Off-By: Nils Bandener <git@bndnr.de>

Fixed imports

33f35f5

Signed-Off-By: Nils Bandener <git@bndnr.de>

Replace non-inclusive terms from configuration settings

33b78b3

Signed-off-by: Peter Nied <petern@amazon.com>

Output of integration tests cleaned.

674be40

Signed-off-by: Lukasz Soszynski <lukasz.soszynski@eliatra.com>

Added API docs

0757c08

Signed-Off-By: Nils Bandener <git@bndnr.de>

Applied spotless rules

08d6500

Signed-Off-By: Nils Bandener <git@bndnr.de>

lukasz-soszynski-eliatra force-pushed the integration-test-framework branch from 6a67779 to 08d6500 Compare September 13, 2022 11:30

A temporary solution to limit the number of warning messages caused b…

67b5446

…y class NodeAndClusterIdConverter during tests execution Signed-off-by: Lukasz Soszynski <lukasz.soszynski@eliatra.com>

peternied suggested changes Sep 13, 2022

View reviewed changes

src/integrationTest/java/org/opensearch/security/SecurityRolesTests.java Outdated Show resolved Hide resolved

src/integrationTest/java/org/opensearch/test/framework/cluster/ClusterManager.java Show resolved Hide resolved

Fixed license headers

6e5e885

Signed-Off-By: Nils Bandener <git@bndnr.de>

peternied previously approved these changes Sep 13, 2022

View reviewed changes

cwperks reviewed Sep 15, 2022

View reviewed changes

DarshitChanpura reviewed Sep 16, 2022

View reviewed changes

Correction after code review, removed some comments and one javadoc a…

9fdb73a

…dded. Signed-off-by: Lukasz Soszynski <lukasz.soszynski@eliatra.com>

lukasz-soszynski-eliatra dismissed peternied’s stale review via 9fdb73a September 19, 2022 09:17

peternied approved these changes Sep 19, 2022

View reviewed changes

DarshitChanpura approved these changes Sep 19, 2022

View reviewed changes

peternied merged commit f591af6 into opensearch-project:main Sep 20, 2022

DarshitChanpura mentioned this pull request Aug 22, 2023

Backport the test framework to 2.x branch #3230

Closed

6 tasks

cwperks mentioned this pull request Aug 23, 2024

Add Nils Bandener (Github: nibix) as a maintainer #4672

Merged

Conversation

nibix commented Jul 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Check List

Uh oh!

peternied left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

peternied commented Jul 28, 2022

Uh oh!

davidlago commented Aug 9, 2022

Uh oh!

peternied commented Aug 12, 2022

Uh oh!

lukasz-soszynski-eliatra commented Aug 17, 2022

Uh oh!

peternied commented Aug 17, 2022

Uh oh!

lukasz-soszynski-eliatra commented Aug 18, 2022

Uh oh!

peternied left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov-commenter commented Aug 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

peternied left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

peternied commented Sep 8, 2022

Uh oh!

peternied left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

peternied commented Sep 13, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DarshitChanpura left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

nibix commented Jul 26, 2022 •

edited

Loading

codecov-commenter commented Aug 19, 2022 •

edited

Loading

DarshitChanpura Sep 19, 2022 •

edited

Loading

DarshitChanpura left a comment •

edited

Loading