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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 8 commits into from
Oct 24, 2018
Merged

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
c5a4566
[test] Introduce strict deprecation mode for REST tests
lipsill Oct 5, 2018
8f78d5c
clean up
lipsill Oct 7, 2018
f59c576
checkStyle fix
lipsill Oct 8, 2018
3211e2f
Merge remote-tracking branch 'origin/master' into strict_deprecation_…
lipsill Oct 16, 2018
fc9106d
addressing reviewers comments
lipsill Oct 17, 2018
38e68cd
Merge remote-tracking branch 'origin/master' into strict_deprecation_…
lipsill Oct 18, 2018
d44f88d
activate strict mode for the high-level rest client
lipsill Oct 18, 2018
5cfb38b
correct issue introduced by merge...
lipsill Oct 18, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions ...rest-high-level/src/test/java/org/elasticsearch/client/ESRestHighLevelClientTestCase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:

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?

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

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.

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

return false;
}
}
45 changes: 39 additions & 6 deletions qa/full-cluster-restart/src/test/java/org/elasticsearch/upgrades/FullClusterRestartIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.greaterThan;
import static org.hamcrest.Matchers.notNullValue;
import static org.hamcrest.Matchers.startsWith;

/**
* Tests to run before and after a full cluster restart. This is run twice,
Expand All @@ -75,8 +76,24 @@ public class FullClusterRestartIT extends AbstractFullClusterRestartTestCase {
private String index;

@Before
public void setIndex() {
public void setIndex() throws IOException {
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.

if (isRunningAgainstOldCluster()) {
XContentBuilder template = jsonBuilder();
template.startObject();
{
template.field("index_patterns", "*");
template.field("order", "0");
template.startObject("settings");
template.field("number_of_shards", 5);
template.endObject();
}
template.endObject();
Request createTemplate = new Request("PUT", "/_template/template");
createTemplate.setJsonEntity(Strings.toString(template));
client().performRequest(createTemplate);
}
}

public void testSearch() throws Exception {
Expand Down Expand Up @@ -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.

👍

mappingsAndSettings.field("order", "1000");
{
mappingsAndSettings.startObject("settings");
mappingsAndSettings.field("number_of_shards", 1);
Expand Down Expand Up @@ -361,6 +379,7 @@ public void testShrink() throws IOException {
client().performRequest(updateSettingsRequest);

Request shrinkIndexRequest = new Request("PUT", "/" + index + "/_shrink/" + shrunkenIndex);
shrinkIndexRequest.addParameter("copy_settings", "true");
shrinkIndexRequest.setJsonEntity("{\"settings\": {\"index.number_of_shards\": 1}}");
client().performRequest(shrinkIndexRequest);

Expand Down Expand Up @@ -843,7 +862,7 @@ public void testSnapshotRestore() throws IOException {

// Stick a template into the cluster so we can see it after the restore
XContentBuilder templateBuilder = JsonXContent.contentBuilder().startObject();
templateBuilder.field("template", "evil_*"); // Don't confuse other tests by applying the template
templateBuilder.field("index_patterns", "evil_*"); // Don't confuse other tests by applying the template
templateBuilder.startObject("settings"); {
templateBuilder.field("number_of_shards", 1);
}
Expand Down Expand Up @@ -948,9 +967,23 @@ private void checkSnapshot(String snapshotName, int count, Version tookOnVersion
assertEquals(singletonList(tookOnVersion.toString()), XContentMapValues.extractValue("snapshots.version", listSnapshotResponse));

// Remove the routing setting and template so we can test restoring them.
Request clearRoutingFromSettings = new Request("PUT", "/_cluster/settings");
clearRoutingFromSettings.setJsonEntity("{\"persistent\":{\"cluster.routing.allocation.exclude.test_attr\": null}}");
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.

client().performRequest(clearRoutingFromSettings);
} catch (ResponseException e) {
if (e.getResponse().hasWarnings()
&& (isRunningAgainstOldCluster() == false || getOldClusterVersion().onOrAfter(Version.V_6_5_0))) {
e.getResponse().getWarnings().stream().forEach(warning -> {
assertThat(warning, containsString(
"setting was deprecated in Elasticsearch and will be removed in a future release! "
+ "See the breaking changes documentation for the next major version."));
assertThat(warning, startsWith("[search.remote."));
});
} else {
throw e;
}
}
client().performRequest(new Request("DELETE", "/_template/test_template"));

// Restore
Expand Down
2 changes: 1 addition & 1 deletion qa/full-cluster-restart/src/test/java/org/elasticsearch/upgrades/QueryBuilderBWCIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,7 @@ public void testQueryBuilderBWC() throws Exception {
QueryBuilder expectedQueryBuilder = (QueryBuilder) CANDIDATES.get(i)[1];
Request request = new Request("GET", "/" + index + "/_search");
request.setJsonEntity("{\"query\": {\"ids\": {\"values\": [\"" + Integer.toString(i) + "\"]}}, " +
"\"docvalue_fields\" : [\"query.query_builder_field\"]}");
"\"docvalue_fields\": [{\"field\":\"query.query_builder_field\", \"format\":\"use_field_mapping\"}]}");
Response rsp = client().performRequest(request);
assertEquals(200, rsp.getStatusLine().getStatusCode());
Map<?, ?> hitRsp = (Map<?, ?>) ((List<?>) ((Map<?, ?>)toMap(rsp).get("hits")).get("hits")).get(0);
Expand Down
18 changes: 18 additions & 0 deletions qa/rolling-upgrade/src/test/java/org/elasticsearch/upgrades/IndexingIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,16 @@

import org.apache.http.util.EntityUtils;
import org.elasticsearch.common.Booleans;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.client.Request;
import org.elasticsearch.client.Response;

import java.io.IOException;
import java.nio.charset.StandardCharsets;

import static org.elasticsearch.common.xcontent.XContentFactory.jsonBuilder;

/**
* Basic test that indexed documents survive the rolling restart. See
* {@link RecoveryIT} for much more in depth testing of the mechanism
Expand Down Expand Up @@ -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.

XContentBuilder template = jsonBuilder();
template.startObject();
{
template.field("index_patterns", "*");
template.startObject("settings");
template.field("number_of_shards", 5);
template.endObject();
}
template.endObject();
Request createTemplate = new Request("PUT", "/_template/template");
createTemplate.setJsonEntity(Strings.toString(template));
client().performRequest(createTemplate);

Request createTestIndex = new Request("PUT", "/test_index");
createTestIndex.setJsonEntity("{\"settings\": {\"index.number_of_replicas\": 0}}");
client().performRequest(createTestIndex);
Expand Down
9 changes: 9 additions & 0 deletions test/framework/src/main/java/org/elasticsearch/test/rest/ESRestTestCase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -451,9 +451,18 @@ protected String getProtocol() {
protected RestClient buildClient(Settings settings, HttpHost[] hosts) throws IOException {
RestClientBuilder builder = RestClient.builder(hosts);
configureClient(builder, settings);
builder.setStrictDeprecationMode(getStrictDeprecationMode());
return builder.build();
}

/**
* Whether the used REST client should return any response containing at
* least one warning header as a failure.
*/
protected boolean getStrictDeprecationMode() {
return true;
}

