Skip to content

Commit

Permalink
Removed the setting plugins.security.privileges_evaluation.precompute…
Browse files Browse the repository at this point in the history 
…d_privileges.include_indices

See discussion in opensearch-project#4380 (comment)

Signed-off-by: Nils Bandener <nils.bandener@eliatra.com>
  • Loading branch information
@nibix
nibix committed Dec 30, 2024
1 parent 6f3a7ea commit 929f07e
Show file tree
Hide file tree
Showing 3 changed files with 7 additions and 64 deletions.
30 changes: 3 additions & 27 deletions src/integrationTest/java/org/opensearch/security/privileges/ActionPrivilegesTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@
import org.opensearch.security.securityconf.impl.CType;
import org.opensearch.security.securityconf.impl.SecurityDynamicConfiguration;
import org.opensearch.security.securityconf.impl.v7.RoleV7;
import org.opensearch.security.support.WildcardMatcher;
import org.opensearch.security.user.User;
import org.opensearch.security.util.MockIndexMetadataBuilder;

Expand Down Expand Up @@ -784,7 +783,7 @@ public void relevantOnly_identity() throws Exception {

assertTrue(
"relevantOnly() returned identical object",
ActionPrivileges.StatefulIndexPrivileges.relevantOnly(metadata, null) == metadata
ActionPrivileges.StatefulIndexPrivileges.relevantOnly(metadata) == metadata
);
}

