Metadata Exchange Filter by gargnupur · Pull Request #2325 · istio/proxy

gargnupur · 2019-07-26T00:02:49Z

Work left:

Move writing of metadata to the callback when TLS handshake is complete.
onNewConnection gets called when connection is established but before handshake is complete. Thus couldnt do metadata write here.

Things to note:

In Upstream Filter, onWrite happens first, thus we write metadata in "OnWrite".
In Downstream Filter, onData happens first, thus we write metadata in "OnData".
Both Upstream and Downstream filter consume metadata in OnData.

istio-testing · 2019-07-26T00:02:55Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: gargnupur
To complete the pull request process, please assign rshriram
You can assign the PR to them by writing /assign @rshriram in a comment when ready.

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

Details

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

kyessenov · 2019-07-29T17:54:07Z

FYI The upstream filter facility is now merge into Envoy, so it should be ready to use. We don't have to combine both filters in one PR, upstream can be done separately from downstream.

kyessenov · 2019-07-29T18:04:07Z

src/envoy/tcp/alpn_proxy/alpn_proxy.cc

+    data.drain(proxy_header_length);
+    if (proxy_header.has_metadata()) {
+      read_callbacks_->connection().streamInfo().setDynamicMetadata(
+          "filters.network.alpn_proxy.upstream", proxy_header.metadata());


Please make this a constant

kyessenov · 2019-07-29T18:04:34Z

src/envoy/tcp/alpn_proxy/alpn_proxy.cc

+}
+
+Network::FilterStatus AlpnProxyFilter::onWrite(Buffer::Instance& data, bool) {
+  config_->stats().metadata_found_.inc();


Why is it incremented per each write?

Left from testing... removed

kyessenov · 2019-07-29T18:05:15Z

src/envoy/tcp/alpn_proxy/alpn_proxy_test.cc

+namespace Tcp {
+namespace AlpnProxy {
+namespace {
+bool protoMapEq(const ProtobufWkt::Struct& obj,


MessageDifferencer is public now, you can find a use of it in envoy.

Thanks..Fixed

WORKSPACE

src/envoy/tcp/alpn_proxy/alpn_proxy.cc

gargnupur · 2019-08-03T23:34:28Z

@kyessenov, @mandarjog , @lizan : Looks like tests are failing because of the following error:
[2019-08-03 16:21:21.167][32983][warning][testing] [external/envoy/test/test_common/environment.cc:175] Testing with IPv6 addresses may not be supported on this machine. If testing fails, set the environment variable ENVOY_IP_TEST_VERSIONS.

and I see this in other PR's too..

mandarjog · 2019-08-05T04:18:51Z

Yes it is failing on PRs since prow nodes were upgraded.

gargnupur · 2019-08-05T17:32:54Z

/retest

kyessenov · 2019-09-27T21:49:42Z

src/envoy/tcp/alpn_proxy/alpn_proxy.cc

+        conn_state_ = WriteMetadata;
+        config_->stats().alpn_protocol_found_.inc();
+      }
+    }


I think this has to fall-through to WriteMetadata. Can you make that explicit or add a comment?

Done.. removed the else stmt.

kyessenov · 2019-09-27T21:59:04Z

src/envoy/tcp/alpn_proxy/alpn_proxy.cc

+      if (config_->filter_direction_ == FilterDirection::Downstream) {
+        writeNodeMetadata();
+      }
+    }


Same here, this is intended to fall through I think.

kyessenov · 2019-09-27T21:59:45Z

src/envoy/tcp/alpn_proxy/alpn_proxy.cc

+  }
+  const uint32_t initial_header_length = sizeof(AlpnProxyInitialHeader);
+  if (data.length() < initial_header_length) {
+    config_->stats().initial_header_not_found_.inc();


This does not seem right to increment header on buffering. This is mostly random depending how TCP data is chunked, right?

Please check replied above...
I thought "return Network::FilterStatus::StopIteration;" is doing that and we don't need to drain "data" and buffer bytes read till now..

kyessenov · 2019-09-27T22:00:54Z

src/envoy/tcp/alpn_proxy/alpn_proxy.cc

+      if (conn_state_ == Invalid) {
+        return Network::FilterStatus::Continue;
+      }
+    }


Same here. If buffer is large, then we need to drain the proxy header.

I thought "return Network::FilterStatus::StopIteration;" is doing that and we don't need to drain "data" and buffer bytes read till now..

kyessenov · 2019-09-27T22:02:00Z

src/envoy/tcp/alpn_proxy/alpn_proxy.cc

+  }
+}
+
+void AlpnProxyFilter::writeMetadata(const std::string key,


nit: naming suggestion setMetadata so as not to confuse with network writes.

Changed.. to be in sync with set.. changed readMetadata to getMetadata too.

kyessenov · 2019-09-27T22:03:25Z

src/envoy/tcp/alpn_proxy/alpn_proxy.cc

+    writeMetadata(UpstreamDynamicDataKey, struct_metadata);
+  } else {
+    writeMetadata(DownstreamDynamicDataKey, struct_metadata);
+  }


set conn_state_ = Done?

State Machine fall through will take care of it.. in default section?

kyessenov · 2019-09-27T22:09:29Z

src/envoy/tcp/alpn_proxy/alpn_proxy.cc

+    case WriteMetadata: {
+      // TODO(gargnupur): Try to move this just after alpn protocol is
+      // determined and first onWrite is called in Upstream filter.
+      if (config_->filter_direction_ == FilterDirection::Upstream) {


I'm thinking this guard may not be necessary. This essentially validates the expectation that onData is called first for downstream and onWrite is called first upstream. However, this is not important since one of them has to be called first anyways, and both callbacks do the right thing and transition out of the writing state (so not possible to write twice).

In fact, it might be necessary to remove this guard for server-first protocol. If the server (envoy) writes first on the downstream connection, then there is a bug with this guard. This can happen with MySQL, for example.

I thought that assumption was needed.. Thanks for the info.. Removed the guard.

kyessenov · 2019-09-27T22:10:30Z

Thank you for changing it to use a state machine. This is more readable IMHO.
@lizan are you still requesting changes? It's preventing the merge.

lizan

From implementation this looks much better than last time.

A few higher level question/request:

Can you have a one pager document (or is there any design doc for the protocol other than https://docs.google.com/document/d/1bWQAsrBZguk5HCmBVDEgEMVGS91r9uh3SIr7D7ELZBk/edit#heading=h.n6jc1obl82s7?) describing what the wire protocol is? I could read from the implementation but it is not clear and hard to read implementation against implementation to check if they can communicate correctly.
The name "ALPN Proxy" is very confusing because it has nothing to do with ALPN, we use ALPN to select this filter but itself doesn't deal with ALPN, I guess it is better to name "metadata_proxy" or something else (cc @PiotrSikora)

src/envoy/tcp/alpn_proxy/alpn_proxy_initial_header.h

lizan · 2019-09-30T23:34:44Z

src/envoy/tcp/alpn_proxy/alpn_proxy_initial_header.h

+struct AlpnProxyInitialHeader {
+  uint32_t magic;  // Magic number in network byte order. Most significant byte
+                   // is placed first.
+  static const uint32_t magic_number = 0x23071961;  // decimal 587667809


how this number is chosen?

It's a random number..

Can you double check this doesn't conflict with major protocols? (i.e. couldn't be first 4 bytes of TLS, HTTP/1.1, HTTP/2, MySQL, etc..)

gargnupur · 2019-10-02T05:52:33Z

@lizan: One Pager Implementation doc can be found at: https://docs.google.com/document/d/15jKV_muOZKX4CccnA1w-X73SkqGt8cV3a9TBXxzs0TA/edit?usp=sharing.
Agree with name change suggestion, in a meeting today at Google, metadata exchange was suggested too.. I am thinking to do name change in a separate PR, if that works for you?

lizan · 2019-10-02T19:02:07Z

Thanks for putting the doc!

I am thinking to do name change in a separate PR, if that works for you?

I'm in favor of doing that in this PR. At very least the registered name should be changed because it appears in config.

mandarjog · 2019-10-02T19:09:52Z

I agree, lets rename it to metadata_exchange .
The alpn link is there in the implementation, but one could use the same filter within plaintext tcp, with prior knowledge.

Re magic_number, we should check against other common magics, like beginning of an http1, http2 transactions. Is there a systematic way to go about this?

If we can't be certain, we should have the ability override magic_number through config.

gargnupur · 2019-10-03T17:30:49Z

@lizan, @mandarjog , @kyessenov , @PiotrSikora: looks like newly generated random 0x3D230467 magic number should not conflict with Http, http/2, TLS, Redis, SQL... (googling this number doesn't say it's any protocol's magic number.)

Summarizing the findings from chat here(thanks to @kyessenov,@PiotrSikora for the help):

HTTP: starts with GET, POST.. binaries of these do not match with our magic number.
HTTP/2: starts with a predefined prefix 0x505249202a20485454502f322e300d0a0d0a534d0d0a0d0a.
TLS: first 8 bits are TLS major version then next 8 bits TLS minor version.it's {3, 3} for TLS 1.2. First 4 bits of our magic number is 1111, so TLS will take a long time to collide.
SQL: https://dev.mysql.com/doc/internals/en/connection-phase-packets.html#packet-Protocol::Handshake. First byte is protocol version
, whose some info can be found here: https://docs.microsoft.com/en-us/openspecs/windows_protocols/ms-tds/135d0ebe-5c4c-4a94-99bf-1811eccb9f4a?redirectedfrom=MSDN). Doesn't look like it will conflict.
Redis: looks like data type is the first byte : https://redis.io/topics/protocol
does not collide with the magic number.

This validates that having these inspectors doesnt affect Metadata Exchange filter

mandarjog · 2019-10-05T01:16:13Z

@lizan ping !

lizan

mostly lgtm. defer to @PiotrSikora

lizan · 2019-10-07T19:53:38Z

src/envoy/tcp/metadata_exchange/config.cc

+
+ProtobufTypes::MessagePtr
+MetadataExchangeUpstreamConfigFactory::createEmptyConfigProto() {
+  return ProtobufTypes::MessagePtr{


nit: std::make_unique

lizan · 2019-10-07T19:57:02Z

src/envoy/tcp/metadata_exchange/metadata_exchange.cc

+  std::string initial_header_buf = std::string(
+      static_cast<const char*>(data.linearize(initial_header_length)),
+      initial_header_length);
+  const MetadataExchangeInitialHeader* initial_header =


I believe this is an undefined behavior (unaligned access) when initial_header_buf.c_str() is not aligned to uint32_t, UBSAN doesn't fail just because of luck.

But shouldn't marking MetadataExchangeInitialHeader struct as packed attribute help with misalignment errors?

No packed struct doesn't mitigate misalignment errors, because you're doing reinterpret_cast from a raw pointer which is not guaranteed to be aligned.

Since you're already doing a copy here, why not just do:

MetadataExchangeInitialHeader initial_header; data.copyOut(0, initial_header_length, &initial_header);

then where you copied to is aligned.

gargnupur requested review from kyessenov and mandarjog July 26, 2019 00:02

istio-testing added the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Jul 26, 2019

googlebot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Jul 26, 2019

istio-testing requested a review from JimmyCYJ July 26, 2019 00:02

istio-testing added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jul 26, 2019

kyessenov reviewed Jul 29, 2019

View reviewed changes

gargnupur changed the title ~~WIP:Alpn Proxy Filter~~ Alpn Proxy Filter Jul 31, 2019

istio-testing removed the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Jul 31, 2019

gargnupur changed the title ~~Alpn Proxy Filter~~ Alpn Proxy Downstream Filter Jul 31, 2019

istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Aug 1, 2019

gargnupur force-pushed the nup_alpn_proxy branch from 5300206 to 1d7be25 Compare August 2, 2019 21:01

istio-testing removed the needs-rebase Indicates a PR needs to be rebased before being merged label Aug 2, 2019

gargnupur requested review from bianpengyuan, duderino, lambdai, linsun, lizan, rshriram, utka, venilnoronha and yxue as code owners August 2, 2019 23:20

lizan suggested changes Aug 3, 2019

View reviewed changes

WORKSPACE Outdated Show resolved Hide resolved

src/envoy/tcp/alpn_proxy/alpn_proxy.cc Outdated Show resolved Hide resolved

src/envoy/tcp/alpn_proxy/alpn_proxy.cc Outdated Show resolved Hide resolved

Fix build errors

b6f306f

kyessenov reviewed Sep 27, 2019

View reviewed changes

Fix build errors

a12ca8f

lizan suggested changes Sep 30, 2019

View reviewed changes

Fixed based on feedback

e595d56

Renamed AlpnProxy to MetadataExchange

df7a9a7

gargnupur changed the title ~~Alpn Proxy Filter~~ Metadata Exchange Filter Oct 2, 2019

gargnupur added 2 commits October 2, 2019 13:44

Fix linting errors

b2e6fdd

Update magic number

d9c582f

Add tls and http inspectors in tcp envoy config

78db2ae

This validates that having these inspectors doesnt affect Metadata Exchange filter

lizan reviewed Oct 7, 2019

View reviewed changes

gargnupur added 3 commits October 7, 2019 14:14

Fixed based on feedback

6bdd5c9

Fixed based on feedback

f15cb06

Fixed based on feedback

0532263

lizan approved these changes Oct 7, 2019

View reviewed changes

istio-testing merged commit d9e6de4 into istio:master Oct 8, 2019

gargnupur deleted the nup_alpn_proxy branch October 17, 2019 17:30

gargnupur mentioned this pull request Feb 25, 2020

Add myself to istio/wg-networking-maintainers-data-plane team member istio/community#282

Merged

Conversation

gargnupur commented Jul 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

istio-testing commented Jul 26, 2019

Uh oh!

kyessenov commented Jul 29, 2019

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!

Uh oh!

Uh oh!

gargnupur commented Aug 3, 2019

Uh oh!

mandarjog commented Aug 5, 2019

Uh oh!

gargnupur commented Aug 5, 2019

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kyessenov commented Sep 27, 2019

Uh oh!

lizan left a comment

Choose a reason for hiding this comment

Uh oh!

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!

gargnupur commented Oct 2, 2019

Uh oh!

lizan commented Oct 2, 2019

Uh oh!

mandarjog commented Oct 2, 2019

Uh oh!

gargnupur commented Oct 3, 2019

Uh oh!

mandarjog commented Oct 5, 2019

gargnupur commented Jul 26, 2019 •

edited

Loading

lizan Oct 7, 2019 •

edited

Loading