Environment & File Variable replacement should escape their contents

[hermanschaaf]

Right now environment & file variable replacement in yaml config files do a simple string replacement, which can lead to hard-to-debug issues. For example, if the contents of a file contains JSON, the following config:

  spec:
    project_id: "cq-playground"
    dataset_id: "cloudquery"
    service_account_key_json: ${file:key.json}

will throw an error about unexpected keys in the JSON. That's because the JSON gets interpreted as an additional yaml object. The workaround is to place the variable in single quotes:

  spec:
    project_id: "cq-playground"
    dataset_id: "cloudquery"
    service_account_key_json: '${file:key.json}'

Similarly, if you were to use double quotes instead of single quotes there, you'd also get a hard-to-debug error:

  spec:
    project_id: "cq-playground"
    dataset_id: "cloudquery"
    service_account_key_json: "${file:key.json}"

That's because the JSON file contents may include double quotes, which won't get escaped. So you'll get service_account_key_json: "{"name":"value"}" which is invalid yaml syntax.

Or if the file contains newlines, you can also run into similar issues. I don't think this is how variables are intended to be used, and we should escape their contents. I'm not sure what the right way will be to do this; we'd still want number fields to be interpreted as numbers, so we can't (for example) add quotes around interpolated variables.

I'm just writing this issue now so I don't forget about it, but won't go into too much detail now.

[hermanschaaf]

Here's another case that can cause issues:

  spec:
    project_id: "cq-playground"
    dataset_id: "cloudquery"
    # service_account_key_json: '${file:key.json}'

Even though the line is commented out, it expands into multiple lines (most of which are not commented out) and so you get an error related to an unexpected yaml key.

[yevgenypats]

I wonder how this is done in other configuration and variable expansion places. Im not sure we can escape the content and I think it should actually be part of the user responsibility. For example I can think of some edge cases that wont make the escape work:

spec:
    project_id: "something${file:key.json}"

so potentially escaping here wont work.

I think the variable substitution should be just simple search and replace. maybe we can have some flag to print the expanded config to help debug it but I wouldn't go into the rabbit whole of understanding where the variable is and trying to understand if and how we need to quote it.

[hermanschaaf]

@yevgenypats I'm not sure I understand why escaping won't work in this case:

spec:
    project_id: "something${file:key.json}"

Can you explain? My thinking is that regardless of what key.json contains, it should all be part of the project_id string, and there should be no possibility of newlines in key.json causing the yaml interpreter to think there are additional keys.

[yevgenypats]

I think it wont work because in that case:

spec:
    project_id: "something${file:key.json}"

this will turn to:

spec:
    project_id: something'content'

which is different from:

spec:
    project_id: somethingcontent

Basically what Im saying is that we don't have any ability to know how to escape variable expansion the only thing we can do is in case of an error print the expanded output so it will be easier to debug. the escaping must happen on the user side weather it's in the env variable or in the file itself.

[erezrokah]

So I think the solution is not to expand the whole config string, but only YAML values based on a value starting with ${} and not allow ${} anywhere in the config.

TLDR: First parse, then expand

[yevgenypats]

So I think the solution is not to expand the whole config string, but only YAML values based on a value starting with ${} and not allow ${} anywhere in the config.

TLDR: First parse, then expand

I don't think I encountered such logic (i.e context based environment expansion, if there is a poopular tool that does that Im curious to see how they did it), the main reason is that this is a footgun with a lot of edge cases and it has to be the user responsability otherwise it is a huge undertake and we are basically trying to solve an unsolvable problem.

[erezrokah]

I don't think I encountered such logic

I haven't searched others tool but I know GitHub has their own YAML parser to handle GitHub actions, which is a huge issue for them, see actions/runner#1182

[hermanschaaf]

I am confident this is solvable without writing our own parser. Like @erezrokah said,

TLDR: First parse, then expand

However, that will be a breaking change for some users who rely on the fact that they can currently store yaml inside their environment variables and it will get interpreted as yaml. This could be handy for storing a list of tables or accounts, for example. Using unix tools to do it first isn't always easy (e.g. in container environments).

Still, I am of the opinion that we should change this behavior. There are legitimate cases right now that don't work or require hacky workarounds. One such case is when the file being expanded contains newlines (e.g. for service_account_key_json): in many cases you want/expect the entire variable to still be stored as one string, but since it gets interpreted as yaml instead, you get a syntax error.

[erezrokah]

However, that will be a breaking change for some users who rely on the fact that they can currently store yaml inside their environment variables and it will get interpreted as yaml. This could be handy for storing a list of tables or accounts, for example. Using unix tools to do it first isn't always easy (e.g. in container environments).

I think that's an unexpected side effect of how our implementation works, and not intended by the feature. The feature is there so you don't put secrets in the config.

I would even be more strict and allow expanding of only specific values in the config and not anywhere as a whole, but that's a bit tricky since the SDK shouldn't be aware of what it should expand (that's plugin specific).

If someone wants to do something fancy they could serialize to YAML. Using the expand feature to re-use config is a bit much and quite error prone. Also there should be other ways to do it like YAML aliases/anchors (not sure we support them)

[yevgenypats]

However, that will be a breaking change for some users who rely on the fact that they can currently store yaml inside their environment variables and it will get interpreted as yaml. This could be handy for storing a list of tables or accounts, for example. Using unix tools to do it first isn't always easy (e.g. in container environments).

I think that's an unexpected side effect of how our implementation works, and not intended by the feature. The feature is there so you don't put secrets in the config.

I would even be more strict and allow expanding of only specific values in the config and not anywhere as a whole, but that's a bit tricky since the SDK shouldn't be aware of what it should expand (that's plugin specific).

If someone wants to do something fancy they could serialize to YAML. Using the expand feature to re-use config is a bit much and quite error prone. Also there should be other ways to do it like YAML aliases/anchors (not sure we support them)

I think we shouldn't support any anchors features in our yaml configs. Those features are known to stretch the limit of YAMLs and break and this is where you will need to move config to code as config doesn't work anymore.

[erezrokah]

I think we shouldn't support any anchors features in our yaml configs. Those features are known to stretch the limit of YAMLs and break and this is where you will need to move config to code as config doesn't work anymore.

💯 Agree we shouldn't (just giving an example of another way to do config re-use instead of env var expansion)

[disq]

Mostly 'fixed' with cloudquery/plugin-pb-go#4 and cloudquery/plugin-pb-go#13 but some points still stand (users attempting to inject YAML e.g. list of tables won't work)