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

Fixed network policy of client_integration_test #2022

Merged
merged 2 commits into from
Feb 4, 2022

Conversation

WillEngFlow
Copy link
Contributor

@WillEngFlow WillEngFlow commented Jan 25, 2022

//test/common/integration:client_integration_test requires networking
which is not allowed by default. This allows the test to use the
localhost network.

Signed-off-by: Will will@engflow.com

Description: //test/common/integration:client_integration_test requires networking which is not allowed by default. This allows the test to use the localhost network.
Risk Level: Low
Testing: See unit_tests workflow
Docs Changes: N/A
Release Notes: N/A
Fixes #2020

@WillEngFlow
Copy link
Contributor Author

@jpsim @goaway @lfpino

goaway
goaway previously approved these changes Jan 26, 2022
@goaway
Copy link
Contributor

goaway commented Jan 26, 2022

Thanks @WillEngFlow!

@WillEngFlow
Copy link
Contributor Author

@jpsim It looks like this is failing because the exec_properties attribute does not exist on envoy_cc_test upstream. Do you mind adding it for me and piping it into the wrapped cc_test?

envoy_cc_test defined here: https://github.com/envoyproxy/envoy/blob/519774f742bb2500cf5f6a00933c9662337acbb2/bazel/envoy_test.bzl#L143

An example of piping exec_properties through via 'envoy_mobile_jni_kt_test`:

def _internal_kt_test(name, srcs, deps = [], data = [], jvm_flags = [], repository = "", exec_properties = {}):

@jpsim
Copy link
Contributor

jpsim commented Jan 26, 2022

Sure I'll plumb that through and see if it helps. I'll do the same for the Swift integration test that similarly had remote exec disabled.

@jpsim
Copy link
Contributor

jpsim commented Jan 26, 2022

@jpsim
Copy link
Contributor

jpsim commented Jan 27, 2022

I've validated that it works: https://github.com/envoyproxy/envoy-mobile/runs/4959253557

My upstream Envoy PR (envoyproxy/envoy#19711) was approved and is being auto-merged. Once that's in, I'll bump the Envoy submodule commit to point to it and you'll be able to rebase this PR.

lizan pushed a commit to envoyproxy/envoy that referenced this pull request Jan 27, 2022
In order to pass properties such as `sandboxNetwork` as we are trying to do in Envoy Mobile here: envoyproxy/envoy-mobile#2022
Risk Level: Low

Signed-off-by: JP Simard <jp@jpsim.com>
@jpsim
Copy link
Contributor

jpsim commented Jan 27, 2022

#2027

@WillEngFlow
Copy link
Contributor Author

#2027

Thanks for all the work @jpsim! I'm tracking the submodule update.

@jpsim
Copy link
Contributor

jpsim commented Feb 3, 2022

I just merged #2027 so if you update, you should be able to pass exec_properties in envoy_cc_test.

//test/common/integration:client_integration_test requires networking
which is not allowed by default. This allows the test to use the
localhost network.

Signed-off-by: Will <will@engflow.com>
@WillEngFlow
Copy link
Contributor Author

I just merged #2027 so if you update, you should be able to pass exec_properties in envoy_cc_test.

Thanks @jpsim! I just pushed the most recent fix. I did get some linter errors on the WORKSPACE file after rebasing (in the pre-push check) but no changes were made to WORKSPACE and my changes checked out so I pushed with --no-verify

@jpsim
Copy link
Contributor

jpsim commented Feb 4, 2022

@WillEngFlow i also often get errors in the pre-push hook, but pre-commit run --all-files works. I need to look into fixing that at some point.

in this case, I think the BUILD file you edited needs to move the attribute to match the definition.

Signed-off-by: Will Martin <will@engflow.com>
Copy link
Contributor

@jpsim jpsim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for seeing this through!

@jpsim jpsim merged commit b65ff70 into envoyproxy:main Feb 4, 2022
@WillEngFlow
Copy link
Contributor Author

Thanks for seeing this through!

Thanks for all the help for a first time contributor!

joshperry pushed a commit to joshperry/envoy that referenced this pull request Feb 13, 2022
…roxy#19711)

In order to pass properties such as `sandboxNetwork` as we are trying to do in Envoy Mobile here: envoyproxy/envoy-mobile#2022
Risk Level: Low

Signed-off-by: JP Simard <jp@jpsim.com>
Signed-off-by: Josh Perry <josh.perry@mx.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CI] Run integration tests on EngFlow
3 participants