Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Initialize api tokens index if it doesn't exist #4899

Closed
wants to merge 8 commits into from
Closed

Initialize api tokens index if it doesn't exist #4899

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
55f184f
Initialize api tokens index if it doesn't exist
derek-ho Nov 13, 2024
b8adfb1
Clean up
derek-ho Nov 13, 2024
2ef69d1
Add test and cleanup
derek-ho Nov 13, 2024
fc13790
PR review
derek-ho Nov 13, 2024
95c390e
Merge branch 'feature/api-tokens' of github.com:opensearch-project/se…
derek-ho Nov 14, 2024
267cfd3
Return completablefuture
derek-ho Nov 14, 2024
ac154fb
Always return true
derek-ho Nov 14, 2024
1245561
Spin up new thread
derek-ho Nov 14, 2024
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
1 change: 1 addition & 0 deletions src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2064,6 +2064,7 @@ public void onNodeStarted(DiscoveryNode localNode) {
this.localNode.set(localNode);
if (!SSLConfig.isSslOnlyMode() && !client && !disabled && !useClusterStateToInitSecurityConfig(settings)) {
cr.initOnNodeStart();
cr.createApiTokenIndex();
}
final Set<ModuleInfo> securityModules = ReflectionHelper.getModulesLoaded();
log.info("{} OpenSearch Security modules loaded so far: {}", securityModules.size(), securityModules);
Expand Down
51 changes: 41 additions & 10 deletions src/main/java/org/opensearch/security/configuration/ConfigurationRepository.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
import java.util.Optional;
import java.util.Set;
import java.util.concurrent.CompletableFuture;
import java.util.concurrent.CompletionException;
import java.util.concurrent.Future;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicBoolean;
Expand Down Expand Up @@ -201,8 +202,8 @@ private void initalizeClusterConfiguration(final boolean installDefaultConfig) {
try (StoredContext ctx = threadContext.stashContext()) {
threadContext.putHeader(ConfigConstants.OPENDISTRO_SECURITY_CONF_REQUEST_HEADER, "true");

createSecurityIndexIfAbsent();
waitForSecurityIndexToBeAtLeastYellow();
createIndexIfAbsent(securityIndex);
Copy link
Member

Choose a reason for hiding this comment

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

I think this class should be reserved for initializing the security index. The security index will always need to be created on bootstrap.

For most plugins, I believe they create a system index when its needed if it doesn't exist instead of creating it on bootstrap. i.e. A user tries to issue an auth token and security tries to store the metadata, but the index doesn't exist so create it at that time.

See this class from AD plugin: https://github.com/opensearch-project/anomaly-detection/blob/main/src/main/java/org/opensearch/ad/indices/ADIndexManagement.java

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure this approach makes sense too. I also thought of this approach at first, but thought that it will add too much overhead to try and create this index for every request if it doesn't exist. However, we can limit this to index operations (create, revoke, etc.), and do not need to perform this for every authC request. I will try to do it this way instead.

waitForIndexToBeAtLeastYellow(securityIndex);

final int initializationDelaySeconds = settings.getAsInt(
ConfigConstants.SECURITY_UNSUPPORTED_DELAY_INITIALIZATION_SECONDS,
Expand Down Expand Up @@ -324,26 +325,26 @@ private void setupAuditConfigurationIfAny(final boolean auditConfigDocPresent) {
}
}

private boolean createSecurityIndexIfAbsent() {
boolean createIndexIfAbsent(String indexName) {
try {
final Map<String, Object> indexSettings = ImmutableMap.of("index.number_of_shards", 1, "index.auto_expand_replicas", "0-all");
final CreateIndexRequest createIndexRequest = new CreateIndexRequest(securityIndex).settings(indexSettings);
final CreateIndexRequest createIndexRequest = new CreateIndexRequest(indexName).settings(indexSettings);
final boolean ok = client.admin().indices().create(createIndexRequest).actionGet().isAcknowledged();
LOGGER.info("Index {} created?: {}", securityIndex, ok);
LOGGER.info("Index {} created?: {}", indexName, ok);
return ok;
} catch (ResourceAlreadyExistsException resourceAlreadyExistsException) {
LOGGER.info("Index {} already exists", securityIndex);
LOGGER.info("Index {} already exists", indexName);
return false;
}
}

private void waitForSecurityIndexToBeAtLeastYellow() {
void waitForIndexToBeAtLeastYellow(String index) {
LOGGER.info("Node started, try to initialize it. Wait for at least yellow cluster state....");
ClusterHealthResponse response = null;
try {
response = client.admin()
.cluster()
.health(new ClusterHealthRequest(securityIndex).waitForActiveShards(1).waitForYellowStatus())
.health(new ClusterHealthRequest(index).waitForActiveShards(1).waitForYellowStatus())
.actionGet();
} catch (Exception e) {
LOGGER.debug("Caught a {} but we just try again ...", e.toString());
Expand All @@ -352,7 +353,7 @@ private void waitForSecurityIndexToBeAtLeastYellow() {
while (response == null || response.isTimedOut() || response.getStatus() == ClusterHealthStatus.RED) {
LOGGER.debug(
"index '{}' not healthy yet, we try again ... (Reason: {})",
securityIndex,
index,
response == null ? "no response" : (response.isTimedOut() ? "timeout" : "other, maybe red cluster")
);
try {
Expand All @@ -362,7 +363,7 @@ private void waitForSecurityIndexToBeAtLeastYellow() {
Thread.currentThread().interrupt();
}
try {
response = client.admin().cluster().health(new ClusterHealthRequest(securityIndex).waitForYellowStatus()).actionGet();
response = client.admin().cluster().health(new ClusterHealthRequest(index).waitForYellowStatus()).actionGet();
} catch (Exception e) {
LOGGER.debug("Caught again a {} but we just try again ...", e.toString());
}
Expand Down Expand Up @@ -431,6 +432,36 @@ Future<Void> executeConfigurationInitialization(final SecurityMetadata securityM
return CompletableFuture.completedFuture(null);
}

public CompletableFuture<Boolean> createApiTokenIndex() {
CompletableFuture<Boolean> apiTokenIndexTask = new CompletableFuture<>();

new Thread(() -> {
try {
// Wait until the cluster is ready
while (clusterService.state().blocks().hasGlobalBlockWithStatus(RestStatus.SERVICE_UNAVAILABLE)) {
LOGGER.info("Waiting for cluster to be available...");
TimeUnit.SECONDS.sleep(1);
}

final ThreadContext threadContext = threadPool.getThreadContext();
try (StoredContext ctx = threadContext.stashContext()) {
Copy link
Member

Choose a reason for hiding this comment

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

why do we need to explicitly stashContext here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Like the above, this is taken from the creation of opendistro_security index, which I assume has this to handle edge cases since the security index may not be initialized yet. Again not completely sure if its needed but I think it is fine?

threadContext.putHeader(ConfigConstants.OPENDISTRO_SECURITY_CONF_REQUEST_HEADER, "true");

createIndexIfAbsent(ConfigConstants.OPENSEARCH_API_TOKENS_INDEX);
waitForIndexToBeAtLeastYellow(ConfigConstants.OPENSEARCH_API_TOKENS_INDEX);
}

LOGGER.info("API token index created successfully.");
apiTokenIndexTask.complete(true);
} catch (Exception e) {
LOGGER.error("Error while creating API token index", e);
apiTokenIndexTask.completeExceptionally(e);
}
}).start();

return apiTokenIndexTask;
}

@Deprecated
public CompletableFuture<Boolean> initOnNodeStart() {
final boolean installDefaultConfig = settings.getAsBoolean(ConfigConstants.SECURITY_ALLOW_DEFAULT_INIT_SECURITYINDEX, false);
Expand Down
3 changes: 3 additions & 0 deletions src/main/java/org/opensearch/security/support/ConfigConstants.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -370,6 +370,9 @@ public enum RolesMappingResolution {
// Variable for initial admin password support
public static final String OPENSEARCH_INITIAL_ADMIN_PASSWORD = "OPENSEARCH_INITIAL_ADMIN_PASSWORD";

// API Tokens index
public static final String OPENSEARCH_API_TOKENS_INDEX = ".opensearch_security_api_tokens";

public static Set<String> getSettingAsSet(
final Settings settings,
final String key,
Expand Down
21 changes: 21 additions & 0 deletions src/test/java/org/opensearch/security/configuration/ConfigurationRepositoryTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,14 @@
import org.opensearch.cluster.ClusterChangedEvent;
import org.opensearch.cluster.ClusterState;
import org.opensearch.cluster.ClusterStateUpdateTask;
import org.opensearch.cluster.block.ClusterBlocks;
import org.opensearch.cluster.metadata.Metadata;
import org.opensearch.cluster.node.DiscoveryNodes;
import org.opensearch.cluster.service.ClusterService;
import org.opensearch.common.Priority;
import org.opensearch.common.settings.Settings;
import org.opensearch.core.action.ActionListener;
import org.opensearch.core.rest.RestStatus;
import org.opensearch.security.auditlog.AuditLog;
import org.opensearch.security.securityconf.impl.CType;
import org.opensearch.security.securityconf.impl.SecurityDynamicConfiguration;
Expand Down Expand Up @@ -60,6 +62,8 @@
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.reset;
import static org.mockito.Mockito.spy;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.verifyNoMoreInteractions;
import static org.mockito.Mockito.when;
Expand Down Expand Up @@ -148,6 +152,23 @@ public void initOnNodeStart_withSecurityIndexNotCreatedShouldNotSetInstallDefaul
assertThat(result.join(), is(false));
}

@Test
public void createApiTokenIndex_shouldCreateApiTokenIndex() {
Settings settings = Settings.builder().put(SECURITY_ALLOW_DEFAULT_INIT_SECURITYINDEX, true).build();
ClusterState mockClusterState = mock(ClusterState.class);
ClusterBlocks mockClusterBlocks = mock(ClusterBlocks.class);

when(clusterService.state()).thenReturn(mockClusterState);
when(mockClusterState.blocks()).thenReturn(mockClusterBlocks);
when(mockClusterBlocks.hasGlobalBlockWithStatus(RestStatus.SERVICE_UNAVAILABLE)).thenReturn(false);

ConfigurationRepository configRepository = spy(createConfigurationRepository(settings));

configRepository.createApiTokenIndex();

verify(configRepository, times(1)).createIndexIfAbsent(ConfigConstants.OPENSEARCH_API_TOKENS_INDEX);
}

@Test
public void getConfiguration_withInvalidConfigurationShouldReturnNewEmptyConfigurationObject() throws IOException {
ConfigurationRepository configRepository = createConfigurationRepository(Settings.EMPTY);
Expand Down
Loading