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.

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

Add .plugins-ml-config to the demo configuration system indices #2993

Merged
merged 1 commit into from
Jul 18, 2023

Conversation

Zhangxunmt
Copy link
Contributor

Description

[Describe what this change achieves]

  • Category (Enhancement, New feature, Bug fix, Test fix, Refactoring, Maintenance, Documentation)
    New feature

  • Why these changes are required?
    .plugins-ml-config is the new system index required to store encryption key.

  • What is the old behavior before changes and new behavior after changes?
    Old behavior does not meet the bar from the app sec team. The new method based on this system index is recommended.

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

  • New functionality includes testing
  • New functionality has been documented
  • Commits are signed per the DCO using --signoff

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
Copy link

codecov bot commented Jul 11, 2023

Codecov Report

Merging #2993 (bd31d70) into main (f1be2d7) will decrease coverage by 0.08%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##               main    #2993      +/-   ##
============================================
- Coverage     62.39%   62.32%   -0.08%     
+ Complexity     3380     3369      -11     
============================================
  Files           267      267              
  Lines         19772    19772              
  Branches       3355     3355              
============================================
- Hits          12336    12322      -14     
- Misses         5798     5806       +8     
- Partials       1638     1644       +6     

see 10 files with indirect coverage changes

@cwperks cwperks added the backport 2.x backport to 2.x branch label Jul 11, 2023
cwperks
cwperks previously approved these changes Jul 11, 2023
@cwperks
Copy link
Member

cwperks commented Jul 12, 2023

@Zhangxunmt Is this targeting 2.9?

@Zhangxunmt
Copy link
Contributor Author

Zhangxunmt commented Jul 12, 2023

@cwperks , Yes this is targeting for 2.9 release. Can you please help find another approval?

willyborankin
willyborankin previously approved these changes Jul 12, 2023
@cwperks
Copy link
Member

cwperks commented Jul 12, 2023

@Zhangxunmt Can you confirm if integ tests with security have been run with this change included in each plugin that relies on the index?

k-nn previously had issues when adding system index protection that was caught in their integ tests. You can see discussion on here and the revert in 2.6 happened in this commit

Copy link
Contributor

@stephen-crawford stephen-crawford left a comment

Choose a reason for hiding this comment

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

This is good but will need the dependency versions forced. @cwperks opened a backport PR directly against 2.9 for you in the meantime.

RyanL1997
RyanL1997 previously approved these changes Jul 12, 2023
@Zhangxunmt
Copy link
Contributor Author

#2996, this change is included in this PR.

cliu123
cliu123 previously approved these changes Jul 12, 2023
@cwperks
Copy link
Member

cwperks commented Jul 17, 2023

@Zhangxunmt Can you please re-base this PR with main?

This PR was included directly into 2.9 in this PR, but still needs to be merged into main and 2.x

@Zhangxunmt Zhangxunmt closed this Jul 17, 2023
@Zhangxunmt
Copy link
Contributor Author

This PR can be closed, since it was included in the earlier PR? @cwperks

@cwperks cwperks reopened this Jul 17, 2023
@cwperks
Copy link
Member

cwperks commented Jul 17, 2023

@Zhangxunmt This PR still needs to be merged into main and 2.x. If you rebase this PR we can merge it to main and have backport bot create a PR for 2.x

@Zhangxunmt
Copy link
Contributor Author

@cwperks rebase finished. Can you approve?

@RyanL1997
Copy link
Collaborator

Re-running the flaky windows bwc test

@cwperks
Copy link
Member

cwperks commented Jul 18, 2023

@Zhangxunmt Sorry to have you rebase one more time. This needs to be rebased to get this change

@Zhangxunmt
Copy link
Contributor Author

@cwperks the rebase is successful and no conflicts. Can you pls merge the PR?

@cwperks cwperks merged commit 809aeda into opensearch-project:main Jul 18, 2023
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jul 18, 2023
Signed-off-by: Xun Zhang <[email protected]>
(cherry picked from commit 809aeda)
cwperks added a commit that referenced this pull request Jul 31, 2023
Backport 809aeda from #2993

Co-authored-by: Xun Zhang <[email protected]>
Co-authored-by: Peter Nied <[email protected]>
Co-authored-by: Craig Perkins <[email protected]>
@peternied peternied changed the title add .plugins-ml-config in the system index Add .plugins-ml-config to the demo configuration system indices Aug 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x backport to 2.x branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants