Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
50 commits
Select commit Hold shift + click to select a range
cd4e5ee
Fix FileSettingsService hang on error update
Aug 25, 2022
4723726
Initial code for reserved role mappings
Aug 25, 2022
2f79c83
Handle security disabled.
Aug 25, 2022
3cfaa82
Tests and conflict check for role mappings
Aug 26, 2022
e7736b1
Update docs/changelog/89667.yaml
grcevski Aug 26, 2022
e67636d
Add hashcode/equals for deduplication
Aug 26, 2022
39803d4
Merge branch 'operator/role_mapping' of github.com:grcevski/elasticse…
Aug 26, 2022
aa38aa8
Attempt to fix test
Aug 26, 2022
3ecd25b
Merge branch 'operator/fix_error_save' into operator/role_mapping
Aug 26, 2022
ebecdeb
Merge branch 'main' into operator/role_mapping
Aug 29, 2022
6fcf396
Refactor for more code reuse
Aug 29, 2022
524572b
Merge branch 'main' into operator/role_mapping
Aug 29, 2022
005e8f2
Move validation earlier and cluster it
Aug 30, 2022
1791ebf
Merge branch 'main' into operator/role_mapping
Sep 1, 2022
00384cb
Merge branch 'main' into operator/role_mapping
Sep 8, 2022
bdcec01
Allow metadata keys with "_"
Sep 8, 2022
4059718
Apply feedback on the post transform step.
Sep 8, 2022
87b6a69
Fix async error propagation, add tests.
Sep 8, 2022
7835d4e
Async post transformation
Sep 12, 2022
b5b1d52
Merge branch 'main' into operator/role_mapping
Sep 12, 2022
f718fcc
Fix merge issue.
Sep 12, 2022
21d84c2
Fix test
Sep 12, 2022
b2955a9
Merge branch 'main' into operator/role_mapping
Sep 13, 2022
8baa259
Reduce to a single cluster state update
Sep 14, 2022
e327472
WIP: More test code
Sep 14, 2022
f03a67b
Add more unit and integration tests
Sep 14, 2022
5d947a6
Remove stale comment
grcevski Sep 14, 2022
ffacba6
Merge branch 'main' into operator/role_mapping
Sep 15, 2022
390bd1b
Merge branch 'operator/role_mapping' of github.com:grcevski/elasticse…
Sep 15, 2022
83cbdd4
Bring back the dedup in test handler provider
Sep 15, 2022
b8d52a6
Apply nit suggestion
grcevski Sep 15, 2022
48094e2
Change Void to void.
Sep 15, 2022
df7f76c
Merge branch 'operator/role_mapping' of github.com:grcevski/elasticse…
Sep 15, 2022
3889f03
Change prepare to private
Sep 15, 2022
c99d0c3
Add reserved transport action tests.
Sep 15, 2022
04046d5
Add more tests for validation
Sep 15, 2022
9f8b4bc
Merge branch 'main' into operator/role_mapping
Sep 19, 2022
caeb188
Apply suggestion on waiting for future
grcevski Sep 19, 2022
1f2ce32
Use addSuppressed for exception collection
grcevski Sep 19, 2022
af2a181
Merge branch 'operator/role_mapping' of github.com:grcevski/elasticse…
Sep 19, 2022
f2afc9f
Apply code review suggestions.
Sep 19, 2022
fb906d2
Fix compile error.
Sep 19, 2022
796e61d
Add test with closed security
Sep 19, 2022
1964bf7
Fix test to handle empty keys
Sep 19, 2022
d2af580
Fix missing throw.
Sep 19, 2022
d41cd20
Extra assert on null setting.
Sep 19, 2022
e988bde
Merge branch 'main' into operator/role_mapping
Sep 20, 2022
5058f74
Fix compile error from merge
Sep 20, 2022
8ac3287
Fix version bug and test bug.
Sep 20, 2022
cea0668
Spotless
Sep 20, 2022
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 docs/changelog/89667.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
pr: 89667
summary: Operator/role mapping
area: Infra/Core
type: enhancement
issues: []
Original file line number Diff line number Diff line change
Expand Up @@ -75,12 +75,12 @@ protected void masterOperation(
}

