Skip to content

mergeOverwrite mutates objects it shouldn't #188

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

Closed
chrisjohnson opened this issue Sep 25, 2019 · 10 comments
Closed

mergeOverwrite mutates objects it shouldn't #188

chrisjohnson opened this issue Sep 25, 2019 · 10 comments
Assignees
Labels

Comments

@chrisjohnson
Copy link

I'm using helm which depends on sprig. I wrote a simple fiddle which includes the helm template I'm testing with, as well as some go code that emulates how helm parses the template. You can see it here https://play.golang.org/p/imA_4tZJvPN

So I've got two objects, $app and $deployments, and if I run mergeOverwrite (dict) $app $deployments I would expect the new dict to first be mutated with the contents of $app and then with the contents of $deployments. But it seems that values from $deployments are being written into $app (as well as the new dict)

@chrisjohnson
Copy link
Author

chrisjohnson commented Sep 26, 2019

Further reduced to clear away irrelevant noise https://play.golang.org/p/nLlAeyqsj7G

https://play.golang.org/p/vSn2f2C3kBf

It seems like the "deep merge" may not be cloning recursively, which may make sense in a programming sense but in a template you would expect to not have to worry about shared objects.

@mattfarina mattfarina added bug and removed bug labels Sep 26, 2019
@mattfarina
Copy link
Member

I created an example that's a little more explicit at https://play.golang.org/p/MuGRKD0PEA-. The idea is to see the exact point $app changes to data you don't expect.

It took a little digging but I think I have an idea of what's going on.

Under the hood we use https://github.com/imdario/mergo to accomplish this. Specifically, MergeWithOverwrite which is a wrapper around Merge with the config option WithOverride being set. The docs note:

non-empty dst attributes will be overridden by non-empty src attribute values.

Digging around in the code I found that it sets the dst to the source rather than a copy of the source. See https://github.com/imdario/mergo/blob/4c317f2286be3bd0c4f1a0e622edc6398ec4656d/merge.go#L180. In our playground examples, the first time mergeOverwrite is run it sets the app object to the dst. Not a copy. Then on the second pass involving deployments is walks the files on the dst which walks that instance on app and changes the values. So, under the hood it's not a deep copy.

If we wanted it to be a deep copy we could use a Transformer that intercepts the work of mergo and does out own copy/merge.

@technosophos any thoughts?

@chrisjohnson
Copy link
Author

Thanks for following up on this for me matt


If anybody's struggling with this my workaround for now is something like this:

{{- define "dbdeployer.deployments" }}
{{- $args := mergeOverwrite (dict) . -}}
{{- $_ := set $args "dbdeployerArgs" (mergeOverwrite (dict) $args.dbdeployerArgs (dict "name" "deployments")) -}}
{{- $_ := set $args "mountDatabases" false -}}
{{- template "dbdeployer.spec" $args }}
{{- end -}}

It doesn't work if you don't know in advance what the to-be-merged structure looks like since it relies on replacing the specific child-object with another one by name but it's letting me move past this until we find a proper solution

@bitsofinfo
Copy link

bitsofinfo commented Sep 30, 2019

i'd leave this method as-is to avoid breaking folks who are unknowingly (or knowingly) reliant on this behavior, document the behavior clearly, then just implement the desired in #149

@mattfarina
Copy link
Member

@bitsofinfo That's a good idea.

@mattfarina mattfarina self-assigned this Sep 30, 2019
@chrisjohnson
Copy link
Author

chrisjohnson commented Sep 30, 2019

Not being familiar with golang templates, are breaking changes an absolute no-no in this space? I would consider the impact and the amount of boilerplate (thing | deepCopy)s that decision would translate to throughout the ecosystem.

And I can't help but wonder how many people would actually suffer from changing mergeOverwrite that depend on that specific behavior? My instinct is that most people aren't actually depending on this shared object behavior, and are either living with a bug without knowing it, or not using a complicated-enough template that it matters. It's not a very old feature is it?

@mattfarina
Copy link
Member

@chrisjohnson This is a great question

When it comes to the Go API, making breaking changes ends up being a fair amount of work for the pkg maintainers. go get prior to modules didn't handle versions and even when people had used a package manager not everyone would follow it. So, a breaking change would lead to issues being filed and in general more work for maintainers. For anything with many users the work and complaints could add up.

I changed an API on a package recently and the issues started coming in and people started tracking me down.

The template functions here are used by many people. More than I know. I was mostly familiar with Helm, the Kubernetes package manager. But, it turns out there are others. If the people who run write these applications update to a newer version of sprig and people who author templates have breaking changes there will be complaints and possibly issues. All of this is more work for we maintainers.

So, we are careful when and how we break APIs because the repercussions is more work on us.

That being said, I'm torn on this issue.

@bitsofinfo
Copy link

bitsofinfo commented Sep 30, 2019

leave mergeOverwrite as-is and just document its existing behavior very clearly, then provide a reference to the new deepCopy function as an alternative.

@mattfarina
Copy link
Member

I created #194 to address this issue.

There are two reasons I didn't change the underlying implementation. They are:

  1. Merging is already used in numerous templates and places. Those same templates will be processed both prior and after this change for some tools (like the one I use). I'm being conservative on changes because it affects production operation situations.
  2. Template functions many times reflect the API of the underlying functions being called. In this case the current situation and the one with this PR reflects the underlying operations.

I understand this is a conservative approach to the change and for some it might not be idea. The way these template functions are used in the generation of application operation manifests for production situations makes me conservative in changes.

@mattfarina
Copy link
Member

Closing as the docs on using deepCopy with merge are now in.

choover-broad added a commit to broadinstitute/terra-helm that referenced this issue Jul 7, 2020
…perf (#64)

* Configure chart to deploy a single standalone Cromwell instance by default
  (Terra's 4-deployment live environment configuration has been migrated to terra-helmfile)
* Remove hardcoded Broad office IPs from default values (these will be supplied by terra-helmfile via a global value)
* Make it possible to disable a configured deployment by adding a new deployment setting, `enabled`
* More careful merging of dictionaries (see Masterminds/sprig#188)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants