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

Updates HELM configuration to adapt to changes in demo admin user setup and bumps appVersion to 2.12.0 #503

Conversation

DarshitChanpura
Copy link
Member

@DarshitChanpura DarshitChanpura commented Dec 7, 2023

Description

This change adapts to the change in security plugin's demo installation script which now requires admin password to be setup when installing demo configuration. Without this password, the cluster will not spin up.

Issues Resolved

Check List

  • Commits are signed per the DCO using --signoff

For any changes to files within Helm chart directories:

  • Helm chart version bumped
  • Helm chart CHANGELOG.md updated to reflect change

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.

Signed-off-by: Darshit Chanpura <[email protected]>
@DarshitChanpura
Copy link
Member Author

DarshitChanpura commented Dec 13, 2023

@prudhvigodithi to add changes to support for initialAdminPassword.txt

Update: security plugin is not supporting password via txt file and hence this is not required.

@DarshitChanpura DarshitChanpura marked this pull request as ready for review December 14, 2023 21:18
@DarshitChanpura DarshitChanpura changed the title Updates configuration to adapt to changes in demo admin user setup Updates HELM configuration to adapt to changes in demo admin user setup Dec 15, 2023
@DarshitChanpura
Copy link
Member Author

@prudhvigodithi @ruanyl Can I get more reviews on this?

charts/opensearch/Chart.yaml Outdated Show resolved Hide resolved
charts/opensearch/values.yaml Show resolved Hide resolved
charts/opensearch/values.yaml Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Member

@peterzhuamazon peterzhuamazon left a comment

Choose a reason for hiding this comment

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

Please also add a CHANGELOG entry for opensearch on 2.18.0.

Signed-off-by: Darshit Chanpura <[email protected]>
Signed-off-by: Darshit Chanpura <[email protected]>
Signed-off-by: Darshit Chanpura <[email protected]>
README.md Outdated Show resolved Hide resolved
charts/opensearch/CHANGELOG.md Outdated Show resolved Hide resolved
Signed-off-by: Darshit Chanpura <[email protected]>
@DarshitChanpura
Copy link
Member Author

@prudhvigodithi Could you please review this?

@DarshitChanpura
Copy link
Member Author

@prudhvigodithi Could you please add 1 more review?

@prudhvigodithi
Copy link
Collaborator

Thanks @DarshitChanpura, this PR should be merged after 2.12.0 release because it clearly said 2.18.0 requires OPENSEARCH_INITIAL_ADMIN_PASSWORD which is not the case if we merge this PR today. So I would suggest to hold on this PR and merge it with 2.12.0 release. Also since this OPENSEARCH_INITIAL_ADMIN_PASSWORD is a mandatory option for new users, the extraEnvs are optional so this might get lost.

I would suggest to use a new parameter as initialAdminPassword in values.yaml (example https://github.com/opensearch-project/helm-charts/blob/main/charts/opensearch/values.yaml#L3) and backend in statefulset pass this as ENV.

With your change can you please test adding a keystore: [] and enabling masterTerminationFix? because if uses the password with these options.

Thanks

@DarshitChanpura
Copy link
Member Author

I would suggest to use a new parameter as initialAdminPassword in values.yaml (example https://github.com/opensearch-project/helm-charts/blob/main/charts/opensearch/values.yaml#L3) and backend in statefulset pass this as ENV.
With your change can you please test adding a keystore: [] and enabling masterTerminationFix? because if uses the password with these options.

@prudhvigodithi I'm unfamiliar with HELM and hence slightly confused at what the ask is here. We are not supplying a default password. User will always have to pass the password. Our intention is to fail cluster startup if no password was supplied. By adding a new parameter "initialAdminPassword" we are essentially supplying a default value. We are moving away from default hardcoded password for admin.

@DarshitChanpura DarshitChanpura changed the title Updates HELM configuration to adapt to changes in demo admin user setup Updates HELM configuration to adapt to changes in demo admin user setup and bumps appVersion to 2.12.0 Feb 20, 2024
@prudhvigodithi
Copy link
Collaborator

Thanks @DarshitChanpura for your contribution, there are some more changes required, I'm closing this PR in favor of #518.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants