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

Fetch .wasm from remote URI without depending on Listener. #204

Merged
merged 2 commits into from
May 5, 2020

Conversation

jplevyak
Copy link

Signed-off-by: John Plevyak jplevyak@gmail.com

Signed-off-by: John Plevyak <jplevyak@gmail.com>
@lambdai
Copy link

lambdai commented Apr 30, 2020

Native question: what is the listener in title? There is no listener either in red or green diff

@@ -211,6 +211,7 @@ TEST_P(WasmFilterConfigTest, YamlLoadFromRemoteWASM) {
cb(filter_callback);
}

#if 0
TEST_P(WasmFilterConfigTest, YamlLoadFromRemoteWASMFailOnUncachedThenSucceed) {

Choose a reason for hiding this comment

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

Why is this test disabled?

Copy link
Author

Choose a reason for hiding this comment

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

The tests are mocks, so they are heavily dependent on the details of the implementation. They also didn't actually provide any confidence since the tests passed and the implementation failed on integration because, well mocks are not real tests IMO. The integration test gives more confidence than these tests. That said I am updating them in envoy-wasm and will backport them here when I get them done.

Copy link
Author

Choose a reason for hiding this comment

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

The tests are working. Reactivated them.

Signed-off-by: John Plevyak <jplevyak@gmail.com>
@jplevyak jplevyak requested a review from PiotrSikora May 2, 2020 19:38
@jplevyak
Copy link
Author

jplevyak commented May 4, 2020

/test test-tsan_envoy

@jplevyak
Copy link
Author

jplevyak commented May 4, 2020

/retest

@PiotrSikora
Copy link

It seems that there is a real data race, so retesting won't make it go away:

SUMMARY: ThreadSanitizer: data race /build/tmp/_bazel_bazel/004a6f22074b0cbbac00100c854c36fb/sandbox/linux-sandbox/6/execroot/envoy/external/com_github_libevent_libevent/signal.c:404:17 in evsig_handler
==================
[2020-05-02 19:54:29.719][46][info][main] [source/server/server.cc:527] caught SIGUSR1. Reopening access logs.
==================
WARNING: ThreadSanitizer: data race (pid=46)
  Read of size 4 at 0x55883c2bf7d0 by thread T1:
    #0 evsig_handler /build/tmp/_bazel_bazel/004a6f22074b0cbbac00100c854c36fb/sandbox/linux-sandbox/6/execroot/envoy/external/com_github_libevent_libevent/signal.c:404:17 (envoy-static+0x9cd90b7)
    #1 __tsan::CallUserSignalHandler(__tsan::ThreadState*, bool, bool, bool, int, __sanitizer::__sanitizer_siginfo*, void*) /local/mnt/workspace/bcain_clang_bcain-ubuntu_23113/llvm/utils/release/final/llvm.src/projects/compiler-rt/lib/tsan/rtl/tsan_interceptors.cc:1914:7 (envoy-static+0x3c0152e)
    #2 gpr_cv_wait /proc/self/cwd/external/com_github_grpc_grpc/src/core/lib/gpr/sync_posix.cc:128:11 (envoy-static+0x8f82915)
    #3 grpc_core::Executor::ThreadMain(void*) /proc/self/cwd/external/com_github_grpc_grpc/src/core/lib/iomgr/executor.cc:246:7 (envoy-static+0x8ee4a88)
    #4 grpc_core::(anonymous namespace)::ThreadInternalsPosix::ThreadInternalsPosix(char const*, void (*)(void*), void*, bool*, grpc_core::Thread::Options const&)::'lambda'(void*)::operator()(void*) const /proc/self/cwd/external/com_github_grpc_grpc/src/core/lib/gprpp/thd_posix.cc:139:27 (envoy-static+0x8f895a8)
    #5 grpc_core::(anonymous namespace)::ThreadInternalsPosix::ThreadInternalsPosix(char const*, void (*)(void*), void*, bool*, grpc_core::Thread::Options const&)::'lambda'(void*)::__invoke(void*) /proc/self/cwd/external/com_github_grpc_grpc/src/core/lib/gprpp/thd_posix.cc:109:25 (envoy-static+0x8f893c8)

  Previous write of size 4 at 0x55883c2bf7d0 by main thread:
    [failed to restore the stack]

  Location is global 'evsig_base_fd' of size 4 at 0x55883c2bf7d0 (envoy-static+0x00000ba8a7d0)

  Thread T1 'default-executo' (tid=54, running) created by main thread at:
    #0 pthread_create /local/mnt/workspace/bcain_clang_bcain-ubuntu_23113/llvm/utils/release/final/llvm.src/projects/compiler-rt/lib/tsan/rtl/tsan_interceptors.cc:967:3 (envoy-static+0x3bfa7ab)
    #1 grpc_core::(anonymous namespace)::ThreadInternalsPosix::ThreadInternalsPosix(char const*, void (*)(void*), void*, bool*, grpc_core::Thread::Options const&) /proc/self/cwd/external/com_github_grpc_grpc/src/core/lib/gprpp/thd_posix.cc:108:10 (envoy-static+0x8f88f50)
    #2 grpc_core::(anonymous namespace)::ThreadInternalsPosix* grpc_core::New<grpc_core::(anonymous namespace)::ThreadInternalsPosix, char const*&, void (*&)(void*), void*&, bool*, grpc_core::Thread::Options const&>(char const*&, void (*&)(void*), void*&, bool*&&, grpc_core::Thread::Options const&) /proc/self/cwd/external/com_github_grpc_grpc/src/core/lib/gprpp/memory.h:37:18 (envoy-static+0x8f88948)
    #3 grpc_core::Thread::Thread(char const*, void (*)(void*), void*, bool*, grpc_core::Thread::Options const&) /proc/self/cwd/external/com_github_grpc_grpc/src/core/lib/gprpp/thd_posix.cc:185:11 (envoy-static+0x8f886bf)
    #4 grpc_core::Executor::SetThreading(bool) /proc/self/cwd/external/com_github_grpc_grpc/src/core/lib/iomgr/executor.cc:180:9 (envoy-static+0x8ee3c01)
    #5 grpc_core::Executor::Init() /proc/self/cwd/external/com_github_grpc_grpc/src/core/lib/iomgr/executor.cc:113:25 (envoy-static+0x8ee365d)
    #6 grpc_core::Executor::InitAll() /proc/self/cwd/external/com_github_grpc_grpc/src/core/lib/iomgr/executor.cc:415:58 (envoy-static+0x8ee5d44)
    #7 grpc_iomgr_init() /proc/self/cwd/external/com_github_grpc_grpc/src/core/lib/iomgr/iomgr.cc:60:3 (envoy-static+0x8ee71b8)
    #8 grpc_init /proc/self/cwd/external/com_github_grpc_grpc/src/core/lib/surface/init.cc:146:5 (envoy-static+0x8c472c0)
    #9 Envoy::Grpc::GoogleGrpcContext::GoogleGrpcContext() /proc/self/cwd/source/common/grpc/google_grpc_context.cc:21:5 (envoy-static+0x8bec891)
    #10 Envoy::MainCommonBase::MainCommonBase(Envoy::OptionsImpl const&, Envoy::Event::TimeSystem&, Envoy::ListenerHooks&, Envoy::Server::ComponentFactory&, std::__1::unique_ptr<Envoy::Runtime::RandomGenerator, std::__1::default_delete<Envoy::Runtime::RandomGenerator> >&&, Envoy::Thread::ThreadFactory&, Envoy::Filesystem::Instance&, std::__1::unique_ptr<Envoy::ProcessContext, std::__1::default_delete<Envoy::ProcessContext> >) /proc/self/cwd/source/exe/main_common.cc:43:17 (envoy-static+0x3cf0250)
    #11 Envoy::MainCommon::MainCommon(int, char const* const*) /proc/self/cwd/source/exe/main_common.cc:140:7 (envoy-static+0x3cf1b76)
    #12 std::__1::__unique_if<Envoy::MainCommon>::__unique_single std::__1::make_unique<Envoy::MainCommon, int&, char**&>(int&, char**&) /opt/llvm/bin/../include/c++/v1/memory:3003:32 (envoy-static+0x3c6a222)
    #13 main /proc/self/cwd/source/exe/main.cc:26:19 (envoy-static+0x3c6932f)

@jplevyak
Copy link
Author

jplevyak commented May 4, 2020

Well that is going to be a bit problematic: libevent/libevent#779

@jplevyak
Copy link
Author

jplevyak commented May 5, 2020

Looks good now.

@istio-testing istio-testing merged commit 0e2c9c0 into istio:master May 5, 2020
@jplevyak jplevyak deleted the fetch branch May 5, 2020 20:05
bianpengyuan pushed a commit to bianpengyuan/envoy that referenced this pull request May 7, 2020
* Fetch .wasm from remote URI without depending on Listener.

Signed-off-by: John Plevyak <jplevyak@gmail.com>

* Reactivate tests.

Signed-off-by: John Plevyak <jplevyak@gmail.com>
istio-testing pushed a commit that referenced this pull request May 7, 2020
* Fetch .wasm from remote URI without depending on Listener. (#204)

* Fetch .wasm from remote URI without depending on Listener.

Signed-off-by: John Plevyak <jplevyak@gmail.com>

* Reactivate tests.

Signed-off-by: John Plevyak <jplevyak@gmail.com>

* Add stats for wasm remote load fetch and cache. (#207)

* Add stats for wasm remote load fetch and cache.

Signed-off-by: John Plevyak <jplevyak@gmail.com>

* Address comments and ensure that the stats have the same lifetime as the
cache.

Signed-off-by: John Plevyak <jplevyak@gmail.com>

* Address comments.

Signed-off-by: John Plevyak <jplevyak@gmail.com>

* Address ASAN issue.

Signed-off-by: John Plevyak <jplevyak@gmail.com>

* Mess around with the tests some more.

Signed-off-by: John Plevyak <jplevyak@gmail.com>

Co-authored-by: John Plevyak <jplevyak@gmail.com>
brian-avery pushed a commit that referenced this pull request Oct 7, 2020
* doc: version 1.12.7

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

* Preserve LWS from the middle of HTTP1 header values that requ… (envoyproxy#12319)

* [http1] Preserve LWS from the middle of HTTP1 header values that require multiple dispatch calls to process (envoyproxy#10886)

Correctly preserve linear whitespace in the middle of HTTP1 header values. The fix in 6a95a21 trimmed away both leading and trailing whitespace when accepting header value fragments which can result in inner LWS in header values being stripped away if the LWS lands at the beginning or end of a buffer slice.

Also various fix to allow build on clang-10

Signed-off-by: Antonio Vicente <avd@google.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>

* http: header map security fixes for duplicate headers (#197) (#204)

Previously header matching did not match on all headers for
non-inline headers. This patch changes the default behavior to
always logically match on all headers. Multiple individual
headers will be logically concatenated with ',' similar to what
is done with inline headers. This makes the behavior effectively
consistent. This behavior can be temporary reverted by setting
the runtime value "envoy.reloadable_features.header_match_on_all_headers"
to "false".

Targeted fixes have been additionally performed on the following
extensions which make them consider all duplicate headers by default as
a comma concatenated list:
1) Any extension using CEL matching on headers.
2) The header to metadata filter.
3) The JWT filter.
4) The Lua filter.
Like primary header matching used in routing, RBAC, etc. this behavior
can be disabled by setting the runtime value
"envoy.reloadable_features.header_match_on_all_headers" to false.

Finally, the setCopy() header map API previously only set the first
header in the case of duplicate non-inline headers. setCopy() now
behaves similiarly to the other set*() APIs and replaces all found
headers with a single value. This may have had security implications
in the extauth filter which uses this API. This behavior can be disabled
by setting the runtime value
"envoy.reloadable_features.http_set_copy_replace_all_headers" to false.

Fixes https://github.com/envoyproxy/envoy-setec/issues/188

Signed-off-by: Matt Klein <mklein@lyft.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>

* fix wasm compile

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

* fix typo

Signed-off-by: Yuchen Dai <silentdai@gmail.com>
istio-testing pushed a commit that referenced this pull request Oct 8, 2020
* doc: version 1.12.7

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

* Preserve LWS from the middle of HTTP1 header values that requ… (envoyproxy#12319)

* [http1] Preserve LWS from the middle of HTTP1 header values that require multiple dispatch calls to process (envoyproxy#10886)

Correctly preserve linear whitespace in the middle of HTTP1 header values. The fix in 6a95a21 trimmed away both leading and trailing whitespace when accepting header value fragments which can result in inner LWS in header values being stripped away if the LWS lands at the beginning or end of a buffer slice.

Also various fix to allow build on clang-10

Signed-off-by: Antonio Vicente <avd@google.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>

* http: header map security fixes for duplicate headers (#197) (#204)

Previously header matching did not match on all headers for
non-inline headers. This patch changes the default behavior to
always logically match on all headers. Multiple individual
headers will be logically concatenated with ',' similar to what
is done with inline headers. This makes the behavior effectively
consistent. This behavior can be temporary reverted by setting
the runtime value "envoy.reloadable_features.header_match_on_all_headers"
to "false".

Targeted fixes have been additionally performed on the following
extensions which make them consider all duplicate headers by default as
a comma concatenated list:
1) Any extension using CEL matching on headers.
2) The header to metadata filter.
3) The JWT filter.
4) The Lua filter.
Like primary header matching used in routing, RBAC, etc. this behavior
can be disabled by setting the runtime value
"envoy.reloadable_features.header_match_on_all_headers" to false.

Finally, the setCopy() header map API previously only set the first
header in the case of duplicate non-inline headers. setCopy() now
behaves similiarly to the other set*() APIs and replaces all found
headers with a single value. This may have had security implications
in the extauth filter which uses this API. This behavior can be disabled
by setting the runtime value
"envoy.reloadable_features.http_set_copy_replace_all_headers" to false.

Fixes https://github.com/envoyproxy/envoy-setec/issues/188

Signed-off-by: Matt Klein <mklein@lyft.com>
Signed-off-by: Yuchen Dai <silentdai@gmail.com>

* fix wasm compile

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

* fix typo

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

Co-authored-by: Yuchen Dai <silentdai@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants