Settings migration to Opensearch #1176

andy840314 · 2021-05-21T03:13:53Z

opendistro-for-elasticsearch/security pull request intake form

Please provide as much details as possible to get feedback/acceptance on your PR quickly

Category: (Enhancement, New feature, Bug fix, Test fix, Refactoring, Maintenance, Documentation)
Refactoring
Github Issue # or road-map entry, if available:
Description of changes:
Several files to be modified

OpenSearchSecurityPlugin (

security/src/main/java/com/amazon/opendistroforelasticsearch/security/OpenDistroSecurityPlugin.java

Line 858 in f698fe6

public List<Setting<?>> getSettings() {

), OpenSearchSecuritySSLPlugin (

security/src/main/java/com/amazon/opendistroforelasticsearch/security/ssl/OpenDistroSecuritySSLPlugin.java

Line 318 in f698fe6

public List<Setting<?>> getSettings() {

):
Change getSettings() to add settings from SecuritySettings/SSLSecuritySettings instead of add settings directly. e.g.
settings.add(Setting.boolSetting(ConfigConstants.SECURITY_SSL_ONLY, ...)); → settings.add(SecuritySettings.SECURITY_SSL_ONLY);

All files that uses ConfigConstants and SSLConfigConstants:
Remove prefix OPENDISTRO_

All files that contains opendistro_security string for settings for several test files):
Change opendistro_security to plugins.security

6 files to be added

ConfigConstants.java:
Copy and modify file LegacyOpenDistroConfigConstants.java. Change any string that contains opendistro_security to plugins.security, remove prefix OPENDISTRO_ for variable names.

SecuritySettings.java:
New class for settings. Use LegacyOpenDistroSecuritySettings as fallbackSetting to ensure backward compatibility for opendistro

LegacyOpenDistroSecuritySettings.java
New class for legacy settings. Define the default value for settings here.

SSLConfigConstants.java:
Copy and modify file LegacyOpenDistroSSLConfigConstants.java. Change any string that contains opendistro_security to plugins.security, remove prefix OPENDISTRO_ for variable names.

SSLSecuritySettings.java:
New class for SSL settings. Use LegacyOpenDistroSSLSecuritySettings as fallbackSetting to ensure backward compatibility for opendistro

LegacyOpenDistroSSLSecuritySettings.java:
New class for legacy SSL settings, Define the default value for SSL settings here.

2 files name changed

ConfigConstants.java → LegacyOpenDistroConfigConstants.java
SSLConfigConstants.java → LegacyOpenDistroSSLConfigConstants.java

1 test file added

SecuritySettingTests.java

Why these changes are required?
Settings rename and backward compatibility for OpenDistro
What is the old behavior before changes and new behavior after changes? (Please add any example/logs/screen-shot if available)
Testing done: (Please provide details of testing done: Unit testing, integration testing and manual testing)
Unit tests
TO-DOs, if any: (Please describe pending items and provide Github issues# for each of them)
Is it backport from main branch? (If yes, please add backport PR # and commits #)

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.

dblock · 2021-05-21T21:41:27Z

.github/workflows/ci.yml

@@ -45,7 +45,7 @@ jobs:

    - name: Build OpenSearch
      working-directory: ./OpenSearch
-      run: ./gradlew publishToMavenLocal -Dbuild.snapshot=false
+      run: ./gradlew publishToMavenLocal -Dbuild.version_qualifier=beta1 -Dbuild.snapshot=false


@dblock Both rc1 and removing -Dbuild.version_qualifier fail security plugin build, which is this issue for.

The problem was introduced in #1174 that removes qualifier but does not update pom dependency. I wonder how that PR was tested and approved.

This works here because you have hardcoded beta1 in pom.xml for the OpenSearch dependency. See opensearch-project/OpenSearch#762 (comment).

Change this to be rc1, and update your code to say rc1 instead beta1 everywhere you find "beta1".

These qualifiers don't do anything in the build as long as they are consistent so that the dependency on OpenSearch can be resolved, but in order for the plugin to start the version of the engine and the plugin will have to match, and the next version is rc1.

This works here because you have hardcoded beta1 in pom.xml for the OpenSearch dependency. See opensearch-project/OpenSearch#762 (comment).

@dblock I guess you refer to CI, but it works only because maven artifacts are cached:

security/.github/workflows/ci.yml

Line 32 in 5a81a42

- name: Cache Maven packages

Without caching maven artifacts or on a fresh box with no prior build of beta1 artifacts, the build would fail.

These qualifiers don't do anything in the build as long as they are consistent so that the dependency on OpenSearch can be resolved, but in order for the plugin to start the version of the engine and the plugin will have to match, and the next version is rc1.

If rc1 does not correspond to a commit/tag in Opensearch, why is it used? If plugins are supposed to be built against the latest commit of 1.x branch, it should use SNAPSHOT (this is the meaning of SNAPSHOT), not an rc1 tag/qualifier.

Yes re: cached maven artifacts, I didn't see that, should probably be removed now

You're right on rc1. If we had Maven we would be able to say exactly what you're building against, which could be an rc1 eventually. Right now if you called everything -SNAPSHOT you wouldn't have a way to rename that to rc1 because it would need a change in code. I think we need to fix opensearch-project/OpenSearch#763 first, then we can undo this craziness. It's just a workaround to produce something called -rc1 everywhere.

Yes re: cached maven artifacts, I didn't see that, should probably be removed now

Caching maven artifacts should not be removed, it works as designed to speed up security plugin build and avoid loading artifacts from maven on each build. What should be removed is using CI to build and publish Opensearch artifacts to local maven repo.

You can be right, or you can prevent a similar problem from happening in the short term. Your call.

Similar problems should be prevented by testing and PR review, not by removing what worked before and is not broken. Even if caching is removed, what prevents CI of using wrong branch/tag and naming it beta1 or rc1?

codecov-commenter · 2021-05-21T21:47:14Z

Codecov Report

Merging #1176 (5a81a42) into main (ea101e2) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 5a81a42 differs from pull request most recent head 4fd9998. Consider uploading reports for the commit 4fd9998 to get more accurate results

@@            Coverage Diff            @@
##               main    #1176   +/-   ##
=========================================
  Coverage     64.57%   64.57%           
+ Complexity     3173     3172    -1     
=========================================
  Files           244      244           
  Lines         17125    17123    -2     
  Branches       3036     3035    -1     
=========================================
- Hits          11059    11058    -1     
+ Misses         4522     4520    -2     
- Partials       1544     1545    +1

Impacted Files	Coverage Δ	Complexity Δ
...ecurity/configuration/ConfigurationRepository.java	`73.07% <100.00%> (+1.33%)`	`24.00 <0.00> (+1.00)`
.../dlic/auth/ldap2/LDAPConnectionFactoryFactory.java	`56.48% <0.00%> (-1.53%)`	`23.00% <0.00%> (-1.00%)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ea101e2...4fd9998. Read the comment docs.

src/main/java/org/opensearch/security/support/ConfigConstants.java

src/test/java/org/opensearch/security/SecuritySettingsTests.java

vrozov · 2021-05-24T19:39:46Z

Was this PR tested for backward compatibility during rolling upgrade from ODFE distribution?

andy840314 · 2021-05-24T19:42:30Z

backward compatibility during rolling upgrade from ODFE distribution?

@vrozov Can you specify more about this?

dblock · 2021-05-24T19:44:36Z

Was this PR tested for backward compatibility during rolling upgrade from ODFE distribution?

Unlikely because opensearch-project/OpenSearch#669 prevents this altogether rn.

src/test/resources/auditlog/endpoints/sink/configuration_all_variants.yml

src/test/resources/auditlog/endpoints/sink/configuration_no_default.yml

vrozov · 2021-05-24T20:11:47Z

Was this PR tested for backward compatibility during rolling upgrade from ODFE distribution?

Unlikely because opensearch-project/OpenSearch#669 prevents this altogether rn.

I highly doubt that even after fixing opensearch-project/OpenSearch#669, it will work in clusters where security plugin is installed and enabled. The PR introduces backward incompatible changes.

andy840314 · 2021-05-24T20:15:22Z

I highly doubt that even after fixing opensearch-project/OpenSearch#669, it will work in clusters where security plugin is installed and enabled. The PR introduces backward incompatible changes.

Can you point out the backward incompatible changes? This code is compatible for the old opendistro_security. settings.

…files

…ix in InitializationIntegrationTests

saratvemulapalli · 2021-05-24T21:24:16Z

I highly doubt that even after fixing opensearch-project/OpenSearch#669, it will work in clusters where security plugin is installed and enabled. The PR introduces backward incompatible changes.

Can you point out the backward incompatible changes? This code is compatible for the old opendistro_security. settings.

+1 @vrozov if there are more changes to be taken care of could you list them down on an issue?
Also curious what they are, and if they are specific to security plugin or all the plugins...

vrozov · 2021-05-24T21:31:10Z

I highly doubt that even after fixing opensearch-project/OpenSearch#669, it will work in clusters where security plugin is installed and enabled. The PR introduces backward incompatible changes.

Can you point out the backward incompatible changes? This code is compatible for the old opendistro_security. settings.

+1 @vrozov if there are more changes to be taken care of could you list them down on an issue?
Also curious what they are, and if they are specific to security plugin or all the plugins...

@andy840314 The change impacts not only settings, but also inter node communication (for example headers). I am not in position to point to all possible backward incompatible changes, so was my request to extensively test those changes.

@saratvemulapalli those are specific to the security plugin, plus it may impact few other plugins that pass info to the security plugin using thread context.

andy840314 · 2021-05-24T22:25:46Z

@andy840314 The change impacts not only settings, but also inter node communication (for example headers). I am not in position to point to all possible backward incompatible changes, so was my request to extensively test those changes.

Actually, only the variable name for those constant are changed.
e.g. OPENDISTRO_SECURITY_USER_HEADER -> SECURITY_USER_HEADER
The constant string in the variable is not modified so it should be fine.

vrozov · 2021-05-25T04:16:04Z

src/main/java/org/opensearch/security/auditlog/config/AuditConfig.java

-            ConfigConstants.OPENDISTRO_SECURITY_COMPLIANCE_HISTORY_WRITE_LOG_DIFFS,
-            ConfigConstants.OPENDISTRO_SECURITY_COMPLIANCE_HISTORY_WRITE_IGNORE_USERS,
-            ConfigConstants.OPENDISTRO_SECURITY_COMPLIANCE_HISTORY_WRITE_WATCHED_INDICES
+            ConfigConstants.SECURITY_AUDIT_ENABLE_REST,


This introduces new key and immediately deprecates it. All those keys are already deprecated and should not be migrated to new keys.

andy840314 · 2021-05-25T20:24:49Z

These changes will split into different PR as discussed.

andy840314 marked this pull request as draft May 21, 2021 03:14

andy840314 force-pushed the settings-migration-to-opensearch branch 2 times, most recently from 29b0021 to 1c8773b Compare May 21, 2021 19:42

andy840314 changed the title ~~Settings migration to opensearch~~ Settings migration to Opensearch May 21, 2021

andy840314 marked this pull request as ready for review May 21, 2021 19:57

cliu123 force-pushed the settings-migration-to-opensearch branch from 4be5f42 to 1c8773b Compare May 21, 2021 20:40

dblock reviewed May 21, 2021

View reviewed changes

dblock mentioned this pull request May 21, 2021

Security plugin build fails on 1.x branch opensearch-project/OpenSearch#762

Closed

cliu123 force-pushed the settings-migration-to-opensearch branch from 70e52a4 to 4fd9998 Compare May 22, 2021 04:13

vrozov mentioned this pull request May 23, 2021

Add build.version_qualifier and make security plugin compatible with OpenSearch 1.0.0-rc1 #1179

Merged

skkosuri-amzn reviewed May 24, 2021

View reviewed changes

src/main/java/org/opensearch/security/support/ConfigConstants.java Show resolved Hide resolved

vrozov reviewed May 24, 2021

View reviewed changes

src/test/java/org/opensearch/security/SecuritySettingsTests.java Show resolved Hide resolved

hsiang9431-amzn reviewed May 24, 2021

View reviewed changes

src/test/resources/auditlog/endpoints/sink/configuration_all_variants.yml Show resolved Hide resolved

hsiang9431-amzn reviewed May 24, 2021

View reviewed changes

src/test/resources/auditlog/endpoints/sink/configuration_all_variants.yml Show resolved Hide resolved

hsiang9431-amzn reviewed May 24, 2021

View reviewed changes

src/test/resources/auditlog/endpoints/sink/configuration_no_default.yml Show resolved Hide resolved

hsiang9431-amzn reviewed May 24, 2021

View reviewed changes

src/test/resources/auditlog/endpoints/sink/configuration_no_default.yml Show resolved Hide resolved

Andy Lin added 8 commits May 24, 2021 13:21

Add settings class and legacy settings class

e224dec

Add SSL settings class

ed017c8

Change opendistro_security. to plugins.security.

218c7de

Change opendistro_security to plugins.security in test/resources yml …

3a8bce9

…files

Replace opendistro_security_config with security_config

398f826

Fix LegacyOpenDistroSSLSecuritySettings

b1fa99a

Delete comments

480a6b6

Add groupSetting fallback

d3b986f

Andy Lin added 9 commits May 24, 2021 13:24

Fix CI

8c0e97b

Add legacy settings in getSettings()

8df238c

Add SSL_DUAL_MODE_SETTING to SecuritySettings

5f19129

Add test

674b659

Add more UT

eecd54e

Remove variable prefix for SlowIntegrationTests

9c990ca

Add UTs for all settings

99ae075

Delete unused imports in test file

f31646a

Remove unused imports in SSLSecuritySettings, remove OPENDISTRO_ pref…

2e5034d

…ix in InitializationIntegrationTests

andy840314 force-pushed the settings-migration-to-opensearch branch from 4fd9998 to 2e5034d Compare May 24, 2021 20:37

hsiang9431-amzn requested a review from saratvemulapalli May 24, 2021 20:52

vrozov reviewed May 25, 2021

View reviewed changes

andy840314 closed this May 25, 2021

nebulon42 mentioned this pull request Aug 30, 2021

unknown setting [opensearch_security.* #1216

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Settings migration to Opensearch #1176

Settings migration to Opensearch #1176

andy840314 commented May 21, 2021 •

edited

Loading

dblock May 21, 2021

cliu123 May 21, 2021 •

edited

Loading

vrozov May 22, 2021

dblock May 22, 2021 •

edited

Loading

vrozov May 22, 2021

dblock May 23, 2021

vrozov May 23, 2021

dblock May 24, 2021

vrozov May 24, 2021

codecov-commenter commented May 21, 2021 •

edited

Loading

vrozov commented May 24, 2021

andy840314 commented May 24, 2021 •

edited

Loading

dblock commented May 24, 2021

vrozov commented May 24, 2021

andy840314 commented May 24, 2021

saratvemulapalli commented May 24, 2021

vrozov commented May 24, 2021

andy840314 commented May 24, 2021

vrozov May 25, 2021

andy840314 commented May 25, 2021

Settings migration to Opensearch #1176

Settings migration to Opensearch #1176

Conversation

andy840314 commented May 21, 2021 • edited Loading

opendistro-for-elasticsearch/security pull request intake form

Choose a reason for hiding this comment

cliu123 May 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dblock May 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented May 21, 2021 • edited Loading

Codecov Report

vrozov commented May 24, 2021

andy840314 commented May 24, 2021 • edited Loading

dblock commented May 24, 2021

vrozov commented May 24, 2021

andy840314 commented May 24, 2021

saratvemulapalli commented May 24, 2021

vrozov commented May 24, 2021

andy840314 commented May 24, 2021

Choose a reason for hiding this comment

andy840314 commented May 25, 2021

andy840314 commented May 21, 2021 •

edited

Loading

cliu123 May 21, 2021 •

edited

Loading

dblock May 22, 2021 •

edited

Loading

codecov-commenter commented May 21, 2021 •

edited

Loading

andy840314 commented May 24, 2021 •

edited

Loading