canonical / pebble

Pebble is a lightweight Linux service manager with layered configuration and an HTTP API.
https://canonical-pebble.readthedocs-hosted.com/
GNU General Public License v3.0
144 stars 54 forks source link

Add $VAR-style interpolation into environment variable handling #182

Open benhoyt opened 1 year ago

benhoyt commented 1 year ago

A while ago we discussed (in a discussion about whether environment variables should be an ordered list or a map - be we settled on a map for simplicity) that it would be good to have a way to insert other environment variables into the definition for new variables. For example:

services:
    svc:
        override: replace
        command: sleep 1000
        environment:
            THINGDIR: /home/${USER}/thing
            CMD_DIR: /root/sleepy
            CMD_SOCKET: $CMD_DIR/.sock

We could use os.Expand for this. Though we'd have to be careful about doing them in the correct order (topological sort) to resolve dependencies correctly, like the one from CMD_SOCKET to CMD_DIR above.

This is similar to the Kubernetes environment variable feature.

cjdcordeiro commented 11 months ago

Note for future reference: in craft and spread, there is already a notation for handling variables. E.g. $(HOST_SECRET: cat myfile). When addressing this issue, we should consider that and make sure we don't introduce mechanisms that might clash or create ambiguity with existing usages of Pebble.

benhoyt commented 10 months ago

We should also consider enabling this in command. Some folks have been using sh -c as a workaround, for example:

services:
  bind9:
    override: replace
    # Note: we use `sh -c` to expand the `$SNAP` environment variable
    command: sh -c "exec systemd-cat -t named $SNAP/bin/run-named"
benhoyt commented 5 months ago

@cjdcordeiro What does the craft/spread notation $(HOST_SECRET: cat myfile) mean? Use the variable of environment variable HOST_SECRET, but if not set, fall back to running the command cat myfile and use that? Or if not set, use the literal string cat myfile?

cjdcordeiro commented 5 months ago

@benhoyt the syntax is $(<scope>:<cmd>). So HOST_SECRET is an identifier of the scope, telling the tool where to run the cmd. It is much more relevant for things like Spread and Rockcraft, since both work with "backends" and sometimes you want to run commands outside the backend. Although similar, it does not behave like the shell syntax ${FOO:-bar}.

I've left that comment above as I think it might be worth considering a similar implementation here. Maybe that's overkill, but worth considering nonetheless. I could imagine something like:

services:
  bind9:
    override: replace
    command: app1 $(othersvc: echo $FOO)

  othersvc:
    override: replace
    command: app2
    environment:
      FOO: bar2

P.S. if simply allowing interpolation for command: echo $FOO is not a problem and doesn't cause a regression for existing users, then I don't think there's a need to go down the path of the $(:) syntax.

IronCore864 commented 1 month ago

A while ago we discussed (in a discussion about whether environment variables should be an ordered list or a map - be we settled on a map for simplicity) that it would be good to have a way to insert other environment variables into the definition for new variables. For example:

services:
    svc:
        override: replace
        command: sleep 1000
        environment:
            THINGDIR: /home/${USER}/thing
            FOO: "The $BAR bazzes."
            BAR: bar

We could use os.Expand for this. Though we'd have to be careful about doing them in the correct order (topological sort) to resolve dependencies correctly, like the one from FOO to BAR above.

My 5 cents: I've given some initial thought to it, and I find this example a bit confusing.

The Confusing Part

When I see this config, to me, FOO: "The $BAR bazzes." means:

To me, the definition is equivalent to:

if [[ -n $BAR ]]; then
  FOO="The $BAR bazzes."
else
  FOO="The bazzes."
fi

So, if:

export user="tiexin"
export BAR="some alue"

I'd translate the layer config to:

variables:
  bar: "some value"

services:
  svc:
    override: replace
    command: sleep 1000
    environment:
      THINGDIR: /home/tiexin/thing
      FOO: "The some value bazzes."
      BAR: "bar"

Then I continued to read the line BAR: bar, I started to realise that maybe here BAR: bar is defined as a variable, the value should be used in the previous line.

I think the ambiguity comes from the fact that $BAR means getting the value from the env var, not something else defined in the same YAML file.

Possible Solution

To solve this, we need to introduce the concept of variables. If we can define something like:

variables:
  bar: "some value"

services:
  svc:
    override: replace
    command: sleep 1000
    environment:
      THINGDIR: /home/${USER}/thing
      FOO: "The var.bar bazzes."
      BAR: "var.bar"

Then it's crystal clear that:

Except we can't use var.bar directly, because it's just a string and hard to parse; and we probably can't use {{ var.bar }} either, since {{ ... }} is the delimiter of text/template, in case we want to render the YAML with variables.

So, maybe [[ var.bar ]]? For reference, GitHub Actions uses ${{ vars.xxx }}. Then, the final config looks like:

variables:
  bar: "some value"

services:
  svc:
    override: replace
    command: sleep 1000
    environment:
      THINGDIR: /home/${USER}/thing
      FOO: "The [[ var.bar ]] bazzes."
      BAR: "[[ var.bar ]]"

Which is exactly how GitHub Actions and Terraform do it.

Final Thought

Adding support to env vars and variables might be an overkill, though. Is there a concrete example of which charm needs this feature?

In my opinion, only adding support to env var is more than enough.

services:
  svc:
    override: replace
    command: sleep 1000
    environment:
      THINGDIR: /home/${USER}/thing
      FOO: "The some value bazzes."
      BAR: "some value"

This is more than OK IMHO, although it has some duplicated content, but is there a concrete example of having lots of duplicated values in the layer config and causing troubles?

benhoyt commented 1 month ago

@IronCore864 and I discussed this briefly today. I personally don't find it confusing: they're all environment variables, it's just whether they come from the parent (Pebble) environment or the current environment being defined in the configuration layer. I've updated the example to be a bit more realistic, however, and the ordering to be more obvious.

Kubernetes uses similar syntax, though it uses parentheses instead of curly brackets (and it looks like only "current environment" is in play here.

Still, I don't think the original proposal is confusing, and I think we should stick with it. Go's os.Expand function handles either $VAR or ${VAR} syntax (you need to use the curly brackets if you have a letter/digit right after the variable name, for example ${VAR}foo).

@IronCore864 is going to create a short (1-2 page) spec officially proposing this as the next spec.

benhoyt commented 1 month ago

We also have to consider:

1) How to get a literal $ in the resulting value? My suggestion is $$. You can achieve this with os.Expand by adding "$": "$" to the mapping function. 2) That this is a backwards-incompatible change. Previously $ in an environment variable value would have meant a literal $. Does this matter? Would anyone be using $?

IronCore864 commented 1 month ago

PoC: https://github.com/IronCore864/yaml-config-env-interpolation/tree/main

Will do a simple spec later.