From 3a1e1ff33a937e2ede256e40bc8f835160c98874 Mon Sep 17 00:00:00 2001 From: Peter Nied Date: Thu, 7 Mar 2024 02:52:05 +0000 Subject: [PATCH] Found out the configuration changes weren't actually pushed Signed-off-by: Peter Nied --- .../security/DefaultConfigurationTests.java | 2 +- .../dlic/rest/api/ConfigUpgradeApiAction.java | 138 ++++++++++-------- 2 files changed, 80 insertions(+), 60 deletions(-) diff --git a/src/integrationTest/java/org/opensearch/security/DefaultConfigurationTests.java b/src/integrationTest/java/org/opensearch/security/DefaultConfigurationTests.java index f5b000188a..c6ceb51704 100644 --- a/src/integrationTest/java/org/opensearch/security/DefaultConfigurationTests.java +++ b/src/integrationTest/java/org/opensearch/security/DefaultConfigurationTests.java @@ -117,7 +117,7 @@ public void securityRolesUgrade() throws Exception { final var afterUpgradeRolesResponse = client.get("_plugins/_security/api/roles/"); final var afterUpgradeRolesNames = extractFieldNames(afterUpgradeRolesResponse.getBodyAs(JsonNode.class)); - assertThat(afterUpgradeRolesNames, equalTo(rolesNames)); + assertThat(afterUpgradeRolesResponse.getBody(), afterUpgradeRolesNames, equalTo(rolesNames)); } } 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 a35dccb7b0..56c2807d9f 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,6 +11,11 @@ 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; @@ -25,27 +30,24 @@ 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; import org.opensearch.rest.RestRequest; import org.opensearch.rest.RestRequest.Method; import org.opensearch.security.configuration.ConfigurationRepository; +import org.opensearch.security.dlic.rest.api.ConfigUpgradeApiAction.ConfigItemChanges; import org.opensearch.security.dlic.rest.support.Utils; import org.opensearch.security.dlic.rest.validation.EndpointValidator; import org.opensearch.security.dlic.rest.validation.RequestContentValidator; @@ -55,13 +57,14 @@ 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 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 com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableSet; public class ConfigUpgradeApiAction extends AbstractApiAction { @@ -90,7 +93,7 @@ public ConfigUpgradeApiAction( void handleCanUpgrade(final RestChannel channel, final RestRequest request, final Client client) throws IOException { withIOException(() -> getAndValidateConfigurationsToUpgrade(request).map(this::configurationDifferences)).valid(differencesList -> { final var allConfigItemChanges = differencesList.stream() - .map(kvp -> new ConfigItemChanges(kvp.v1(), classifyChanges(kvp.v2()))) + .map(kvp -> new ConfigItemChanges(kvp.v1(), kvp.v2())) .collect(Collectors.toList()); final var upgradeAvaliable = allConfigItemChanges.stream().anyMatch(ConfigItemChanges::hasChanges); @@ -124,15 +127,33 @@ void handleUpgrade(final RestChannel channel, final RestRequest request, final C ValidationResult> applyDifferences( final RestRequest request, + final Client client, final List> differencesToUpdate ) throws IOException { final var updatedResources = new ArrayList>(); for (final Tuple difference : differencesToUpdate) { updatedResources.add( loadConfiguration(difference.v1(), false, false).map( - configuration -> patchEntities(request, difference.v2(), SecurityConfiguration.of(null, configuration)).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( patchResults -> { - final var itemsGroupedByOperation = new ConfigItemChanges(difference.v1(), classifyChanges(difference.v2())); + + + final var itemsGroupedByOperation = new ConfigItemChanges(difference.v1(), difference.v2()); return ValidationResult.success(itemsGroupedByOperation); } ) @@ -143,49 +164,6 @@ ValidationResult> applyDifferences( return ValidationResult.merge(updatedResources); } - /** Add syntaxic sugar for interacting with config changes */ - static class ConfigItemChanges { - - private final CType config; - private final Map> itemsGroupedByOperation; - - public ConfigItemChanges(final CType config, final Map> itemsGroupedByOperation) { - this.config = config; - this.itemsGroupedByOperation = itemsGroupedByOperation; - } - - public boolean hasChanges() { - return !itemsGroupedByOperation.isEmpty(); - } - - public void addToNode(final ObjectNode node) { - final var allOperations = JsonNodeFactory.instance.objectNode(); - itemsGroupedByOperation.forEach((operation, items) -> { - final var arrayNode = allOperations.putArray(operation); - items.forEach(arrayNode::add); - }); - node.set(config.toLCString(), allOperations); - } - } - - private static Map> classifyChanges(final JsonNode differences) { - final var items = new HashMap(); - differences.forEach(node -> { - final var item = pathRoot(node); - final var operation = node.get("op").asText(); - if (items.containsKey(item) && !items.get(item).equals(operation)) { - items.put(item, "modify"); - } else { - items.put(item, operation); - } - }); - - final var itemsGroupedByOperation = items.entrySet() - .stream() - .collect(Collectors.groupingBy(Map.Entry::getValue, Collectors.mapping(Map.Entry::getKey, Collectors.toList()))); - return itemsGroupedByOperation; - } - private ValidationResult>> verifyHasDifferences(List> diffs) { if (diffs.isEmpty()) { return ValidationResult.error(RestStatus.BAD_REQUEST, badRequestMessage("Unable to upgrade, no differences found")); @@ -325,7 +303,7 @@ public Map allowedKeys() { } /** More permissions validation that default ContentValidator */ - public static class ConfigUpgradeContentValidator extends RequestContentValidator { + static class ConfigUpgradeContentValidator extends RequestContentValidator { protected ConfigUpgradeContentValidator(final ValidationContext validationContext) { super(validationContext); @@ -338,4 +316,46 @@ public ValidationResult validate(final RestRequest request, final Json } + /** Tranforms config changes from a raw PATCH into simplier view */ + static class ConfigItemChanges { + + private final CType config; + private final Map> itemsGroupedByOperation; + + public ConfigItemChanges(final CType config, final JsonNode differences) { + this.config = config; + this.itemsGroupedByOperation = classifyChanges(differences); + } + + public boolean hasChanges() { + return !itemsGroupedByOperation.isEmpty(); + } + + public void addToNode(final ObjectNode node) { + final var allOperations = JsonNodeFactory.instance.objectNode(); + itemsGroupedByOperation.forEach((operation, items) -> { + final var arrayNode = allOperations.putArray(operation); + items.forEach(arrayNode::add); + }); + node.set(config.toLCString(), allOperations); + } + + private static Map> classifyChanges(final JsonNode differences) { + final var items = new HashMap(); + differences.forEach(node -> { + final var item = pathRoot(node); + final var operation = node.get("op").asText(); + if (items.containsKey(item) && !items.get(item).equals(operation)) { + items.put(item, "modify"); + } else { + items.put(item, operation); + } + }); + + final var itemsGroupedByOperation = items.entrySet() + .stream() + .collect(Collectors.groupingBy(Map.Entry::getValue, Collectors.mapping(Map.Entry::getKey, Collectors.toList()))); + return itemsGroupedByOperation; + } + } }