protected static void configureClient(RestClientBuilder builder, Settings settings) throws IOException {
String keystorePath = settings.get(TRUSTSTORE_PATH);
if (keystorePath != null) {
Expand Down
5 changes: 5 additions & 0 deletions test/framework/src/main/java/org/elasticsearch/test/rest/yaml/ESClientYamlSuiteTestCase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -408,4 +408,9 @@ protected final RestClientBuilder getClientBuilderWithSniffedHosts() throws IOEx
configureClient(builder, restClientSettings());
return builder;
}

@Override
protected boolean getStrictDeprecationMode() {
return false;
}
}
4 changes: 2 additions & 2 deletions ...node-tests/src/test/java/org/elasticsearch/xpack/ml/transforms/PainlessDomainSplitIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,7 @@ public void testIsolated() throws Exception {
String mapAsJson = Strings.toString(jsonBuilder().map(params));
logger.info("params={}", mapAsJson);

Request searchRequest = new Request("GET", "/painless/test/_search");
Request searchRequest = new Request("GET", "/painless/_search");
searchRequest.setJsonEntity(
"{\n" +
" \"query\" : {\n" +
Expand All @@ -207,7 +207,7 @@ public void testIsolated() throws Exception {
" \"domain_split\" : {\n" +
" \"script\" : {\n" +
" \"lang\": \"painless\",\n" +
" \"inline\": \"" + DomainSplitFunction.function +
" \"source\": \"" + DomainSplitFunction.function +
" return domainSplit(params['host'], params); \",\n" +
" \"params\": " + mapAsJson + "\n" +
" }\n" +
Expand Down
19 changes: 19 additions & 0 deletions ...l-cluster-restart/src/test/java/org/elasticsearch/xpack/restart/FullClusterRestartIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,10 @@
import org.elasticsearch.client.Request;
import org.elasticsearch.client.Response;
import org.elasticsearch.client.ResponseException;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.util.concurrent.ThreadContext;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.XContentType;
import org.elasticsearch.common.xcontent.support.XContentMapValues;
import org.elasticsearch.rest.RestStatus;
Expand Down Expand Up @@ -41,6 +43,7 @@
import java.util.stream.Collectors;

import static org.elasticsearch.common.unit.TimeValue.timeValueSeconds;
import static org.elasticsearch.common.xcontent.XContentFactory.jsonBuilder;
import static org.hamcrest.Matchers.anyOf;
import static org.hamcrest.Matchers.containsInAnyOrder;
import static org.hamcrest.Matchers.containsString;
Expand All @@ -59,6 +62,22 @@ public class FullClusterRestartIT extends AbstractFullClusterRestartTestCase {
@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.

XContentBuilder template = jsonBuilder();
template.startObject();
{
template.field("index_patterns", "*");
template.field("order", "0");
template.startObject("settings");
template.field("number_of_shards", 5);
template.endObject();
}
template.endObject();
Request createTemplate = new Request("PUT", "/_template/template");
createTemplate.setJsonEntity(Strings.toString(template));
client().performRequest(createTemplate);
}
}

@Override
Expand Down
19 changes: 19 additions & 0 deletions x-pack/qa/rolling-upgrade/src/test/java/org/elasticsearch/upgrades/IndexingIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,16 @@

import org.apache.http.util.EntityUtils;
import org.elasticsearch.common.Booleans;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.client.Request;
import org.elasticsearch.client.Response;

import java.io.IOException;
import java.nio.charset.StandardCharsets;

import static org.elasticsearch.common.xcontent.XContentFactory.jsonBuilder;

/**
* Basic test that indexed documents survive the rolling restart.
* <p>
Expand Down Expand Up @@ -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.

template.startObject();
{
template.field("index_patterns", "*");
template.field("order", "0");
template.startObject("settings");
template.field("number_of_shards", 5);
template.endObject();
}
template.endObject();
Request createTemplate = new Request("PUT", "/_template/template");
createTemplate.setJsonEntity(Strings.toString(template));
client().performRequest(createTemplate);

Request createTestIndex = new Request("PUT", "/test_index");
createTestIndex.setJsonEntity("{\"settings\": {\"index.number_of_replicas\": 0}}");
client().performRequest(createTestIndex);
Expand Down
18 changes: 18 additions & 0 deletions ...lling-upgrade/src/test/java/org/elasticsearch/upgrades/TokenBackwardsCompatibilityIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,17 +13,35 @@
import org.elasticsearch.client.Response;
import org.elasticsearch.client.ResponseException;
import org.elasticsearch.client.RestClient;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.test.rest.yaml.ObjectPath;

import java.io.IOException;
import java.util.ArrayList;
import java.util.List;
import java.util.Map;

import static org.elasticsearch.common.xcontent.XContentFactory.jsonBuilder;

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.

template.startObject();
{
template.field("index_patterns", "*");
template.startObject("settings");
template.field("number_of_shards", 5);
template.endObject();
}
template.endObject();
Request createTemplate = new Request("PUT", "/_template/template");
createTemplate.setJsonEntity(Strings.toString(template));
client().performRequest(createTemplate);

Request createTokenRequest = new Request("POST", "_xpack/security/oauth2/token");
createTokenRequest.setJsonEntity(
"{\n" +
Expand Down