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

[Bug Fix] Fix the demo configuration script and remove the admin credential from internal_user.yml #3449

Closed

Conversation

RyanL1997
Copy link
Collaborator

@RyanL1997 RyanL1997 commented Oct 4, 2023

Description

By working with @DarshitChanpura for addressing the following issue:

opensearch-project/opensearch-build#4094 (comment)

We need the initiation of the cluster to be failed once we encountered a failure of our demo configuration script. This PR introduces the method that if we failed to set up a admin credential during the set up of demo configuration script, it will lead to an absent of admin credential section in internal_users.yml, so that the initiation of the cluster will be failed.

  • Category (Enhancement, New feature, Bug fix, Test fix, Refactoring, Maintenance, Documentation)
    Bug fix
  • What is the old behavior before changes and new behavior after changes?
    The cluster initiation will be succeed and keep using the admin:admin credential even if the there is an hard exit on the set up on demo configuration script.

Issues Resolved

Testing

CI

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 Oct 4, 2023

Codecov Report

Merging #3449 (a3fdf3c) into main (7924da1) will decrease coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #3449      +/-   ##
============================================
- Coverage     64.56%   64.54%   -0.02%     
+ Complexity     3545     3544       -1     
============================================
  Files           269      269              
  Lines         20363    20363              
  Branches       3376     3376              
============================================
- Hits          13147    13143       -4     
- Misses         5525     5529       +4     
  Partials       1691     1691              

see 2 files with indirect coverage changes

@RyanL1997 RyanL1997 added the backport 2.x backport to 2.x branch label Oct 4, 2023
@RyanL1997
Copy link
Collaborator Author

Note: the BWC failure is expected due to the release related version changes.

@@ -52,6 +58,33 @@ jobs:
- name: Checkout security
uses: actions/checkout@v4

- name: Insert Admin Credential on Linux
Copy link
Member

Choose a reason for hiding this comment

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

What is broken without this section?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Based on the log:

 - org.opensearch.security.InitializationIntegrationTests.testDefaultConfig
 - org.opensearch.security.InitializationIntegrationTests.testInvalidDefaultConfig
 - org.opensearch.security.TransportUserInjectorIntegTest.testSecurityUserInjection
 - org.opensearch.security.SlowIntegrationTests.testDelayInSecurityIndexInitialization

For example, the test of InitializationIntegrationTests.testDefaultConfig is using the admin admin credential to send out the request:

HttpResponse res = rh.executeGetRequest("/_cluster/health", encodeBasicHeader("admin", "admin"));

@@ -10,13 +10,6 @@ _meta:

## Demo users

admin:
Copy link
Member

Choose a reason for hiding this comment

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

Does the cluster fail to startup with this change? Could you include what that looks like?

@DarshitChanpura
Copy link
Member

We do not want a cluster to start without admin credentials. What do you think about closing this @RyanL1997 ?

@peternied
Copy link
Member

@RyanL1997 Since the password change was reverted in 2.11 - lets regroup and figure out how to approach this. I'm going to close out this PR - we can reopen if we need it. Thanks

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.

3 participants