Skip to content

Commit

Permalink
Phase 2 support for operator privileges: Cluster settings (#66684)
Browse files Browse the repository at this point in the history
Add a new OperatorDynamic enum to differentiate between operator-only and
regular dynamic cluster settings. The Setting constructor validates that Dynamic 
and OperatorDynamic cannot be both specified. Operator-only settings behave
as the follows:

* When the feature is enabled, operator-only settings cannot be updated  with 
   PUT cluster settings API unless the user is an operator.
* The restore behaviour for operator-only settings will be identical for either
  operator or non-operator user. That is, when operator privileges are enabled,
  operator-only settings will not be restored. Otherwise (if the feature is
  disabled), the behaviour is the same as of today.
  • Loading branch information
ywangd authored Feb 4, 2021
1 parent bc0d37d commit 8e49ba9
Show file tree
Hide file tree
Showing 21 changed files with 607 additions and 58 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,9 @@ public class RestoreSnapshotRequest extends MasterNodeRequest<RestoreSnapshotReq
private Settings indexSettings = EMPTY_SETTINGS;
private String[] ignoreIndexSettings = Strings.EMPTY_ARRAY;

// This field does not get serialised (except toString for debugging purpose) because it is always set locally by authz
private boolean skipOperatorOnlyState = false;

@Nullable // if any snapshot UUID will do
private String snapshotUuid;

Expand Down Expand Up @@ -437,6 +440,14 @@ public String snapshotUuid() {
return snapshotUuid;
}

public boolean skipOperatorOnlyState() {
return skipOperatorOnlyState;
}

public void skipOperatorOnlyState(boolean skipOperatorOnlyState) {
this.skipOperatorOnlyState = skipOperatorOnlyState;
}

/**
* Parses restore definition
*
Expand Down Expand Up @@ -499,6 +510,12 @@ public RestoreSnapshotRequest source(Map<String, Object> source) {
@Override
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
builder.startObject();
toXContentFragment(builder, params);
builder.endObject();
return builder;
}

private void toXContentFragment(XContentBuilder builder, Params params) throws IOException {
builder.startArray("indices");
for (String index : indices) {
builder.value(index);
Expand Down Expand Up @@ -528,8 +545,6 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
builder.value(ignoreIndexSetting);
}
builder.endArray();
builder.endObject();
return builder;
}

@Override
Expand All @@ -554,20 +569,27 @@ public boolean equals(Object o) {
Objects.equals(renameReplacement, that.renameReplacement) &&
Objects.equals(indexSettings, that.indexSettings) &&
Arrays.equals(ignoreIndexSettings, that.ignoreIndexSettings) &&
Objects.equals(snapshotUuid, that.snapshotUuid);
Objects.equals(snapshotUuid, that.snapshotUuid) &&
skipOperatorOnlyState == that.skipOperatorOnlyState;
}

@Override
public int hashCode() {
int result = Objects.hash(snapshot, repository, indicesOptions, renamePattern, renameReplacement, waitForCompletion,
includeGlobalState, partial, includeAliases, indexSettings, snapshotUuid);
includeGlobalState, partial, includeAliases, indexSettings, snapshotUuid, skipOperatorOnlyState);
result = 31 * result + Arrays.hashCode(indices);
result = 31 * result + Arrays.hashCode(ignoreIndexSettings);
return result;
}

@Override
public String toString() {
return Strings.toString(this);
return Strings.toString((ToXContentObject) (builder, params) -> {
builder.startObject();
toXContentFragment(builder, params);
builder.field("skipOperatorOnlyState", skipOperatorOnlyState);
builder.endObject();
return builder;
});
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,11 @@ public enum Property {
*/
Dynamic,

/**
* Operator only Dynamic setting
*/
OperatorDynamic,

/**
* mark this setting as final, not updateable even when the context is not dynamic
* ie. Setting this property on an index scoped setting will fail update when the index is closed
Expand Down Expand Up @@ -157,9 +162,13 @@ private Setting(Key key, @Nullable Setting<T> fallbackSetting, Function<Settings
this.properties = EMPTY_PROPERTIES;
} else {
final EnumSet<Property> propertiesAsSet = EnumSet.copyOf(Arrays.asList(properties));
if (propertiesAsSet.contains(Property.Dynamic) && propertiesAsSet.contains(Property.Final)) {
if ((propertiesAsSet.contains(Property.Dynamic) || propertiesAsSet.contains(Property.OperatorDynamic))
&& propertiesAsSet.contains(Property.Final)) {
throw new IllegalArgumentException("final setting [" + key + "] cannot be dynamic");
}
if (propertiesAsSet.contains(Property.Dynamic) && propertiesAsSet.contains(Property.OperatorDynamic)) {
throw new IllegalArgumentException("setting [" + key + "] cannot be both dynamic and operator dynamic");
}
checkPropertyRequiresIndexScope(propertiesAsSet, Property.NotCopyableOnResize);
checkPropertyRequiresIndexScope(propertiesAsSet, Property.InternalIndex);
checkPropertyRequiresIndexScope(propertiesAsSet, Property.PrivateIndex);
Expand Down Expand Up @@ -284,7 +293,14 @@ public final Key getRawKey() {
* Returns <code>true</code> if this setting is dynamically updateable, otherwise <code>false</code>
*/
public final boolean isDynamic() {
return properties.contains(Property.Dynamic);
return properties.contains(Property.Dynamic) || properties.contains(Property.OperatorDynamic);
}

/**
* Returns <code>true</code> if this setting is dynamically updateable by operators, otherwise <code>false</code>
*/
public final boolean isOperatorOnly() {
return properties.contains(Property.OperatorDynamic);
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@
import org.elasticsearch.common.lucene.Lucene;
import org.elasticsearch.common.regex.Regex;
import org.elasticsearch.common.settings.ClusterSettings;
import org.elasticsearch.common.settings.Setting;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.index.Index;
import org.elasticsearch.index.IndexSettings;
Expand All @@ -81,6 +82,7 @@
import java.util.function.Function;
import java.util.function.Predicate;
import java.util.stream.Collectors;
import java.util.stream.Stream;

import static java.util.Collections.unmodifiableSet;
import static org.elasticsearch.cluster.metadata.IndexMetadata.SETTING_AUTO_EXPAND_REPLICAS;
Expand Down Expand Up @@ -427,6 +429,22 @@ restoreUUID, snapshot, overallState(RestoreInProgress.State.INIT, shards),
if (request.includeGlobalState()) {
if (metadata.persistentSettings() != null) {
Settings settings = metadata.persistentSettings();
if (request.skipOperatorOnlyState()) {
// Skip any operator-only settings from the snapshot. This happens when operator privileges are enabled
Set<String> operatorSettingKeys = Stream.concat(
settings.keySet().stream(), currentState.metadata().persistentSettings().keySet().stream())
.filter(k -> {
final Setting<?> setting = clusterSettings.get(k);
return setting != null && setting.isOperatorOnly();
})
.collect(Collectors.toSet());
if (false == operatorSettingKeys.isEmpty()) {
settings = Settings.builder()
.put(settings.filter(k -> false == operatorSettingKeys.contains(k)))
.put(currentState.metadata().persistentSettings().filter(operatorSettingKeys::contains))
.build();
}
}
clusterSettings.validateUpdate(settings);
mdBuilder.persistentSettings(settings);
}
Expand All @@ -441,6 +459,7 @@ restoreUUID, snapshot, overallState(RestoreInProgress.State.INIT, shards),
if (RepositoriesMetadata.TYPE.equals(cursor.key) == false
&& DataStreamMetadata.TYPE.equals(cursor.key) == false
&& cursor.value instanceof Metadata.NonRestorableCustom == false) {
// TODO: Check request.skipOperatorOnly for Autoscaling policies (NonRestorableCustom)
// Don't restore repositories while we are working with them
// TODO: Should we restore them at the end?
// Also, don't restore data streams here, we already added them to the metadata builder above
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

import org.elasticsearch.action.support.IndicesOptions;
import org.elasticsearch.common.bytes.BytesReference;
import org.elasticsearch.common.io.stream.BytesStreamOutput;
import org.elasticsearch.common.io.stream.Writeable;
import org.elasticsearch.common.xcontent.NamedXContentRegistry;
import org.elasticsearch.common.xcontent.ToXContent;
Expand All @@ -29,6 +30,8 @@
import java.util.List;
import java.util.Map;

import static org.hamcrest.Matchers.containsString;

public class RestoreSnapshotRequestTests extends AbstractWireSerializingTestCase<RestoreSnapshotRequest> {
private RestoreSnapshotRequest randomState(RestoreSnapshotRequest instance) {
if (randomBoolean()) {
Expand Down Expand Up @@ -123,4 +126,37 @@ public void testSource() throws IOException {

assertEquals(original, processed);
}

public void testSkipOperatorOnlyWillNotBeSerialised() throws IOException {
RestoreSnapshotRequest original = createTestInstance();
assertFalse(original.skipOperatorOnlyState()); // default is false
if (randomBoolean()) {
original.skipOperatorOnlyState(true);
}
Map<String, Object> map = convertRequestToMap(original);
// It is not serialised as xcontent
assertFalse(map.containsKey("skip_operator_only"));

// Xcontent is not affected by the value of skipOperatorOnlyState
original.skipOperatorOnlyState(original.skipOperatorOnlyState() == false);
assertEquals(map, convertRequestToMap(original));

// Nor does it serialise to streamInput
final BytesStreamOutput streamOutput = new BytesStreamOutput();
original.writeTo(streamOutput);
final RestoreSnapshotRequest deserialized = new RestoreSnapshotRequest(streamOutput.bytes().streamInput());
assertFalse(deserialized.skipOperatorOnlyState());
}

public void testToStringWillIncludeSkipOperatorOnlyState() {
RestoreSnapshotRequest original = createTestInstance();
assertThat(original.toString(), containsString("skipOperatorOnlyState"));
}

private Map<String, Object> convertRequestToMap(RestoreSnapshotRequest request) throws IOException {
XContentBuilder builder = request.toXContent(XContentFactory.jsonBuilder(), new ToXContent.MapParams(Collections.emptyMap()));
XContentParser parser = XContentType.JSON.xContent().createParser(
NamedXContentRegistry.EMPTY, null, BytesReference.bytes(builder).streamInput());
return parser.mapOrdered();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -939,10 +939,16 @@ public void testRejectNullProperties() {

public void testRejectConflictingDynamicAndFinalProperties() {
IllegalArgumentException ex = expectThrows(IllegalArgumentException.class,
() -> Setting.simpleString("foo.bar", Property.Final, Property.Dynamic));
() -> Setting.simpleString("foo.bar", Property.Final, randomFrom(Property.Dynamic, Property.OperatorDynamic)));
assertThat(ex.getMessage(), containsString("final setting [foo.bar] cannot be dynamic"));
}

public void testRejectConflictingDynamicAndOperatorDynamicProperties() {
IllegalArgumentException ex = expectThrows(IllegalArgumentException.class,
() -> Setting.simpleString("foo.bar", Property.Dynamic, Property.OperatorDynamic));
assertThat(ex.getMessage(), containsString("setting [foo.bar] cannot be both dynamic and operator dynamic"));
}

public void testRejectNonIndexScopedNotCopyableOnResizeSetting() {
final IllegalArgumentException e = expectThrows(
IllegalArgumentException.class,
Expand Down Expand Up @@ -1240,4 +1246,11 @@ public boolean innerMatch(LogEvent event) {
mockLogAppender.stop();
}
}

public void testDynamicTest() {
final Property property = randomFrom(Property.Dynamic, Property.OperatorDynamic);
final Setting<String> setting = Setting.simpleString("foo.bar", property);
assertTrue(setting.isDynamic());
assertEquals(setting.isOperatorOnly(), property == Property.OperatorDynamic);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -416,7 +416,7 @@ public Set<DiscoveryNodeRole> getRoles() {
public static final String MACHINE_MEMORY_NODE_ATTR = "ml.machine_memory";
public static final String MAX_JVM_SIZE_NODE_ATTR = "ml.max_jvm_size";
public static final Setting<Integer> CONCURRENT_JOB_ALLOCATIONS =
Setting.intSetting("xpack.ml.node_concurrent_job_allocations", 2, 0, Property.Dynamic, Property.NodeScope);
Setting.intSetting("xpack.ml.node_concurrent_job_allocations", 2, 0, Property.OperatorDynamic, Property.NodeScope);
/**
* The amount of memory needed to load the ML native code shared libraries. The assumption is that the first
* ML job to run on a given node will do this, and then subsequent ML jobs on the same node will reuse the
Expand All @@ -430,7 +430,7 @@ public Set<DiscoveryNodeRole> getRoles() {
// controls the types of jobs that can be created, and each job alone is considerably smaller than what each node
// can handle.
public static final Setting<Integer> MAX_MACHINE_MEMORY_PERCENT =
Setting.intSetting("xpack.ml.max_machine_memory_percent", 30, 5, 200, Property.Dynamic, Property.NodeScope);
Setting.intSetting("xpack.ml.max_machine_memory_percent", 30, 5, 200, Property.OperatorDynamic, Property.NodeScope);
/**
* This boolean value indicates if `max_machine_memory_percent` should be ignored and a automatic calculation is used instead.
*
Expand All @@ -446,10 +446,10 @@ public Set<DiscoveryNodeRole> getRoles() {
public static final Setting<Boolean> USE_AUTO_MACHINE_MEMORY_PERCENT = Setting.boolSetting(
"xpack.ml.use_auto_machine_memory_percent",
false,
Property.Dynamic,
Property.OperatorDynamic,
Property.NodeScope);
public static final Setting<Integer> MAX_LAZY_ML_NODES =
Setting.intSetting("xpack.ml.max_lazy_ml_nodes", 0, 0, Property.Dynamic, Property.NodeScope);
Setting.intSetting("xpack.ml.max_lazy_ml_nodes", 0, 0, Property.OperatorDynamic, Property.NodeScope);

// Before 8.0.0 this needs to match the max allowed value for xpack.ml.max_open_jobs,
// as the current node could be running in a cluster where some nodes are still using
Expand All @@ -464,7 +464,7 @@ public Set<DiscoveryNodeRole> getRoles() {

public static final Setting<TimeValue> PROCESS_CONNECT_TIMEOUT =
Setting.timeSetting("xpack.ml.process_connect_timeout", TimeValue.timeValueSeconds(10),
TimeValue.timeValueSeconds(5), Setting.Property.Dynamic, Setting.Property.NodeScope);
TimeValue.timeValueSeconds(5), Property.OperatorDynamic, Setting.Property.NodeScope);

// Undocumented setting for integration test purposes
public static final Setting<ByteSizeValue> MIN_DISK_SPACE_OFF_HEAP =
Expand All @@ -483,7 +483,7 @@ public Set<DiscoveryNodeRole> getRoles() {
}
return value;
},
Property.Dynamic,
Property.OperatorDynamic,
Property.NodeScope
);

Expand All @@ -496,7 +496,7 @@ public Set<DiscoveryNodeRole> getRoles() {
public static final Setting<ByteSizeValue> MAX_ML_NODE_SIZE = Setting.byteSizeSetting(
"xpack.ml.max_ml_node_size",
ByteSizeValue.ZERO,
Property.Dynamic,
Property.OperatorDynamic,
Property.NodeScope);

private static final Logger logger = LogManager.getLogger(MachineLearning.class);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
public class MlConfigMigrationEligibilityCheck {

public static final Setting<Boolean> ENABLE_CONFIG_MIGRATION = Setting.boolSetting(
"xpack.ml.enable_config_migration", true, Setting.Property.Dynamic, Setting.Property.NodeScope);
"xpack.ml.enable_config_migration", true, Setting.Property.OperatorDynamic, Setting.Property.NodeScope);

private volatile boolean isConfigMigrationEnabled;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ public class ResultsPersisterService {
20,
0,
50,
Setting.Property.Dynamic,
Setting.Property.OperatorDynamic,
Setting.Property.NodeScope);
private static final int MAX_RETRY_SLEEP_MILLIS = (int)Duration.ofMinutes(15).toMillis();
private static final int MIN_RETRY_SLEEP_MILLIS = 50;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,13 @@ dependencies {
javaRestTestImplementation project.sourceSets.main.runtimeClasspath
}

File repoDir = file("$buildDir/testclusters/repo")

javaRestTest {
/* To support taking snapshots, we have to set path.repo setting */
systemProperty 'tests.path.repo', repoDir
}

testClusters.all {
testDistribution = 'DEFAULT'
numberOfNodes = 3
Expand All @@ -27,6 +34,7 @@ testClusters.all {
setting 'xpack.security.enabled', 'true'
setting 'xpack.security.http.ssl.enabled', 'false'
setting 'xpack.security.operator_privileges.enabled', "true"
setting 'path.repo', repoDir.absolutePath

user username: "test_admin", password: 'x-pack-test-password', role: "superuser"
user username: "test_operator", password: 'x-pack-test-password', role: "limited_operator"
Expand Down
Loading

0 comments on commit 8e49ba9

Please sign in to comment.