@Override
protected Optional<String> reservedStateHandlerName() {
public Optional<String> reservedStateHandlerName() {
return Optional.of(ReservedRepositoryAction.NAME);
}

@Override
protected Set<String> modifiedKeys(DeleteRepositoryRequest request) {
public Set<String> modifiedKeys(DeleteRepositoryRequest request) {
return Set.of(request.name());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -72,12 +72,12 @@ protected void masterOperation(
}

@Override
protected Optional<String> reservedStateHandlerName() {
public Optional<String> reservedStateHandlerName() {
return Optional.of(ReservedRepositoryAction.NAME);
}

@Override
protected Set<String> modifiedKeys(PutRepositoryRequest request) {
public Set<String> modifiedKeys(PutRepositoryRequest request) {
return Set.of(request.name());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -131,12 +131,12 @@ private static boolean checkClearedBlockAndArchivedSettings(
}

@Override
protected Optional<String> reservedStateHandlerName() {
public Optional<String> reservedStateHandlerName() {
return Optional.of(ReservedClusterSettingsAction.NAME);
}

@Override
protected Set<String> modifiedKeys(ClusterUpdateSettingsRequest request) {
public Set<String> modifiedKeys(ClusterUpdateSettingsRequest request) {
Settings allSettings = Settings.builder().put(request.persistentSettings()).put(request.transientSettings()).build();
return allSettings.keySet();
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

package org.elasticsearch.action.support;

import org.elasticsearch.action.ActionListener;
import org.elasticsearch.action.ActionRequest;
import org.elasticsearch.action.ActionResponse;
import org.elasticsearch.cluster.service.ClusterService;
import org.elasticsearch.common.io.stream.Writeable;
import org.elasticsearch.reservedstate.ActionWithReservedState;
import org.elasticsearch.tasks.Task;
import org.elasticsearch.transport.TransportService;

/**
* An extension of the {@link HandledTransportAction} class, which wraps the doExecute call with a check for clashes
* with the reserved cluster state.
*/
public abstract class ReservedStateAwareHandledTransportAction<Request extends ActionRequest, Response extends ActionResponse> extends
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since the role mappings don't use TransportMasterNodeAction (which is cluster state specific) we add this small wrapper class to ensure the reserved state information is checked before allowing REST actions to create/update/delete role mappings.

HandledTransportAction<Request, Response>
implements
ActionWithReservedState<Request> {
private final ClusterService clusterService;

protected ReservedStateAwareHandledTransportAction(
String actionName,
ClusterService clusterService,
TransportService transportService,
ActionFilters actionFilters,
Writeable.Reader<Request> requestReader
) {
super(actionName, transportService, actionFilters, requestReader);
this.clusterService = clusterService;
}

/**
* A doExecute method wrapped with a check for clashes with updates to the reserved cluster state
*/
protected abstract void doExecuteProtected(Task task, Request request, ActionListener<Response> listener);

@Override
protected void doExecute(Task task, Request request, ActionListener<Response> listener) {
assert reservedStateHandlerName().isPresent();

validateForReservedState(clusterService.state(), reservedStateHandlerName().get(), modifiedKeys(request), request.toString());
doExecuteProtected(task, request, listener);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@
import org.elasticsearch.cluster.block.ClusterBlockException;
import org.elasticsearch.cluster.block.ClusterBlockLevel;
import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver;
import org.elasticsearch.cluster.metadata.ReservedStateMetadata;
import org.elasticsearch.cluster.node.DiscoveryNode;
import org.elasticsearch.cluster.node.DiscoveryNodes;
import org.elasticsearch.cluster.service.ClusterService;
Expand All @@ -34,7 +33,7 @@
import org.elasticsearch.gateway.GatewayService;
import org.elasticsearch.index.IndexNotFoundException;
import org.elasticsearch.node.NodeClosedException;
import org.elasticsearch.reservedstate.ReservedClusterStateHandler;
import org.elasticsearch.reservedstate.ActionWithReservedState;
import org.elasticsearch.tasks.CancellableTask;
import org.elasticsearch.tasks.Task;
import org.elasticsearch.tasks.TaskCancelledException;
Expand All @@ -44,11 +43,7 @@
import org.elasticsearch.transport.TransportException;
import org.elasticsearch.transport.TransportService;

import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.Optional;
import java.util.Set;
import java.util.function.Predicate;

import static org.elasticsearch.core.Strings.format;
Expand All @@ -57,7 +52,9 @@
* A base class for operations that needs to be performed on the master node.
*/
public abstract class TransportMasterNodeAction<Request extends MasterNodeRequest<Request>, Response extends ActionResponse> extends
HandledTransportAction<Request, Response> {
HandledTransportAction<Request, Response>
implements
ActionWithReservedState<Request> {

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

Expand Down Expand Up @@ -149,65 +146,24 @@ private ClusterBlockException checkBlockIfStateRecovered(Request request, Cluste
}
}

/**
* Override this method if the master node action also has an {@link ReservedClusterStateHandler}
* interaction.
* <p>
* We need to check if certain settings or entities are allowed to be modified by the master node
* action, depending on if they are set as reserved in 'operator' mode (file based settings, modules, plugins).
*
* @return an Optional of the {@link ReservedClusterStateHandler} name
*/
protected Optional<String> reservedStateHandlerName() {
return Optional.empty();
}

/**
* Override this method to return the keys of the cluster state or cluster entities that are modified by
* the Request object.
* <p>
* This method is used by the reserved state handler logic (see {@link ReservedClusterStateHandler})
* to verify if the keys don't conflict with an existing key set as reserved.
*
* @param request the TransportMasterNode request
* @return set of String keys intended to be modified/set/deleted by this request
*/
protected Set<String> modifiedKeys(Request request) {
return Collections.emptySet();
}

// package private for testing
void validateForImmutableState(Request request, ClusterState state) {
void validateForReservedState(Request request, ClusterState state) {
Optional<String> handlerName = reservedStateHandlerName();
assert handlerName.isPresent();

Set<String> modified = modifiedKeys(request);
List<String> errors = new ArrayList<>();

for (ReservedStateMetadata metadata : state.metadata().reservedStateMetadata().values()) {
Set<String> conflicts = metadata.conflicts(handlerName.get(), modified);
if (conflicts.isEmpty() == false) {
errors.add(format("[%s] set as read-only by [%s]", String.join(", ", conflicts), metadata.namespace()));
}
}

if (errors.isEmpty() == false) {
throw new IllegalArgumentException(
format("Failed to process request [%s] with errors: [%s]", request, String.join(", ", errors))
);
}
validateForReservedState(state, handlerName.get(), modifiedKeys(request), request.toString());
}

// package private for testing
boolean supportsImmutableState() {
boolean supportsReservedState() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are simple renames, which were somehow missed before. We did rename from immutable to reserved.

return reservedStateHandlerName().isPresent();
}

@Override
protected void doExecute(Task task, final Request request, ActionListener<Response> listener) {
ClusterState state = clusterService.state();
if (supportsImmutableState()) {
validateForImmutableState(request, state);
if (supportsReservedState()) {
validateForReservedState(request, state);
}
logger.trace("starting processing request [{}] with cluster state version [{}]", request, state.version());
if (task != null) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -283,6 +283,13 @@ public Builder putHandler(ReservedStateHandlerMetadata handler) {
return this;
}

/**
* Returns the current handler metadata stored in the builder
*/
public ReservedStateHandlerMetadata getHandler(String handlerName) {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: maybe this is better named getHandlerMetadata?

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'll address this in a followup, it will increase the size of this PR because the method is used in many tests.

return this.handlers.get(handlerName);
}

/**
* Builds an {@link ReservedStateMetadata} from this builder.
*
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

package org.elasticsearch.reservedstate;

import org.elasticsearch.cluster.ClusterState;
import org.elasticsearch.cluster.metadata.ReservedStateMetadata;

import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.Optional;
import java.util.Set;

import static org.elasticsearch.core.Strings.format;

public interface ActionWithReservedState<T> {
/**
* Override this method if the master node action also has an {@link ReservedClusterStateHandler}
* interaction.
* <p>
* We need to check if certain settings or entities are allowed to be modified by the master node
* action, depending on if they are set as reserved in 'operator' mode (file based settings, modules, plugins).
*
* @return an Optional of the {@link ReservedClusterStateHandler} name
*/
default Optional<String> reservedStateHandlerName() {
return Optional.empty();
}

/**
* Override this method to return the keys of the cluster state or cluster entities that are modified by
* the Request object.
* <p>
* This method is used by the reserved state handler logic (see {@link ReservedClusterStateHandler})
* to verify if the keys don't conflict with an existing key set as reserved.
*
* @param request the TransportMasterNode request
* @return set of String keys intended to be modified/set/deleted by this request
*/
default Set<String> modifiedKeys(T request) {
return Collections.emptySet();
}

/**
* Helper method that verifies for key clashes on reserved state updates
* @param state the current cluster state
* @param handlerName the name of the reserved state handler related to this implementation
* @param modified the set of modified keys by the related request
* @param request a string representation of the request for error reporting purposes
*/
default void validateForReservedState(ClusterState state, String handlerName, Set<String> modified, String request) {
List<String> errors = new ArrayList<>();

for (ReservedStateMetadata metadata : state.metadata().reservedStateMetadata().values()) {
Set<String> conflicts = metadata.conflicts(handlerName, modified);
if (conflicts.isEmpty() == false) {
errors.add(format("[%s] set as read-only by [%s]", String.join(", ", conflicts), metadata.namespace()));
}
}

if (errors.isEmpty() == false) {
throw new IllegalArgumentException(
format("Failed to process request [%s] with errors: [%s]", request, String.join(", ", errors))
);
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

package org.elasticsearch.reservedstate;

import java.util.Set;

/**
* A wrapper class for notifying listeners on non cluster state transformation operation completion.
* <p>
* Certain {@link ReservedClusterStateHandler} implementations may need to perform additional
* operations other than modifying the cluster state. This can range from cache
* invalidation to implementing state handlers that do not write to the cluster state, e.g. role mappings.
* These additional transformation steps are implemented as separate async operation after the validation of
* the cluster state update steps (trial run in {@link org.elasticsearch.reservedstate.service.ReservedClusterStateService}).
*/
public record NonStateTransformResult(String handlerName, Set<String> updatedKeys) {}
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,24 @@

package org.elasticsearch.reservedstate;

import org.elasticsearch.action.ActionListener;
import org.elasticsearch.cluster.ClusterState;

import java.util.Set;
import java.util.function.Consumer;

/**
* A {@link ClusterState} wrapper used by the ReservedClusterStateService to pass the
* current state as well as previous keys set by an {@link ReservedClusterStateHandler} to each transform
* step of the cluster state update.
*
* Each {@link ReservedClusterStateHandler} can also provide a non cluster state transform consumer that should run after
* the cluster state is fully validated. This allows for handlers to perform extra steps, like clearing caches or saving
* other state outside the cluster state. The consumer, if provided, must return a {@link NonStateTransformResult} with
* the keys that will be saved as reserved in the cluster state.
*/
public record TransformState(ClusterState state, Set<String> keys) {}
public record TransformState(ClusterState state, Set<String> keys, Consumer<ActionListener<NonStateTransformResult>> nonStateTransform) {
public TransformState(ClusterState state, Set<String> keys) {
this(state, keys, null);
}
}
Loading