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

Use configured helm home when reading default TLS settings #210

Merged
merged 2 commits into from
Feb 20, 2019
Merged

Use configured helm home when reading default TLS settings #210

merged 2 commits into from
Feb 20, 2019

Conversation

yorinasub17
Copy link
Contributor

This is a proposed changeset that tweaks the default values of client_key, client_certificate, and ca_certificate.

Specifically, the current behavior mimics the client in looking for the files key.pem, cert.pem, and ca.pem in HELM_HOME. However, if you have HELM_HOME env var unset, then it does not fall back to looking in the default helm home $HOME/.helm because it renders the env var directly.

This PR instead attempts to use the value of the home option on the provider, which does the proper resolution of the helm home directory.

Additionally, I added some debug statements around the TLS settings. This was helpful in realizing my TLS config wasn't being read despite having them in my home directory. Otherwise, you get an unhelpful error message:

2019-02-08T10:37:54.021-0800 [DEBUG] plugin.terraform-provider-helm: 2019/02/08 10:37:54 [DEBUG] Created tunnel using local port: '50838'
2019-02-08T10:37:54.669-0800 [DEBUG] plugin.terraform-provider-helm: 2019/02/08 10:37:54 [DEBUG] could not get release rpc error: code = Unavailable desc = tr
ansport is closing

@yorinasub17
Copy link
Contributor Author

/cc @rileykarson

@rporres
Copy link

rporres commented Feb 14, 2019

This is a breaking change, @yorinasub17. Those who rely on HELM_HOME will see their setup broken, and that should not happen. Furthermore, the provider should mimic helm client behaviour unless it's necessary to depart.

Any reason why you can't to set HELM_HOME env variable?

@yorinasub17
Copy link
Contributor Author

yorinasub17 commented Feb 14, 2019

Hey @rporres thanks for the feedback!

If I am not mistaken, HELM_HOME should still work because the home config var defaults to it, unless I am misunderstanding this line of code.

In terms of mimicing the client behavior, that is actually what I was trying to do here :) I think this more closely aligns with the client. In the client, if you don't have a HELM_HOME set, the home dir is assumed to be ~/.helm, and the TLS conf will be read from there. So all you have to do for the client is download the TLS certs into ~/.helm and it works with no env vars set, though you still need to pass in --tls.

This isn't the same with the provider. In the current way, the provider doesn't fall back to ~/.helm if HELM_HOME isn't set, looking for TLS settings in the root folder, which I think is less intuitive and more divergent from the client.

This change addresses that gap by relying on the default lookup logic implemented in the home config var, which does the proper resolution (the set value => HELM_HOME => ~/.helm). I could duplicate the logic with the TLS settings as well, but I figured it would be nice to use the home config because there is a bonus where you can set a custom home value there and it will lookup for the TLS settings there, in addition to supporting HELM_HOME and ~/.helm.

Or, are you referring to the fact that this would break workflows where there is a custom home config that is DIFFERENT from HELM_HOME? If so, I actually would like to know that use case better because that is something I hadn't thought about.

FWIW, if I am not misunderstanding this logic, the helm client uses settings.Home for the TLS default lookup (https://github.com/helm/helm/blob/master/cmd/helm/helm.go#L87), which would follow the same behavior as relying on the configured home as proposed in this PR. E.g if you set both --home AND the HELM_HOME env var in the client, the value of --home would be preferred, and in turn would be where the TLS certs are looked up.

With all that said, if you think this is not worth inclusion, I am fine right now with the work around by setting HELM_HOME to ~/.helm, or adding in ~/.helm/key.pem and what not to the provider config. This was more because I found the current behavior slightly unintuitive.

@rporres
Copy link

rporres commented Feb 20, 2019

Thanks a lot for your long and thoughtful reply, @yorinasub17. After reading it, it makes more sense the way we mimic the command behaviour.

@rporres rporres merged commit 96d2b5d into hashicorp:master Feb 20, 2019
@ghost ghost locked and limited conversation to collaborators Apr 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants