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

Add option to set user for test cluster to perform initial health checks #11641

Closed
wants to merge 5 commits into from
Closed

Add option to set user for test cluster to perform initial health checks #11641

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8d1ccec
Avoid prefixing sys props with tests.opensearch
jmazanec15 Dec 19, 2023
cc46549
Revert "Avoid prefixing sys props with tests.opensearch"
jmazanec15 Dec 19, 2023
8725838
Set user option to set credentials for the node
jmazanec15 Dec 19, 2023
50a027c
Add changelog entry
jmazanec15 Dec 20, 2023
0feaa31
Fix style check
jmazanec15 Dec 20, 2023
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 CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
- [BWC and API enforcement] Introduce checks for enforcing the API restrictions ([#11175](https://github.com/opensearch-project/OpenSearch/pull/11175))
- Create separate transport action for render search template action ([#11170](https://github.com/opensearch-project/OpenSearch/pull/11170))
- Add additional handling in SearchTemplateRequest when simulate is set to true ([#11591](https://github.com/opensearch-project/OpenSearch/pull/11591))
- Add option to set user for test cluster to perform health checks during run with security enabled ([#11641](https://github.com/opensearch-project/OpenSearch/pull/11641))

### Dependencies
- Bump Lucene from 9.7.0 to 9.8.0 ([10276](https://github.com/opensearch-project/OpenSearch/pull/10276))
Expand Down
19 changes: 18 additions & 1 deletion buildSrc/src/main/java/org/opensearch/gradle/testclusters/OpenSearchNode.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -703,7 +703,24 @@
}

@Override
public void user(Map<String, String> userSpec) {}
public void user(Map<String, String> userSpec) {
if (userSpec == null) {
return;

Check warning on line 708 in buildSrc/src/main/java/org/opensearch/gradle/testclusters/OpenSearchNode.java

View check run for this annotation

Codecov / codecov/patch

buildSrc/src/main/java/org/opensearch/gradle/testclusters/OpenSearchNode.java#L708 

Added line #L708 was not covered by tests
}

if (userSpec.containsKey("username") && userSpec.containsKey("password")) {
this.credentials.get(0).put("username", userSpec.get("username"));
Copy link
Collaborator

Choose a reason for hiding this comment

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

The credentials would conflict with start method I believe

Copy link
Member Author

Choose a reason for hiding this comment

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

For start, we would allow users to override if the sys properties are set. We could get rid of reading from sys properties during start and require user to be used, but I think this would break things for other plugins.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am trying to trace where this method is called but the only place I found is OpenSearchCluster::user, could you please help me to figure out who actually populates the userSpec in the first place?

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, this would be from the consumer of the gradle plugin. So for instance in k-NN, we configure the test cluster here: https://github.com/opensearch-project/k-NN/blob/main/build.gradle#L259-L280.

For security, it would look something like:

testClusters.integTest {
    testDistribution = "ARCHIVE"

    // Optionally install security
    if (System.getProperty("security.enabled") != null) {
        configurations.zipArchive.asFileTree.each {
            plugin(provider(new Callable<RegularFile>() {
                @Override
                RegularFile call() throws Exception {
                    return new RegularFile() {
                        @Override
                        File getAsFile() {
                            return it
                        }
                    }
                }
            }))
        }

        user(Map.of("username", "admin", "password", "admin"))

        extraConfigFile("admin-cert.pem", new File("$rootDir/src/test/resources/security/admin-cert.pem"))
        extraConfigFile("admin-key.pem", new File("$rootDir/src/test/resources/security/admin-key.pem"))
        extraConfigFile("node-cert.pem", new File("$rootDir/src/test/resources/security/node-cert.pem"))
        extraConfigFile("node-key.pem", new File("$rootDir/src/test/resources/security/node-key.pem"))
        extraConfigFile("root-ca.pem", new File("$rootDir/src/test/resources/security/root-ca.pem"))

        setting("plugins.security.ssl.transport.pemcert_filepath", "node-cert.pem")
        setting("plugins.security.ssl.transport.pemkey_filepath", "node-key.pem")
        setting("plugins.security.ssl.transport.pemtrustedcas_filepath", "root-ca.pem")
        setting("plugins.security.ssl.transport.enforce_hostname_verification", "false")
        setting("plugins.security.ssl.http.enabled", "true")
        setting("plugins.security.ssl.http.pemcert_filepath", "node-cert.pem")
        setting("plugins.security.ssl.http.pemkey_filepath", "node-key.pem")
        setting("plugins.security.ssl.http.pemtrustedcas_filepath", "root-ca.pem")
        setting("plugins.security.allow_unsafe_democertificates", "true")
        setting("plugins.security.allow_default_init_securityindex", "true")
        setting("plugins.security.unsupported.inject_user.enabled", "true")

        setting("plugins.security.authcz.admin_dn", "\n- CN=kirk,OU=client,O=client,L=test, C=de")
        setting('plugins.security.restapi.roles_enabled', '["all_access", "security_rest_api_access"]')
        setting('plugins.security.system_indices.enabled', "true")
        setting('plugins.security.system_indices.indices', '[".plugins-ml-config", ".plugins-ml-connector", ".plugins-ml-model-group", ".plugins-ml-model", ".plugins-ml-task", ".plugins-ml-conversation-meta", ".plugins-ml-conversation-interactions", ".opendistro-alerting-config", ".opendistro-alerting-alert*", ".opendistro-anomaly-results*", ".opendistro-anomaly-detector*", ".opendistro-anomaly-checkpoints", ".opendistro-anomaly-detection-state", ".opendistro-reports-*", ".opensearch-notifications-*", ".opensearch-notebooks", ".opensearch-observability", ".ql-datasources", ".opendistro-asynchronous-search-response*", ".replication-metadata-store", ".opensearch-knn-models", ".geospatial-ip2geo-data*"]')

        setSecure(true)
    }

Copy link
Collaborator

@reta reta Dec 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Right, this would be from the consumer of the gradle plugin.

Gotcha, thanks a lot @jmazanec15 , it looks quite logical to me to allow user credentials configuration this way, so what about altering OpenSearchNode::start() to consult system properties only if credentials were not set? That should not break anyone I think (hard to imagine cluster and node(s) to have different credentials)

Copy link
Member Author

Choose a reason for hiding this comment

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

That should not break anyone I think (hard to imagine cluster and node(s) to have different credentials)
Btw: its also possible from consuming plugin to call user per node via something like:

cluster.getFirstNode.user()

I think thats an option. However, right now, it looks like the system properties are intended to perform the override:

        if (System.getProperty("tests.opensearch.username") != null) {
            this.credentials.get(0).put("username", System.getProperty("tests.opensearch.username"));
            LOGGER.info("Overwriting username to: " + this.getCredentials().get(0).get("username"));
        }
        if (System.getProperty("tests.opensearch.password") != null) {
            this.credentials.get(0).put("password", System.getProperty("tests.opensearch.password"));
            LOGGER.info("Overwriting password to: " + this.getCredentials().get(0).get("password"));
        }

Id be curious if the security team has any strong preference before doing that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Id be curious if the security team has any strong preference before doing that.

Sure, let's wait for their feedback, thanks!

this.credentials.get(0).put("password", userSpec.get("password"));
return;

Check warning on line 714 in buildSrc/src/main/java/org/opensearch/gradle/testclusters/OpenSearchNode.java

View check run for this annotation

Codecov / codecov/patch

buildSrc/src/main/java/org/opensearch/gradle/testclusters/OpenSearchNode.java#L712-L714 

Added lines #L712 - L714 were not covered by tests
}

if (!userSpec.containsKey("username")) {
LOGGER.warn("Unable to set user. userSpec must contain username.");

Check warning on line 718 in buildSrc/src/main/java/org/opensearch/gradle/testclusters/OpenSearchNode.java

View check run for this annotation

Codecov / codecov/patch

buildSrc/src/main/java/org/opensearch/gradle/testclusters/OpenSearchNode.java#L718 

Added line #L718 was not covered by tests
}
if (!userSpec.containsKey("password")) {
LOGGER.warn("Unable to set user. userSpec must contain password.");

Check warning on line 721 in buildSrc/src/main/java/org/opensearch/gradle/testclusters/OpenSearchNode.java

View check run for this annotation

Codecov / codecov/patch

buildSrc/src/main/java/org/opensearch/gradle/testclusters/OpenSearchNode.java#L721 

Added line #L721 was not covered by tests
}
}

Check warning on line 723 in buildSrc/src/main/java/org/opensearch/gradle/testclusters/OpenSearchNode.java

View check run for this annotation

Codecov / codecov/patch

buildSrc/src/main/java/org/opensearch/gradle/testclusters/OpenSearchNode.java#L723 

Added line #L723 was not covered by tests

private void runOpenSearchBinScriptWithInput(String input, String tool, CharSequence... args) {
if (Files.exists(getDistroDir().resolve("bin").resolve(tool)) == false
Expand Down
Loading