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

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

Merged
merged 8 commits into from
Oct 24, 2018

Conversation

lipsill
Copy link
Contributor

@lipsill lipsill commented Oct 5, 2018

#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
Copy link
Collaborator

Pinging @elastic/es-core-infra

@nik9000
Copy link
Member

nik9000 commented Oct 7, 2018

@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
Copy link
Member

nik9000 commented Oct 16, 2018

@elasticmachine, test this please

Copy link
Member

@nik9000 nik9000 left a comment

Choose a reason for hiding this comment

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

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

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

@Override
protected boolean getStrictDeprecationMode() {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it is deprecated

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.

Copy link
Member

Choose a reason for hiding this comment

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

❤️

Copy link
Member

Choose a reason for hiding this comment

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

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

@@ -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);
Copy link
Member

Choose a reason for hiding this comment

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

👍

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

Choose a reason for hiding this comment

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

Two things here:

  1. 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));
  2. 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.

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

Choose a reason for hiding this comment

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

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.

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

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

Choose a reason for hiding this comment

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

Same note as above.

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

Choose a reason for hiding this comment

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

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

@@ -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();
Copy link
Member

Choose a reason for hiding this comment

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

Same comment as above.

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();
Copy link
Member

Choose a reason for hiding this comment

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

Same comment as above.

@lipsill
Copy link
Contributor Author

lipsill commented Oct 18, 2018

@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
Copy link
Contributor Author

lipsill commented Oct 18, 2018

@nik9000 can you trigger another CI?

@nik9000
Copy link
Member

nik9000 commented Oct 18, 2018

@elasticmachine, test this please.

@nik9000
Copy link
Member

nik9000 commented Oct 18, 2018

btw how comes that a build is automatically started?

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

Copy link
Member

@nik9000 nik9000 left a comment

Choose a reason for hiding this comment

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

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

@lipsill
Copy link
Contributor Author

lipsill commented Oct 18, 2018

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
Copy link
Member

nik9000 commented Oct 18, 2018

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

@elsticmachine, retest this please

@lipsill
Copy link
Contributor Author

lipsill commented Oct 18, 2018

@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
Copy link
Member

nik9000 commented Oct 24, 2018

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

@nik9000 nik9000 removed the v6.5.0 label Oct 24, 2018
@nik9000
Copy link
Member

nik9000 commented Oct 24, 2018

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.

tlrx added a commit to tlrx/elasticsearch that referenced this pull request Oct 26, 2018
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
Copy link
Contributor Author

lipsill commented Oct 26, 2018

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.

tlrx added a commit that referenced this pull request Oct 26, 2018
…#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
Copy link
Member

nik9000 commented Oct 26, 2018

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
Copy link
Contributor Author

lipsill commented Oct 26, 2018

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

kcm pushed a commit that referenced this pull request Oct 30, 2018
#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.
kcm pushed a commit that referenced this pull request Oct 30, 2018
…#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.
@jimczi jimczi added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019
@mark-vieira mark-vieira added the Team:Delivery Meta label for Delivery team label Nov 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Delivery/Build Build or test infrastructure >enhancement Team:Delivery Meta label for Delivery team v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants