Migrate `repository-s3` YAML tests to Java REST tests #117628

DaveCTurner · 2024-11-27T13:38:31Z

Today these YAML tests rely on a bunch of rather complex setup organised
by Gradle, and contain lots of duplication and coincident strings,
mostly because that was the only way to achieve what we wanted before we
could orchestrate test clusters and fixtures directly from Java test
suites. We're not actually running the YAML tests in ways that take
advantage of their YAMLness (e.g. in mixed-version clusters, or from
other client libraries).

This commit replaces these tests with Java REST tests which enormously
simplifies this area of code.

Relates ES-9984

Today these YAML tests rely on a bunch of rather complex setup organised by Gradle, and contain lots of duplication and coincident strings, mostly because that was the only way to achieve what we wanted before we could orchestrate test clusters and fixtures directly from Java test suites. We're not actually running the YAML tests in ways that take advantage of their YAMLness (e.g. in mixed-version clusters, or from other client libraries). This commit replaces these tests with Java REST tests which enormously simplifies this area of code. Relates ES-9984

elasticsearchmachine · 2024-11-27T17:43:38Z

Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination)

DaveCTurner · 2024-11-27T19:05:30Z

In case it helps with reviews: there's quite some noise in this PR but the main change here is to drop the essentially-identical .yml tests (30/40/50/60 and most of 20) and perform the same test actions in AbstractRepositoryS3RestTestCase instead. This way, we have the test actions written down in one place, with subclasses defining all the various different ways that the repository could get the appropriate credentials to run the tests. This still leaves the YAML tests in place to run the 10_basic and the first bit of 20_repository_permanent_credentials that wasn't duplicated, but these bits only ran with simple access-key/secret-key credentials so we don't need all the junk in build.gradle to configure the other credentials mechanisms. It also renders much of the gradle.build script as unused, and hence this PR removes all the unused bits. Or at least, this stuff was already kinda unused but that wasn't clear. It's unused now for sure. It also removes a bunch of places where we had multiple instances of the same string literal in order to make the tests work: with this change most (all?) relevant strings now vary run-to-run to avoid this confusion.

There's a couple of other changes here, namely to expose the bucket name in the MinIO fixture (which was previously hard-coded as bucket, although this was not obvious from the tangle of environment vars and sysprops that did so) and fixing a small bug in S3Service which would close the client needed to obtain credentials from STS the first time a S3BlobStore shut down, which would sometimes cause these new tests to fail depending on the order in which they ran.

nicktindall

LGTM, nice tidy up

ywangd

LGTM

The REST version is much cleaner. One question that I have is whether the client team (or other teams) depends on the existing yaml tests. I seem to remember they are used for intergration tests. Probably not an issue since we still have 10_basic and some of the 20. The rest is mostly duplicated for different type of credentials. A heads-up ping to the client team could still be helpful.

ywangd · 2024-11-29T06:39:25Z

...rc/javaRestTest/java/org/elasticsearch/repositories/s3/AbstractRepositoryS3RestTestCase.java

