Skip to content

Fixed network policy of client_integration_test#2022

Merged
jpsim merged 2 commits intoenvoyproxy:mainfrom
EngFlow:will-fix-client_integration_test
Feb 4, 2022
Merged

Fixed network policy of client_integration_test#2022
jpsim merged 2 commits intoenvoyproxy:mainfrom
EngFlow:will-fix-client_integration_test

Conversation

@WillEngFlow
Copy link
Copy Markdown
Contributor

@WillEngFlow WillEngFlow commented Jan 25, 2022 •
edited
Loading

//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
Copy Markdown
Contributor Author

WillEngFlow commented Jan 25, 2022

@jpsim @goaway @lfpino

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

goaway commented Jan 26, 2022

Thanks @WillEngFlow!

@WillEngFlow
Copy link
Copy Markdown
Contributor Author

WillEngFlow commented Jan 26, 2022

@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`:

envoy-mobile/bazel/kotlin_test.bzl

Line 4 in cef4ec0

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

@jpsim
Copy link
Copy Markdown
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 jpsim mentioned this pull request Jan 26, 2022
Merged
@jpsim
Copy link
Copy Markdown
Contributor

jpsim commented Jan 26, 2022

[bazel] allow specifying exec_properties in envoy_cc_test

@jpsim jpsim mentioned this pull request Jan 26, 2022
Closed
@jpsim
Copy link
Copy Markdown
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
@jpsim
[bazel] allow specifying exec_properties in envoy_cc_test (#19711)
71248e5 
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
Copy Markdown
Contributor

jpsim commented Jan 27, 2022

#2027

@WillEngFlow
Copy link
Copy Markdown
Contributor Author

WillEngFlow commented Jan 28, 2022

#2027

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

@jpsim
Copy link
Copy Markdown
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.

@WillEngFlow
Fixed network policy of client_integration_test
a02e4ba 
//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 WillEngFlow force-pushed the will-fix-client_integration_test branch from cb1c46f to a02e4ba Compare February 3, 2022 23:59
@WillEngFlow
Copy link
Copy Markdown
Contributor Author

WillEngFlow commented Feb 4, 2022

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
Copy Markdown
Contributor

jpsim commented Feb 4, 2022 •
edited
Loading

@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.

@WillEngFlow
Fix linter
cbdeb36 
Signed-off-by: Will Martin <will@engflow.com>
jpsim
jpsim approved these changes Feb 4, 2022
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

WillEngFlow commented Feb 4, 2022

Thanks for seeing this through!

Thanks for all the help for a first time contributor!

@jpsim jpsim mentioned this pull request Feb 4, 2022
Closed
joshperry pushed a commit to joshperry/envoy that referenced this pull request Feb 13, 2022
@jpsim
[bazel] allow specifying exec_properties in envoy_cc_test (envoyp…
d53de0d 
…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

Reviewers

@jpsim jpsim jpsim approved these changes

@goaway goaway goaway left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[CI] Run integration tests on EngFlow

3 participants

@WillEngFlow @goaway @jpsim