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

Support block-network by default for tests #3453

Closed
gorset opened this issue Jul 26, 2017 · 5 comments
Closed

Support block-network by default for tests #3453

gorset opened this issue Jul 26, 2017 · 5 comments
Labels

Comments

@gorset
Copy link

gorset commented Jul 26, 2017

We would like a way to enable network sandboxing by default. Currently this can only be enabled by adding the block-network tag to every test, which is error prone and cumbersome.

We have a bunch of tests that can crash when multiple tests accidentally binds to the same port. This is not the common case, but happens often enough to be annoying.

Adding block-network tag solves this on linux, but fails on macos with java.net.SocketException: Operation not permitted. See #2669 for more details for sandboxing problems with macos.

Support for a bazel startup flag would make it possible for us to run with block-network as default on linux machines and avoid any issues with macos.

Relevant code from the current master:

/** Returns true if this specific spawn requires network access. */
static boolean shouldAllowNetwork(Spawn spawn) {
// If the Spawn requests to block network access, do so.
if (spawn.getExecutionInfo().containsKey("block-network")) {
return false;
}
// Network access is allowed by default.
return true;

@ittaiz
Copy link
Member

ittaiz commented Jul 26, 2017

@gorset have you considered the option of using a "remote-local" worker using docker? That should fix the macos problem.
It will still leave the problem of having to specify this tag on each test.
@philwo are there still substantial costs for using block-network? If not I think I'd also like to be able to turn it on by default

@philwo
Copy link
Member

philwo commented Jul 26, 2017

@ulfjack This might be a use case for our generic "--execution_info" flag, so that @gorset could just use something like --execution_info=TestAction+=block-network (exact flag syntax to be discussed) to make this the default for their setup.

@ittaiz I haven't checked the most recent Linux kernels whether they solved the performance problem or not - unfortunately there's nothing on the Bazel side that we can do.

For long-running tests it might not be too bad anyway - it was mostly an issue when using it as the default and then running a highly parallelized build consisting of many small actions.

I'd just give it a try.

@evanj
Copy link

evanj commented Mar 19, 2018

FYI: I did a bit of research about this Linux kernel issue: This issue was at least improved in 4.12. There was a discussion on an unrelated Github issue, which led me to a LKML discussion in April 2017, which led to a patch that was included in 4.12 AFAICS. It may be worth attempting to re-evaluate this cost on recent versions of Linux?

Even without that: I would also like a flag to add this tag, so targets have to "opt out." Its so easy to create tests that accidentally depend on the network, which is not only flaky, but in some cases dangerous (e.g. if a developer runs a unit test that accidentally can talk across a network using the user's own credentials, which happens with the Google Cloud APIs for example).

@AustinSchuh
Copy link
Contributor

Take a look at the --experimental_sandbox_default_allow_network flag. It allows you to change the default.

@aiuto aiuto added team-Local-Exec Issues and PRs for the Execution (Local) team untriaged labels Apr 15, 2019
@susinmotion
Copy link
Contributor

It looks like this is fixed based on the comment above. Please reopen if you are still having issues here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

9 participants