Expand All @@ -798,7 +797,7 @@ public void relevantOnly_closed() throws Exception {
assertNotNull("Original metadata contains index_open_1", metadata.get("index_open_1"));
assertNotNull("Original metadata contains index_closed", metadata.get("index_closed"));

Map<String, IndexAbstraction> filteredMetadata = ActionPrivileges.StatefulIndexPrivileges.relevantOnly(metadata, null);
Map<String, IndexAbstraction> filteredMetadata = ActionPrivileges.StatefulIndexPrivileges.relevantOnly(metadata);

assertNotNull("Filtered metadata contains index_open_1", filteredMetadata.get("index_open_1"));
assertNull("Filtered metadata does not contain index_closed", filteredMetadata.get("index_closed"));
Expand All @@ -811,35 +810,12 @@ public void relevantOnly_dataStreamBackingIndices() throws Exception {
assertNotNull("Original metadata contains backing index", metadata.get(".ds-data_stream_1-000001"));
assertNotNull("Original metadata contains data stream", metadata.get("data_stream_1"));

Map<String, IndexAbstraction> filteredMetadata = ActionPrivileges.StatefulIndexPrivileges.relevantOnly(metadata, null);
Map<String, IndexAbstraction> filteredMetadata = ActionPrivileges.StatefulIndexPrivileges.relevantOnly(metadata);

assertNull("Filtered metadata does not contain backing index", filteredMetadata.get(".ds-data_stream_1-000001"));
assertNotNull("Filtered metadata contains data stream", filteredMetadata.get("data_stream_1"));
}

@Test
public void relevantOnly_includePattern() throws Exception {
Map<String, IndexAbstraction> metadata = //
indices("index_a11", "index_a12", "index_b1")//
.alias("alias_a")
.of("index_a11")//
.build()
.getIndicesLookup();

assertNotNull("Original metadata contains index_a11", metadata.get("index_a11"));
assertNotNull("Original metadata contains index_b1", metadata.get("index_b1"));
assertNotNull("Original metadata contains alias_a", metadata.get("alias_a"));

Map<String, IndexAbstraction> filteredMetadata = ActionPrivileges.StatefulIndexPrivileges.relevantOnly(
metadata,
WildcardMatcher.from("index_a*", "alias_a*")
);

assertNotNull("Filtered metadata contains index_a11", filteredMetadata.get("index_a11"));
assertNull("Filtered metadata does not contain index_b1", filteredMetadata.get("index_b1"));
assertNotNull("Filtered metadata contains alias_a", filteredMetadata.get("alias_a"));
}

@Test
public void backingIndexToDataStream() {
Map<String, IndexAbstraction> metadata = indices("index").dataStream("data_stream").build().getIndicesLookup();
Expand Down
1 change: 0 additions & 1 deletion src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2062,7 +2062,6 @@ public List<Setting<?>> getSettings() {

// Privileges evaluation
settings.add(ActionPrivileges.PRECOMPUTED_PRIVILEGES_MAX_HEAP_SIZE);
settings.add(ActionPrivileges.PRECOMPUTED_PRIVILEGES_INCLUDE_INDICES);
}

return settings;
Expand Down
40 changes: 4 additions & 36 deletions src/main/java/org/opensearch/security/privileges/ActionPrivileges.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
import java.util.function.Supplier;
import java.util.stream.Collectors;

import com.google.common.base.Strings;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import org.apache.commons.collections4.CollectionUtils;
Expand Down Expand Up @@ -64,29 +63,13 @@ public class ActionPrivileges extends ClusterStateMetadataDependentPrivileges {
* This settings defaults to 10 MB. This is a generous limit. Experiments have shown that an example setup with
* 10,000 indices and 1,000 roles requires about 1 MB of heap. 100,000 indices and 100 roles require about 9 MB of heap.
* (Of course, these numbers can vary widely based on the actual role configuration).
* <p>
* The setting plugins.security.privileges_evaluation.precomputed_privileges.include_indices can be used to control
* for which indices the precomputed privileges shall be created. This allows to lower the heap utilization.
*/
public static Setting<ByteSizeValue> PRECOMPUTED_PRIVILEGES_MAX_HEAP_SIZE = Setting.memorySizeSetting(
"plugins.security.privileges_evaluation.precomputed_privileges.max_heap_size",
new ByteSizeValue(10, ByteSizeUnit.MB),
Setting.Property.NodeScope
);

/**
* Determines the indices which shall be included in the precomputed index privileges. Included indices get
* the fasted privilege evaluation.
* <p>
* You can use patterns like "index_*".
* <p>
* Defaults to all indices.
*/
public static Setting<String> PRECOMPUTED_PRIVILEGES_INCLUDE_INDICES = Setting.simpleString(
"plugins.security.privileges_evaluation.precomputed_privileges.include_indices",
Setting.Property.NodeScope
);

private static final Logger log = LogManager.getLogger(ActionPrivileges.class);

private final ClusterPrivileges cluster;
Expand All @@ -97,7 +80,6 @@ public class ActionPrivileges extends ClusterStateMetadataDependentPrivileges {
private final ImmutableSet<String> wellKnownIndexActions;
private final Supplier<Map<String, IndexAbstraction>> indexMetadataSupplier;
private final ByteSizeValue statefulIndexMaxHeapSize;
private final WildcardMatcher statefulIndexIncludeIndices;

private final AtomicReference<StatefulIndexPrivileges> statefulIndex = new AtomicReference<>();

Expand All @@ -118,10 +100,6 @@ public ActionPrivileges(
this.wellKnownIndexActions = wellKnownIndexActions;
this.indexMetadataSupplier = indexMetadataSupplier;
this.statefulIndexMaxHeapSize = PRECOMPUTED_PRIVILEGES_MAX_HEAP_SIZE.get(settings);
String statefulIndexIncludeIndices = PRECOMPUTED_PRIVILEGES_INCLUDE_INDICES.get(settings);
this.statefulIndexIncludeIndices = Strings.isNullOrEmpty(statefulIndexIncludeIndices)
? null
: WildcardMatcher.from(statefulIndexIncludeIndices);
}

public ActionPrivileges(
Expand Down Expand Up @@ -241,7 +219,7 @@ public PrivilegesEvaluatorResponse hasExplicitIndexPrivilege(
void updateStatefulIndexPrivileges(Map<String, IndexAbstraction> indices, long metadataVersion) {
StatefulIndexPrivileges statefulIndex = this.statefulIndex.get();

indices = StatefulIndexPrivileges.relevantOnly(indices, statefulIndexIncludeIndices);
indices = StatefulIndexPrivileges.relevantOnly(indices);

if (statefulIndex == null || !statefulIndex.indices.equals(indices)) {
long start = System.currentTimeMillis();
Expand Down Expand Up @@ -1004,10 +982,9 @@ static class StatefulIndexPrivileges {
.getEstimatedByteSize() > statefulIndexMaxHeapSize.getBytes()) {
log.info(
"Size of precomputed index privileges exceeds configured limit ({}). Using capped data structure."
+ "This might lead to slightly lower performance during privilege evaluation. Consider raising {} or limiting the performance critical indices using {}.",
+ "This might lead to slightly lower performance during privilege evaluation. Consider raising {}.",
statefulIndexMaxHeapSize,
PRECOMPUTED_PRIVILEGES_MAX_HEAP_SIZE.getKey(),
PRECOMPUTED_PRIVILEGES_INCLUDE_INDICES.getKey()
PRECOMPUTED_PRIVILEGES_MAX_HEAP_SIZE.getKey()
);
break top;
}
Expand Down Expand Up @@ -1125,16 +1102,11 @@ static String backingIndexToDataStream(String index, Map<String, IndexAbstractio
* <li>Indices which are not matched by includeIndices
* </ul>
*/
static Map<String, IndexAbstraction> relevantOnly(Map<String, IndexAbstraction> indices, WildcardMatcher includeIndices) {
static Map<String, IndexAbstraction> relevantOnly(Map<String, IndexAbstraction> indices) {
// First pass: Check if we need to filter at all
boolean doFilter = false;

for (IndexAbstraction indexAbstraction : indices.values()) {
if (includeIndices != null && !includeIndices.test(indexAbstraction.getName())) {
doFilter = true;
break;
}

if (indexAbstraction instanceof IndexAbstraction.Index) {
if (indexAbstraction.getParentDataStream() != null
|| indexAbstraction.getWriteIndex().getState() == IndexMetadata.State.CLOSE) {
Expand All @@ -1152,10 +1124,6 @@ static Map<String, IndexAbstraction> relevantOnly(Map<String, IndexAbstraction>
ImmutableMap.Builder<String, IndexAbstraction> builder = ImmutableMap.builder();

for (IndexAbstraction indexAbstraction : indices.values()) {
if (includeIndices != null && !includeIndices.test(indexAbstraction.getName())) {
continue;
}

if (indexAbstraction instanceof IndexAbstraction.Index) {
if (indexAbstraction.getParentDataStream() == null
&& indexAbstraction.getWriteIndex().getState() != IndexMetadata.State.CLOSE) {
Expand Down

0 comments on commit 929f07e

Please sign in to comment.