Skip to content
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

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

Open
benhoyt opened this issue Jan 23, 2023 · 9 comments
Open

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

benhoyt opened this issue Jan 23, 2023 · 9 comments
Labels
Feature A feature request Low Priority The opposite of "Priority"

Comments

@benhoyt
Copy link
Contributor

benhoyt commented Jan 23, 2023

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.

@benhoyt benhoyt changed the title Add $PATH-style interpolation into environment variable handling Add $VAR-style interpolation into environment variable handling Jan 23, 2023
@cjdcordeiro
Copy link
Collaborator

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
Copy link
Contributor Author

benhoyt commented Nov 9, 2023

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
Copy link
Contributor Author

benhoyt commented Apr 3, 2024

@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
Copy link
Collaborator

@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.

@benhoyt benhoyt added 24.04 and removed 24.04 labels May 17, 2024
@IronCore864
Copy link
Contributor

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:

  • Create an env var named FOO whose value is The $BAR bazzes..
  • The $BAR part means to get the value from env var BAR. If it's not set, it should be empty.

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:

  • ${USER} means getting the value from env var
  • var.bar means getting the value from variables defined before the services.

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
Copy link
Contributor Author

benhoyt commented Aug 5, 2024

@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
Copy link
Contributor Author

benhoyt commented Aug 5, 2024

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
Copy link
Contributor

IronCore864 commented Aug 5, 2024

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

Will do a simple spec later.

@benhoyt
Copy link
Contributor Author

benhoyt commented Sep 30, 2024

Link to spec OP048 for reference.

@benhoyt benhoyt removed the 24.10 label Oct 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature A feature request Low Priority The opposite of "Priority"
Projects
None yet
Development

No branches or pull requests

3 participants