From c1c8817eaea460e8687576a3169a368753d8c504 Mon Sep 17 00:00:00 2001 From: Hendrik Muhs Date: Fri, 5 Jun 2020 08:48:47 +0200 Subject: [PATCH] [7.x][Transform] improve update API (#57685) rewrite config on update if either version is outdated, credentials change, the update changes the config or deprecated settings are found. Deprecated settings get migrated to the new format. The upgrade can be easily extended to do any necessary re-writes. fixes #56499 backport #57648 --- .../transform/transforms/TransformConfig.java | 35 +++ .../TransformInternalIndexConstants.java | 6 +- .../transforms/TransformConfigTests.java | 74 ++++- .../integration/TransformRestTestCase.java | 6 +- .../integration/TransformUpdateIT.java | 261 ++++++++++++++++++ .../TransportUpdateTransformAction.java | 17 +- 6 files changed, 391 insertions(+), 8 deletions(-) create mode 100644 x-pack/plugin/transform/qa/single-node-tests/src/test/java/org/elasticsearch/xpack/transform/integration/TransformUpdateIT.java diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/transform/transforms/TransformConfig.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/transform/transforms/TransformConfig.java index 8989ca3594b..5970e33c212 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/transform/transforms/TransformConfig.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/transform/transforms/TransformConfig.java @@ -423,6 +423,41 @@ public class TransformConfig extends AbstractDiffable implement return lenient ? LENIENT_PARSER.apply(parser, optionalTransformId) : STRICT_PARSER.apply(parser, optionalTransformId); } + /** + * Rewrites the transform config according to the latest format, for example moving deprecated + * settings to its new place. + * + * @param transformConfig original config + * @return a rewritten transform config if a rewrite was necessary, otherwise the given transformConfig + */ + public static TransformConfig rewriteForUpdate(final TransformConfig transformConfig) { + + // quick checks for deprecated features, if none found just return the original + if (transformConfig.getPivotConfig() == null || transformConfig.getPivotConfig().getMaxPageSearchSize() == null) { + return transformConfig; + } + + Builder builder = new Builder(transformConfig); + + if (transformConfig.getPivotConfig() != null && transformConfig.getPivotConfig().getMaxPageSearchSize() != null) { + // create a new pivot config but set maxPageSearchSize to null + PivotConfig newPivotConfig = new PivotConfig( + transformConfig.getPivotConfig().getGroupConfig(), + transformConfig.getPivotConfig().getAggregationConfig(), + null + ); + builder.setPivotConfig(newPivotConfig); + + Integer maxPageSearchSizeDeprecated = transformConfig.getPivotConfig().getMaxPageSearchSize(); + Integer maxPageSearchSize = transformConfig.getSettings().getMaxPageSearchSize() != null + ? transformConfig.getSettings().getMaxPageSearchSize() + : maxPageSearchSizeDeprecated; + + builder.setSettings(new SettingsConfig(maxPageSearchSize, transformConfig.getSettings().getDocsPerSecond())); + } + return builder.setVersion(Version.CURRENT).build(); + } + public static class Builder { private String id; private SourceConfig source; diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/transform/transforms/persistence/TransformInternalIndexConstants.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/transform/transforms/persistence/TransformInternalIndexConstants.java index 59a80f6c959..86115c06c37 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/transform/transforms/persistence/TransformInternalIndexConstants.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/transform/transforms/persistence/TransformInternalIndexConstants.java @@ -6,6 +6,8 @@ package org.elasticsearch.xpack.core.transform.transforms.persistence; +import org.elasticsearch.Version; + public final class TransformInternalIndexConstants { /* Constants for internal indexes of the transform plugin @@ -24,6 +26,7 @@ public final class TransformInternalIndexConstants { // internal index // version is not a rollover pattern, however padded because sort is string based + public static final Version INDEX_VERSION_LAST_CHANGED = Version.V_7_7_0; public static final String INDEX_VERSION = "005"; public static final String INDEX_PATTERN = ".transform-internal-"; public static final String LATEST_INDEX_VERSIONED_NAME = INDEX_PATTERN + INDEX_VERSION; @@ -42,7 +45,6 @@ public final class TransformInternalIndexConstants { public static final String AUDIT_INDEX_READ_ALIAS = ".transform-notifications-read"; public static final String AUDIT_INDEX = AUDIT_INDEX_PREFIX + AUDIT_TEMPLATE_VERSION; - private TransformInternalIndexConstants() { - } + private TransformInternalIndexConstants() {} } diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/transform/transforms/TransformConfigTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/transform/transforms/TransformConfigTests.java index b1b25061edb..bb05c2f765d 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/transform/transforms/TransformConfigTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/transform/transforms/TransformConfigTests.java @@ -313,9 +313,81 @@ public class TransformConfigTests extends AbstractSerializingTransformTestCase createTransformResponse = entityAsMap(client().performRequest(createTransformRequest)); + + assertThat(createTransformResponse.get("acknowledged"), equalTo(Boolean.TRUE)); + Request getRequest = createRequestWithAuth("GET", getTransformEndpoint() + transformId, BASIC_AUTH_VALUE_TRANSFORM_USER); + Map transforms = entityAsMap(client().performRequest(getRequest)); + assertEquals(1, XContentMapValues.extractValue("count", transforms)); + Map transform = ((List>) XContentMapValues.extractValue("transforms", transforms)).get(0); + assertThat(XContentMapValues.extractValue("pivot.max_page_search_size", transform), equalTo(555)); + + final Request updateRequest = createRequestWithAuth( + "POST", + getTransformEndpoint() + transformId + "/_update", + BASIC_AUTH_VALUE_TRANSFORM_ADMIN_1 + ); + updateRequest.setJsonEntity("{}"); + + Map updateResponse = entityAsMap(client().performRequest(updateRequest)); + + assertNull(XContentMapValues.extractValue("pivot.max_page_search_size", updateResponse)); + assertThat(XContentMapValues.extractValue("settings.max_page_search_size", updateResponse), equalTo(555)); + + getRequest = createRequestWithAuth("GET", getTransformEndpoint() + transformId, BASIC_AUTH_VALUE_TRANSFORM_USER); + transforms = entityAsMap(client().performRequest(getRequest)); + assertEquals(1, XContentMapValues.extractValue("count", transforms)); + transform = ((List>) XContentMapValues.extractValue("transforms", transforms)).get(0); + + assertNull(XContentMapValues.extractValue("pivot.max_page_search_size", transform)); + assertThat(XContentMapValues.extractValue("settings.max_page_search_size", transform), equalTo(555)); + } + + @SuppressWarnings("unchecked") + public void testUpdateTransferRights() throws Exception { + String transformId = "transform1"; + // Note: Due to a bug the transform does not fail to start after deleting the user and role, therefore invalidating + // the credentials stored with the config. As a workaround we use a 2nd transform that uses the same config + // once the bug is fixed, delete this 2nd transform + String transformIdCloned = "transform2"; + String transformDest = transformId + "_idx"; + setupDataAccessRole(DATA_ACCESS_ROLE, REVIEWS_INDEX_NAME, transformDest); + setupDataAccessRole(DATA_ACCESS_ROLE_2, REVIEWS_INDEX_NAME, transformDest); + + final Request createTransformRequest = createRequestWithAuth( + "PUT", + getTransformEndpoint() + transformId, + BASIC_AUTH_VALUE_TRANSFORM_ADMIN_2 + ); + + final Request createTransformRequest_2 = createRequestWithAuth( + "PUT", + getTransformEndpoint() + transformIdCloned, + BASIC_AUTH_VALUE_TRANSFORM_ADMIN_2 + ); + + String config = "{ \"dest\": {\"index\":\"" + + transformDest + + "\"}," + + " \"source\": {\"index\":\"" + + REVIEWS_INDEX_NAME + + "\"}," + + " \"pivot\": {" + + " \"group_by\": {" + + " \"reviewer\": {" + + " \"terms\": {" + + " \"field\": \"user_id\"" + + " } } }," + + " \"aggregations\": {" + + " \"avg_rating\": {" + + " \"avg\": {" + + " \"field\": \"stars\"" + + " } } }" + + " }" + + "}"; + + createTransformRequest.setJsonEntity(config); + Map createTransformResponse = entityAsMap(client().performRequest(createTransformRequest)); + + assertThat(createTransformResponse.get("acknowledged"), equalTo(Boolean.TRUE)); + Request getRequest = createRequestWithAuth("GET", getTransformEndpoint() + transformId, BASIC_AUTH_VALUE_TRANSFORM_ADMIN_2); + Map transforms = entityAsMap(client().performRequest(getRequest)); + assertEquals(1, XContentMapValues.extractValue("count", transforms)); + + // create a 2nd, identical one + createTransformRequest_2.setJsonEntity(config); + createTransformResponse = entityAsMap(client().performRequest(createTransformRequest_2)); + assertThat(createTransformResponse.get("acknowledged"), equalTo(Boolean.TRUE)); + + // delete the user _and_ the role to access the data + deleteUser(TEST_ADMIN_USER_NAME_2); + deleteDataAccessRole(DATA_ACCESS_ROLE_2); + + // getting the transform with the just deleted admin 2 user should fail + try { + client().performRequest(getRequest); + fail("request should have failed"); + } catch (ResponseException e) { + assertThat(e.getResponse().getStatusLine().getStatusCode(), equalTo(401)); + } + + // get the transform with admin 1 + getRequest = createRequestWithAuth("GET", getTransformEndpoint() + transformId, BASIC_AUTH_VALUE_TRANSFORM_ADMIN_1); + transforms = entityAsMap(client().performRequest(getRequest)); + assertEquals(1, XContentMapValues.extractValue("count", transforms)); + + // start using admin 1, but as the header is still admin 2 + // BUG: this should fail, because the transform can not access the source index any longer + startAndWaitForTransform(transformId, transformDest, BASIC_AUTH_VALUE_TRANSFORM_ADMIN_1); + + assertBusy(() -> { + Map transformStatsAsMap = getTransformStateAndStats(transformId); + assertThat(XContentMapValues.extractValue("stats.documents_indexed", transformStatsAsMap), equalTo(0)); + }, 3, TimeUnit.SECONDS); + + // update the transform with an empty body, the credentials (headers) should change + final Request updateRequest = createRequestWithAuth( + "POST", + getTransformEndpoint() + transformIdCloned + "/_update", + BASIC_AUTH_VALUE_TRANSFORM_ADMIN_1 + ); + updateRequest.setJsonEntity("{}"); + assertOK(client().performRequest(updateRequest)); + + // get should still work + getRequest = createRequestWithAuth("GET", getTransformEndpoint() + transformIdCloned, BASIC_AUTH_VALUE_TRANSFORM_ADMIN_1); + transforms = entityAsMap(client().performRequest(getRequest)); + assertEquals(1, XContentMapValues.extractValue("count", transforms)); + + // start with updated configuration should succeed + startAndWaitForTransform(transformIdCloned, transformDest, BASIC_AUTH_VALUE_TRANSFORM_ADMIN_1); + + assertBusy(() -> { + Map transformStatsAsMap = getTransformStateAndStats(transformIdCloned); + assertThat(XContentMapValues.extractValue("stats.documents_indexed", transformStatsAsMap), equalTo(27)); + }, 15, TimeUnit.SECONDS); + } + + private void deleteUser(String user) throws IOException { + Request request = new Request("DELETE", "/_security/user/" + user); + client().performRequest(request); + } + + protected void deleteDataAccessRole(String role) throws IOException { + Request request = new Request("DELETE", "/_security/role/" + role); + client().performRequest(request); + } +} diff --git a/x-pack/plugin/transform/src/main/java/org/elasticsearch/xpack/transform/action/TransportUpdateTransformAction.java b/x-pack/plugin/transform/src/main/java/org/elasticsearch/xpack/transform/action/TransportUpdateTransformAction.java index cab9dd72e1e..174f21e0a05 100644 --- a/x-pack/plugin/transform/src/main/java/org/elasticsearch/xpack/transform/action/TransportUpdateTransformAction.java +++ b/x-pack/plugin/transform/src/main/java/org/elasticsearch/xpack/transform/action/TransportUpdateTransformAction.java @@ -56,6 +56,7 @@ import org.elasticsearch.xpack.core.transform.transforms.TransformConfigUpdate; import org.elasticsearch.xpack.core.transform.transforms.TransformDestIndexSettings; import org.elasticsearch.xpack.core.transform.transforms.TransformState; import org.elasticsearch.xpack.core.transform.transforms.TransformTaskState; +import org.elasticsearch.xpack.core.transform.transforms.persistence.TransformInternalIndexConstants; import org.elasticsearch.xpack.transform.TransformServices; import org.elasticsearch.xpack.transform.notifications.TransformAuditor; import org.elasticsearch.xpack.transform.persistence.SeqNoPrimaryTermAndIndex; @@ -193,9 +194,18 @@ public class TransportUpdateTransformAction extends TransportTasksAction { - final TransformConfig config = configAndVersion.v1(); + final TransformConfig oldConfig = configAndVersion.v1(); + final TransformConfig config = TransformConfig.rewriteForUpdate(oldConfig); + // If it is a noop don't bother even writing the doc, save the cycles, just return here. - if (update.isNoop(config)) { + // skip when: + // - config is in the latest index + // - rewrite did not change the config + // - update is not making any changes + if (config.getVersion() != null + && config.getVersion().onOrAfter(TransformInternalIndexConstants.INDEX_VERSION_LAST_CHANGED) + && config.equals(oldConfig) + && update.isNoop(config)) { listener.onResponse(new Response(config)); return; } @@ -213,8 +223,7 @@ public class TransportUpdateTransformAction extends TransportTasksAction { request.setConfig(updateResponse.getConfig());