NodeUtils: export upcall variables to the environment - #705
Conversation
|
The docs now state it explicitly: exported variables take precedence over any same-named variables from the calling environment. Flagging for the 1.11 release notes as a minor behavior change: upcall commands no longer inherit caller-exported |
| @@ -217,8 +217,11 @@ def _upcall_read(self, cmdtpl, args=dict()): | |||
| """ | |||
| cmdline = Template(self.upcalls[cmdtpl]).safe_substitute(args) | |||
There was a problem hiding this comment.
if we start supporting variable interpolation through real shell variables (through environment), it seems we do not need the template anymore? The shell variable supports below will handle everything, no?
There was a problem hiding this comment.
Nope. The shell doesn't expand variables inside single quotes, and single-quoted $GROUP is used in the field (just look at man groups.conf, single quotes are used in the example). So the environment export complements the template (for shell-side expansions like ${GROUP:-default}) rather than replacing it.
There was a problem hiding this comment.
OK, a bit weird, as now the expansion will happen sometimes under single quote, and sometimes not. User may be surprised.
There was a problem hiding this comment.
Template.safe_substitute() is pure text replacement and has always ignored shell quoting. $GROUP is always replaced if found like that in the command text. The only surprise might come from existing ${GROUP:-default} patterns not in single quote that were not resolved and that will then be resolved with this patch. It's actually the bug I am trying to fix here as I saw it in the field. :)
That said, I agree a single mechanism (shell-only) would probably be cleaner, but that breaks single-quoted $GROUP configs, including provided example configs and man pages. If we think it's a big deal (not sure), it's something we could consider to deprecate in a 2.0 maybe.
There was a problem hiding this comment.
OK, so, what would be the less surprising to people over time would be for the variable to be 100% shell variables. So, let's keep the patch as-is, but, in the same time, we must change examples to use double quotes where appropriate. Let's write them as we were only supporting shell variables. We could deprecate that later.
There was a problem hiding this comment.
Sounds good to me and actually that's a very good point: it would be possible to have an empty variable with this I think, so it is highly recommended to double-quote variables in upcalls. Best practice anyway (SC2086). Thus, this will start a clean migration to shell-only variables in the future. I'll update this PR (docs, examples) with this.
Group source upcalls only support textual substitution of $GROUP,
$NODE, $SOURCE and $CFGDIR. Also export them as environment variables
so shell parameter expansions like ${GROUP:-default} work as users
expect, instead of silently reading the caller's environment.
Write the example upcall commands in plain shell style, with
double-quoted variables, so they work with either mechanism.
Signed-off-by: Stephane Thiell <stephane@thiell.com>
2 participants
Upcall commands already inherit the caller's full environment. This change simply sets the four substitution variables (
$GROUP,$NODE,$SOURCE,$CFGDIR) in that environment, so shell parameter expansions that are not replaced by the library, like${GROUP:-default}, expand to the same values instead of silently reading whatever the caller exported.$GROUPand$NODEare set empty when they do not apply to the upcall. Textual substitution is unchanged and still happens first, so existing configs behave identically.This makes it easy for a map upcall to support a default when the group name is empty, e.g. for query-backed sources:
Here each Prometheus scrape job is a node group, resolving to the hostnames of its targets:
@prometheus:node_exporterexpands to the nodes scraped by thenode_exporterjob, and@prometheus:(empty group name) expands to the nodes of all jobs (useful for map-only sources, where@prometheus:*cannot work).See #609 for a related earlier discussion (FYI).
Docs updated: groups.conf(5) + sphinx config.
Closes #704