-
Notifications
You must be signed in to change notification settings - Fork 302
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
Allow attempt to load security config in case of plugin restart even … #1154
Allow attempt to load security config in case of plugin restart even … #1154
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1154 +/- ##
============================================
- Coverage 64.61% 64.58% -0.03%
- Complexity 3169 3172 +3
============================================
Files 244 244
Lines 17124 17122 -2
Branches 3036 3035 -1
============================================
- Hits 11064 11058 -6
- Misses 4517 4520 +3
- Partials 1543 1544 +1
Continue to review full report at Codecov.
|
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.
I believe alternate interpretation to the issue is "Put security configs if absent in the security index". It may be missing if manually deleted or error-ed out while loading.
...va/com/amazon/opendistroforelasticsearch/security/configuration/ConfigurationRepository.java
Outdated
Show resolved
Hide resolved
...va/com/amazon/opendistroforelasticsearch/security/configuration/ConfigurationRepository.java
Outdated
Show resolved
Hide resolved
...test/java/com/amazon/opendistroforelasticsearch/security/InitializationIntegrationTests.java
Outdated
Show resolved
Hide resolved
5ed5a4f
to
d51611d
Compare
...test/java/com/amazon/opendistroforelasticsearch/security/InitializationIntegrationTests.java
Outdated
Show resolved
Hide resolved
...t/java/com/amazon/opendistroforelasticsearch/security/test/helper/cluster/ClusterHelper.java
Outdated
Show resolved
Hide resolved
...test/java/com/amazon/opendistroforelasticsearch/security/InitializationIntegrationTests.java
Outdated
Show resolved
Hide resolved
...test/java/com/amazon/opendistroforelasticsearch/security/InitializationIntegrationTests.java
Outdated
Show resolved
Hide resolved
...t/java/com/amazon/opendistroforelasticsearch/security/test/helper/cluster/ClusterHelper.java
Outdated
Show resolved
Hide resolved
...t/java/com/amazon/opendistroforelasticsearch/security/test/helper/cluster/ClusterHelper.java
Outdated
Show resolved
Hide resolved
e954faa
to
26ab99a
Compare
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.
Thank you for rebasing
…if security index already exists Signed-off-by: Dhiresh Jain <dhireshj@amazon.com>
Signed-off-by: Dhiresh Jain <dhireshj@amazon.com>
Signed-off-by: Dhiresh Jain <dhireshj@amazon.com>
Signed-off-by: Dhiresh Jain <dhireshj@amazon.com>
Signed-off-by: Dhiresh Jain <dhireshj@amazon.com>
26ab99a
to
7a2e593
Compare
…if security index already exists (opensearch-project#1154) (cherry picked from commit 5a81a42)
…if security index already exists (opensearch-project#1154) (cherry picked from commit 5a81a42)
…if security index already exists (opensearch-project#1154) (cherry picked from commit 5a81a42)
…if security index already exists (opensearch-project#1154) (cherry picked from commit 5a81a42)
…if security index already exists (opensearch-project#1154) (cherry picked from commit 5a81a42)
…if security index already exists (opensearch-project#1154) (cherry picked from commit 5a81a42)
…if security index already exists (opensearch-project#1154) (cherry picked from commit 5a81a42)
…if security index already exists (opensearch-project#1154) (cherry picked from commit 5a81a42)
…if security index already exists (opensearch-project#1154) (cherry picked from commit 5a81a42)
…if security index already exists (opensearch-project#1154) (cherry picked from commit 5a81a42)
…if security index already exists test dummy(opensearch-project#1154) (cherry picked from commit 5a81a42)
…if security index already exists(opensearch-project#1154) (cherry picked from commit 5a81a42)
…if security index already exists(testing) (opensearch-project#1154) (cherry picked from commit 5a81a42)
…if security index already exists (opensearch-project#1154)
…if security index already exists (opensearch-project#1154)
…if security index already exists
Signed-off-by: Dhiresh Jain dhireshj@amazon.com
opendistro-for-elasticsearch/security pull request intake form
Please provide as much details as possible to get feedback/acceptance on your PR quickly
NOTE: This commit has been moved from opendistro repo to opensearch. The manual test was done using docker in opendistro. Docker testing is currently unavailable in opensearch so did not perform a manual test here yet.
Category: (_Enhancement)
Github Issue # or road-map entry, if available:
Description of changes: Put security configs if absent in the security index. Security config could be absent in index due to missing files or errored out during loading of files
Why these changes are required? In case of issues with loading static configuration from files during plugin bootstrap, we currently need to fix the issue, delete security index and restart a plugin. This change will allow us to just fix the issue and restart the plugin without needing to delete the security index.
What is the old behaviour before changes and new behaviour after changes? (Please add any example/logs/screen-shot if available)
[Docker Setup - 2 nodes cluster]
Old Behaviour:
Corrupt one of the static securityconfig files and start the nodes in the cluster. Verify cluster requests fail. Fix the corrupt file and restart a node. Verify cluster requests fail.
New Behaviour:
Corrupt one of the static securityconfig files and start the nodes in the cluster. Verify cluster requests fail. Fix the corrupt file and restart a node. Verify cluster requests succeed.
Note: Each time security plugin bootstraps, there will be an attempt to load all securityconfig files into security index. Attempting to index a document from static files when it already exists will throw a
VersionConflictEngineException
which is anyway swallowed and is a no/op, so this will not be a cause of concern.Testing done: (Please provide details of testing done: Unit testing, integration testing and manual testing)
Old Behaviour:
New Behaviour:
-> Checkout https://github.com/opendistro-for-elasticsearch/opendistro-build
-> cd into {$PATH}/opendistro-build/elasticsearch/docker
-> Create directory build/elasticsearch/plugins/
-> Create file build/elasticsearch/plugins/plugins.list
-> Pull changes of this PR locally
-> cd into plugin directory
-> Add invalid config to any configuration file in
securityconfig
directory-> Build the plugin: mvn clean package -Padvanced -DskipTests
-> Copy artifact target/releases/*.zip to ../opendistro-build/elasticsearch/docker/build/elasticsearch/plugins/
-> cd into {$PATH}/opendistro-build/elasticsearch/docker
-> Add the artifact name to the plugins.list file
-> run make-build [TIP: Remove knn dependencies from Dockerfile to speed up build. We do no require knn anyway)
-> run make-cluster
-> Once the cluster is up with two nodes, verify requests fail with
Open Distro Security not initialised
-> Log into a docker container
docker exec -it elasticsearch1 /bin/bash
-> Fix the corrupted configuration file in
plugins/opendistro_security/securityconfig/
directory-> Exit docker container and restart it
docker restart elasticsearch1
-> Cluster should be up and requests to it should succeed now
-> Without the plugin changes, the previous step would still fail
By making a contribution to this project, I certify that:
(a) The contribution was created in whole or in part by me and I
have the right to submit it under the open source license
indicated in the file; or
(b) The contribution is based upon previous work that, to the best
of my knowledge, is covered under an appropriate open source
license and I have the right under that license to submit that
work with modifications, whether created in whole or in part
by me, under the same open source license (unless I am
permitted to submit under a different license), as indicated
in the file; or
(c) The contribution was provided directly to me by some other
person who certified (a), (b) or (c) and I have not modified
it.
(d) I understand and agree that this project and the contribution
are public and that a record of the contribution (including all
personal information I submit with it, including my sign-off) is
maintained indefinitely and may be redistributed consistent with
this project or the open source license(s) involved.
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.