Skip to content

Commit

Permalink
Block settings in sql query settings API and add more unit tests (#2407)
Browse files Browse the repository at this point in the history
Signed-off-by: Vamsi Manohar <[email protected]>
(cherry picked from commit 2f2ecd2)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
  • Loading branch information
github-actions[bot] committed Nov 1, 2023
1 parent 6e17ae6 commit d7910da
Show file tree
Hide file tree
Showing 4 changed files with 35 additions and 4 deletions.
6 changes: 3 additions & 3 deletions docs/user/admin/settings.rst
Original file line number Diff line number Diff line change
Expand Up @@ -328,7 +328,7 @@ You can update the setting with a new value like this.

SQL query::

sh$ curl -sS -H 'Content-Type: application/json' -X PUT localhost:9200/_plugins/_query/settings \
sh$ curl -sS -H 'Content-Type: application/json' -X PUT localhost:9200/_cluster/settings \
... -d '{"transient":{"plugins.query.executionengine.spark.session.limit":200}}'
{
"acknowledged": true,
Expand Down Expand Up @@ -365,7 +365,7 @@ You can update the setting with a new value like this.

SQL query::

sh$ curl -sS -H 'Content-Type: application/json' -X PUT localhost:9200/_plugins/_query/settings \
sh$ curl -sS -H 'Content-Type: application/json' -X PUT localhost:9200/_cluster/settings \
... -d '{"transient":{"plugins.query.executionengine.spark.refresh_job.limit":200}}'
{
"acknowledged": true,
Expand Down Expand Up @@ -402,7 +402,7 @@ You can update the setting with a new value like this.

SQL query::

sh$ curl -sS -H 'Content-Type: application/json' -X PUT localhost:9200/_plugins/_query/settings \
sh$ curl -sS -H 'Content-Type: application/json' -X PUT localhost:9200/_cluster/settings \
... -d '{"transient":{"plugins.query.datasources.limit":25}}'
{
"acknowledged": true,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,8 @@ public class RestQuerySettingsAction extends BaseRestHandler {
private static final String LEGACY_SQL_SETTINGS_PREFIX = "opendistro.sql.";
private static final String LEGACY_PPL_SETTINGS_PREFIX = "opendistro.ppl.";
private static final String LEGACY_COMMON_SETTINGS_PREFIX = "opendistro.query.";
private static final String EXECUTION_ENGINE_SETTINGS_PREFIX = "plugins.query.executionengine";
public static final String DATASOURCES_SETTINGS_PREFIX = "plugins.query.datasources";
private static final List<String> SETTINGS_PREFIX =
ImmutableList.of(
SQL_SETTINGS_PREFIX,
Expand All @@ -48,6 +50,9 @@ public class RestQuerySettingsAction extends BaseRestHandler {
LEGACY_PPL_SETTINGS_PREFIX,
LEGACY_COMMON_SETTINGS_PREFIX);

private static final List<String> DENY_LIST_SETTINGS_PREFIX =
ImmutableList.of(EXECUTION_ENGINE_SETTINGS_PREFIX, DATASOURCES_SETTINGS_PREFIX);

public static final String SETTINGS_API_ENDPOINT = "/_plugins/_query/settings";
public static final String LEGACY_SQL_SETTINGS_API_ENDPOINT = "/_opendistro/_sql/settings";

Expand Down Expand Up @@ -133,6 +138,10 @@ private Settings getAndFilterSettings(Map<String, ?> source) {
}
return true;
});
// Applying DenyList Filter.
settingsBuilder
.keys()
.removeIf(key -> DENY_LIST_SETTINGS_PREFIX.stream().anyMatch(key::startsWith));
return settingsBuilder.build();
} catch (IOException e) {
throw new OpenSearchGenerationException("Failed to generate [" + source + "]", e);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,25 +8,33 @@
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.doReturn;
import static org.mockito.Mockito.doThrow;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
import static org.opensearch.sql.spark.constants.TestConstants.DEFAULT_RESULT_INDEX;
import static org.opensearch.sql.spark.constants.TestConstants.EMRS_APPLICATION_ID;
import static org.opensearch.sql.spark.constants.TestConstants.EMRS_EXECUTION_ROLE;
import static org.opensearch.sql.spark.constants.TestConstants.EMRS_JOB_NAME;
import static org.opensearch.sql.spark.constants.TestConstants.EMR_JOB_ID;
import static org.opensearch.sql.spark.constants.TestConstants.ENTRY_POINT_START_JAR;
import static org.opensearch.sql.spark.constants.TestConstants.QUERY;
import static org.opensearch.sql.spark.constants.TestConstants.SPARK_SUBMIT_PARAMETERS;

import com.amazonaws.services.emrserverless.AWSEMRServerless;
import com.amazonaws.services.emrserverless.model.CancelJobRunResult;
import com.amazonaws.services.emrserverless.model.GetJobRunResult;
import com.amazonaws.services.emrserverless.model.JobRun;
import com.amazonaws.services.emrserverless.model.StartJobRunRequest;
import com.amazonaws.services.emrserverless.model.StartJobRunResult;
import com.amazonaws.services.emrserverless.model.ValidationException;
import java.util.HashMap;
import java.util.List;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.mockito.ArgumentCaptor;
import org.mockito.Captor;
import org.mockito.Mock;
import org.mockito.junit.jupiter.MockitoExtension;
import org.opensearch.sql.common.setting.Settings;
Expand All @@ -40,6 +48,8 @@ public class EmrServerlessClientImplTest {

@Mock private OpenSearchSettings settings;

@Captor private ArgumentCaptor<StartJobRunRequest> startJobRunRequestArgumentCaptor;

@BeforeEach
public void setUp() {
doReturn(emptyList()).when(settings).getSettings();
Expand All @@ -64,7 +74,16 @@ void testStartJobRun() {
SPARK_SUBMIT_PARAMETERS,
new HashMap<>(),
false,
null));
DEFAULT_RESULT_INDEX));
verify(emrServerless, times(1)).startJobRun(startJobRunRequestArgumentCaptor.capture());
StartJobRunRequest startJobRunRequest = startJobRunRequestArgumentCaptor.getValue();
Assertions.assertEquals(EMRS_APPLICATION_ID, startJobRunRequest.getApplicationId());
Assertions.assertEquals(EMRS_EXECUTION_ROLE, startJobRunRequest.getExecutionRoleArn());
Assertions.assertEquals(
ENTRY_POINT_START_JAR, startJobRunRequest.getJobDriver().getSparkSubmit().getEntryPoint());
Assertions.assertEquals(
List.of(QUERY, DEFAULT_RESULT_INDEX),
startJobRunRequest.getJobDriver().getSparkSubmit().getEntryPointArguments());
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,4 +18,7 @@ public class TestConstants {
public static final String TEST_CLUSTER_NAME = "TEST_CLUSTER";
public static final String MOCK_SESSION_ID = "s-0123456";
public static final String MOCK_STATEMENT_ID = "st-0123456";
public static final String ENTRY_POINT_START_JAR =
"file:///home/hadoop/.ivy2/jars/org.opensearch_opensearch-spark-sql-application_2.12-0.1.0-SNAPSHOT.jar";
public static final String DEFAULT_RESULT_INDEX = "query_execution_result_ds1";
}

0 comments on commit d7910da

Please sign in to comment.