Combined Wasm stats + metadata implementations. #48

jplevyak · 2019-03-26T20:56:04Z

Combined Wasm stats + metadata implementations.

istio-testing · 2019-03-26T20:56:13Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approvers:

If they are not already assigned, you can assign the PR to them by writing /assign in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

stale · 2019-04-02T21:05:59Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

jplevyak · 2019-04-03T20:05:20Z

Keep alive ping.

mandarjog · 2019-04-04T16:27:20Z

source/server/server.cc

@@ -320,8 +320,9 @@ void InstanceImpl::initialize(const Options& options,
  if (bootstrap_.wasm_service_size() > 0) {
    auto factory = Registry::FactoryRegistry<Configuration::WasmFactory>::getFactory("envoy.wasm");
    if (factory) {
+      auto scope = Stats::ScopeSharedPtr(stats_store_.createScope("wasm."));


why do we need to create a scope called wasm? If at all we do create this scope, it should record wasm implementation specific metrics.

I think that the loaded wasm modules get their own namespace with a configurable name.

I just copied this from what another service did. yes, perhaps we can add an additional scope which includes the VM id (which is unique for each wasm use case).

mandarjog · 2019-04-04T16:37:32Z

test/extensions/wasm/test_data/stats_cpp.cc

+  c->increment(1, "test_tag", 7, true);
+  logTrace(std::string("get counter = ") + std::to_string(c->get("test_tag", 7, true)));
+  auto simple_c = c->resolve("test_tag", 7, true);
+  simple_c++;


Is this remaining in the worker thread and sent into the envoy singleton periodically, or is every call going into the underlying envoy scope? If it is the latter it may cause a lot of contention especially at ingress where there may be many worker thread.

This is a test for the wasm "service". This is a singleton. It isn't used with any of the per-stream stuff, but just background activity, so it shouldn't contend. There are no calls going into it except the Timer which is going to be called on the single thread that the singleton is bound to.

mandarjog · 2019-04-11T20:52:32Z

source/extensions/common/wasm/wasm.h

+  virtual std::string getProtocol(StreamType type);
+
+  // Metadata
+  // When used with MetadataType::Request/Response refers to metadata with name "envoy.wasm": the


we will have multiple filters of type envoy.wasm inserted. It is not clear how we can assume just 1.

example

name: envoy.wasm config: Name: istio.metadataFilter type: “inbound|outbound|gateway” vm_config: vm: "envoy.wasm.vm.wavm" code: filename: "/var/lib/istio_metadata_http.wasm" allow_precompiled: true

I just checked experimentally in this branch. Multiple filters of type envoy.wasm, works.

mandarjog

Need to test stats API, but the structure is ok.
What happens when multiple filters "envoy.wasm" filters are loaded?

LGTM for API, we will continue to iterate.

I would like @PiotrSikora and @silentdai to review code as well.

lambdai

Just some nits
Please feel to ignore.

What I am worry about is the naming and the ownership.
e.g. Callback is actually function ptr instead of value

lambdai · 2019-04-11T21:04:24Z

api/wasm/cpp/proxy_wasm_intrinsics.pb.cc

@@ -0,0 +1,364 @@
+// Generated by the protocol buffer compiler. DO NOT EDIT!


Why is generated files are checked in?

Because these are part of the WASM api which would be copied by users instead of used as part of envoy with bazel. In the future when we sort out the tool chain we may do this differently, but it was expedient until we sort that.

lambdai · 2019-04-11T21:09:37Z

source/extensions/common/wasm/wasm.h

@@ -343,20 +387,41 @@ class Wasm : public Envoy::Server::Wasm,

 private:
  friend class Context;
+  static const uint32_t kMetricTypeMask = 0x3;


nit: could you put Mask between 0x2 and 0x4?
Super nit: it might be confusing if Mask is a type...

Done (first bit) Yes, it would be confusing... I am open to suggestions. kMaskForMetricType ?

lambdai · 2019-04-11T21:13:52Z

source/extensions/common/wasm/wasm.h


  void registerCallbacks();    // Register functions called out from WASM.
  void establishEnvironment(); // Language specific enviroments.
  void getFunctions();         // Get functions call into WASM.

  Upstream::ClusterManager& cluster_manager_;
  Event::Dispatcher& dispatcher_;
+  Stats::Scope& scope_;
  std::string id_;


Question: the scope_ is either reference to higher level, or to owner_scope_, correct?

Correct, commented.

lambdai · 2019-04-11T21:21:48Z

source/extensions/common/wasm/wasm.h

@@ -213,6 +241,18 @@ class Context : public Http::StreamFilter,
  virtual void httpRespond(const Pairs& response_headers, absl::string_view body,
                           const Pairs& response_trailers);

+  // Stats/Metrics
+  enum class MetricType : uint32_t {


I am not sure why enum is required. Even so we should be able to reuse the value above

static const uint32_t kMetricTypeCounter = 0x0;

The enum is a duplicate of one in the API and it is supposed to be opaque. The implementation uses the same numbers to encode the type of the metric into the metric_id. Yes, they could be reused, but then it would require a bunch of casting . How about I add a comment and some static_assert(s) to ensure that they remain the same.

lambdai · 2019-04-11T21:33:46Z

source/extensions/common/wasm/wasm.h

 using WasmCallback3Int = uint32_t (*)(void*, uint32_t, uint32_t, uint32_t);
 using WasmCallback5Int = uint32_t (*)(void*, uint32_t, uint32_t, uint32_t, uint32_t, uint32_t);
 using WasmCallback9Int = uint32_t (*)(void*, uint32_t, uint32_t, uint32_t, uint32_t, uint32_t,
                                      uint32_t, uint32_t, uint32_t, uint32_t);
+// Using the standard g++/clang mangling algorithm:
+using WasmCallback_Zjl = void (*)(void*, uint32_t, int64_t);


Could you add the comment what Zjl Zjm mjj are referring to?

…ict with rust. Fix compilation fo the wasm exmple using full libprotobuf.

PiotrSikora · 2019-04-12T21:41:51Z

source/extensions/filters/http/well_known_names.h

@@ -53,7 +53,7 @@ class HttpFilterNameValues {
  // Tap filter
  const std::string Tap = "envoy.filters.http.tap";
  // Wasm filter
-  const std::string Wasm = "envoy.filters.http.wasm";
+  const std::string Wasm = "envoy.wasm";


Why do we need that?

I am just following the lua example. The fact that it is an http filter is given by context.

This reverts commit c336863.

* [1.7] Fix CVE-2020-25017 (#47) * Patch 1 * Patch 3 * Patch 4 * Remove manual * Revert "[1.7] Fix CVE-2020-25017 (#47)" (#48) This reverts commit c336863. * [1.7] Fix CVE-2020-25017 (#49) * Patch 1 * Patch 3 * Patch 4 * Remove manual * Add fix from 1.6

In our environment, the file system directory is as follows: Tue Jun 04 22:28:35][#48# ]$df -h Filesystem Size Used Avail Use% Mounted on tmpfs 77G 104K 77G 1% /dev/shm tmpfs 31G 9.8M 31G 1% /run tmpfs 5.0M 0 5.0M 0% /run/lock tmpfs 4.0M 0 4.0M 0% /sys/fs/cgroup /dev/mapper/atomicos-root 150G 144G 5.8G 97% /sysroot /dev/vda2 483M 84M 400M 18% /boot /dev/vdc 1.2T 87G 1.1T 8% /sysroot/home/centos/external We have a directory named /sysroot. If the envoy config file is the that directory, envoy can not start up. [2024-06-04 22:28:35.581][3382724][critical][main] [source/server/server.cc:131] error initializing configuration 'configs/envoy.yaml': Invalid path: configs/envoy.yaml [2024-06-04 22:28:35.581][3382724][info][main] [source/server/server.cc:972] exiting Invalid path: configs/envoy.yaml In my mind, envoy should only check the default system directory such as /dev /sys /proc as illegal path. So it is better to use exact match instead of startwith match. Signed-off-by: Zhang Bo <bozhang@ebay.com>

Collapsed PR adding Stats + Metadata.

13e049c

jplevyak requested a review from mandarjog March 26, 2019 20:56

stale bot added the stale label Apr 2, 2019

stale bot removed the stale label Apr 3, 2019

mandarjog reviewed Apr 4, 2019

View reviewed changes

Merge remote-tracking branch 'upstream/wasm' into wasm-stats-meta

3f8ae2c

mandarjog reviewed Apr 11, 2019

View reviewed changes

mandarjog approved these changes Apr 11, 2019

View reviewed changes

jplevyak requested a review from PiotrSikora April 11, 2019 21:04

lambdai approved these changes Apr 11, 2019

View reviewed changes

jplevyak added 2 commits April 11, 2019 16:07

Change API -> CPP_API in the C++ Makefile.base* to prevent name confl…

22114bf

…ict with rust. Fix compilation fo the wasm exmple using full libprotobuf.

Update with respect to comments.

a478f47

PiotrSikora reviewed Apr 12, 2019

View reviewed changes

PiotrSikora approved these changes Apr 12, 2019

View reviewed changes

jplevyak merged commit 5aff91e into istio:wasm Apr 12, 2019

brian-avery added a commit that referenced this pull request Sep 29, 2020

Revert "[1.7] Fix CVE-2020-25017 (#47)" (#48)

58bd05c

This reverts commit c336863.

		@@ -0,0 +1,364 @@
		// Generated by the protocol buffer compiler. DO NOT EDIT!

Combined Wasm stats + metadata implementations. #48

Combined Wasm stats + metadata implementations. #48

Uh oh!

Conversation

jplevyak commented Mar 26, 2019

Uh oh!

istio-testing commented Mar 26, 2019

Uh oh!

stale bot commented Apr 2, 2019

Uh oh!

jplevyak commented Apr 3, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mandarjog Apr 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mandarjog left a comment

Choose a reason for hiding this comment

Uh oh!

lambdai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mandarjog Apr 4, 2019 •

edited

Loading