Support for external authorization grpc service. by saumoh · Pull Request #2415 · envoyproxy/envoy

saumoh · 2018-01-19T17:06:51Z

Title
Authorization TCP and HTTP filters using an external gRPC service.
Patch 1 of 3: gRPC client and common framework.

Description
As per the feedback on #2359 I am breaking it up into three PR's.

The patch in this PR adds support for gRPC client that will be used by TCP and HTTP filters to make authorization checks with an external service.
This PR implements the discussing we had in #2291.

Testing
I have tested this on my local setup with an external gRPC authorization server. I have also added UT's for the gRPC client side.

Risk
Low: Because only the users of this filter will be impacted. It should not impact general stability of Envoy it self.

Caveats

I did not test with a Google gRPC service configuration.
I am aware that grpc: singleton manager/factory pattern for gRPC async clients. #2393 will impact this PR. After that PR is merged into master I'll merge this branch with master and make the required changes to the gRPC client implementation.

Signed-off-by: Saurabh Mohan saurabh+github@tigera.io

Signed-off-by: Saurabh Mohan <saurabh+github@tigera.io>

htuch · 2018-01-23T17:02:14Z

Thanks @saumoh for the PR series. Now that #2393 is merged, can you rebase this and use the client factory for gRPC services provided there?

…ommon Signed-off-by: Saurabh Mohan <saurabh+github@tigera.io>

Signed-off-by: Saurabh Mohan <saurabh+github@tigera.io>

louiscryan · 2018-01-24T08:05:52Z

@qiwzhang can you follow along here so that we are confident about convergence with https://github.com/istio/proxy/tree/master/src/envoy/mixer

…

On Tue, Jan 23, 2018 at 9:02 AM, htuch ***@***.***> wrote: Thanks @saumoh <https://github.com/saumoh> for the PR series. Now that #2393 <#2393> is merged, can you rebase this and use the client factory for gRPC services provided there? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#2415 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AIoKPD6enrLseQ8Evbk1u2Ol5NuPRxp3ks5tNhCcgaJpZM4RkxoI> .

saumoh · 2018-01-24T22:43:46Z

@htuch I've merged with the new gRPC manager changes.

htuch · 2018-01-24T23:02:18Z

@saumoh can you get the build to pass? I'll take an in-depth pass tomorrow, but I think it's helpful to see the CI build passing. Thanks.

htuch · 2018-01-24T23:04:25Z

source/common/ext_authz/ext_authz_impl.cc

+  headers.iterate(fillHttpHeaders, mhdrs);
+
+  std::unique_ptr<AttributeContext_Request> req = std::unique_ptr<AttributeContext_Request>{new AttributeContext_Request()};
+  ASSERT(req);


Very brief drive-by nits here:

Don't need to ASSERT on a basic memory allocation, we assume memory allocations always succeed or the world ends in Envoy.

Should write this as auto req = std::make_unique<Attribute_Request>(); for C++14.

htuch · 2018-01-24T23:05:03Z

source/common/ext_authz/ext_authz_impl.h

