-
Notifications
You must be signed in to change notification settings - Fork 281
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.
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
Test related to security plugin configuration updates. #2155
Test related to security plugin configuration updates. #2155
Conversation
src/integrationTest/java/org/opensearch/security/SecurityConfigurationTests.java
Outdated
Show resolved
Hide resolved
src/integrationTest/java/org/opensearch/security/DefaultConfigurationTests.java
Show resolved
Hide resolved
src/integrationTest/java/org/opensearch/test/framework/cluster/TestRestClient.java
Outdated
Show resolved
Hide resolved
3eb83ba
to
81bc473
Compare
81bc473
to
ef4cece
Compare
|
||
class ConfigurationFiles { | ||
|
||
public static void createRoleMappingFile(File destination) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we need methods to create other configurations like roles, internal_users and so on for future?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, other configuration files can be created by the method org.opensearch.security.ConfigurationFiles#createConfigurationDirectory
. Role mapping file has its own dedicated method createRoleMappingFile
because the role mapping file is used in configuration update test (org.opensearch.security.SecurityConfigurationTests#shouldUseSecurityAdminTool
) where only roles mapping are update
ef4cece
to
b978491
Compare
@@ -70,6 +70,7 @@ private Settings.Builder minimumOpenSearchSettingsBuilder(int node, boolean sslO | |||
builder.putList("plugins.security.authcz.admin_dn", testCertificates.getAdminDNs()); | |||
builder.put("plugins.security.compliance.salt", "1234567890123456"); | |||
builder.put("plugins.security.audit.type", "noop"); | |||
builder.put("plugins.security.background_init_if_securityindex_not_exist", "false"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for my knowledge, what does this setting do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This configuration parameter is used in method org.opensearch.security.configuration.ConfigurationRepository#initOnNodeStart
. If the value is true
then the thread defined in the constructor org.opensearch.security.configuration.ConfigurationRepository#ConfigurationRepository
is started. The thread is responsible for:
- Loading default configuration (if another configuration parameter
plugins.security.allow_default_init_securityindex
is equal totrue
, but the default isfalse
) - Loading security plugin configuration from index
.opendistro_security
to local node cache.
Both operations are not needed during the tests because in most cases a new cluster is created, security plugin configuration is inserted into index .opendistro_security
and then a configuration update request is sent to the cluster, please see method org.opensearch.test.framework.TestSecurityConfig#initIndex
In practice switching the parameter plugins.security.background_init_if_securityindex_not_exist
to true
causes logging additional WARN messages.
@nibix please correct me if I went wrong.
f4cc20a
b978491
to
f4cc20a
Compare
Bwc tests are broken on main. Tracking issue here: #2221 |
f4cc20a
to
92d8abc
Compare
92d8abc
to
1a520a3
Compare
Codecov Report
@@ Coverage Diff @@
## main #2155 +/- ##
============================================
- Coverage 61.16% 60.97% -0.19%
+ Complexity 3274 3261 -13
============================================
Files 259 259
Lines 18337 18336 -1
Branches 3248 3248
============================================
- Hits 11215 11180 -35
- Misses 5535 5565 +30
- Partials 1587 1591 +4
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
186f7ec
to
738994d
Compare
@lukasz-soszynski-eliatra Please rebase/merge from main this should resolve the CI issues so this change can be merged. Note; I tried to update this PR myself, but I could not push it to your branch. |
Signed-off-by: Lukasz Soszynski <[email protected]>
…n updating. Signed-off-by: Lukasz Soszynski <[email protected]>
…acksPreventionTests corrected. Signed-off-by: Lukasz Soszynski <[email protected]>
738994d
to
37a2df7
Compare
Done, rebased into newest main |
Signed-off-by: Lukasz Soszynski [email protected]
Description
[Describe what this change achieves]
Integration tests which verify security plugin behaviour after the configuration update
Issues Resolved
[List any issues this PR will resolve]
Is this a backport? If so, please add backport PR # and/or commits #
Testing
[Please provide details of testing done: unit testing, integration testing and manual testing]
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.