+
+    @Before
+    public void skipIfFips() {
+        assumeFalse("doesn't work in a FIPS JVM, but that's ok", inFipsJvm());


Is this still necessary? The existing yaml tests do not seem to need it. But we can also tackle this in a separate PR.

I think you're right now that you've fixed the fixtures to make long enough keys in #117675. Removed.

ywangd · 2024-11-29T06:48:48Z

...rc/javaRestTest/java/org/elasticsearch/repositories/s3/AbstractRepositoryS3RestTestCase.java

+    protected abstract String getClientName();
+
+    protected static String getIdentifierPrefix(String testSuiteName) {
+        return testSuiteName + "-" + Integer.toString(Murmur3HashFunction.hash(testSuiteName + System.getProperty("tests.seed")), 16) + "-";


This is in a method. I think we can just use the usual randomIdentifier and friends instead of performing hash manually?

Unfortunately not, this is called when initializing some static fields in the test suite classes, which happens too early to use randomIdentifier().

ywangd · 2024-11-29T06:52:31Z

modules/repository-s3/src/main/java/org/elasticsearch/repositories/s3/S3Service.java

+    public void onBlobStoreClose() {
+        releaseCachedClients();
+    }
+
    public void close() throws IOException {


Nit: missing @Override here.

ywangd · 2024-11-29T06:53:20Z

...est/java/org/elasticsearch/repositories/s3/RepositoryS3RegionalStsClientYamlTestSuiteIT.java

-        // The AWS STS SDK requires the role and session names to be set. We can verify that they are sent to S3S in the
-        // S3HttpFixtureWithSTS fixture


Is this comment no longer relevant?

It was never correct - we don't ever actually send any requests to S3 in this test suite, we're just doing some basic initialization to make sure that ES doesn't encounter any problems with e.g. the security manager.

ywangd · 2024-11-29T06:55:51Z

...lRestTest/resources/rest-api-spec/test/repository_s3/20_repository_permanent_credentials.yml

-            bucket: @permanent_bucket@
+            bucket: bucket
            client: integration_test_permanent
-            base_path: "@permanent_base_path@"
+            base_path: base_path_integration_tests


Just making sure I understand this: These yaml tests do not run against 3rd party services so that passing these variables from the build.gradle file was never really useful for them. So we can just hard code them?

arteam

LGTM3

elasticsearchmachine · 2024-11-29T09:59:22Z

💔 Backport failed

Status	Branch	Result
❌	8.x	Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 117628

DaveCTurner · 2024-11-29T10:33:30Z

Backport pending on backport of #117749 first.

Today these YAML tests rely on a bunch of rather complex setup organised by Gradle, and contain lots of duplication and coincident strings, mostly because that was the only way to achieve what we wanted before we could orchestrate test clusters and fixtures directly from Java test suites. We're not actually running the YAML tests in ways that take advantage of their YAMLness (e.g. in mixed-version clusters, or from other client libraries). This commit replaces these tests with Java REST tests which enormously simplifies this area of code. Relates ES-9984 Backport of elastic#117628 to 8.x

Today these YAML tests rely on a bunch of rather complex setup organised by Gradle, and contain lots of duplication and coincident strings, mostly because that was the only way to achieve what we wanted before we could orchestrate test clusters and fixtures directly from Java test suites. We're not actually running the YAML tests in ways that take advantage of their YAMLness (e.g. in mixed-version clusters, or from other client libraries). This commit replaces these tests with Java REST tests which enormously simplifies this area of code. Relates ES-9984 Backport of #117628 to 8.x

DaveCTurner added >test Issues or PRs that are addressing/adding tests :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs auto-backport Automatically create backport pull requests when merged >tech debt v9.0.0 v8.18.0 labels Nov 27, 2024

DaveCTurner force-pushed the 2024/11/25/repository-s3-java-rest-tests branch from f727f81 to b32777e Compare November 27, 2024 17:42

DaveCTurner force-pushed the 2024/11/25/repository-s3-java-rest-tests branch from b32777e to 5ff3d75 Compare November 27, 2024 17:42

DaveCTurner marked this pull request as ready for review November 27, 2024 17:43

DaveCTurner requested review from nicktindall, arteam and ywangd November 27, 2024 17:43

elasticsearchmachine added the Team:Distributed Coordination Meta label for Distributed Coordination team label Nov 27, 2024

nicktindall approved these changes Nov 29, 2024

View reviewed changes

ywangd approved these changes Nov 29, 2024

View reviewed changes

DaveCTurner added 4 commits November 29, 2024 07:04

Merge branch 'main' into 2024/11/25/repository-s3-java-rest-tests

4d2b324

Missing override

ed9dc2d

Tests now FIPS-compatible

2b25d35

Spotless

7163dee

DaveCTurner added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Nov 29, 2024

arteam approved these changes Nov 29, 2024

View reviewed changes

DaveCTurner added 2 commits November 29, 2024 08:53

Keys no longer available as sysprops

74a442b

Reject null keystore settings

40bcea9

elasticsearchmachine merged commit c3f9e01 into elastic:main Nov 29, 2024
16 checks passed

DaveCTurner deleted the 2024/11/25/repository-s3-java-rest-tests branch November 29, 2024 09:58

elasticsearchmachine added the backport pending label Nov 29, 2024

DaveCTurner mentioned this pull request Nov 29, 2024

Migrate repository-s3 YAML tests to Java REST tests #117756

Merged

DaveCTurner removed the backport pending label Nov 29, 2024

DaveCTurner mentioned this pull request Dec 9, 2024

[s3-repository] Recover in case a broken AWS HTTP client #115304

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate `repository-s3` YAML tests to Java REST tests #117628

Migrate `repository-s3` YAML tests to Java REST tests #117628

DaveCTurner commented Nov 27, 2024 •

edited

Loading

elasticsearchmachine commented Nov 27, 2024

DaveCTurner commented Nov 27, 2024

nicktindall left a comment •

edited

Loading

ywangd left a comment

ywangd Nov 29, 2024

DaveCTurner Nov 29, 2024

ywangd Nov 29, 2024

DaveCTurner Nov 29, 2024 •

edited

Loading

ywangd Nov 29, 2024

DaveCTurner Nov 29, 2024

ywangd Nov 29, 2024

DaveCTurner Nov 29, 2024

ywangd Nov 29, 2024

DaveCTurner Nov 29, 2024

arteam left a comment

elasticsearchmachine commented Nov 29, 2024

DaveCTurner commented Nov 29, 2024

		// The AWS STS SDK requires the role and session names to be set. We can verify that they are sent to S3S in the
		// S3HttpFixtureWithSTS fixture

Migrate repository-s3 YAML tests to Java REST tests #117628

Migrate repository-s3 YAML tests to Java REST tests #117628

Conversation

DaveCTurner commented Nov 27, 2024 • edited Loading

elasticsearchmachine commented Nov 27, 2024

DaveCTurner commented Nov 27, 2024

nicktindall left a comment • edited Loading

Choose a reason for hiding this comment

ywangd 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

DaveCTurner Nov 29, 2024 • edited Loading

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

arteam left a comment

Choose a reason for hiding this comment

elasticsearchmachine commented Nov 29, 2024

💔 Backport failed

DaveCTurner commented Nov 29, 2024

Migrate `repository-s3` YAML tests to Java REST tests #117628

Migrate `repository-s3` YAML tests to Java REST tests #117628

DaveCTurner commented Nov 27, 2024 •

edited

Loading

nicktindall left a comment •

edited

Loading

DaveCTurner Nov 29, 2024 •

edited

Loading