search

grafana / k6

A modern load testing tool, using Go and JavaScript - https://k6.io
GNU Affero General Public License v3.0
26.2k stars 1.27k forks source link

Relax k6's enviroment variables constrains #2730

Open olegbespalov opened 2 years ago

olegbespalov commented 2 years ago

What?

During the implementation of the cloud environment variables, we found an inconsistent treating of the variable names validation.

A flag --env uses the pattern: https://github.com/grafana/k6/blob/c6f57c6e22e6966e526643d2d0944e48df0ea36c/cmd/runtime_options.go#L19

And a flag --include-system-env-vars doesn't check the environment variables names for that pattern.

That's how task #2650 appears, a PR #2652 was implemented and merged.

Later we found that it leads to not the best UX, because, on windows machines, every k6 run will produce warnings, and the plan was to fix these warnings in #2720. But during the PR and inside the internal team discussion, we decided to re-think the approach there and maybe just relax constraints for the variable names.

That's why the original code from #2652 is reverted in #2723

Potential issues

The main risk is that the k6 cloud has also implemented such contains; if we change the behavior, the cloud app should also be aligned.

Why?

https://github.com/grafana/k6/pull/2720#issuecomment-1274834942

na-- commented 2 years ago

FWIW, as the person who implemented the "environment variables" feature ages ago (https://github.com/grafana/k6/pull/495), I think it was a mistake to use the words "environment variables" and the -e / --env flag for it... :disappointed:

It's much more accurate to say that they are script parameters, since that is what they are. And we could have said that, by default, we include the system environment variables as script parameters for k6 run (but not for k6 cloud), e.g. have --use-system-env-vars-as-params instead of --include-system-env-vars. And I am not sure if we shouldn't have also made that opt-in even for k6 run... :sweat_smile:

In any case, the current regular expression that validates the environment variable names are POSIX compliant doesn't really have a reason to exist. k6 should't have any issues with "environment variables" that are valid JS/JSON strings, even unicode ones.

So, given the fact that there are no technical limitations on the k6 side and the fact that on Windows it seems like almost anything can be an environment variable name, I personally think we should remove this validation on the k6 side.

Can anyone think of any reasons to keep this validation around?

imiric commented 2 years ago

I agree with @na--.

Given that k6 is cross-platform, there's no reason we should enforce that env vars must be POSIX compliant.

FWIW, I received the same validation error on Amazon Linux 2022 and Bash, for the env var BASH_FUNC_which%%, which is known to cause all sorts of issues. While it's probably a bad idea to define env vars like this, k6 really shouldn't care how they're defined, so :+1: for removing the validation.

olegbespalov commented 1 year ago

Somehow connected case: https://github.com/grafana/k6/pull/2904