+class ExtAuthzCheckRequestGenerator: public CheckRequestGenerator {
+public:
+  ExtAuthzCheckRequestGenerator() {}
+  ~ExtAuthzCheckRequestGenerator() {}


Quick nit: the default constructor/destructors are provided by the compiler, no need to add them.

Signed-off-by: Saurabh Mohan <saurabh+github@tigera.io>

saumoh · 2018-01-25T22:28:33Z

@htuch Fixed the build and addressed your last set of comments. If you could please have a look. I am working on fixing coverage in the mean while. thanks.

htuch · 2018-01-25T23:30:13Z

@saumoh there is coverage regression here, see Code coverage 97.7 is lower than limit of 98.0 in CI. Have you generated a full coverage report and looked to see where you are missing tests?

saumoh · 2018-01-25T23:46:20Z

@htuch I am adding more tests for some of the newly added code. That should help creep the numbers closer to 100%.

htuch

OK great, this is looking like it's heading in the right direction, thanks for taking this on!

htuch · 2018-01-25T23:33:44Z

bazel/repositories.bzl

+    auth_bind_targets = [
+        "auth"
+    ]
+    for t in auth_bind_targets:


FWIW, the recent merges of envoyproxy/data-plane-api#421 and #2430 will break this (and other parts of your PR). Be prepared to update when you merge master or pull in an updated data-plane-api SHA.

Thanks for the heads up. I'll do another merge with master to pull in the latest data-plane-api and rework the impacted code.

htuch · 2018-01-25T23:34:37Z

include/envoy/ext_authz/ext_authz.h

+  /**
+   * Called when a check request is complete. The resulting status is supplied.
+   */
+  virtual void complete(CheckStatus status) PURE;


Nit: I think onComplete is a clearer name.

htuch · 2018-01-25T23:35:35Z

include/envoy/ext_authz/ext_authz.h

+  virtual ~ClientFactory() {}
+
+  /**
+   * Return a new authz client.


@param docs for all parameters on include/envoy interface headers.

Also @return for the return type & text.

htuch · 2018-01-25T23:37:43Z

include/envoy/ext_authz/ext_authz.h

+ * and fill out the details in the authorization protobuf that is sent to authorization
+ * service.
+ */
+class CheckRequestGenerator {


This is really a helper utility class. Generally we don't put these in include/envoy. A litmus test is "will I need to mock this class?" and "will this class have potentially more than one implementation?". If no to both, put it in the source/common subtree.

Initially I had it in the souce/common subtree. When I started writing the UT's for the filters I had to mock it. That's when I moved it here.
The usage in http filter is here

I think that the mock is needed. @htuch Should I just keep it here or move to source/common and have the mock reference it from there? please confirm. thx.

Do you really need to mock the proto creation? These are basically straight line pure functions. Personally, I would mock the input parameters, since it's just as easy to do and results in easier to understand code.

i think i know what you mean here. Let me try to redo the ut's without using the mock.

@htuch last commit removes the mock for proto creation.

htuch · 2018-01-25T23:38:26Z

source/common/access_log/grpc_access_log_impl.cc

+
+  // TODO(saumoh): Uncomment once unauthorization has been added to accesslog.proto
+  //
+  // if (request_info.getResponseFlag(RequestInfo::ResponseFlag::Unauthorized)) {


I would remove all of this for now and keep it locally. It will bit rot otherwise.

htuch · 2018-01-25T23:42:23Z

source/common/ext_authz/ext_authz_impl.cc

+  async_client_factory_ = async_client_manager.factoryForGrpcService(grpc_service, scope);
+}
+
+ClientPtr GrpcFactoryImpl::create(const Optional<std::chrono::milliseconds>& timeout) {


I don't think you need the GrpcFactoryImpl if all you're doing is wrapping the Grpc::AsyncClientManager/Factory calls, just use those classes directly.

htuch · 2018-01-25T23:47:05Z

source/common/ext_authz/ext_authz_impl.cc

+std::unique_ptr<::envoy::api::v2::Address> ExtAuthzCheckRequestGenerator::getProtobufAddress(
+    const Network::Address::InstanceConstSharedPtr& instance) {
+
+  auto addr = std::make_unique<::envoy::api::v2::Address>();


I think it's more natural in protobuf to invert this construction. What you have is:

getConnectionPeer calls getProtobufAddress to get some remote address PB.

getConnectionPeer then does a set_allocated_address with the result.

A much more common pattern is.

getConnectionPeer calls mutable_allocated_addresss() to allocate memory and obtain a pointer.

getProtobufAddress takes this pointer and then writes directly into the memory.

Of course, you would probably rename the methods. See https://github.com/envoyproxy/envoy/blob/master/source/common/config/address_json.cc#L10 and its callers for an example.

What you have is correct, I just find it noisier, since we have to deal with smart pointer creation and release, rather than idiomatically having the protobuf library mutable_foo() allocators do their work.

htuch · 2018-01-25T23:49:53Z

source/common/ext_authz/ext_authz_impl.cc

+  req->set_allocated_http(httpreq);
+
+  return req;
+}


I think it would be instructive to compare with https://github.com/envoyproxy/envoy/blob/master/source/common/access_log/grpc_access_log_impl.cc#L251. There is a lot of overlap between the access log proto data and construction and the auth request.

As a non-actionable reflection, it would have been great if we could have converged the common objects for both access logs and auth, but I don't think this is going to happen (@mattklein123 @wora).

@htuch This really depends on how big the scale we are targeting. If we target millions of services, billions of requests per second, we can surely refactor the code, but it will take quite a bit work.

The general approach would be something like this:

Reach an agreement on the standard Envoy attributes, and define them as a proto like AttributeContext.

Envoy natively generate the standard attributes.

Design the standard logs, metrics, events, traces , and other telemetry schemas.

Generate standard telemetry data based on the standard attributes.

Optional. Generate legacy telemetry data, such as access logs, from the standard attributes.

New users of Envoy won't need the legacy telemetry data, so it would just be disabled by a flag. We can delete the legacy code after a year, or just keep them on the side.

This refactoring would only be useful if we can agree on a rich set of standard telemetry data that appeal to wide range of users, so a customer can correlate telemetry data across services written by different vendors.

@htuch Ref to the first paragraph, are you suggesting that I write this in a way that it's done in https://github.com/envoyproxy/envoy/blob/master/source/common/access_log/grpc_access_log_impl.cc#L251.
I am happy to do that but just want to confirm.

Separately, thanks for all the other review comments. I am working on making those changes.

@wora agreed, I'm just waxing philosophical.

@saumoh just suggesting you give that code a pass and see if there's anything in there that your code would benefit from following.

htuch · 2018-01-25T23:51:15Z

source/common/ext_authz/ext_authz_impl.h

+  ClientPtr create(const Optional<std::chrono::milliseconds>&) override {
+    return ClientPtr{new NullClientImpl()};
+  }
+};


I'm guessing you modeled this on the RateLimiterImpl, do you really need a null implementation?

Signed-off-by: Saurabh Mohan <saurabh+github@tigera.io>

…ommon Signed-off-by: Saurabh Mohan <saurabh+github@tigera.io>

Signed-off-by: Saurabh Mohan <saurabh+github@tigera.io>

saumoh · 2018-01-30T19:21:17Z

@htuch Thanks for working with me on this PR. I've updated with the last set of pr comments.

Merged with latest data-plane-api re-org Split base API package into sub-packages data-plane-api#421
Moved the CheckRequestGenerator out of the include/ to source/common
Added more UT's
Rewrote the proto buf creation to avoid creating/destroying smart pointers (thanks for the reference code)
Addressed rest of the comments from the previous reviews.

Signed-off-by: Saurabh Mohan <saurabh+github@tigera.io>

htuch · 2018-02-01T03:21:29Z

I think if you can do a few small PRs for extra utilities for header strings and network address proto that would be good to do now; the more general separation of attribute context stuff I'll leave for you and @wora to discuss.

wora

Separation of attribute context is done, you can sync up to it. I think it is easier to do the split before it is being used.

wora · 2018-02-01T04:11:36Z

source/common/ext_authz/ext_authz_impl.cc

+  }
+
+  // Fill in the headers
+  auto mhdrs = httpreq.mutable_headers();


