charts/csi-powermax: Migrate reverse proxy config from ConfigMap to Secret

[lukeatdell]

Is this a new chart?

No

What this PR does / why we need it:

Migration of the reverse proxy configuration from a ConfigMap to a Secret allows customers to utilize password vaults and CSM-Authorization with the csi reverse proxy.

Which issue(s) is this PR associated with:

• [FEATURE]: CSI-PowerMax - Mount credentials secret to the reverse-proxy (Customer Ask) csm#1614

Special notes for your reviewer:

• key, useSecret, has been added.
  • If the parameter is set to false, the reverse proxy ConfigMap will be created. Otherwise, the user must create the new secret.
• environment variable, X_CSI_POWERMAX_ENDPOINT, has been removed because, now that the reverse proxy is a requirement for installing csi-powermax, it appears to no longer be in use.
• deprecation notices have been added to deprecated parameters in the values file. Templates have been updated to use the values only if the new useSecret is set to false or does not exist as a key in the values file.

Checklist:

• Chart Version bumped
• Title of the PR starts with the chart name (e.g. [charts_dir/mychartname]) if applicable

Tests:

• Manually deployed with global.useSecret: false and global.deployAsSidecar: false and ran cert-csi volume-io tests.
• Manually deployed with global.useSecret: false and global.deployAsSidecar: true and ran cert-csi volume-io tests.

[tdawe]

Clever way to set this parameter without having to explicitly provide it in the values files.

[lukeatdell]

So, the caveat here is, if the user is using their old values file during an upgrade, which does not have secretName in it, the new csireverseproxy values file will provide the default value and try to deploy with the new secret.
As far as I was able to find, there isn't a way to avoid this behavior unless we set secretName: "" in the default csireverseproxy values file.

[tdawe]

You can use hasKey along with the current condition in the helm template to check if Values.secretName exists (https://helm.sh/docs/chart_template_guide/function_list/#haskey). Then it will still work even if they are using the old values file.

[falfaroc]

question: Should this be set to false? Also, I see that it is in two different Values files, should that be the case?

[lukeatdell]

Is that because the example is false or because we don't want to enable it by default?

There doesn't have to be two instances of it, but since the driver's chart has a dependency on the proxy chart, we can set the values for the proxy from here. So these are just the same proxy chart values provided for convenience, and the values will be passed through. Anything not set here will default to whatever is set in the proxy's chart default values file.

[falfaroc]

I think per discussion, we want to default to the config map approach. @tdawe correct me if I'm wrong.

I got it confused with this but this is only used if the reverseproxy is deployed on its own, right?

Lastly, should this be located in the global section of the values? Reason being, since it technically drives the complete configuration and mounting of the controller and driver and not just the reverseproxy.

Just a thought.

[lukeatdell]

hmm I'll wait for @tdawe 's reply.

That's correct. They're the same exact parameter, but the one coming from the main chart supersedes the one you linked.

Your last point makes sense to me, considering the reverse proxy is a requirement for powemax. Only issue is, the parameter might be unavailable if someone were to (for whatever reason) deploy the proxy on its own. But with the requirement in mind, it sounds as if we might want to combine the two charts anyway.

[tdawe]

We'll want to use the ConfigMap by default to support backwards compatibility. Upgrading to this version of the helm chart should be successful if still using the ConfigMap.

[lukeatdell]

Got it. I guess I could have avoided a few PR comments if I'd remembered that conversation lol.
Thank you, both.

[kumarp20]

@lukeatdell Users should be able to use the new helm chart with old values.yaml if they wanted the old behavior of using a configMap ? The PR description says they would need to set the secretName to an empty string. Is that necessary ?

[kumarp20]

Reading through the documentation changes now, I feel setting useSecret to true by default makes sense. We are deprecating the configMap, so it makes sense to start using the secret now as recommended. If user wants to explicitly use the configMap approach, they can choose to do that by toggling the userSecret flag. That makes sense to me.
But when we started this work, we said default behavior would be to use configMap. Perhaps we should discuss with Anand?

[lukeatdell]

Reading through the documentation changes now, I feel setting useSecret to true by default makes sense. We are deprecating the configMap, so it makes sense to start using the secret now as recommended. If user wants to explicitly use the configMap approach, they can choose to do that by toggling the userSecret flag. That makes sense to me. But when we started this work, we said default behavior would be to use configMap. Perhaps we should discuss with Anand?

Right, that was my line of thinking as well. I must have missed the portion of the conversation where we decided to continue using the ConfigMap by default. That appears to be the desired path forward. I'm making changes now to align.