From 0a77c175e3fa4c60f41ea5c73307246c4fbb436d Mon Sep 17 00:00:00 2001 From: Peter Nied Date: Thu, 7 Mar 2024 19:02:08 +0000 Subject: [PATCH] Make sure upgraded configs are commited Signed-off-by: Peter Nied --- .../security/DefaultConfigurationTests.java | 11 ++-- .../dlic/rest/api/AbstractApiAction.java | 50 ++++++++++----- .../dlic/rest/api/ConfigUpgradeApiAction.java | 64 ++++++++----------- 3 files changed, 67 insertions(+), 58 deletions(-) diff --git a/src/integrationTest/java/org/opensearch/security/DefaultConfigurationTests.java b/src/integrationTest/java/org/opensearch/security/DefaultConfigurationTests.java index c6ceb51704..fe6bf2d7a3 100644 --- a/src/integrationTest/java/org/opensearch/security/DefaultConfigurationTests.java +++ b/src/integrationTest/java/org/opensearch/security/DefaultConfigurationTests.java @@ -11,9 +11,10 @@ import java.io.IOException; import java.nio.file.Path; -import java.util.ArrayList; +import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Set; import com.carrotsearch.randomizedtesting.annotations.ThreadLeakScope; import com.fasterxml.jackson.databind.JsonNode; @@ -121,9 +122,9 @@ public void securityRolesUgrade() throws Exception { } } - private List extractFieldNames(final JsonNode json) { - final var list = new ArrayList(); - json.fieldNames().forEachRemaining(list::add); - return list; + private Set extractFieldNames(final JsonNode json) { + final var set = new HashSet(); + json.fieldNames().forEachRemaining(set::add); + return set; } } diff --git a/src/main/java/org/opensearch/security/dlic/rest/api/AbstractApiAction.java b/src/main/java/org/opensearch/security/dlic/rest/api/AbstractApiAction.java index ef8a00d700..e53a53715d 100644 --- a/src/main/java/org/opensearch/security/dlic/rest/api/AbstractApiAction.java +++ b/src/main/java/org/opensearch/security/dlic/rest/api/AbstractApiAction.java @@ -35,13 +35,16 @@ import org.opensearch.client.node.NodeClient; import org.opensearch.cluster.service.ClusterService; import org.opensearch.common.CheckedSupplier; +import org.opensearch.common.action.ActionFuture; import org.opensearch.common.util.concurrent.ThreadContext.StoredContext; -import org.opensearch.common.xcontent.XContentHelper; import org.opensearch.common.xcontent.XContentType; import org.opensearch.core.action.ActionListener; import org.opensearch.core.common.Strings; +import org.opensearch.core.common.bytes.BytesReference; import org.opensearch.core.common.transport.TransportAddress; import org.opensearch.core.rest.RestStatus; +import org.opensearch.core.xcontent.ToXContent; +import org.opensearch.core.xcontent.XContentHelper; import org.opensearch.index.engine.VersionConflictEngineException; import org.opensearch.rest.BaseRestHandler; import org.opensearch.rest.BytesRestResponse; @@ -336,7 +339,7 @@ final void saveOrUpdateConfiguration( final SecurityDynamicConfiguration configuration, final OnSucessActionListener onSucessActionListener ) { - saveAndUpdateConfigs(securityApiDependencies.securityIndexName(), client, getConfigType(), configuration, onSucessActionListener); + saveAndUpdateConfigsAsync(securityApiDependencies, client, getConfigType(), configuration, onSucessActionListener); } protected final String nameParam(final RestRequest request) { @@ -485,30 +488,45 @@ public final void onFailure(Exception e) { } - public static void saveAndUpdateConfigs( - final String indexName, + public static ActionFuture saveAndUpdateConfigs( + final SecurityApiDependencies dependencies, + final Client client, + final CType cType, + final SecurityDynamicConfiguration configuration + ) { + final var request = createIndexRequestForConfig(dependencies, cType, configuration); + return client.index(request); + } + + public static void saveAndUpdateConfigsAsync( + final SecurityApiDependencies dependencies, final Client client, final CType cType, final SecurityDynamicConfiguration configuration, final ActionListener actionListener ) { - final IndexRequest ir = new IndexRequest(indexName); - final String id = cType.toLCString(); + final var ir = createIndexRequestForConfig(dependencies, cType, configuration); + client.index(ir, new ConfigUpdatingActionListener<>(new String[] { cType.toLCString() }, client, actionListener)); + } + private static IndexRequest createIndexRequestForConfig( + final SecurityApiDependencies dependencies, + final CType cType, + final SecurityDynamicConfiguration configuration + ) { configuration.removeStatic(); - + final BytesReference content; try { - client.index( - ir.id(id) - .setRefreshPolicy(RefreshPolicy.IMMEDIATE) - .setIfSeqNo(configuration.getSeqNo()) - .setIfPrimaryTerm(configuration.getPrimaryTerm()) - .source(id, XContentHelper.toXContent(configuration, XContentType.JSON, false)), - new ConfigUpdatingActionListener<>(new String[] { id }, client, actionListener) - ); - } catch (IOException e) { + content = XContentHelper.toXContent(configuration, XContentType.JSON, ToXContent.EMPTY_PARAMS, false); + } catch (final IOException e) { throw ExceptionsHelper.convertToOpenSearchException(e); } + + return new IndexRequest(dependencies.securityIndexName()).id(cType.toLCString()) + .setRefreshPolicy(RefreshPolicy.IMMEDIATE) + .setIfSeqNo(configuration.getSeqNo()) + .setIfPrimaryTerm(configuration.getPrimaryTerm()) + .source(cType.toLCString(), content); } protected static class ConfigUpdatingActionListener implements ActionListener { diff --git a/src/main/java/org/opensearch/security/dlic/rest/api/ConfigUpgradeApiAction.java b/src/main/java/org/opensearch/security/dlic/rest/api/ConfigUpgradeApiAction.java index 56c2807d9f..8e171e1e34 100644 --- a/src/main/java/org/opensearch/security/dlic/rest/api/ConfigUpgradeApiAction.java +++ b/src/main/java/org/opensearch/security/dlic/rest/api/ConfigUpgradeApiAction.java @@ -11,11 +11,6 @@ package org.opensearch.security.dlic.rest.api; -import static org.opensearch.security.dlic.rest.api.Responses.badRequestMessage; -import static org.opensearch.security.dlic.rest.api.Responses.response; -import static org.opensearch.security.dlic.rest.support.Utils.addRoutesPrefix; -import static org.opensearch.security.dlic.rest.support.Utils.withIOException; - import java.io.IOException; import java.nio.file.Path; import java.security.AccessController; @@ -30,17 +25,21 @@ import java.util.Set; import java.util.stream.Collectors; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableSet; +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.node.ArrayNode; +import com.fasterxml.jackson.databind.node.JsonNodeFactory; +import com.fasterxml.jackson.databind.node.ObjectNode; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; -import org.opensearch.action.index.IndexResponse; + import org.opensearch.client.Client; import org.opensearch.cluster.service.ClusterService; -import org.opensearch.common.action.ActionFuture; import org.opensearch.common.collect.Tuple; import org.opensearch.common.inject.Inject; import org.opensearch.common.settings.Settings; import org.opensearch.common.xcontent.XContentType; -import org.opensearch.core.action.ActionListener; import org.opensearch.core.rest.RestStatus; import org.opensearch.rest.BytesRestResponse; import org.opensearch.rest.RestChannel; @@ -57,14 +56,13 @@ import org.opensearch.security.support.ConfigHelper; import org.opensearch.threadpool.ThreadPool; -import com.fasterxml.jackson.databind.JsonNode; -import com.fasterxml.jackson.databind.node.ArrayNode; -import com.fasterxml.jackson.databind.node.JsonNodeFactory; -import com.fasterxml.jackson.databind.node.ObjectNode; import com.flipkart.zjsonpatch.DiffFlags; import com.flipkart.zjsonpatch.JsonDiff; -import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableSet; + +import static org.opensearch.security.dlic.rest.api.Responses.badRequestMessage; +import static org.opensearch.security.dlic.rest.api.Responses.response; +import static org.opensearch.security.dlic.rest.support.Utils.addRoutesPrefix; +import static org.opensearch.security.dlic.rest.support.Utils.withIOException; public class ConfigUpgradeApiAction extends AbstractApiAction { @@ -113,7 +111,7 @@ void handleCanUpgrade(final RestChannel channel, final RestRequest request, fina void handleUpgrade(final RestChannel channel, final RestRequest request, final Client client) throws IOException { withIOException(() -> getAndValidateConfigurationsToUpgrade(request).map(this::configurationDifferences)).map( this::verifyHasDifferences - ).map(diffs -> applyDifferences(request, diffs)).valid(updatedConfigs -> { + ).map(diffs -> applyDifferences(request, client, diffs)).valid(updatedConfigs -> { final var response = JsonNodeFactory.instance.objectNode(); response.put("status", "OK"); @@ -134,29 +132,21 @@ ValidationResult> applyDifferences( for (final Tuple difference : differencesToUpdate) { updatedResources.add( loadConfiguration(difference.v1(), false, false).map( - configuration -> patchEntities(request, difference.v2(), SecurityConfiguration.of(null, configuration)) - .map(patchResults -> { - saveAndUpdateConfigs(securityApiDependencies.securityIndexName(), client, difference.v1(), configuration, new ActionListener<>(){ - - @Override - public void onResponse(IndexResponse response) { - // TODO: oh my - how did we get here - } - - @Override - public void onFailure(Exception e) { - } - }); - - }) - .map( + configuration -> patchEntities(request, difference.v2(), SecurityConfiguration.of(null, configuration)).map( patchResults -> { - - - final var itemsGroupedByOperation = new ConfigItemChanges(difference.v1(), difference.v2()); - return ValidationResult.success(itemsGroupedByOperation); + final var response = saveAndUpdateConfigs( + securityApiDependencies, + client, + difference.v1(), + patchResults.configuration() + ); + return ValidationResult.success(response.actionGet()); } - ) + ).map(indexResponse -> { + + final var itemsGroupedByOperation = new ConfigItemChanges(difference.v1(), difference.v2()); + return ValidationResult.success(itemsGroupedByOperation); + }) ) ); } @@ -351,7 +341,7 @@ private static Map> classifyChanges(final JsonNode differen items.put(item, operation); } }); - + final var itemsGroupedByOperation = items.entrySet() .stream() .collect(Collectors.groupingBy(Map.Entry::getValue, Collectors.mapping(Map.Entry::getKey, Collectors.toList())));