Skip to content

Move Security to use auto-managed system indices #67114

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

Merged
merged 15 commits into from
Feb 2, 2021
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,17 @@ public ClusterState execute(ClusterState currentState) throws Exception {
}

final SystemIndexDescriptor descriptor = systemIndices.findMatchingDescriptor(indexName);
CreateIndexClusterStateUpdateRequest updateRequest = descriptor != null && descriptor.isAutomaticallyManaged()
final boolean isSystemIndex = descriptor != null && descriptor.isAutomaticallyManaged();

if (isSystemIndex && state.nodes().getMaxNodeVersion().after(state.nodes().getMinNodeVersion())) {
Copy link
Member

Choose a reason for hiding this comment

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

This concerns me if we cannot auto create a system index in a mixed version cluster even if the descriptor itself does not differ between versions. I think we should allow for creation still. I wonder if we should consider an approach to pull the most up to date version of the descriptor and apply those when creating the index; however there may be cases where that would fail if a feature was used that couldn't be validated on the master. In those cases I would consider falling back to the master version and allowing the master to update the mappings?

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 don't think we can pull the most up-to-date version of the descriptor into the master, at least not without building infrastructure to make it pull-able.

How about: in the auto-create case, we always use the master's descriptor. If we release some new feature that needs to create a system index and relies on particular features being present, perhaps it can explicitly create the index using the right settings, either waiting until the cluster is in the right state, or retrying until the creation is successful. Admittedly this is just shifting the problem, but the general system index infra can't know at the moment whether it's OK to attempt creation in a mixed cluster.

I suppose we could extend the descriptor to apply a version range? e.g.

SystemIndexDescriptor.builder().setMinimimVersion(Version.V_8_0_0)

Then the auto-create, explicit create, mappings and settings methods can check the minimum descriptor version against state.nodes().getMinNodeVersion(). Code that relied on new features would still need to concern themselves with whether it was possible yet to create their system index. If that case becomes common/annoying enough, we can build more support infra.

I think I still want the SystemIndexManager to hold off upgrading mappings in a mixed cluster - partly in case of a failed upgrade, and partly to avoid disagreements around mappings versions. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

the general system index infra can't know at the moment whether it's OK to attempt creation in a mixed cluster

I think this is the crux, and we have to delegate this responsibility to the caller (eg Security(IndexManager)). My suggestion to fail the creation in the mixed cluster scenario was the simplest, in a sense, because the caller doesn't have to guard the system document index call with anything. But I haven't really considered the practical aspect that much more upgrades are done than changes to the system index's mapping. Moreover, most mapping changes are focused on a small portion of the total mapping. Preventing the creation of the system index in all these cases is wasteful, as the creation with the old mapping would be OK.

So, if the decision is to not reject system index creation (or mapping updates) in a mixed cluster scenario (whatever the strategy ultimately is), when indexing a document relying on new mappings (eg a new type of API key) we have to look at the cluster state, and probably reject the indexing (there could be other options, depending on the particular case). I think that's OK; I hoped we could avoid it, but not creating any index in the other cases, is probably not worth it.

final String messsage = "Cannot auto-create system index ["
+ descriptor.getPrimaryIndex()
+ "] in a mixed-version cluster";
logger.warn(messsage);
throw new IllegalStateException(messsage);
}

CreateIndexClusterStateUpdateRequest updateRequest = isSystemIndex
? buildSystemIndexUpdateRequest(descriptor)
: buildUpdateRequest(indexName);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@

package org.elasticsearch.action.admin.indices.create;

import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.elasticsearch.action.ActionListener;
import org.elasticsearch.action.admin.indices.alias.Alias;
import org.elasticsearch.action.support.ActionFilters;
Expand All @@ -45,6 +47,7 @@
* Create index action.
*/
public class TransportCreateIndexAction extends TransportMasterNodeAction<CreateIndexRequest, CreateIndexResponse> {
private static final Logger logger = LogManager.getLogger(TransportCreateIndexAction.class);

private final MetadataCreateIndexService createIndexService;
private final SystemIndices systemIndices;
Expand Down Expand Up @@ -76,7 +79,16 @@ protected void masterOperation(Task task, final CreateIndexRequest request, fina
final String indexName = indexNameExpressionResolver.resolveDateMathExpression(request.index());

final SystemIndexDescriptor descriptor = systemIndices.findMatchingDescriptor(indexName);
final CreateIndexClusterStateUpdateRequest updateRequest = descriptor != null && descriptor.isAutomaticallyManaged()
final boolean isSystemIndex = descriptor != null && descriptor.isAutomaticallyManaged();

if (isSystemIndex && state.nodes().getMaxNodeVersion().after(state.nodes().getMinNodeVersion())) {
final String message = "Cannot create system index [" + descriptor.getPrimaryIndex() + "] in a mixed-version cluster";
logger.warn(message);
listener.onFailure(new IllegalStateException(message));
return;
}

final CreateIndexClusterStateUpdateRequest updateRequest = isSystemIndex
? buildSystemIndexUpdateRequest(request, cause, descriptor)
: buildUpdateRequest(request, cause, indexName);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@
*/
package org.elasticsearch.action.admin.indices.mapping.put;

import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.elasticsearch.action.ActionListener;
import org.elasticsearch.action.support.ActionFilters;
import org.elasticsearch.action.support.master.AcknowledgedResponse;
Expand All @@ -30,6 +32,7 @@
import org.elasticsearch.cluster.service.ClusterService;
import org.elasticsearch.common.inject.Inject;
import org.elasticsearch.index.Index;
import org.elasticsearch.indices.SystemIndices;
import org.elasticsearch.tasks.Task;
import org.elasticsearch.threadpool.ThreadPool;
import org.elasticsearch.transport.TransportService;
Expand All @@ -38,7 +41,10 @@

public class TransportAutoPutMappingAction extends AcknowledgedTransportMasterNodeAction<PutMappingRequest> {

private static final Logger logger = LogManager.getLogger(TransportAutoPutMappingAction.class);

private final MetadataMappingService metadataMappingService;
private final SystemIndices systemIndices;

@Inject
public TransportAutoPutMappingAction(
Expand All @@ -47,10 +53,12 @@ public TransportAutoPutMappingAction(
final ThreadPool threadPool,
final MetadataMappingService metadataMappingService,
final ActionFilters actionFilters,
final IndexNameExpressionResolver indexNameExpressionResolver) {
final IndexNameExpressionResolver indexNameExpressionResolver,
final SystemIndices systemIndices) {
super(AutoPutMappingAction.NAME, transportService, clusterService, threadPool, actionFilters,
PutMappingRequest::new, indexNameExpressionResolver, ThreadPool.Names.SAME);
this.metadataMappingService = metadataMappingService;
this.systemIndices = systemIndices;
}

@Override
Expand All @@ -72,6 +80,14 @@ protected ClusterBlockException checkBlock(PutMappingRequest request, ClusterSta
protected void masterOperation(Task task, final PutMappingRequest request, final ClusterState state,
final ActionListener<AcknowledgedResponse> listener) {
final Index[] concreteIndices = new Index[] {request.getConcreteIndex()};

final String message = TransportPutMappingAction.checkForSystemIndexViolations(systemIndices, concreteIndices, request);
if (message != null) {
logger.warn(message);
listener.onFailure(new IllegalStateException(message));
return;
}

performMappingUpdate(concreteIndices, request, listener, metadataMappingService);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver;
import org.elasticsearch.cluster.metadata.MetadataMappingService;
import org.elasticsearch.cluster.service.ClusterService;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.inject.Inject;
import org.elasticsearch.index.Index;
import org.elasticsearch.index.IndexNotFoundException;
Expand Down Expand Up @@ -92,22 +93,17 @@ protected void masterOperation(Task task, final PutMappingRequest request, final
final ActionListener<AcknowledgedResponse> listener) {
try {
final Index[] concreteIndices = resolveIndices(state, request, indexNameExpressionResolver);
final String mappingSource = request.source();

final Optional<Exception> maybeValidationException = requestValidators.validateRequest(request, state, concreteIndices);
if (maybeValidationException.isPresent()) {
listener.onFailure(maybeValidationException.get());
return;
}

final List<String> violations = checkForSystemIndexViolations(concreteIndices, mappingSource);
if (violations.isEmpty() == false) {
final String message = "Cannot update mappings in "
+ violations
+ ": system indices can only use mappings from their descriptors, "
+ "but the mappings in the request did not match those in the descriptors(s)";
final String message = checkForSystemIndexViolations(systemIndices, concreteIndices, request);
if (message != null) {
logger.warn(message);
listener.onFailure(new IllegalArgumentException(message));
listener.onFailure(new IllegalStateException(message));
return;
}

Expand Down Expand Up @@ -160,21 +156,36 @@ public void onFailure(Exception t) {
});
}

private List<String> checkForSystemIndexViolations(Index[] concreteIndices, String requestMappings) {
static String checkForSystemIndexViolations(SystemIndices systemIndices, Index[] concreteIndices, PutMappingRequest request) {
// Requests that a cluster generates itself are permitted to have a difference in mappings
// so that rolling upgrade scenarios still work. We check this via the request's origin.
if (Strings.isNullOrEmpty(request.origin()) == false) {
return null;
}

List<String> violations = new ArrayList<>();

final String requestMappings = request.source();

for (Index index : concreteIndices) {
final SystemIndexDescriptor descriptor = systemIndices.findMatchingDescriptor(index.getName());
if (descriptor != null && descriptor.isAutomaticallyManaged()) {
final String descriptorMappings = descriptor.getMappings();

// Technically we could trip over a difference in whitespace here, but then again nobody should be trying to manually
// update a descriptor's mappings.
if (descriptorMappings.equals(requestMappings) == false) {
violations.add(index.getName());
}
}
}
return violations;

if (violations.isEmpty() == false) {
return "Cannot update mappings in "
+ violations
+ ": system indices can only use mappings from their descriptors, "
+ "but the mappings in the request did not match those in the descriptors(s)";
}

return null;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,6 @@

package org.elasticsearch.action.admin.indices.settings.put;

import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.stream.Collectors;

import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.apache.logging.log4j.message.ParameterizedMessage;
Expand All @@ -40,6 +33,7 @@
import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver;
import org.elasticsearch.cluster.metadata.MetadataUpdateSettingsService;
import org.elasticsearch.cluster.service.ClusterService;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.inject.Inject;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.index.Index;
Expand All @@ -49,6 +43,13 @@
import org.elasticsearch.threadpool.ThreadPool;
import org.elasticsearch.transport.TransportService;

import java.util.ArrayList;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.stream.Collectors;

public class TransportUpdateSettingsAction extends AcknowledgedTransportMasterNodeAction<UpdateSettingsRequest> {

private static final Logger logger = LogManager.getLogger(TransportUpdateSettingsAction.class);
Expand Down Expand Up @@ -90,16 +91,15 @@ protected void masterOperation(Task task, final UpdateSettingsRequest request, f
final Index[] concreteIndices = indexNameExpressionResolver.concreteIndices(state, request);
final Settings requestSettings = request.settings();


final Map<String, List<String>> systemIndexViolations = checkForSystemIndexViolations(concreteIndices, requestSettings);
final Map<String, List<String>> systemIndexViolations = checkForSystemIndexViolations(concreteIndices, request);
if (systemIndexViolations.isEmpty() == false) {
final String message = "Cannot override settings on system indices: "
+ systemIndexViolations.entrySet()
.stream()
.map(entry -> "[" + entry.getKey() + "] -> " + entry.getValue())
.collect(Collectors.joining(", "));
logger.warn(message);
listener.onFailure(new IllegalArgumentException(message));
listener.onFailure(new IllegalStateException(message));
return;
}

Expand Down Expand Up @@ -129,11 +129,18 @@ public void onFailure(Exception t) {
* that the system index's descriptor expects.
*
* @param concreteIndices the indices being updated
* @param requestSettings the settings to be applied
* @param request the update request
* @return a mapping from system index pattern to the settings whose values would be overridden. Empty if there are no violations.
*/
private Map<String, List<String>> checkForSystemIndexViolations(Index[] concreteIndices, Settings requestSettings) {
final Map<String, List<String>> violations = new HashMap<>();
private Map<String, List<String>> checkForSystemIndexViolations(Index[] concreteIndices, UpdateSettingsRequest request) {
// Requests that a cluster generates itself are permitted to have a difference in settings
// so that rolling upgrade scenarios still work. We check this via the request's origin.
if (Strings.isNullOrEmpty(request.origin()) == false) {
return Map.of();
}

final Map<String, List<String>> violationsByIndex = new HashMap<>();
final Settings requestSettings = request.settings();

for (Index index : concreteIndices) {
final SystemIndexDescriptor descriptor = systemIndices.findMatchingDescriptor(index.getName());
Expand All @@ -150,10 +157,11 @@ private Map<String, List<String>> checkForSystemIndexViolations(Index[] concrete
}

if (failedKeys.isEmpty() == false) {
violations.put(descriptor.getIndexPattern(), failedKeys);
violationsByIndex.put(descriptor.getIndexPattern(), failedKeys);
}
}
}
return violations;

return violationsByIndex;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

package org.elasticsearch.action.admin.indices.settings.put;

import org.elasticsearch.Version;
import org.elasticsearch.action.ActionRequestValidationException;
import org.elasticsearch.action.IndicesRequest;
import org.elasticsearch.action.support.IndicesOptions;
Expand Down Expand Up @@ -53,13 +54,17 @@ public class UpdateSettingsRequest extends AcknowledgedRequest<UpdateSettingsReq
private IndicesOptions indicesOptions = IndicesOptions.fromOptions(false, false, true, true);
private Settings settings = EMPTY_SETTINGS;
private boolean preserveExisting = false;
private String origin = "";

public UpdateSettingsRequest(StreamInput in) throws IOException {
super(in);
indices = in.readStringArray();
indicesOptions = IndicesOptions.readIndicesOptions(in);
settings = readSettingsFromStream(in);
preserveExisting = in.readBoolean();
if (in.getVersion().onOrBefore(Version.V_8_0_0)) {
origin = in.readString();
}
}

public UpdateSettingsRequest() {
Expand Down Expand Up @@ -171,13 +176,25 @@ public UpdateSettingsRequest settings(Map<String, ?> source) {
return this;
}

public String origin() {
return origin;
}

public UpdateSettingsRequest origin(String origin) {
this.origin = Objects.requireNonNull(origin, "origin cannot be null");
return this;
}

@Override
public void writeTo(StreamOutput out) throws IOException {
super.writeTo(out);
out.writeStringArrayNullable(indices);
indicesOptions.writeIndicesOptions(out);
writeSettingsToStream(settings, out);
out.writeBoolean(preserveExisting);
if (out.getVersion().onOrBefore(Version.V_8_0_0)) {
out.writeString(origin);
}
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ public void clusterChanged(ClusterChangedEvent event) {
if (state.blocks().hasGlobalBlock(GatewayService.STATE_NOT_RECOVERED_BLOCK)) {
// wait until the gateway has recovered from disk, otherwise we may think we don't have some
// indices but they may not have been restored from the cluster state on disk
logger.debug("system indices manager waiting until state has been recovered");
logger.debug("Waiting until state has been recovered");
return;
}

Expand All @@ -84,6 +84,11 @@ public void clusterChanged(ClusterChangedEvent event) {
return;
}

if (state.nodes().getMaxNodeVersion().after(state.nodes().getMinNodeVersion())) {
logger.debug("Skipping system indices up-to-date check as cluster has mixed versions");
return;
}

if (isUpgradeInProgress.compareAndSet(false, true)) {
final List<SystemIndexDescriptor> descriptors = getEligibleDescriptors(state.getMetadata()).stream()
.filter(descriptor -> getUpgradeStatus(state, descriptor) == UpgradeStatus.NEEDS_MAPPINGS_UPDATE)
Expand All @@ -101,6 +106,8 @@ public void clusterChanged(ClusterChangedEvent event) {
} else {
isUpgradeInProgress.set(false);
}
} else {
logger.trace("Update already in progress");
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1587,7 +1587,7 @@ clusterService, indicesService, threadPool, shardStateAction, mappingUpdatedActi
new SystemIndices(Map.of())));
actions.put(AutoPutMappingAction.INSTANCE,
new TransportAutoPutMappingAction(transportService, clusterService, threadPool, metadataMappingService,
actionFilters, indexNameExpressionResolver));
actionFilters, indexNameExpressionResolver, systemIndices));
final ResponseCollectorService responseCollectorService = new ResponseCollectorService(clusterService);
final SearchTransportService searchTransportService = new SearchTransportService(transportService, client,
SearchExecutionStatsCollector.makeWrapper(responseCollectorService));
Expand Down
Loading