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

test: Unsafe usage of env::set_var #7651

Closed
olix0r opened this issue Jan 20, 2022 · 1 comment · Fixed by linkerd/linkerd2-proxy#1437
Closed

test: Unsafe usage of env::set_var #7651

olix0r opened this issue Jan 20, 2022 · 1 comment · Fixed by linkerd/linkerd2-proxy#1437

Comments

@olix0r
Copy link
Member

olix0r commented Jan 20, 2022

In the proxy's test initialization code, we use std::env::set_var to configure a log format:

pub fn trace_subscriber(default: impl ToString) -> (Dispatch, Handle) {
    let log_level = env::var("LINKERD2_PROXY_LOG")
        .or_else(|_| env::var("RUST_LOG"))
        .unwrap_or_else(|_| default.to_string());
    let log_format = env::var("LINKERD2_PROXY_LOG_FORMAT").unwrap_or_else(|_| "PLAIN".to_string());
    env::set_var("LINKERD2_PROXY_LOG_FORMAT", &log_format);
    // This may fail, since the global log compat layer may have been
    // initialized by another test.
    let _ = init_log_compat();
    Settings::for_test(log_level, log_format).build()
}

My understanding is that this is unsafe, per rust-lang/rust#90308, because our tests run in multiple threads. It would be great if we could avoid environment mutation in our codebase.

Can we remove this? Do we actually care about JSON formatting in our tests?

Are there tools we could use to, for instance, make clippy fail on new uses of set_var/remove_var?

@hawkw
Copy link
Contributor

hawkw commented Jan 20, 2022

In this case, we can just remove the set_var calls. I never understood why they were there to begin with --- I think this is an artifact of back before we were able to use the same logging configuration for the test driver thread and the test proxy?

hawkw added a commit to linkerd/linkerd2-proxy that referenced this issue Jan 20, 2022
Per rust-lang/rust#90308, this is potentially a data race. In practice,
I don't think it was actually problematic here, but it also wasn't doing
anything of value, so we should remove it.

This is currently the only `env::set_var` or `env::remove_var` call in
the proxy.

Closes linkerd/linkerd2#7651
hawkw added a commit to linkerd/linkerd2-proxy that referenced this issue Jan 20, 2022
Per rust-lang/rust#90308, this is potentially a data race. In practice,
I don't think it was actually problematic here, but it also wasn't doing
anything of value, so we should remove it.

This is currently the only `env::set_var` or `env::remove_var` call in
the proxy.

To prevent uses of these functions in the future, I also added a 
`disallowed-methods` configuration in `.clippy.toml` to emit a
warning for any uses of `std::env::set_var` and
`std::env::remove_var`. Unfortunately, this required adding a
`deny` attribute basically everywhere, which is why this diff touches
so many files.

Closes linkerd/linkerd2#7651
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 20, 2022
olix0r pushed a commit to linkerd/linkerd2-proxy that referenced this issue Mar 30, 2022
Per rust-lang/rust#90308, this is potentially a data race. In practice,
I don't think it was actually problematic here, but it also wasn't doing
anything of value, so we should remove it.

This is currently the only `env::set_var` or `env::remove_var` call in
the proxy.

To prevent uses of these functions in the future, I also added a
`disallowed-methods` configuration in `.clippy.toml` to emit a
warning for any uses of `std::env::set_var` and
`std::env::remove_var`. Unfortunately, this required adding a
`deny` attribute basically everywhere, which is why this diff touches
so many files.

Closes linkerd/linkerd2#7651

(cherry picked from commit 8f7be6f)
Signed-off-by: Oliver Gould <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants