[test] Introduce strict deprecation mode for REST tests #34338

lipsill · 2018-10-05T19:59:07Z

#33708 introduced a strict deprecation mode that makes a REST request fail if there is a warning header in the response returned by Elasticsearch (usually a deprecation message signaling that a feature or a field has been deprecated).
This change adds the strict deprecation mode into the REST integration tests, and makes the tests fail if a deprecated feature is used. Also any test using a deprecated feature has been modified to pass the build.

There are two notable exceptions:

High-level REST client (addressed separately by [HLRC] Documentation examples cleanup #34009)
YAML tests were the only REST integration tests that actually analyzed the http warnings headers and failed if there was an unexpected warning (not defined as expected in the rest-spec tests). Adding the new mode to the YAML tests will not bring any tangible benefit.

If a deprecation warning has been returned by the server, the REST tests fail.

elasticmachine · 2018-10-06T04:01:38Z

Pinging @elastic/es-core-infra

nik9000 · 2018-10-07T23:22:51Z

@lipsill, I'm travelling this week and won't be particularly responsive.

I skimmed this so I can say, @elasticmachine, test this please.

I've not thought through the implications of that template setting the number of shards. I promise all do that the next chance I get.

nik9000 · 2018-10-16T14:59:30Z

@elasticmachine, test this please

nik9000

I left a few inline comments but I've realized a thing: could you pull the template creation into the AbstractXXTestCase classes in its own @Before method? That'd solve a bunch of these

nik9000 · 2018-10-16T15:00:23Z

...nt/rest-high-level/src/test/java/org/elasticsearch/client/ESRestHighLevelClientTestCase.java

@@ -153,4 +153,9 @@ protected Settings restClientSettings() {
            .put(ThreadContext.PREFIX + ".Authorization", token)
            .build();
    }
+
+    @Override
+    protected boolean getStrictDeprecationMode() {


Do you think we should turn this to true now that I've merged #34338?

I would love to! But in the mean time #34135 got merged and it added a deprecated setting:

elasticsearch/client/rest-high-level/build.gradle

Line 93 in 55eaf7a

setting 'xpack.security.transport.ssl.truststore.password', 'testnode'

I think this is the reason for the two tests below to fail once the strict deprecation mode is on:

./gradlew :client:rest-high-level:integTestRunner -Dtests.seed=B82DCBDDD63664E9 -Dtests.class=org.elasticsearch.client.documentation.ClusterClientDocumentationIT -Dtests.method="testClusterGetSettings" ./gradlew :client:rest-high-level:integTestRunner -Dtests.seed=B82DCBDDD63664E9 -Dtests.class=org.elasticsearch.client.ClusterClientIT -Dtests.method="testClusterGetSettingsWithDefault"

[xpack.security.transport.ssl.truststore.password] setting was deprecated in Elasticsearch and will be removed in a future release! See the breaking changes documentation for the next major version.

I did not see what setting to use instead of xpack.security.transport.ssl.truststore.password. TBH I am not really sure if the setting should be deprecated in the first place as I did not find it mentioned in any docs...

@nik9000 do you have any ideas / pointers?

I would love to! But in the mean time #34135 got merged and it added a deprecated setting

@jkakavas, can you comment on this setting being deprecated?

Yes, it is deprecated

elasticsearch/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ssl/SSLConfigurationSettings.java

Lines 92 to 95 in 46c7b5e

private static final Function<String, Setting<SecureString>> LEGACY_TRUSTSTORE_PASSWORD_TEMPLATE = key ->

new Setting<>(key, "", SecureString::new, Property.Deprecated, Property.Filtered, Property.NodeScope);

public static final Setting<SecureString> LEGACY_TRUSTSTORE_PASSWORD_PROFILES = Setting.affixKeySetting("transport.profiles.",

"xpack.security.ssl.truststore.password", LEGACY_TRUSTSTORE_PASSWORD_TEMPLATE);

and I shouldn't have used it. I'll raise a follow-up PR tomorrow my morning to address this.

I pushed 6874ee8 that fixes this, sorry for the inconvenience @nik9000, @lipsill

nik9000 · 2018-10-16T16:08:48Z

qa/full-cluster-restart/src/test/java/org/elasticsearch/upgrades/FullClusterRestartIT.java

@@ -283,7 +300,8 @@ public void testClusterState() throws Exception {
        if (isRunningAgainstOldCluster()) {
            XContentBuilder mappingsAndSettings = jsonBuilder();
            mappingsAndSettings.startObject();
-            mappingsAndSettings.field("template", index);
+            mappingsAndSettings.field("index_patterns", index);


nik9000 · 2018-10-16T16:08:53Z

qa/full-cluster-restart/src/test/java/org/elasticsearch/upgrades/FullClusterRestartIT.java

        index = getTestName().toLowerCase(Locale.ROOT);
+        // TODO prevents 6.x warnings; remove in 8.0


Two things here:

Could you add something like assertThat("we don't need this branch if we aren't compatible with 6.0", Version.CURRENT.minimumIndexCompatibilityVersion(), lessThanOrEqualTo(Version.6.0.0));

Could you check if getOldClusterVersion() is less than 7 before doing this? It won't matter right now, but once we release 7.0 it'll show up as an "old" version and we don't want this template.

nik9000 · 2018-10-16T16:11:31Z

qa/full-cluster-restart/src/test/java/org/elasticsearch/upgrades/FullClusterRestartIT.java

-        client().performRequest(clearRoutingFromSettings);
+        try {
+            Request clearRoutingFromSettings = new Request("PUT", "/_cluster/settings");
+            clearRoutingFromSettings.setJsonEntity("{\"persistent\":{\"cluster.routing.allocation.exclude.test_attr\": null}}");


Huh. So it warns you whenever you update settings if any setting is deprecated? So sometimes this'll fail and sometimes it won't based on the test run order. Fun.

nik9000 · 2018-10-16T16:11:58Z

qa/rolling-upgrade/src/test/java/org/elasticsearch/upgrades/IndexingIT.java

@@ -60,6 +64,20 @@ public void testIndexing() throws IOException {
        }

        if (CLUSTER_TYPE == ClusterType.OLD) {
+            // TODO prevents 6.x warnings; remove in 8.0


Same note as above.

nik9000 · 2018-10-16T16:15:01Z

...full-cluster-restart/src/test/java/org/elasticsearch/xpack/restart/FullClusterRestartIT.java

@@ -59,6 +62,22 @@
    @Before
    public void waitForMlTemplates() throws Exception {
        XPackRestTestHelper.waitForMlTemplates(client());
+        // TODO prevents 6.x warnings; remove in 8.0
+        if (isRunningAgainstOldCluster()) {


Same note as above about the checking if the version is in range and asserting the index compatible version.

nik9000 · 2018-10-16T16:15:32Z

x-pack/qa/rolling-upgrade/src/test/java/org/elasticsearch/upgrades/IndexingIT.java

@@ -45,6 +49,21 @@ public void testIndexing() throws IOException {
        }

        if (CLUSTER_TYPE == ClusterType.OLD) {
+            // TODO prevents 6.x warnings; remove in 8.0
+            XContentBuilder template = jsonBuilder();


Same comment as above.

nik9000 · 2018-10-16T16:17:48Z

.../rolling-upgrade/src/test/java/org/elasticsearch/upgrades/TokenBackwardsCompatibilityIT.java

 public class TokenBackwardsCompatibilityIT extends AbstractUpgradeTestCase {

    public void testGeneratingTokenInOldCluster() throws Exception {
        assumeTrue("this test should only run against the old cluster", CLUSTER_TYPE == ClusterType.OLD);
+
+        XContentBuilder template = jsonBuilder();


Same comment as above.

…mode

lipsill · 2018-10-18T11:30:06Z

@nik9000 ~~can trigger another CI? Thanks!~~
Let's see what is left to be fixed ;)

btw how comes that a build is automatically started?

lipsill · 2018-10-18T13:56:46Z

@nik9000 can you trigger another CI?

nik9000 · 2018-10-18T14:34:40Z

@elasticmachine, test this please.

nik9000 · 2018-10-18T14:35:27Z

btw how comes that a build is automatically started?

I didn't see, did one start without one of us saying the magic words?

nik9000

It looks good to me! Let's see what CI thinks.....

lipsill · 2018-10-18T15:16:14Z

I didn't see, did one start without one of us saying the magic words?

I think so ;) After pushing 4 commits, the CI started :
https://elasticsearch-ci.elastic.co/job/elastic+elasticsearch+pull-request/212/
I did not see anybody saying the magic words ;)

nik9000 · 2018-10-18T18:26:00Z

That failure doesn't look related. Let me see if I can reproduce that one on my own....

@elsticmachine, retest this please

lipsill · 2018-10-18T18:52:13Z

@nik9000 thanks for following up and starting another CI ;) I actually opened #34610 about the failure as I could reproduce it locally...
Let's see how the second run goes ;)

nik9000 · 2018-10-24T12:24:49Z

OK! I've merged to master and started the backport to 6.x. Thanks for working on this @lipsill!

nik9000 · 2018-10-24T13:51:59Z

I think I'm going to abandon backporting this. 6.x logs a ton of warnings because of the default number of shards change. Instead of just being in the BWC tests it is everywhere. We work around it in the yaml tests but we don't have a similar workaround for all of the REST tests. So this'll stay in master only. I think that is fairly safe.

The pull request elastic#34338 added strict deprecation mode to the REST tests and adds the copy_settings param when testing the shrink of an index. This parameter has been added in 6.4.0 and will be removed in 8.0, so the test now needs to take care of the old cluster version when adding the copy_settings param.

lipsill · 2018-10-26T08:43:47Z

Ops... I guess the simplest approach would be to create a template with 5 shards in @Before. This way the default shards warning will be avoided. Or it can even be pushed to the LL Rest Client: hide any warnings regarding the default shards.

+1 for merging only in 7.0 as I am not really sure whether there are so many benefits of bringing it in 6.x. I mean that the docs changes for the HLRC have already made their way to 6.5, so docs-wise most probably ES is good to go.

Another option will be to merge the fixes but activate the strict mode only for the docs in 6.x; the idea here to try to guarantee that the examples for the released version are valid. TBH I am not sure how the docs snippets are triggered but it shouldn't be that hard to figure it out ;)

@nik9000 what do you think?
I find options 2 and 3 viable; the first one I guess the effort does not outweigh the benefit for a branch that is (kind of) not actively under development.

…#34853) The pull request #34338 added strict deprecation mode to the REST tests and adds the copy_settings param when testing the shrink of an index. This parameter has been added in 6.4.0 and will be removed in 8.0, so the test now needs to take care of the old cluster version when adding the copy_settings param.

nik9000 · 2018-10-26T13:14:10Z

Another option will be to merge the fixes but activate the strict mode only for the docs in 6.x; the idea here to try to guarantee that the examples for the released version are valid. TBH I am not sure how the docs snippets are triggered but it shouldn't be that hard to figure it out ;)

I like this option quite a bit! I had just merged this to master and couldn't get the strict mode passing without a lot of changes.

6.x is under active development, but 99% of the time patches land in master first and we backport so having strict mode in 6.x is a lot less important.

Would you like to open a PR against 6.x that backports just the fixes without the strict mode? If not I'll add it to my list.

lipsill · 2018-10-26T16:44:43Z

@nik9000 I can take this one over ;)
I will be leaving the strict mode off for all projects (including the docs)

#33708 introduced a strict deprecation mode that makes a REST request fail if there is a warning header in the response returned by Elasticsearch (usually a deprecation message signaling that a feature or a field has been deprecated). This change adds the strict deprecation mode into the REST integration tests, and makes the tests fail if a deprecated feature is used. Also any test using a deprecated feature has been modified to pass the build. The YAML integration tests already analyzed HTTP warnings so they do not use this mode, keeping their "expected vs actual" behavior.

…#34853) The pull request #34338 added strict deprecation mode to the REST tests and adds the copy_settings param when testing the shrink of an index. This parameter has been added in 6.4.0 and will be removed in 8.0, so the test now needs to take care of the old cluster version when adding the copy_settings param.

[test] Introduce strict deprecation mode for REST tests

c5a4566

If a deprecation warning has been returned by the server, the REST tests fail.

lipsill mentioned this pull request Oct 5, 2018

REST client: introduce a strict deprecation mode #33708

Merged

nik9000 self-requested a review October 6, 2018 04:01

nik9000 added :Delivery/Build Build or test infrastructure v7.0.0 v6.5.0 labels Oct 6, 2018

clean up

8f78d5c

checkStyle fix

f59c576

nik9000 requested changes Oct 16, 2018

View reviewed changes

lipsill added 4 commits October 16, 2018 19:28

Merge remote-tracking branch 'origin/master' into strict_deprecation_…

3211e2f

…mode

addressing reviewers comments

fc9106d

Merge remote-tracking branch 'origin/master' into strict_deprecation_…

38e68cd

…mode

activate strict mode for the high-level rest client

d44f88d

correct issue introduced by merge...

5cfb38b

nik9000 approved these changes Oct 18, 2018

View reviewed changes

nik9000 merged commit d5ad3de into elastic:master Oct 24, 2018

nik9000 added backport pending >enhancement labels Oct 24, 2018

nik9000 removed the backport pending label Oct 24, 2018

nik9000 removed the v6.5.0 label Oct 24, 2018

tlrx mentioned this pull request Oct 25, 2018

[Test] Fix FullClusterRestartIT.testShrink() with copy_settings param #34853

Merged

jimczi added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

mark-vieira added the Team:Delivery Meta label for Delivery team label Nov 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[test] Introduce strict deprecation mode for REST tests #34338

[test] Introduce strict deprecation mode for REST tests #34338

lipsill commented Oct 5, 2018

elasticmachine commented Oct 6, 2018

nik9000 commented Oct 7, 2018

nik9000 commented Oct 16, 2018

nik9000 left a comment

nik9000 Oct 16, 2018

lipsill Oct 17, 2018

nik9000 Oct 17, 2018

jkakavas Oct 17, 2018

nik9000 Oct 17, 2018

jkakavas Oct 18, 2018

nik9000 Oct 16, 2018

nik9000 Oct 16, 2018

nik9000 Oct 16, 2018

nik9000 Oct 16, 2018

nik9000 Oct 16, 2018

nik9000 Oct 16, 2018

nik9000 Oct 16, 2018

lipsill commented Oct 18, 2018 •

edited

Loading

lipsill commented Oct 18, 2018

nik9000 commented Oct 18, 2018

nik9000 commented Oct 18, 2018

nik9000 left a comment

lipsill commented Oct 18, 2018 •

edited

Loading

nik9000 commented Oct 18, 2018

lipsill commented Oct 18, 2018

nik9000 commented Oct 24, 2018

nik9000 commented Oct 24, 2018

lipsill commented Oct 26, 2018

nik9000 commented Oct 26, 2018

lipsill commented Oct 26, 2018

	private static final Function<String, Setting<SecureString>> LEGACY_TRUSTSTORE_PASSWORD_TEMPLATE = key ->
	new Setting<>(key, "", SecureString::new, Property.Deprecated, Property.Filtered, Property.NodeScope);
	public static final Setting<SecureString> LEGACY_TRUSTSTORE_PASSWORD_PROFILES = Setting.affixKeySetting("transport.profiles.",
	"xpack.security.ssl.truststore.password", LEGACY_TRUSTSTORE_PASSWORD_TEMPLATE);

		index = getTestName().toLowerCase(Locale.ROOT);
		// TODO prevents 6.x warnings; remove in 8.0

[test] Introduce strict deprecation mode for REST tests #34338

[test] Introduce strict deprecation mode for REST tests #34338

Conversation

lipsill commented Oct 5, 2018

elasticmachine commented Oct 6, 2018

nik9000 commented Oct 7, 2018

nik9000 commented Oct 16, 2018

nik9000 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lipsill commented Oct 18, 2018 • edited Loading

lipsill commented Oct 18, 2018

nik9000 commented Oct 18, 2018

nik9000 commented Oct 18, 2018

nik9000 left a comment

Choose a reason for hiding this comment

lipsill commented Oct 18, 2018 • edited Loading

nik9000 commented Oct 18, 2018

lipsill commented Oct 18, 2018

nik9000 commented Oct 24, 2018

nik9000 commented Oct 24, 2018

lipsill commented Oct 26, 2018

nik9000 commented Oct 26, 2018

lipsill commented Oct 26, 2018

lipsill commented Oct 18, 2018 •

edited

Loading

lipsill commented Oct 18, 2018 •

edited

Loading