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

Remove default opensearch.yml config in Values.yaml to avoid security plugin conflicts #618

Merged
merged 7 commits into from
Nov 24, 2024

Conversation

LemonDouble
Copy link
Contributor

Description

[Describe what this change achieves.]

Remove default opensearch.yml config in Values.yaml to avoid security plugin conflicts.
Now, when setting up Opensearch with the default configuration, it installs correctly with the demo configuration.

Issues Resolved

[List any issues this PR will resolve. You should likely open an issue if one does not already exist.]

Resolve #617

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.

@LemonDouble
Copy link
Contributor Author

I've modified the PR as it seems there was an issue with the version bump section.

@peterzhuamazon
Copy link
Member

Hi @DarshitChanpura @derek-ho I remember you guys specifically changed this section.
Could you check if this PR?

Thanks.

@cwperks
Copy link
Member

cwperks commented Nov 21, 2024

Thank you for fixing this @LemonDouble!

I encountered the same issue and raised a nearly identical PR. The reason this is happening now is because of opensearch-project/security#4793 and that the values.yml for the opensearch chart currently predefines the configuration of the security plugin. By commenting it out in this PR, it allows the security plugin to install the demo configuration which includes:

  1. Writing settings to opensearch.yml
  2. Installing certificates in config folder
  3. Installing the demo security config files in config/opensearch-security

@prudhvigodithi
Copy link
Member

Thanks @cwperks and @LemonDouble, in this PR can we keep

opensearch.yml: |
    cluster.name: opensearch-cluster
    # Bind to all interfaces because we don't know what IP address Docker will assign to us.
    network.host: 0.0.0.0

This way users know how to add/update the opensearch.yml using the values file.

Signed-off-by: LemonDouble <[email protected]>
@LemonDouble LemonDouble requested a review from cwperks November 21, 2024 20:22
@LemonDouble
Copy link
Contributor Author

@cwperks

It seems that the opensearch.yml section in charts/opensearch/ci/ci-ingress-class-name-values.yaml was not commented out, causing the CI to fail. How should I handle this?

@LemonDouble
Copy link
Contributor Author

LemonDouble commented Nov 21, 2024

It all appears to be Demo Configuration, so it seems possible to either comment out everything or remove it entirely.

Co-authored-by: Craig Perkins <[email protected]>
Signed-off-by: LemonDouble <[email protected]>
@LemonDouble LemonDouble requested a review from cwperks November 21, 2024 22:28
@cwperks
Copy link
Member

cwperks commented Nov 21, 2024

@LemonDouble I'm also new to this repo, but I believe you need to comment out the demo security values from all the files in https://github.com/opensearch-project/helm-charts/tree/main/charts/opensearch/ci as well for the CI Checks to pass.

@LemonDouble
Copy link
Contributor Author

Confirmed. I have also added comments to the Values.yaml file in the CI pipeline to fix the CI pipeline.

Copy link
Member

@cwperks cwperks left a comment

Choose a reason for hiding this comment

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

Thank you @LemonDouble . Left 2 more comments.

charts/opensearch/CHANGELOG.md Outdated Show resolved Hide resolved
charts/opensearch/ci/ci-ingress-class-name-values.yaml Outdated Show resolved Hide resolved
Co-authored-by: Craig Perkins <[email protected]>
Signed-off-by: LemonDouble <[email protected]>
@prudhvigodithi
Copy link
Member

Thanks @cwperks and @LemonDouble, in this PR can we keep

opensearch.yml: |
    cluster.name: opensearch-cluster
    # Bind to all interfaces because we don't know what IP address Docker will assign to us.
    network.host: 0.0.0.0

This way users know how to add/update the opensearch.yml using the values file.

@LemonDouble @cwperks please check.

Copy link
Member

@cwperks cwperks left a comment

Choose a reason for hiding this comment

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

Thank you @LemonDouble!

@LemonDouble
Copy link
Contributor Author

@prudhvigodithi I reviewed your comments and, following cwperks' review, disabled only the Security Plugin while keeping the rest uncommented.

@prudhvigodithi
Copy link
Member

Thanks @LemonDouble and @cwperks I will merge the PR once the CI's pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

[BUG][Opensearch] It is not possible to deploy the chart with the default settings.
4 participants