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

Bridge: switch envsubst for shellexpand #1105

Merged
merged 1 commit into from
Oct 19, 2023
Merged

Conversation

svix-onelson
Copy link
Contributor

@svix-onelson svix-onelson commented Oct 17, 2023

The implementation used for variable substitution in envsubst makes it illegal for env vars to point to values with characters that could make the replacement text look like a new variable to be substituted. This restriction is needed to make replacements deterministic.

The crux of the issue for us is this validation prevents us from defining env vars that point to JSON strings, which is necessary for GCP's credential handling scheme and can otherwise show up in the wild as in the case of #1084 where vars injected by vscode contained braces which triggered a validation error.

shellexpand has no such requirement, allowing JSON to appear in values without failing.

Key difference:

  1. shellexpand reads directly from the env instead of from a supplied HashMap.
  2. shellexpand supports more token styles than envsubst.

For 1, this was initially thought to be a benefit of envsubst since it separates isolates us from the full env by default, allowing us to filter vars if we so choose. In shellexpand they have a concept of a context which is a function that can be used for this purpose. I've leveraged this, allowing us to continue to read from a HashMap (which we build from the env). No additional filtering is performed, but it did help to retain the original signature for Config::from_src.

For 2, I mean, it's fine. The initial aim was just to keep things simple. Having support for more token styles, e.g. $FOO in addition to ${FOO} as well as ${FOO:fallback}. I think these styles are fine, but won't advertise them in the readme for now.

Existing tests for variable substitution continued to pass except for one: test_variable_substitution_requires_braces. Naturally this test should fail now that we actually support $FOO style vars. The test has been updated to match the new reality, effectively putting us on the hook to support this style moving forward, but that's probably fine.

@svix-onelson
Copy link
Contributor Author

OSS Review: shellexpand

This was the 2nd choice from my initial lib search in #986. Initially, it was not selected out of a desire to support as few token syntax styles as possible, but that's about all.

While this crate supports more than we care about, like directory path expansion, only minimal support has been enabled via cargo features.

N.b. ownership of the crate was transferred as of 2022-08-05.

The original repo was: https://github.com/netvl/shellexpand
Whereas the current maintainer's repo is https://gitlab.com/ijackson/rust-shellexpand

The original repo shows a long-running maintenance cycle and 84 stars.

The new repo ... not so much. Still, the crate continues to get tens of thousands of daily downloads.
A good signal is that prominent projects such as apollo-router are listed as dependents.


I'd like to give this one a shot and if it gives us trouble for some reason, I think we should just roll our own.

@tasn
Copy link
Member

tasn commented Oct 17, 2023

Only question I have: shellexpand sounds like something that would allow command execution. I just want to make sure that it's not the case. It's just env var expansion?

@svix-onelson
Copy link
Contributor Author

svix-onelson commented Oct 17, 2023

Only question I have: shellexpand sounds like something that would allow command execution. I just want to make sure that it's not the case. It's just env var expansion?

Correct: there's no command execution. This is all string-play from what I saw in the implementation.
Further, the name is really to say "shell-style expansion."

Ref: https://gitlab.com/ijackson/rust-shellexpand/-/blob/main/src/funcs.rs?ref_type=heads#L367

@svix-onelson svix-onelson force-pushed the onelson/fix-var-subst branch from b7cdf1c to b2e45d7 Compare October 17, 2023 21:32
@svix-onelson svix-onelson marked this pull request as ready for review October 17, 2023 21:59
@svix-onelson svix-onelson requested a review from a team October 17, 2023 21:59
@tasn
Copy link
Member

tasn commented Oct 17, 2023

OSS: approved!

The implementation used for variable substitution in `envsubst` makes it
illegal for env vars to point to values with characters that could make
the replacement text look like a new variable to be substituted. This
restriction is needed to make replacements deterministic.

The crux of the issue for us is this validation prevents us from
defining env vars that point to JSON strings, which is necessary for
GCP's credential handling scheme and can otherwise show up in the wild
as in the case of #1084
where vars injected by vscode contained braces which triggered a validation
error.

`shellexpand` has no such requirement, allowing JSON to appear in values
without failing.

Key difference:

1. `shellexpand` reads directly from the env instead of from a supplied `HashMap`.
2. `shellexpand` supports more token styles than `envsubst`.

For 1, this was initially thought to be a benefit since it separates
isolates us from the full env by default, allowing us to filter vars if
we so choose. In `shellexpand` they have a concept of a `context` which
is a function that can be used for this purpose. I've leveraged this,
allowing us to continue to read from a `HashMap` (which we build from
the env). No additional filtering is performed, but it did help to
retain the original signature for `Config::from_src`.

For 2, I mean, it's fine. The initial aim was just to keep things
simple. Having support for more token styles, e.g. `$FOO` in addition to
`${FOO}` as well as `${FOO:fallback}`. I think these styles are fine,
but won't advertise them in the readme for now.

Existing tests for variable substitution continued to pass _except for
one_: `test_variable_substitution_requires_braces`. Naturally this test
should fail now that we actually support `$FOO` style vars.
The test has been updated to match the new reality, effectively putting
us on the hook to support this style moving forward, but that's probably
fine.
@svix-onelson svix-onelson merged commit 07cb72e into main Oct 19, 2023
4 checks passed
@svix-onelson svix-onelson deleted the onelson/fix-var-subst branch October 19, 2023 18:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants