opensearch-project / helm-charts

:wheel_of_dharma: A community repository for Helm Charts of OpenSearch Project.
https://opensearch.org/docs/latest/opensearch/install/helm/
Apache License 2.0
173 stars 234 forks source link

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

Closed DarshitChanpura closed 9 months ago

DarshitChanpura commented 11 months ago

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

For any changes to files within Helm chart directories:

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.

DarshitChanpura commented 11 months ago

~@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 commented 10 months ago

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

DarshitChanpura commented 9 months ago

@prudhvigodithi Could you please review this?

DarshitChanpura commented 9 months ago

@prudhvigodithi Could you please add 1 more review?

prudhvigodithi commented 9 months ago

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 commented 9 months ago

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.

prudhvigodithi commented 9 months ago

Thanks @DarshitChanpura for your contribution, there are some more changes required, I'm closing this PR in favor of https://github.com/opensearch-project/helm-charts/pull/518.