-
Notifications
You must be signed in to change notification settings - Fork 391
Fix sensitive values being replaced with '(sensitive value)' in Helm deployments #1644
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
Conversation
…deployments When using set_sensitive in the helm provider, the actual values deployed to Kubernetes were incorrectly replaced with the string '(sensitive value)' instead of the real sensitive value. Root cause: The logValues() function was using maps.Clone() which only creates a shallow copy. Since Helm values are nested maps (e.g., {'configmap': {'foo': 'test'}}), the inner maps were shared between the original and clone. When cloakSetValues() modified the 'cloned' map to mask sensitive values for logging, it was actually modifying the original map that was then passed to Helm for deployment. Fix: - Implemented deepCloneMap() function that recursively clones nested maps - Updated logValues() to use deepCloneMap instead of maps.Clone - Updated setReleaseAttributes() to use deepCloneMap for consistency - Removed unused 'maps' import This ensures sensitive values are properly masked in logs and Terraform state display while the actual values are correctly deployed to Kubernetes. Fixes the issue where ConfigMaps would contain '(sensitive value)' instead of the actual sensitive data when using set_sensitive blocks.
@jrhouston would you mind taking a look? |
This PR fixes the |
Thanks for jumping in to fix this @abatilo. I've pushed a reproducer test for this to your branch, which passes with your change. It looks like the code that got missed in the migration from SDKv2 is this janky JSON Unmarshal/Marshal step which I guess is a cheat way to do a deep copy. terraform-provider-helm/helm/resource_release.go Lines 1425 to 1431 in afc64ed
|
Of course @jrhouston , happy to try and contribute. Sorry, can you help me understand what I should do next? I see some acceptance tests are failing. Should I be the one trying to fix those? Or are you taking over the branch, etc? |
My apologies, that test was failing because I added a secret to the test chart for testing the sensitive values but forgot to update one of the tests which asserts the number of templates in the chart. I've fixed the test and pushed it, everything should turn green now. I think this PR will be good to go, we will need a second approval from @jaylonmcshan19-x before I can hit merge. |
Amazing. Thank you so much. |
I know this does not help in any way, but: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing this bug @abatilo @jrhouston :)
When using set_sensitive in the helm provider, the actual values deployed to
Kubernetes were incorrectly replaced with the string '(sensitive value)' instead
of the real sensitive value.
Root cause:
The logValues() function was using maps.Clone() which only creates a shallow
copy. Since Helm values are nested maps (e.g., {'configmap': {'foo': 'test'}}),
the inner maps were shared between the original and clone. When cloakSetValues()
modified the 'cloned' map to mask sensitive values for logging, it was actually
modifying the original map that was then passed to Helm for deployment.
Fix:
This ensures sensitive values are properly masked in logs and Terraform state
display while the actual values are correctly deployed to Kubernetes.
Fixes the issue where ConfigMaps would contain '(sensitive value)' instead of
the actual sensitive data when using set_sensitive blocks.