consul-template: revert function_denylist logic

[DerekStrickland]

Closes #11923

This PR fixes a regression that was introduced in 1.2.4 around the Consul Client Template configuration. The changes in that release erroneously merged user configuration with the default configuration for Consul Template. This resulted in a situation where the user could never override the default function deny list. The default FunctionDenylist values should only be applied if the user does not specify that field in their config.

[tgross]

This feels like a bug to me: when would we ever want this to be the result after a merged configuration? Are we validating it elsewhere?

[DerekStrickland]

Yeah, it probably is, and I think you commented on the issue about that. Unfortunately, I pulled old code and confirmed that this currently does not fail validation. I added that comment to the issue thread as well.

My two thoughts other than continuing to allow this are either:

• Remove this check and open an issue to validate empty strings (or remove them?)
• Update this PR to validate empty strings (or remove them?)

Thoughts?

[tgross]

I'd say let's fix this issue once and for all here instead of trying to do it in another issue and having to touch this code again. There are two issues in #11923, but we're really only discussing one of them.

This is the JSON client configuration block from #11923 (comment):

"template": {
  "function_denylist": [
    ""
  ]
}

This is an operator error: we should either correct it silently or return an error on startup. It's not clear to me at any point in the discussion in #11923 whether this configuration actually worked prior to 1.2.4. Generally speaking we don't silently correct things in the configuration, so my take is that this should blow up so the operator can fix their configuration. Especially because this is a security-related configuration.

The second issue is whether the configuration is being merged correctly with the defaults when we're done. It's not clear to me that the merging bug is a problem at all if the configuration were:

"template": {
  "function_denylist": []
}

Because in that scenario the configuration is an empty slice and there's nothing to iterate over during the merge.

[tgross]

Do we only ever call Merge when merging onto the default configuration? If not, we're potentially dropping the merged-onto result by doing this, and it doesn't seem to be protecting us from the [""] case (see the test below).

[shoenig]

@DerekStrickland just double checking I understand - are these all true?

• this is just a reversion of trying to merge function_denylist
• merging function_denylist only happened with the default denylist, which isn't the correct behavior
• we're not trying to fix the [""] quirk here

[DerekStrickland]

@DerekStrickland just double checking I understand - are these all true?

• this is just a reversion of trying to merge function_denylist
• merging function_denylist only happened with the default denylist, which isn't the correct behavior
• we're not trying to fix the [""] quirk here

Exactly that!

[shoenig]

LGTM

[github-actions]

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.