We need a way to control which headers to send.

wora · 2018-02-01T04:32:02Z

source/common/ext_authz/ext_authz_impl.cc

+      }
+    }
+  }
+  peer.set_principal(principal);


We probably need some format normalization here. At least document what is the format used here?

wora · 2018-02-01T04:32:16Z

source/common/ext_authz/ext_authz_impl.cc

+  peer.set_principal(principal);
+
+  if (!service.empty()) {
+    peer.set_service(service);


Also document the format?

@wora: The fulfillment of the AttributeContext is as-per the documentation in the data-plane-api specification 1. The code here is adhering to that interface agreement.
I can make a note of that at the top of this class.

saumoh · 2018-02-01T05:25:47Z

@htuch added a new pr #2504 for the changes for address and http changes.

saumoh · 2018-02-01T05:43:21Z

@htuch @wora For the separation of the AttributeContext in the data-plane-api 1
Should I create a utility.[cc|h] in a new directory source/common/auth which will have a static class CreateProtoAttributeContext that will fill up the protobuf.
How do u suggest this be structured in the code? Thanks.

htuch · 2018-02-02T04:45:47Z

I'm not sure if we need to refactor it today; it's done at the API level already, we don't have any extra consumers yet.

@wora if you feel strongly about pulling the AttributeContext code out of the ext_authz impl, can you enumerate some other potential users in the Envoy code base? That will allow us to see how this is going to be used.

We know it's not going to be used for logging, so I'm guessing it's for other auth filters?

Title Add a few utility functions to the http/protocol and http_header_map.h files Another utility function to copy Address::Instance into it's protobuf representation Testing I have tested and used these new functions in the PR #2415 added a ut for the http changes. Risk Low: like really low. Signed-off-by: Saurabh Mohan <saurabh+github@tigera.io>

Signed-off-by: Saurabh Mohan <saurabh+github@tigera.io>

htuch · 2018-02-05T03:51:40Z

Please merge master following #2495. @saumoh is this ready for another review pass?

saumoh · 2018-02-05T07:19:39Z

@htuch Will merge #2495. thanks for the heads up.
The current branch has all the changes except for per-thread grpc client. I am working on a prototype for it. Do you think I should do the per-thread grpc client post-merge of this PR? I feel that would be a big enough change that it merits a more focused pr. wdyt?

htuch · 2018-02-05T16:56:39Z

@saumoh Yeah, you can do that as a followup PR (I did something similar with the Google gRPC client library PRs, it makes it easier to separate out reasoning about thread stuff from the semantics of the changes).

…ommon Signed-off-by: Saurabh Mohan <saurabh+github@tigera.io>

saumoh · 2018-02-05T20:35:47Z

@htuch I merged with #2495 and think i've addressed all the review comments.

htuch

@saumoh This is ready to ship once comments are addressed. They are mostly nits. Thanks!

htuch · 2018-02-06T02:03:08Z

include/envoy/ext_authz/ext_authz.h

+  /**
+   * Cancel an inflight Check request.
+   */
+  virtual void cancel() PURE;


Thinking about this a bit more, maybe we don't need to do the TLS thing at all here. This is unary RPC, so we don't have a significant cost in rebuilding the client each time, it should be a fairly small construction/destruction cost. I think what you have in this PR is fine, I would be more concerned if we had a bidi streaming auth check.

htuch · 2018-02-06T02:08:30Z

source/common/ext_authz/ext_authz_impl.cc

+
+  // Set the address
+  auto addr = peer.mutable_address();
+  if (!local) {


Nit: can you invert this such that it reads if (local) {, same below, it's a bit cleaner.

htuch · 2018-02-06T03:02:44Z

source/common/ext_authz/ext_authz_impl.cc

+  Envoy::Http::StreamDecoderFilterCallbacks* cb =
+      const_cast<Envoy::Http::StreamDecoderFilterCallbacks*>(callbacks);
+
+  std::string service = getHeaderStr(headers.EnvoyDownstreamServiceCluster());


Nit: const std::string

htuch · 2018-02-06T03:04:02Z

source/common/ext_authz/ext_authz_impl.h

+typedef ConstSingleton<ConstantValues> Constants;
+
+// TODO(htuch): We should have only one client per thread, but today we create one per filter stack.
+// This will require support for more than one outstanding request per client.


As mentioned above, I think it's different for authz, since you are unary RPC, so you can probably put this aside for now.

htuch · 2018-02-06T03:04:56Z

source/common/ext_authz/ext_authz_impl.h

+   * createHttpCheck is used to extract the attributes from the stream and the http headers
+   * and fill them up in the CheckRequest proto message.
+   * @param callbacks supplies the Http stream context from which data can be extracted.
+   * @param headers supplies the heeader map with http headers that will be used to create the


Nit: s/heeader/header/

htuch · 2018-02-06T03:06:54Z

test/common/ext_authz/ext_authz_impl_test.cc

+    client_.onSuccess(std::move(response), span_);
+  }
+
+  {


Can you split this into multiple tests? The scoped-grouping-of-a-bunch-of-related-tests is fine for small simple tests, but I think here it would be clearer to have them as distinct tests.

htuch · 2018-02-06T03:07:27Z

test/common/ext_authz/ext_authz_impl_test.cc

+
+    client_.onCreateInitialMetadata(headers);
+
+    response.reset(new envoy::service::auth::v2::CheckResponse());


Nit: prefer std::make_unique, here and elsewhere there is a reset(new ...) pattern.

htuch · 2018-02-06T03:08:42Z

test/common/ext_authz/ext_authz_impl_test.cc

+    response.reset(new envoy::service::auth::v2::CheckResponse());
+    ::google::rpc::Status* status = new ::google::rpc::Status();
+    status->set_code(Grpc::Status::GrpcStatus::Ok);
+    response->set_allocated_status(status);


Prefer the mutable_foo pattern to raw allocations and set. Sorry if I'm being a pain here, it's just a reflection of the Envoy style where we prefer to always have smart pointers to simplify reasoning about correctness. This can be relaxed in tests when really needed, but I don't think this is one of those places.

You are fine. It is more a reflection of my old-school C heritage/archaic behavior. The mutable_foo pattern and the smart pointers are much better.

htuch · 2018-02-06T03:17:12Z

test/common/ext_authz/ext_authz_impl_test.cc

+  CreateCheckRequest::createTcpCheck(&net_callbacks_, request);
+}
+
+TEST_F(CreateCheckRequestTest, BasicHttp) {


https://23972-65214191-gh.circle-artifacts.com/0/build/envoy/generated/coverage/coverage.source_common_ext_authz_ext_authz_impl.cc.html tells me you have full coverage here, but I'm a bit surprised looking at just these tests, since they don't seem to be driving all the paths (at least directly). Could you say a few words on why we get full coverage here in this thread?

@htuch
The underlying structures like Ssl::Connection context are mocks hence they are there but empty. The calls from it here thus invoks all the conditional code.
The other connditional is around local boolean which invokes with both true and false within the createTcpCheck and createHttpCheck it self.
The remaining conditional checks like protocol and service is invoked because those structures are getting initialized in the tests.

I've addressed the rest of the feedback as well. Thanks for the review...it has made the code much better.

htuch · 2018-02-06T03:19:56Z

source/common/ext_authz/ext_authz_impl.cc

+      }
+    }
+  }
+  peer.set_principal(principal);


