Use a per-install name for securityconfig secret

[smlx]

Description

Give the securityconfig secret an autogenerated unique name to
facilitate installing the chart multiple times in the same namespace.

This helps with the common case of sharing the securityconfig between
multiple instantiations of this chart to construct an Opensearch
cluster.

Issues Resolved

Closes: #40

Check List

• 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.

[DandyDeveloper]

The problem with this is the previous implementation allowed someone to pass in their own template/secret.yaml in their child chart.

This actually becomes more restrictive.

We should support both. Maybe with a {{ default (printf "%s-securityconfig" include "opensearch.uname" . ) .Values.securityConfig.config.securityConfigSecret }} in the statefulset references to the volume?

[smlx]

The problem with this is the previous implementation allowed someone to pass in their own template/secret.yaml in their child chart.

Could you explain how this was supposed to work previously?

The volume bindings on the statefulset have conditionals like this, which is identical to the conditional on the creation of the securityconfig secret object:

{{- if and .Values.securityConfig.config.securityConfigSecret .Values.securityConfig.config.data }}

This means that they wouldn't have been able to supply an external secret name and have the statefulset use it with this mechanism.

I think you can still use an external secret by specifying extraVolumes + extraVolumeMounts?

[DandyDeveloper]

@smlx I'm going to say this wasn't ported very well from the original. Those conditions are a bug, but I think your change still introduces more restrictions.

It'd be more logical to just remove the verbose (and unnecessary) conditions you've outlined.

[smlx]

Ok, so what if we changed it to have this kind of logic:

• if you define anything under data, the chart will automatically create a secret and mount it (this is what the PR does now)
• if you define securityConfigSecret, the chart will assume this secret is created externally and try to mount it.
• it is an error to define both data and securityConfigSecret

Does that satisfy all the use cases? I'll obviously add some comments explaining how this works.

[DandyDeveloper]

@smlx Perfect middle ground. Agreed.

[TheAlgo]

Ok, so what if we changed it to have this kind of logic:

• if you define anything under data, the chart will automatically create a secret and mount it (this is what the PR does now)
• if you define securityConfigSecret, the chart will assume this secret is created externally and try to mount it.
• it is an error to define both data and securityConfigSecret

Does that satisfy all the use cases? I'll obviously add some comments explaining how this works.

@smlx Can we add this explanation in the README for OpenSearch chart. It will be great to avoid any confusions for users.

[peterzhuamazon]

Any progress on this PR @smlx ?

[ashish1099]

Any update on this one, cant pass security setting due to this.

[peterzhuamazon]

Sorry about my misclick on closing the PR, open again.
Hi @DandyDeveloper @TheAlgo can you please take a look and approve?
Thanks.

[smlx]

@DandyDeveloper I updated the logic as described above, please take a look.

[peterzhuamazon]

@DandyDeveloper I updated the logic as described above, please take a look.

Hi @DandyDeveloper @TheAlgo can you guys take a look also @smlx needs to bump versions.
Thanks.

[DandyDeveloper]

@smlx @peterzhuamazon This will probably need to be bumped again depending on what gets merged first.

I've fixed the conflicts and approved.

[smlx]

I've rebased and bumped the version.

[TheAlgo]

Ok, so what if we changed it to have this kind of logic:

• if you define anything under data, the chart will automatically create a secret and mount it (this is what the PR does now)
• if you define securityConfigSecret, the chart will assume this secret is created externally and try to mount it.
• it is an error to define both data and securityConfigSecret

Does that satisfy all the use cases? I'll obviously add some comments explaining how this works.

@smlx Can we add this explanation in the README for OpenSearch chart. It will be great to avoid any confusions for users.

@smlx Can we add it in the README as well?

[smlx]

@TheAlgo ok, added a line to the README.

[peterzhuamazon]

Waiting for @TheAlgo to approve since he already check this.
Thanks.

[TheAlgo]

@peterzhuamazon @mprimeaux @smlx @DandyDeveloper Should be make this 1.1.0?

@smlx Rest of the changes looks good to me.

[mprimeaux]

@peterzhuamazon Yes to 1.1.0 as this introduces a new non-breaking feature and therefore mandates a minor version number increment.

[smlx]

OK, changed version to 1.1.0.

[peterzhuamazon]

@peterzhuamazon Yes to 1.1.0 as this introduces a new non-breaking feature and therefore mandates a minor version number increment.

Agree. 1.1.0.

[bitnik]

FYI: I think this was a breaking change and required a major upgrade. Because securityConfig.config.securityConfigSecret was used as name for internally created secret before and now it is used for an externally defined one. Previously you had to set both of them to make chart create a secret but now after this update it fails with "Only one of .Values.securityConfig.config.data and .Values.securityConfig.config.securityConfigSecret may be defined. Please see the comment in values.yaml describing usage.".

[smlx]

Thanks for the heads up @bitnik.

@TheAlgo would you accept a PR to bump the chart to 2.0.0 to address this?

[DandyDeveloper]

@smlx I think that's the best way to address this.