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

cliu123 · 2021-05-22T04:41:36Z

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)
Maintenance
Github Issue # or road-map entry, if available:
Description of changes:

Build OpenSearch 1.0.0-rc1 on CI.
Make security plugin compatible with OpenSearch 1.0.0-rc1.

Why these changes are required?
OpenSearch release 1.0.0-rc1
What is the old behavior before changes and new behavior after changes? (Please add any example/logs/screen-shot if available)
No changes in behavior
Testing done: (Please provide details of testing done: Unit testing, integration testing and manual testing)
UT
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.

codecov-commenter · 2021-05-22T05:54:06Z

Codecov Report

Merging #1179 (b478f83) into main (ea101e2) will increase coverage by 0.06%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##               main    #1179      +/-   ##
============================================
+ Coverage     64.57%   64.64%   +0.06%     
  Complexity     3173     3173              
============================================
  Files           244      244              
  Lines         17125    17123       -2     
  Branches       3036     3035       -1     
============================================
+ Hits          11059    11069      +10     
+ Misses         4522     4509      -13     
- Partials       1544     1545       +1

Impacted Files	Coverage Δ	Complexity Δ
...a/org/opensearch/security/tools/SecurityAdmin.java	`47.26% <0.00%> (-0.27%)`	`76.00% <0.00%> (-1.00%)`
...ecurity/configuration/ConfigurationRepository.java	`73.07% <0.00%> (+1.33%)`	`24.00% <0.00%> (+1.00%)`
...nsearch/security/dlic/rest/api/AuditApiAction.java	`68.08% <0.00%> (+4.25%)`	`15.00% <0.00%> (ø%)`
...org/opensearch/security/rest/TenantInfoAction.java	`88.05% <0.00%> (+10.44%)`	`15.00% <0.00%> (ø%)`
...ecurity/configuration/StaticResourceException.java	`25.00% <0.00%> (+25.00%)`	`1.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...b478f83. Read the comment docs.

vrozov · 2021-05-22T16:27: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=rc1 -Dbuild.snapshot=false


What makes this rc1? The checkout references 1.x branch that is a moving target.

Yes, 1.x is a moving target.
CI checkout 1.x branch of OpenSearch core, but the version of OpenSearch core is 1.0.0-rc1 for RC, so plugins CI also builds opensearch-1.0.0-rc1 and plugins are required to be built on it. This is an agreement between plugins and OpenSearch core, for example AD and Index Management that have already made the changes.

@cliu123 Please see my comment #1176 (comment) regarding difference between SNAPSHOT and other tags. Either CI should use https://github.com/opensearch-project/OpenSearch/releases/tag/1.0.0-rc1 or it should use SNAPSHOT. Mixing both is confusing.

As @dblock mentioned on #1176: "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."
I don't really see any potential risks to build OpenSearch with the qualifier rc1 that is consistent with the versions of both OpenSearch core and plugins. It would help to keep consistent across all plugins unless there are risks to do so.

rc1 must correspond to the actual rc1 candidate of OpenSearch, not just to a random/latest commit on the 1.x branch.

@vrozov You're right, what should really happen is that we take an XYZ build of OpenSearch, test it, and label it as rc1 after we're done. However today this is proving itself to be difficult because versions must match across OpenSearch and plugins for everything to start together, including the rc1 qualifier, if any. So it's a chicken and egg problem. I described what we should be doing in opensearch-project/OpenSearch#763, please add your comments.

For this PR I suggest letting it slide to make rc1 because I don't think we can fix 763 in time. But for rc2 or afterwards we should, I'll followup on getting that assigned.

@dblock I don't see a chicken and egg problem here and there was no such problem with the ODFE releases: ES releases first, plugins use released ES artifacts at build, test and runtime. The same process should be followed with Opensearch: rc1 is tagged (best will be to publish artifacts) and plugins should use the tag. Once rc2 is available on Opensearch, plugins should update to that tag. Using latest commit on 1.x branch, but pretending that it is rc1 makes security plugin build unstable and more importantly testing of security plugin would be done against latest commit, not rc1, so it can't guarantee that plugin is compatible with the actual rc1 Opensearch.

@MMakaria: this is an ask to effectively change the release train sequence, I had brought it up before

Changing that would make security plugin built on different version/commits of OpenSearch core from other plugins. Let's have a decision before changing this. Whatever the decision is going to be, all plugins will need to take the same action or to stick with building plugins on top of 1.x branch of OpenSearch core.

build.gradle

…OpenSearch 1.0.0-rc1

vrozov · 2021-05-23T05:32:04Z

.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=rc1 -Dbuild.snapshot=false


rc1 must correspond to the actual rc1 candidate of OpenSearch, not just to a random/latest commit on the 1.x branch.

dblock · 2021-05-24T18:00:49Z

If OpenSearch was built with -rc1 and this is built with -snapshot I think the plugin will fail to start as the versions must match exactly. See opensearch-project/OpenSearch#763. But it doesn't matter for CI, it can be foobar and it will work. Merge on 🍏

dblock

OpenSearch official build is currently building with rc1, so this needs to be rc1, or the plugin won't start.

This has been discussed in the comments of the PR.

…OpenSearch 1.0.0-rc1 (opensearch-project#1179)

Signed-off-by: Peter Nied <[email protected]>

dblock previously approved these changes May 22, 2021

View reviewed changes

dblock mentioned this pull request May 22, 2021

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

Closed

cliu123 requested a review from a team May 22, 2021 14:45

andy840314 previously approved these changes May 22, 2021

View reviewed changes

vrozov suggested changes May 22, 2021

View reviewed changes

cliu123 dismissed stale reviews from andy840314 and dblock via af7cefb May 22, 2021 22:14

cliu123 requested a review from vrozov May 22, 2021 22:16

Add build.version_qualifier and make security plugin compatible with …

5041b33

…OpenSearch 1.0.0-rc1

cliu123 force-pushed the bump_plugin_version_to_1.0.0.0-rc1 branch from af7cefb to 5041b33 Compare May 22, 2021 23:08

vrozov previously requested changes May 23, 2021

View reviewed changes

cliu123 mentioned this pull request May 24, 2021

Allowing granular access for data-stream related transport actions #1170

Merged

dblock previously approved these changes May 24, 2021

View reviewed changes

update the opensearch dependency from rc1 to snapshot

55fcef8

cliu123 dismissed dblock’s stale review via 55fcef8 May 24, 2021 17:44

cliu123 requested review from vrozov, dblock, andy840314 and camerski May 24, 2021 17:45

cliu123 added 2 commits May 24, 2021 11:25

Change qualifier and dependencies back from snapshot to rc1

8704c15

Revert the change in plugin version

b478f83

dblock approved these changes May 24, 2021

View reviewed changes

camerski approved these changes May 24, 2021

View reviewed changes

cliu123 merged commit 5ffabaa into opensearch-project:main May 24, 2021

cliu123 deleted the bump_plugin_version_to_1.0.0.0-rc1 branch May 24, 2021 19:43

cliu123 added the maintenance Project maintenance label May 26, 2021

lbreinig pushed a commit to lbreinig/security that referenced this pull request Dec 23, 2021

Add build.version_qualifier and make security plugin compatible with …

943510a

…OpenSearch 1.0.0-rc1 (opensearch-project#1179)

wuychn pushed a commit to ochprince/security that referenced this pull request Mar 16, 2023

Add build.version_qualifier and make security plugin compatible with …

988f324

…OpenSearch 1.0.0-rc1 (opensearch-project#1179)

gaobinlong pushed a commit to gaobinlong/security that referenced this pull request Aug 30, 2023

Support checkout out #.X branch types (opensearch-project#1179)

d46e60a

Signed-off-by: Peter Nied <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

cliu123 commented May 22, 2021

codecov-commenter commented May 22, 2021 •

edited

Loading

vrozov May 22, 2021

cliu123 May 22, 2021 •

edited

Loading

vrozov May 23, 2021

cliu123 May 23, 2021

vrozov May 23, 2021

dblock May 23, 2021

vrozov May 23, 2021

dblock May 24, 2021

dblock May 24, 2021

cliu123 May 24, 2021

vrozov May 23, 2021

dblock commented May 24, 2021 •

edited

Loading

dblock left a comment •

edited

Loading

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

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

Conversation

cliu123 commented May 22, 2021

opendistro-for-elasticsearch/security pull request intake form

codecov-commenter commented May 22, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

cliu123 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

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

dblock commented May 24, 2021 • edited Loading

dblock left a comment • edited Loading

Choose a reason for hiding this comment

codecov-commenter commented May 22, 2021 •

edited

Loading

cliu123 May 22, 2021 •

edited

Loading

dblock commented May 24, 2021 •

edited

Loading

dblock left a comment •

edited

Loading