Nit: you're actually doing an additional string copy here that could be skipped if you did the set_principal directly in the if cases.

Signed-off-by: Saurabh Mohan <saurabh+github@tigera.io>

htuch · 2018-02-07T01:51:01Z

include/envoy/ext_authz/ext_authz.h

+  /**
+   * Cancel an inflight Check request.
+   */
+  virtual void cancel() PURE;


Yeah, for Envoy gRPC, this will come for free from the cluster pool management. In theory, Google gRPC also does this, based on good authority, but I haven't validated. Let's work on your other PRs first and we can circle back to this later.

htuch · 2018-02-07T01:54:24Z

@saumoh Thanks for patience during the review process, this is a nice contribution, can you rebase your other changes and get them passing tests, I'll take them one at a time? Ta!

mattklein123 · 2018-02-07T05:47:37Z

source/common/access_log/grpc_access_log_impl.cc

    const RequestInfo::RequestInfo& request_info) {

-  static_assert(RequestInfo::ResponseFlag::LastFlag == 0x800,
+  static_assert(RequestInfo::ResponseFlag::LastFlag == 0x1000,


Seems like gRPC access logging protos should be enhanced for unauthorized? Can you add a TODO and take care of this in one of your follow ups?

yes. I'll do it in the follow up pr; thanks.

In the proxy we've added an Unauthorized response flag. This PR adds the same to filter access logs. Once this PR is merged it will be possible to set the flag in source/common/access_log/grpc_access_log_impl.cc See also, comment in envoyproxy/envoy#2415 Signed-off-by: Saurabh Mohan <saurabh+github@tigera.io>

And properly clean up reachability ref. Signed-off-by: JP Simard <jp@jpsim.com>

In the proxy we've added an Unauthorized response flag. This PR adds the same to filter access logs. Once this PR is merged it will be possible to set the flag in source/common/access_log/grpc_access_log_impl.cc See also, comment in envoyproxy/envoy#2415 Signed-off-by: Saurabh Mohan <saurabh+github@tigera.io>

Support for external authroization grpc service.

79b97f0

Signed-off-by: Saurabh Mohan <saurabh+github@tigera.io>

This was referenced Jan 19, 2018

Support for external authorization tcp filter. #2416

Merged

Support for external authorization http filter. #2417

Merged

Add tcp and http filters for external authorization service. #2359

Closed

mattklein123 assigned htuch Jan 19, 2018

Saurabh Mohan added 2 commits January 23, 2018 14:44

Merge remote-tracking branch 'upstream/master' into ext-auth-review-c…

06a0eec

…ommon Signed-off-by: Saurabh Mohan <saurabh+github@tigera.io>

Merge with gRPC singleton manager.

bf7d7be

Signed-off-by: Saurabh Mohan <saurabh+github@tigera.io>

htuch reviewed Jan 24, 2018

View reviewed changes

Saurabh Mohan added 5 commits January 24, 2018 16:27

Fix build with clang and test of request_info.

7834705

Signed-off-by: Saurabh Mohan <saurabh+github@tigera.io>

Format fixes for common files.

48edb72

Signed-off-by: Saurabh Mohan <saurabh+github@tigera.io>

Use auto and make_unique.

1612396

Signed-off-by: Saurabh Mohan <saurabh+github@tigera.io>

Fix test common format.

4a197fb

Signed-off-by: Saurabh Mohan <saurabh+github@tigera.io>

More format issues in common.

141a203

Signed-off-by: Saurabh Mohan <saurabh+github@tigera.io>

htuch reviewed Jan 25, 2018

View reviewed changes

Saurabh Mohan and others added 7 commits January 26, 2018 11:15

Check generator class testing code.

0df4b9a

Signed-off-by: Saurabh Mohan <saurabh+github@tigera.io>

Merge remote-tracking branch 'upstream/master' into ext-auth-review-c…

cea2ed6

…ommon Signed-off-by: Saurabh Mohan <saurabh+github@tigera.io>

Merge with data-plane-api for common.

b4c7fea

Signed-off-by: Saurabh Mohan <saurabh+github@tigera.io>

Review feedback for common files.

93776d8

Signed-off-by: Saurabh Mohan <saurabh+github@tigera.io>

Merge branch 'master' into ext-auth-review-common

195df83

Fix format.

e0e6b77

Signed-off-by: Saurabh Mohan <saurabh+github@tigera.io>

Add more ut for context peer.

1f84caa

Signed-off-by: Saurabh Mohan <saurabh+github@tigera.io>

Remove mock for proto generation: common changes.

737171c

Signed-off-by: Saurabh Mohan <saurabh+github@tigera.io>

wora reviewed Feb 1, 2018

View reviewed changes

saumoh mentioned this pull request Feb 1, 2018

Add few more utility functions for address and http proto's. #2504

Merged

Saurabh Mohan added 3 commits February 2, 2018 15:26

Merge with upstream changes.

61f8f8f

Signed-off-by: Saurabh Mohan <saurabh+github@tigera.io>

Minor review fixes.

8ffb69d

Signed-off-by: Saurabh Mohan <saurabh+github@tigera.io>

Remove code post merge.

906ec7c

Signed-off-by: Saurabh Mohan <saurabh+github@tigera.io>

Merge remote-tracking branch 'upstream/master' into ext-auth-review-c…

3d88e76

…ommon Signed-off-by: Saurabh Mohan <saurabh+github@tigera.io>

htuch reviewed Feb 6, 2018

View reviewed changes

Review comments.

00c957e

Signed-off-by: Saurabh Mohan <saurabh+github@tigera.io>

htuch approved these changes Feb 7, 2018

View reviewed changes

htuch merged commit 4bd671d into envoyproxy:master Feb 7, 2018

mattklein123 reviewed Feb 7, 2018

View reviewed changes

saumoh mentioned this pull request Feb 20, 2018

Add unauthorized boolean the accesslog. envoyproxy/data-plane-api#505

Merged

saumoh deleted the ext-auth-review-common branch May 4, 2018 17:28

jpsim added a commit that referenced this pull request Nov 28, 2022

iOS: fix retain cycles in EnvoyNetworkMonitor (#2415)

978158a

And properly clean up reachability ref. Signed-off-by: JP Simard <jp@jpsim.com>

jpsim added a commit that referenced this pull request Nov 29, 2022

iOS: fix retain cycles in EnvoyNetworkMonitor (#2415)

d3dab1e

And properly clean up reachability ref. Signed-off-by: JP Simard <jp@jpsim.com>


		client_.onCreateInitialMetadata(headers);

		response.reset(new envoy::service::auth::v2::CheckResponse());

Conversation

saumoh commented Jan 19, 2018

Uh oh!

htuch commented Jan 23, 2018

Uh oh!

louiscryan commented Jan 24, 2018 via email

Uh oh!

saumoh commented Jan 24, 2018

Uh oh!

htuch commented Jan 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saumoh commented Jan 25, 2018

Uh oh!

htuch commented Jan 25, 2018

Uh oh!

saumoh commented Jan 25, 2018

Uh oh!

htuch 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!

Choose a reason for hiding this comment

Uh oh!

htuch Jan 25, 2018 • 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!

saumoh commented Jan 30, 2018

Uh oh!

htuch commented Feb 1, 2018

Uh oh!

wora 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!

saumoh commented Feb 1, 2018

Uh oh!

saumoh commented Feb 1, 2018

htuch Jan 25, 2018 •

edited

Loading

htuch commented Feb 2, 2018 •

edited

Loading