Mutual TLS: Update port to address as listener identity. by wattli · Pull Request #570 · envoyproxy/envoy

wattli · 2017-03-14T19:54:07Z

fixes #507

wattli · 2017-03-14T20:11:27Z

@mattklein123 , please take a look, this PR fixes #507, please take a look. thanks.

lizan

test cases for 0.0.0.0?

lizan · 2017-03-14T20:31:32Z

source/common/network/listen_socket_impl.cc

  }
 }

+TcpListenSocket::TcpListenSocket(std::string address, bool bind_to_port) {


nit: const std::string&

lizan · 2017-03-14T20:31:40Z

source/common/network/listen_socket_impl.cc

  local_address_.reset(new Address::Ipv4Instance(port));
 }

+TcpListenSocket::TcpListenSocket(int fd, std::string address) {


lizan · 2017-03-14T20:33:25Z

source/exe/hot_restart.cc


  RpcGetListenSocketRequest rpc;
-  rpc.port_ = port;
+  memcpy(rpc.address_, address.c_str(), address.length() + 1);


make sure address.length() < 256?

Good point. Done.

lizan · 2017-03-14T20:35:12Z

test/common/network/dns_impl_test.cc

    ReadFilter(TestDnsServerQuery& parent) : parent_(parent) {}

-    // Network::ReadFilter
+    // Network::ReadFilter'


wattli · 2017-03-14T20:49:55Z

@myidpt , FYI

mattklein123 · 2017-03-14T22:42:01Z

@htuch can you possibly take the first pass on this?

htuch · 2017-03-14T22:46:35Z

wattli@ I can take a look at this. Before beginning, it would be helpful if you can you rebase from master, squash all the commits down to a single commit and provide a descriptive commit message? Thanks.

htuch · 2017-03-14T22:51:05Z

Also, Travis CI tests are failing as this isn't building. I think rebasing from master might fix it if it's working locally for you.

wattli · 2017-03-15T00:17:08Z

@htuch please take a look. Thanks.

htuch · 2017-03-15T00:32:14Z

@wattli Travis CI is still failing. Also, you haven't rebased from master, squashed down the commits and put together a descriptive comment. You should squash locally with "git rebase -i" until you have a single commit, and then "git push -f" to this branch. This will kill the working history and leave us with a single commit to start the review with. It's fine to later push smaller fixup commits, but I'd like to begin with a base that is most of the way there.

If you're new to git and need some help, I'll be available most of tomorrow EDT on https://gitter.im/lyft/envoy and can assist with the git flow.

wattli · 2017-03-15T04:23:38Z

@htuch , it should be ready for review now. Thanks.

kyessenov · 2017-03-14T20:44:20Z

source/common/network/listener_impl.cc

+     * the address returned by getOriginalDst() match the listener address. In this case the
+     * listener handles the connection directly and does not hand it off.
+     */
+    if (listener->socket_.localAddress()->asString() != final_local_address->asString()) {


I wonder if we want to have address wildcards. For ingress on Kubernetes pods, what's the IP address in the listener? Is it pod IP, service IP, or 127.0.0.1? If it can be either pod IP or service IP, then we need to have a list of addresses in the listener spec.

Is this instead of 0.0.0.0 which is too broad? E.g. you're after just {pod IP, 127.0.0.1} but not service IP? Then yeah, a list might make sense. This seems like it's beyond the scope of this PR though - you can use 0.0.0.0 for backwards compatibility with Envoy today with the current PR. It would make sense to file a PR for extending to a list once this PR is in though.

Agree with htuch's point. We can start from one address with 0.0.0.0 as the wild card. And then extend to a list if necessary.

htuch

In general, looks great. In addition to the comments below, I agree with @lizan that we should have some tests for 0.0.0.0 wildcard matching, in particular given the new logic in ConnectionHandlerImpl::findListenerByPort().

Can you also add some tests for use_original_dst that deal with different addresses?

htuch · 2017-03-15T12:03:30Z

source/exe/hot_restart.cc

Either sizeof(rpc.address_) (which should work, as this is an explicitly sized array) or use a named constant both here and in hot_restart.h.

Good point, done.

htuch · 2017-03-15T12:08:54Z

source/common/network/listener_impl.cc

Addresses have overloaded == operator, so should be comparable directly without having to convert to strings.

Great, done.

htuch · 2017-03-15T13:51:07Z

source/server/connection_handler_impl.cc

Ditto, shouldn't need to be performing direct string comparisons here.

htuch · 2017-03-15T13:51:50Z

source/server/connection_handler_impl.cc

This was a problem in the original code - we shouldn't use single letter variables like l, too easy to confuse with 1. can we rename to "auto listener" ?

Updated here and everywhere else in this file.

htuch · 2017-03-15T13:53:05Z

source/server/connection_handler_impl.cc

Maybe we should have an attribute on Address that lets us know if it's 0.0.0.0 without having to do string compares?

Good point, i added a member function isWildcard().

htuch · 2017-03-15T13:54:52Z

test/common/network/listen_socket_impl_test.cc

Do you want to EXPECT on the return value for listen? Same above, I know this was an issue in the original test.

htuch · 2017-03-15T13:58:05Z

test/common/network/listen_socket_impl_test.cc

Can you comment on why we have this test as well as the socket2? In general, all these socket cases could do with a one line comment explaining the intent.

wattli · 2017-03-15T18:30:39Z

@htuch , wildcard test case is covered as well as use_original_dst. Thanks.

htuch · 2017-03-15T18:55:21Z

@wattli I'll take a look, but tests are failing, so please look into them as well.

htuch · 2017-03-15T18:55:43Z

include/envoy/network/address.h

htuch · 2017-03-15T18:56:30Z

include/envoy/network/address.h

isAllHostsWildcard()?

I was thinking of adding helpers for creating the ANY address instances, equivalent to (but with shared instances):

namespace Network { namespace Address { InstancePtr anyIpv4() { return InstancePtr(new Ipv4Instance(0)); } InstancePtr anyIpv6() { return InstancePtr(new Ipv6Instance(0)); } } // Address } // Network

That should make the impl a bit easier.

I don't know if this solves the problem that isWildcard() is being used for though - the idea is you have an address that is something like 0.0.0.0:443 and want to know if just the IP part (ignoring the port) is wildcard.

htuch · 2017-03-15T18:57:53Z

source/common/network/address_impl.h

Wouldn't this be :: ?

Why return false? As Harvey indicates, the IPv6 equivalent of IPv4's "0.0.0.0" is "::", which is short for "0:0:0:0:0:0:0:0".

htuch · 2017-03-15T18:59:33Z

source/common/network/listener_impl.cc

I think this might be a source of compile errors?

htuch · 2017-03-15T19:00:56Z

test/server/connection_handler_test.cc

htuch · 2017-03-15T19:01:57Z

test/common/network/listen_socket_impl_test.cc

According to man (2) listen: "On success, zero is returned. On error, -1 is returned, and errno is set appropriately."

htuch · 2017-03-15T19:02:44Z

test/common/network/listen_socket_impl_test.cc

Shouldn't need to explicitly construct a std::string?

Otherwise it will complain 'ambiguous message type'.

htuch · 2017-03-15T19:08:40Z

test/common/network/listener_impl_test.cc

Isn't the idea here that we're trying to test findListenerByAddress? But instead it seems that this method is mocked, so it doesn't get exercised..

The unit test of findlistenerbyaddress is in connection_handler_test.cc. This here is for use_original_dst, which can't be tested there.

Sorry, my bad, this looks good.

jamessynge · 2017-03-15T20:42:17Z

This PR seems to have strayed away from the original goal, listen on the loopback address, and into the territory of "Update Envoy to use Network::Address::InstancePtr instead of port number". Can we separate these? I've been doing a bit of the latter as part of working on #351, which could be split into separate issues as appropriate.

wattli · 2017-03-15T20:45:18Z

James, do you mean not to introduce isAllHostsWild() function? If that's the case, i am fine with that.

…

On Wed, Mar 15, 2017 at 1:42 PM, James Synge ***@***.***> wrote: This PR seems to have strayed away from the original goal, listen on the loopback address, and into the territory of "Update Envoy to use Network::Address::InstancePtr instead of port number". Can we separate these? I've been doing a bit of the latter as part of working on #351 <#351>, which could be split into separate issues as appropriate. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#570 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AXQIZvXS1VqA9drC6a6toW_rGCLzUfJyks5rmE0vgaJpZM4MdErn> .

jamessynge · 2017-03-15T20:49:40Z

Well, if we use the word "any" in the functions I suggested (rather than saying "all hosts wild"), then we should probably call the predicate function something like "isAny" or "isAnyAddress".

wattli · 2017-03-15T20:52:55Z

@htuch, so can I revert this function, and only do string comparison and leave the logic to James?

…

On Wed, Mar 15, 2017 at 1:49 PM, James Synge ***@***.***> wrote: Well, if we use the word "any" in the functions I suggested (rather than saying "all hosts wild"), then we should probably call the predicate function something like "isAny" or "isAnyAddress". — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#570 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AXQIZqGAf2qSvIYo3vj0Oiv2YrQyjCDCks5rmE7ogaJpZM4MdErn> .

htuch · 2017-03-15T20:55:39Z

I'd strongly prefer not to be matching on strings. I just spoke with @jamessynge and he is fine with you adding isAnyAddress(), as long as tests are added as well.

wattli · 2017-03-16T03:02:49Z

@htuch , the comments are addressed. Thanks.

mattklein123

Overall great work. Some comments.

mattklein123 · 2017-03-16T03:07:58Z

docs/configuration/listeners/listeners.rst

Can we get an example of what a TCP listener address looks like, and how to specify wildcard vs. specific.

mattklein123 · 2017-03-16T03:09:30Z

include/envoy/network/connection_handler.h

const Network::Address::Instance&

mattklein123 · 2017-03-16T03:10:09Z

include/envoy/server/hot_restart.h

const std::string&

mattklein123 · 2017-03-16T03:10:53Z

include/envoy/server/configuration.h

const std::string& (or potentially have this just return AddressPtr directly).

mattklein123 · 2017-03-16T03:11:10Z

include/envoy/server/instance.h

const std::string& address

mattklein123 · 2017-03-16T03:16:38Z

source/exe/hot_restart.cc

Please use strlcpy here

mattklein123 · 2017-03-16T03:17:18Z

source/server/configuration_impl.h

mattklein123 · 2017-03-16T03:18:38Z

source/server/connection_handler_impl.cc

You are comparing pointers here. This is almost definitely not what you want. Please make sure you have tests that catch this. (Though if you fix per other comment to const Instance& it will be obvious)

done and more tests are added

mattklein123 · 2017-03-16T03:20:05Z

source/server/connection_handler_impl.cc

It would be kind of nice to not have to do 2 searches here. I think this was commented on elsewhere, but wondering if we should do this in follow up. I wonder if we should have different options, "use_original_port" and "use_original_address". Not a huge deal, but throwing it out there.

Yes, it is mentioned above we can use a map to improve efficiency. I prefer leaving a TODO here and get rid of this big PR. :)

mattklein123 · 2017-03-16T03:21:04Z

source/server/server.cc

You have made it work for UDS, you can remove this TODO. :)

removed 👍

htuch

A few more minor comments then I think it's good to hand over to mattklein123@ for approval. Thanks for your patience with the nits.

htuch · 2017-03-16T03:21:11Z

source/common/network/listener_impl.cc

Best to use // C++ comment style for method internal comments. I think the Javadoc style is only for stuff that Doxygen processes.

htuch · 2017-03-16T03:22:33Z

test/common/network/listener_impl_test.cc

Sorry, my bad, this looks good.

htuch · 2017-03-16T03:25:38Z

test/common/network/listener_impl_test.cc

Do you also want to verify that newConnection isn't called on listenerDst?

htuch · 2017-03-16T03:26:35Z

test/common/network/listener_impl_test.cc

The name of this test is a bit confusing, since use_original_dst is set false. Shouldn't it be UseActualDst?

htuch · 2017-03-16T03:31:37Z

source/common/network/listen_socket_impl.cc

Can TcpListenSocket::TcpListenSocket(uint32_t port, bool bind_to_port) below be implemented in terms of this method? That would reduce the amount of code duplication.

Matt raised similar issue above. In config, admin still uses port. So we can have a follow up PR to update admin from using port to address and make corresponding change later. I will let him decide

Yes we can do in follow up. Please add TODO somewhere.

htuch · 2017-03-16T03:32:00Z

source/common/network/listen_socket_impl.cc

Ditto but with TcpListenSocket::TcpListenSocket(int fd, uint32_t port).

Same as above.

wattli · 2017-03-16T20:33:47Z

@mattklein123 , all checks have passed. Please take a look, thanks.

mattklein123 · 2017-03-16T21:02:31Z

source/exe/hot_restart.cc

    return -1;
  }
-  StringUtil::strlcpy(rpc.address_, address.c_str(), address.length());
+  StringUtil::strlcpy(rpc.address_, address.c_str(), address.length() + 1);


The last param is the max length of the target buffer, which is why it didn't work before.

I misunderstood what you meant. Updated

wattli · 2017-03-16T21:06:36Z

Right.

…

On Thu, Mar 16, 2017 at 2:02 PM, Matt Klein ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In source/exe/hot_restart.cc <#570 (comment)>: > @@ -173,7 +173,7 @@ int HotRestartImpl::duplicateParentListenSocket(const std::string& address) { if (address.length() >= sizeof(rpc.address_)) { return -1; } - StringUtil::strlcpy(rpc.address_, address.c_str(), address.length()); + StringUtil::strlcpy(rpc.address_, address.c_str(), address.length() + 1); The last param is the max length of the target buffer, which is why it didn't work before. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#570 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AXQIZjGO7Vp0YRUEegjA8lDJB8s2NA1Hks5rmaNrgaJpZM4MdErn> .

wattli · 2017-03-16T21:13:38Z

@mattklein123 , do you have more comments on this PR?

mattklein123 · 2017-03-16T21:14:37Z

I haven't looked yet. Can you fix the thing I just mentioned? The last param should be sizeof(rpc.address_)

wattli · 2017-03-16T22:05:12Z

@mattklein123 , checks passed, please take a look when available, thanks :)

mattklein123 · 2017-03-17T00:07:51Z

configs/envoy_double_proxy.template.json

    {# TCP listener for external port 443 (SSL). Assumes a TCP LB in front such as ELB which
       supports proxy proto. #}
-    {{ listener(9300,True,True) }},
+    {{ listener("tcp://127.0.0.1:9300",True,True) }},


Please add TODO somewhere

mattklein123 · 2017-03-17T00:08:03Z

configs/envoy_double_proxy.template.json

    {# TCP listener for external port 80 (non-SSL). Assumes a TCP LB in front such as ELB which
       supports proxy proto. #}
-    {{ listener(9301,False,True) }}
+    {{ listener("tcp://127.0.0.1:9301",False,True) }}


mattklein123 · 2017-03-17T00:08:20Z

configs/envoy_front_proxy.template.json

       supports proxy proto. #}
-    {{ listener(9300, ssl=True, proxy_proto=True, router_file='envoy_router.template.json') }},
-    {{ listener(9301, ssl=False, proxy_proto=True, router_file='envoy_router.template.json') }},
+    {{ listener("tcp://127.0.0.1:9300", ssl=True, proxy_proto=True, router_file='envoy_router.template.json') }},


mattklein123 · 2017-03-17T00:08:24Z

configs/envoy_front_proxy.template.json

-    {{ listener(9300, ssl=True, proxy_proto=True, router_file='envoy_router.template.json') }},
-    {{ listener(9301, ssl=False, proxy_proto=True, router_file='envoy_router.template.json') }},
+    {{ listener("tcp://127.0.0.1:9300", ssl=True, proxy_proto=True, router_file='envoy_router.template.json') }},
+    {{ listener("tcp://127.0.0.1:9301", ssl=False, proxy_proto=True, router_file='envoy_router.template.json') }},


mattklein123 · 2017-03-17T00:08:35Z

configs/envoy_front_proxy.template.json

    {# TCP listener for backhaul traffic from the double proxy.
       See envoy_double_proxy.template.json #}
-    {{ listener(9400, ssl=True, proxy_proto=False, pin_double_proxy_client=True,
+    {{ listener("tcp://127.0.0.1:9400", ssl=True, proxy_proto=False, pin_double_proxy_client=True,


mattklein123 · 2017-03-17T00:12:41Z

include/envoy/network/connection_handler.h

   */
-  virtual Network::Listener* findListenerByPort(uint32_t port) PURE;
+  virtual Network::Listener*
+  findListenerByAddress(const Network::Address::InstancePtr& address) PURE;


const Network::Address::Instance& (not InstancePtr)

I tried, it does not work in testing file because no comparison of Address::Instance is defined.

That doesn't make sense, as you end up comparing them in the end. https://github.com/lyft/envoy/blob/master/include/envoy/network/address.h#L80

You had some other compiler error. Please try again or ask someone for an over the shoulder check.

mattklein123 · 2017-03-17T00:13:45Z

source/common/network/address_impl.h

  struct IpHelper : public Ip {
    const std::string& addressAsString() const override { return friendly_address_; }
+    bool isAnyAddress() const override {
+      return memcmp(&ipv6_.address_.sin6_addr, &in6addr_any, sizeof(struct in6_addr));


I think you actually want 0 == memcmp(...). Please add test for this.

mattklein123 · 2017-03-17T00:14:08Z

source/common/network/listen_socket_impl.cc

Yes we can do in follow up. Please add TODO somewhere.

mattklein123 · 2017-03-17T00:16:12Z

source/exe/hot_restart.cc


  RpcGetListenSocketRequest rpc;
-  rpc.port_ = port;
+  if (address.length() >= sizeof(rpc.address_)) {


not sure if I would bother with this check. strlcpy() will protect. I would just put in an ASSERT(address.length() < sizeof(rpc.address_))

mattklein123 · 2017-03-17T00:17:51Z

source/server/server.cc


-int InstanceImpl::getListenSocketFd(uint32_t port) {
+int InstanceImpl::getListenSocketFd(const std::string& address) {
+  Network::Address::InstancePtr addr = Network::Utility::resolveUrl(address);


No point in resolving this, just do the string compare directly. Either it matches or it doesn't.

I have to, address is in format 'tcp://address:port' while localAddress()->asString() is in format 'address:port'

mattklein123 · 2017-03-17T16:18:32Z

configs/configgen.py

 # optional external service ports: built from external_virtual_hosts above. Each external host
 #                                  that Envoy proxies to listens on its own port.
 # optional mongo ports: built from mongos_servers above.
+# TODO(wattli): investigate why a bad config (with port but not address) can


Sorry this is not an acceptable TODO, it will cause production issues.

The issue is that you are returning an std::string from the configuration, and it doesn't get bound to an address until the listeners actually start running on the workers. I mentioned this a while ago, but please have the configuration return an Address::InstancePtr instead of a string. This will cause an exception to be thrown if the address cannot be resolved at config load time.

mattklein123 · 2017-03-17T17:36:29Z

source/common/network/listen_socket_impl.cc


 // TODO(wattli): remove this once the admin port is updated with address.
-TcpListenSocket::TcpListenSocket(const std::string& address, bool bind_to_port) {
+TcpListenSocket::TcpListenSocket(const Address::InstancePtr& address, bool bind_to_port) {


Just pass Address::InstancePtr here (passing by const& can lead to subtle bugs). Same below.

mattklein123 · 2017-03-17T17:44:49Z

include/envoy/server/configuration.h

+   * @return std::string the address.
   */
-  virtual uint64_t port() PURE;
+  virtual const std::string& address() PURE;


@wattli this is where you need to return Address::InstancePtr. Resolve it inside the configuration. Otherwise https://github.com/lyft/envoy/blob/master/test/example_configs_test.cc#L32 will still potentially pass, since you first resolve it when the server starts running.

wattli · 2017-03-17T19:26:21Z

@mattklein123 @htuch , please take a look.

mattklein123 · 2017-03-17T23:17:41Z

source/server/configuration_impl.h

    Network::FilterChainFactory& filterChainFactory() override { return *this; }
-    const std::string& address() override { return address_; }
+    Network::Address::InstancePtr address() override {
+      return Network::Utility::resolveUrl(address_);


Still not right. It doesn't help you if you don't resolve until you access. Resolve at construction time and change address_ to Address::InstancePtr

Done, thanks.

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

Description: I believe this was probably a bad copy paste. Risk Level: low Testing: builds. Signed-off-by: Jose Nino <jnino@lyft.com> Signed-off-by: JP Simard <jp@jpsim.com>

…pha.0 in the k8s-io group across 1 directory (#570)

lizan reviewed Mar 14, 2017

View reviewed changes

wattli force-pushed the dev branch from 5ffb4f0 to 90a34c5 Compare March 15, 2017 00:04

wattli force-pushed the dev branch 4 times, most recently from ab673ba to 5daa5c4 Compare March 15, 2017 04:23

kyessenov reviewed Mar 15, 2017

View reviewed changes

htuch reviewed Mar 15, 2017

View reviewed changes

mattklein123 reviewed Mar 16, 2017

View reviewed changes

htuch reviewed Mar 16, 2017

View reviewed changes

Tao Li added 2 commits March 16, 2017 10:11

Update port field with address for envoy config

bc288e1

Add more tests and address comments.

11fb257

Address a small typo

350981c

mattklein123 reviewed Mar 16, 2017

View reviewed changes

Address comment

b6a109f

mattklein123 reviewed Mar 17, 2017

View reviewed changes

Tao Li added 3 commits March 16, 2017 20:51

Address comments

473108a

Add a TODO to investigate envoy compile error

aad46bf

Fix test

97dc3e9

mattklein123 reviewed Mar 17, 2017

View reviewed changes

Address comment

e42dd0d

mattklein123 reviewed Mar 17, 2017

View reviewed changes

address comments

2f1b193

mattklein123 reviewed Mar 17, 2017

View reviewed changes

address comments

d94dd7a

mattklein123 approved these changes Mar 17, 2017

View reviewed changes

mattklein123 merged commit fb42918 into envoyproxy:master Mar 18, 2017

htuch mentioned this pull request Mar 20, 2017

Error emitted during first build in clean workspace #587

Closed

wattli changed the title ~~Update port to address as listener identity.~~ Mutual TLS: Update port to address as listener identity. Sep 16, 2018

lambdai pushed a commit to lambdai/envoy-dai that referenced this pull request Jul 21, 2020

Add support for wavm tests. (envoyproxy#570)

05dd983

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

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

nit: remove unneeded variable (#570)

85162b1

Description: I believe this was probably a bad copy paste. Risk Level: low Testing: builds. Signed-off-by: Jose Nino <jnino@lyft.com> Signed-off-by: JP Simard <jp@jpsim.com>

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

nit: remove unneeded variable (#570)

9b28a09

Description: I believe this was probably a bad copy paste. Risk Level: low Testing: builds. Signed-off-by: Jose Nino <jnino@lyft.com> Signed-off-by: JP Simard <jp@jpsim.com>

mathetake pushed a commit that referenced this pull request Mar 3, 2026

build(deps): bump k8s.io/apimachinery from 0.33.0-beta.0 to 0.34.0-al…

8485630

…pha.0 in the k8s-io group across 1 directory (#570)

Conversation

wattli commented Mar 14, 2017 • edited by mattklein123 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wattli commented Mar 14, 2017

Uh oh!

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

wattli commented Mar 14, 2017

Uh oh!

mattklein123 commented Mar 14, 2017

Uh oh!

htuch commented Mar 14, 2017

Uh oh!

htuch commented Mar 14, 2017

Uh oh!

wattli commented Mar 15, 2017

Uh oh!

htuch commented Mar 15, 2017

Uh oh!

wattli commented Mar 15, 2017

Uh oh!

kyessenov Mar 14, 2017 • 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!

htuch Mar 15, 2017 • 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!

htuch left a comment • 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!

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

wattli commented Mar 14, 2017 •

edited by mattklein123

Loading

kyessenov Mar 14, 2017 •

edited

Loading

htuch Mar 15, 2017 •

edited

Loading

htuch left a comment •

edited

Loading