Common: Introduce StopAllIteration filter status for decoding and encoding filters by soya3129 · Pull Request #5954 · envoyproxy/envoy

soya3129 · 2019-02-14T15:36:55Z

Signed-off-by: Yang Song yasong@google.com

Description: Created a new filter return status for headers: StopAllTypesIteration. Returning this status means the filter and the filters following it stop iteration on not only headers but also data and trailers. The upstream filters will not be impacted. The change only includes decoding path.

Risk Level: High. HCM refactor. Should be a no-op as no filters return the new status but a lot of code is changing.
Testing: Integration test for decoding path.
Docs Changes: Comments added for the new status.

#5842

Signed-off-by: Yang Song <yasong@google.com>

soya3129 · 2019-02-14T15:37:27Z

I only include the decoding path change in this PR. Because the code change is very simple, I suspect I must have missed some cases. It's better to get feedback before I am too far away from the right path, ;)

The plan is to change decoding path first, and then change the existing filters (I think all the impacted filters are decoding filters?). After that, I will modify metadata decoding path accordingly. WDYT?

soya3129 · 2019-02-14T15:38:25Z

cc @birenroy

Signed-off-by: Yang Song <yasong@google.com>

junr03 · 2019-02-14T17:54:24Z

@soya3129 nit: do you mind adding a more expressive PR title e.g introduce StopAllTypesIteration filter status for decoding filters it makes it easier to understand what the content of the PR is.

soya3129 · 2019-02-14T18:26:13Z

@soya3129 nit: do you mind adding a more expressive PR title e.g introduce StopAllTypesIteration filter status for decoding filters it makes it easier to understand what the content of the PR is.

Oops. Thanks for the reminder! Fixed.

mattklein123

Very nice. A few comments to get started. Thank you!

/wait

mattklein123 · 2019-02-14T19:59:54Z

include/envoy/http/filter.h

+  // rate-limiting filter can be added before the filter that can return StopAllTypesIteration.
+  // Only used in decoding path.
+  // TODO(soya3129): add tests for encoding path, and remove the decoding path condition.
+  StopAllTypesIteration


nit: I would call this StopAllIteration personally.

Also, I think we likely need StopAllIterationAndBuffer as well as StopAllIterationAndWatermark or something along those lines to match the options we give on on data? WDYT?

Fixed. Thanks!

mattklein123 · 2019-02-14T20:00:37Z

source/common/http/conn_manager_impl.cc

                   static_cast<const void*>(this));
  ASSERT(stopped_);
  stopped_ = false;
+  stopped_all_ = false;


nit: Instead of stopped_ and _stopped_all_ can we make this a tri-state enum? I think it will help the logic be more clear.

Fixed. Thanks!

mattklein123 · 2019-02-14T20:02:45Z

source/common/http/conn_manager_impl.cc

    return false;
+  } else if (status == FilterHeadersStatus::StopAllTypesIteration) {
+    stopped_ = true;
+    stopped_all_ = true;


Where do we look at stopped_all_ to determine to not call any filter callbacks? I don't see that? This is the place also that for data we will have to decide on buffer vs watermark processing?

I checked stopped_all_ value in line 1597 and line 1623 in conn_manager_impl.cc. Let's say there are 3 filters:

filter1 ----- filter2 ----- filter3

where filter2::decodeHeaders() returns StopAllIteration. We still allow data to call filter1::decodeData(), and filter2::decodeData(), but will not call filter3::decodeData(). This is achieved by the check filter2::stop_all_ value in line 1597. If filter2::stop_all_ is true, we will buffer data but not call commonContinue(), and because commonHandleAfterDataCallback returns false, filter3::decodeData() will not be called.

The same logic applies to trailers, where stop_all_ value is checked in line 1623. But I can be completely wrong. Please let me know the feedback. Thanks!

OK I see. I think though we don't want to call any other filter2 methods, including data and trailers. This is how most filters would use this I think, including auth, rate limit, etc. where they make an outbound call as part of the decodeHeaders() path. Right?

Assuming ^ is what we want, I think this means that you need to check stop all status before calling filter functions, and then also handle buffer/watermark for data?

Ah, I see! I mistakenly thought we didn't allow callbacks for filters after filter2, excluding filter2. Will make the change. Thanks!

About stop-all options for data. I thought we want stop-all for headers only at least for now? (#5842 (comment)). But it makes sense to be consistent with data as well. Will make the change and address other comments as well. Thanks for the feedback!

About stop-all options for data. I thought we want stop-all for headers only at least for now?

We do want it only for headers, but what I'm saying is that we need to tell the HCM how to handle subsequent data. Do we buffer up to the limit and then error? Or do we buffer and try to apply flow control? Those are the two options.

Sorry I misread the comment!

My original thoughts was if users don't want to buffer large data or want the flow control feature, they may add a rate-limiting filter before the filter that can return StopAllIteration for headers:

"// To avoid buffering large amount of data, a rate-limiting filter can be added before the filter that
// can return StopAllTypesIteration."

If users don't care, we can buffer up to the limit and then error. But I think I missed to set decoder_filters_streaming_ to be false in this case. Sorry, I will fix it.

Do you think it's reasonable to let users add rate-limiting filter, otherwise, fails when the limit is reached?

Do you think it's reasonable to let users add rate-limiting filter, otherwise, fails when the limit is reached?

I don't think so. I think it would be more natural given the rest of the API to offer StopAllIterationAndBuffer and StopAllIterationAndWatermark, then given the return code, subsequent data can be handled in either the streaming or buffer way as the data return codes do. I don't think it would be very hard to implement. WDYT?

I see! Is the suggestion to introduce StopAllIterationAndBuffer and StopAllIterationAndWatermark as decodeHeaders()'s return status (not as data's status)? I think it totally makes sense! Sorry for being slow!! I will change accordingly.

Yup exactly! I think these are new headers return codes, and they guide what we do when data comes in. You aren't being slow, this is very tough stuff! :)

rshriram · 2019-02-18T16:31:45Z

include/envoy/http/filter.h

+  // that filter stop processing. The filters before are not impacted. continueDecoding() MUST be
+  // called if continued filter iteration is desired. To avoid buffering large amount of data, a
+  // rate-limiting filter can be added before the filter that can return StopAllTypesIteration.
+  // Only used in decoding path.


would be nice to add a line on the use case for this type of status

Fixed. Thanks for the feedback.

Signed-off-by: Yang Song <yasong@google.com>

soya3129 · 2019-02-21T15:10:35Z

I am still working on enabling h2 for one of the tests. But I figure I should ask for another review in case I am off the track again, :) Thanks!

Signed-off-by: Yang Song <yasong@google.com>

mattklein123

Thanks this definitely looks like it's on the right track. Great work! A couple of questions.

/wait

mattklein123 · 2019-02-21T20:58:41Z

source/common/http/conn_manager_impl.cc

    stopped_ = true;
    return false;
+  } else if (status == FilterHeadersStatus::StopAllIterationAndBuffer) {
+    stopped_ = true;


Can we merge stopped_ and stop_all_state_ into a single enum? I think it will make it easier to understand the overall flow? Is that possible?

Good point! Fixed.

mattklein123 · 2019-02-21T21:00:51Z

source/common/http/conn_manager_impl.cc

    entry = decoder_filters_.begin();
  } else {
-    entry = std::next(filter->entry());
+    if ((*(filter->entry()))->iterate_from_current_filter_) {


Is there a way to remove iterate_from_current_filter_ to make the logic simpler? I'm wondering if we check the current filter's stop state, and if stopped, just do the common data after stop logic?

Good call! Fixed.

stale · 2019-02-28T21:07:48Z

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!

soya3129 · 2019-02-28T21:14:49Z

Sorry about the delay! I am working on the comments. Thanks!

Signed-off-by: Yang Song <yasong@google.com>

danzh2010

Looks good over all! I left some nit-picking comments, and a general suggestion here to document the new functions in .h file to follow styles like below:
/**

description for function
@param description for parameter
*/

danzh2010 · 2019-03-28T15:18:31Z

source/common/http/conn_manager_impl.h

    void addStreamEncoderFilterWorker(StreamEncoderFilterSharedPtr filter, bool dual_filter);
    void chargeStats(const HeaderMap& headers);
+    // Returns the encoder filter to start iteration with. If the function is called from a filter
+    // that should always iterate from the next filter, always_start_next should be set to true.


always_start_next is not one of the arguments.

Fixed. Thanks!

danzh2010 · 2019-03-28T15:19:05Z

source/common/http/conn_manager_impl.h

+    commonEncodePrefix(ActiveStreamEncoderFilter* filter, bool end_stream,
+                       FilterIterationStartState filter_iteration_start_state);
+    // Returns the decoder filter to start iteration with. If the function is called from a filter
+    // that should always iterate from the next filter, always_start_next should be set to true.


always_start_next is not one of the arguments.

Fixed. Thanks!

danzh2010 · 2019-03-28T15:21:42Z

source/common/http/conn_manager_impl.h

    void maybeEndEncode(bool end_stream);
    uint64_t streamId() { return stream_id_; }
+    // Returns true if filter has stopped iteration for all frame types. Otherwise, returns false.
+    // filter_streaming is the variable to indicate if stream is streaming.


Indicate |filter_streaming| is output.

Fixed. Thanks!

danzh2010 · 2019-03-28T15:46:39Z

test/integration/filters/decode_headers_return_stop_all_filter.cc

+  size_t added_size_ = 0;
+  size_t buffer_limit_ = 0;
+  bool watermark_enabled_ = false;
+  bool first_trigger_ = false;


nit: rename to is_first_trigger to reflect it's a boolean.

Fixed. Thanks!

danzh2010 · 2019-03-28T16:32:20Z

source/common/http/conn_manager_impl.cc

+    ActiveStreamDecoderFilter* filter, FilterIterationStartState filter_iteration_start_state) {
+  if (!filter) {
+    return decoder_filters_.begin();
+  } else {


early returned already, no need for else statement

Fixed. Thanks!

Signed-off-by: Yang Song <yasong@google.com>

danzh2010

A few more nits in tests. Looks good all else!

danzh2010 · 2019-04-01T14:34:54Z

test/integration/filters/decode_headers_return_stop_all_filter.cc

+    ASSERT(entry_content != nullptr && entry_added != nullptr);
+    content_size_ = std::stoul(std::string(entry_content->value().getStringView()));
+    added_size_ = std::stoul(std::string(entry_added->value().getStringView()));
+    Http::HeaderEntry* entry_is_first_trigger =


misfire of s/first_trigger/is_first_trigger?

Fixed. Thanks!

Not fixed. Is the name of entry_is_first_trigger is a misfire of s/first_trigger/is_first_trigger?

No. entry_is_first_trigger means the variable is the entry for "is_first_trigger" header key.

danzh2010 · 2019-04-01T14:52:28Z

test/integration/filters/decode_headers_return_stop_all_filter.cc

+      } else {
+        // Create a new timer to try again later.
+        delay_timer_->disableTimer();
+        delay_timer_.reset();


These 2 lines are not needed:
delay_timer_->disableTimer();
delay_timer_.reset();

Fixed. Thanks!

danzh2010 · 2019-04-01T15:00:19Z

test/integration/filters/decode_headers_return_stop_all_filter.cc

+      // If decodeHeaders() returns StopAllIterationAndBuffer, triggers the timer when all the
+      // request data has been received. If decodeHeaders() returns StopAllIterationAndWatermark,
+      // triggers the timer when received data exceed buffer limit.
+      if ((content_size_ > 0 &&


What would happen if "buffer_limit" or "content_size" is 0 in request headers? Will the pipe line stop forever?

I think buffer_limit should not be 0 otherwise that says we don't want to buffer any data. content_size can be 0. It just means we send 0 data, :)

Can you add DCHECK to prevent "buffer_limit" from being set to 0? Though we have full control over the test input, but it's better to fail the test early for easy debugging.

If "content_size" is 0, according to my reading of the if condition, continueDecoding() will never be called. Do we need content_size > 0 there?

This filter is designed to call continueDecoding() when all the data have been received. It is for the ease of test of StopAll status, not for checking if continueDecoding() is called. I think it should be OK, since it's testing only, :)

danzh2010 · 2019-04-01T15:48:42Z

test/integration/protocol_integration_test.cc

+  } else {
+    EXPECT_EQ(count_ * size_ + added_decoded_data_size_ * 1, upstream_request_->bodyLength());
+  }
+  EXPECT_EQ(true, upstream_request_->complete());


The check after getting response is repeated in above tests. Can you move the response verification into a helper function and call it at the end of the test?

Helper function added. Thanks!

danzh2010 · 2019-04-01T15:52:04Z

test/integration/protocol_integration_test.cc

+TEST_P(DownstreamProtocolIntegrationTest, testDecodeHeadersReturnsStopAll) {
+  config_helper_.addFilter(R"EOF(
+name: call-decodedata-once-filter
+)EOF");


Can you add comment why call-decodedata-once-filter's expectation can be met after decode-headers-return-stop-all-filter?

I have comments in call_decodedata_once_filter.cc to explain the number. Basically, depending on where decodeData is triggerred, either from decodeTrailer()'s addDecodedData() or from filter's callback decodeData(), the data received can be different.

danzh2010 · 2019-04-01T15:52:29Z

test/integration/protocol_integration_test.cc

+name: decode-headers-return-stop-all-filter
+)EOF");
+  config_helper_.addFilter(R"EOF(
+name: passthrough-filter


Why do we need a passthrough-filter at the beginning?

This is to test we can stop-all when there is another filter in front of the filter chain. It's not required. Just for testing, :)

danzh2010 · 2019-04-01T16:09:25Z

test/integration/protocol_integration_test.cc

+name: passthrough-filter
+)EOF");
+
+  // Sets initial stream window to min value to make the client sensitive to a low watermark.


s/to make the client sensitive to a low watermark/to make sure the client initially send less data than the buffer_limit.

I think it may not guarantee the client sends less than buffer_limit. I think when watermark is reached, we stop reading, but client can still send, :)

I think it may not guarantee the client sends less than buffer_limit.
Even not at the beginning of sending? I thought we use the small flow control window to prevent client from sending more than the buffer_limit in order to enable filter to start buffering.

When buffer_limit is reached, we stop reading. But it can't guarantee the client sends less than buffer_limit.

mattklein123

Thanks this looks awesome, just 2 small nits. Really love how much we were able to simplify/clarify this. will defer to @alyssawilk and @danzh2010 for further review and merge.

mattklein123 · 2019-04-01T18:02:58Z

source/common/http/conn_manager_impl.h

    ~ActiveStream();

+    // Indicates which filter to start the iteration with.
+    enum class FilterIterationStartState { Always_start_from_next, Can_start_from_current };


nit: AlwaysStartFromNext and CanStartFromCurrent

Fixed. Thanks!

mattklein123 · 2019-04-01T18:03:47Z

source/common/http/conn_manager_impl.h

    void addStreamDecoderFilterWorker(StreamDecoderFilterSharedPtr filter, bool dual_filter);
    void addStreamEncoderFilterWorker(StreamEncoderFilterSharedPtr filter, bool dual_filter);
    void chargeStats(const HeaderMap& headers);
+    // Returns the encoder filter to start iteration with. If the function is called from a filter


nit: now with the enum I think the 2nd part of this comment is obvious and can be removed, same below.

Fixed. Thanks!

Signed-off-by: Yang Song <yasong@google.com>

danzh2010 · 2019-04-02T16:02:44Z

test/integration/filters/decode_headers_return_stop_all_filter.cc

+      // If decodeHeaders() returns StopAllIterationAndBuffer, triggers the timer when all the
+      // request data has been received. If decodeHeaders() returns StopAllIterationAndWatermark,
+      // triggers the timer when received data exceed buffer limit.
+      if ((content_size_ > 0 &&


Can you add DCHECK to prevent "buffer_limit" from being set to 0? Though we have full control over the test input, but it's better to fail the test early for easy debugging.

If "content_size" is 0, according to my reading of the if condition, continueDecoding() will never be called. Do we need content_size > 0 there?

danzh2010 · 2019-04-02T16:07:23Z

test/integration/protocol_integration_test.cc

+name: passthrough-filter
+)EOF");
+
+  // Sets initial stream window to min value to make the client sensitive to a low watermark.


I think it may not guarantee the client sends less than buffer_limit.
Even not at the beginning of sending? I thought we use the small flow control window to prevent client from sending more than the buffer_limit in order to enable filter to start buffering.

danzh2010 · 2019-04-02T16:10:40Z

test/integration/filters/decode_headers_return_stop_all_filter.cc

+    ASSERT(entry_content != nullptr && entry_added != nullptr);
+    content_size_ = std::stoul(std::string(entry_content->value().getStringView()));
+    added_size_ = std::stoul(std::string(entry_added->value().getStringView()));
+    Http::HeaderEntry* entry_is_first_trigger =


Not fixed. Is the name of entry_is_first_trigger is a misfire of s/first_trigger/is_first_trigger?

danzh2010 · 2019-04-02T16:25:06Z

source/common/http/conn_manager_impl.cc

@@ -1044,9 +1048,29 @@ ConnectionManagerImpl::ActiveStream::commonEncodePrefix(ActiveStreamEncoderFilte

  if (!filter) {


This block can be combined with the if block above.

Fixed. Thanks!

danzh2010 · 2019-04-02T17:20:43Z

test/integration/filters/call_decodedata_once_filter.cc

+
+namespace Envoy {
+
+// A filter that only allows decodeData() to be called once with fixed data length.


Where do we guarantee that decodeData() can only be called once?

Signed-off-by: Yang Song <yasong@google.com>

alyssawilk · 2019-04-03T17:33:27Z

The good news is this is ready to land. Hurrah!

The bad news is our release got pushed back from last week to this week, and after some discussion on #envoy-maintainers we're both close enough to cutting this release (and doing our first round of security patches) I think we'll merge this first thing after the release is cut (theoretically Friday). Sorry about the additional delay and thanks again for all your work getting this tricky feature ready to land!

mattklein123 · 2019-04-03T17:42:53Z

Thank you so much @soya3129 for your awesome work on this change. I know it took a lot of iterations but I'm very happy with how it turned out. We will get this merged ASAP!

soya3129 · 2019-04-03T18:23:49Z

Thanks for the opportunity and all the reviews! I learned a lot, :)

mattklein123 · 2019-04-05T18:51:16Z

@soya3129 can you merge master and we can get this in? Thank you!

/wait

Signed-off-by: Yang Song <yasong@google.com>

* master: (137 commits) test: router upstream log to v2 config stubs (envoyproxy#6499) remove idle timeout validation (envoyproxy#6500) build: Change namespace of chromium_url. (envoyproxy#6506) coverage: exclude chromium_url (envoyproxy#6498) fix(tracing): allow 256 chars in path tag (envoyproxy#6492) Common: Introduce StopAllIteration filter status for decoding and encoding filters (envoyproxy#5954) build: update PGV url (envoyproxy#6495) subset lb: avoid partitioning host lists on worker threads (envoyproxy#6302) ci: Make envoy_select_quiche no-op. (envoyproxy#6393) watcher: notify when watched files are modified (envoyproxy#6215) stat: Add counterFromStatName(), gaugeFromStatName(), and histogramFromStatName() (envoyproxy#6475) bump to 1.11.0-dev (envoyproxy#6490) release: bump to 1.10.0 (envoyproxy#6489) hcm: path normalization. (#1) build: import manually minified Chrome URL lib. (envoyproxy#3) codec: reject embedded NUL in headers. (envoyproxy#2) Added veryfication if path contains query params and add them to path header (envoyproxy#6466) redis: basic integration test for redis_proxy (envoyproxy#6450) stats: report sample count as an integer to prevent loss of precision (envoyproxy#6274) Added VHDS protobuf message and updated RouteConfig to include it. (envoyproxy#6418) ... Signed-off-by: Michael Puncel <mpuncel@squareup.com>

Yang Song added 2 commits February 13, 2019 18:40

Enable StopAllTypesIteration for decoding path

3835768

Signed-off-by: Yang Song <yasong@google.com>

update comments

9cf21dd

Signed-off-by: Yang Song <yasong@google.com>

fix clang_tidy

4429d12

Signed-off-by: Yang Song <yasong@google.com>

mattklein123 self-assigned this Feb 14, 2019

soya3129 changed the title ~~Stopall~~ Introduce StopAllTypesIteration filter status for decoding filters Feb 14, 2019

mattklein123 requested changes Feb 14, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Feb 14, 2019

soya3129 changed the title ~~Introduce StopAllTypesIteration filter status for decoding filters~~ Introduce StopAllIteration filter status for decoding filters Feb 14, 2019

rshriram reviewed Feb 18, 2019

View reviewed changes

Yang Song added 4 commits February 20, 2019 18:22

address comments

94c4d44

Signed-off-by: Yang Song <yasong@google.com>

add use case explanation

eabbeb6

Signed-off-by: Yang Song <yasong@google.com>

fix format

0bd2315

Signed-off-by: Yang Song <yasong@google.com>

merge from master

b437b67

Signed-off-by: Yang Song <yasong@google.com>

repokitteh-read-only bot removed the waiting label Feb 21, 2019

Yang Song added 2 commits February 21, 2019 10:04

fix format

50b7e7c

Signed-off-by: Yang Song <yasong@google.com>

fix format and add new files

5c9fc64

Signed-off-by: Yang Song <yasong@google.com>

Yang Song added 4 commits February 21, 2019 10:25

remove unused variable

b3527f7

Signed-off-by: Yang Song <yasong@google.com>

cleanups

eaa2ceb

Signed-off-by: Yang Song <yasong@google.com>

emtpy commit

48a3833

Signed-off-by: Yang Song <yasong@google.com>

emtpy commit

ac8a553

Signed-off-by: Yang Song <yasong@google.com>

mattklein123 requested changes Feb 21, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Feb 21, 2019

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Feb 28, 2019

Yang Song added 2 commits March 27, 2019 12:12

address conn_manager_impl_test.cc comments

beb19b8

Signed-off-by: Yang Song <yasong@google.com>

address review comments

d875ca4

Signed-off-by: Yang Song <yasong@google.com>

repokitteh-read-only bot removed the waiting label Mar 27, 2019

danzh2010 reviewed Mar 28, 2019

View reviewed changes

mattklein123 added the waiting label Mar 28, 2019

Yang Song added 2 commits March 28, 2019 14:35

address comments

e7cebab

Signed-off-by: Yang Song <yasong@google.com>

avoid saved state in addEn/decodedData

d02d056

Signed-off-by: Yang Song <yasong@google.com>

repokitteh-read-only bot removed the waiting label Mar 28, 2019

danzh2010 reviewed Apr 1, 2019

View reviewed changes

mattklein123 previously approved these changes Apr 1, 2019

View reviewed changes

mattklein123 assigned danzh2010 and unassigned mattklein123 Apr 1, 2019

address comments

c2becd0

Signed-off-by: Yang Song <yasong@google.com>

soya3129 dismissed mattklein123’s stale review via c2becd0 April 1, 2019 22:48

danzh2010 reviewed Apr 2, 2019

View reviewed changes

address comment

9e2e461

Signed-off-by: Yang Song <yasong@google.com>

danzh2010 approved these changes Apr 2, 2019

View reviewed changes

alyssawilk approved these changes Apr 3, 2019

View reviewed changes

mattklein123 added the no stalebot Disables stalebot from closing an issue label Apr 3, 2019

repokitteh-read-only bot added the waiting label Apr 5, 2019

mattklein123 self-assigned this Apr 5, 2019

merge master

4ca783e

Signed-off-by: Yang Song <yasong@google.com>

repokitteh-read-only bot removed the waiting label Apr 5, 2019

mattklein123 merged commit 7c62727 into envoyproxy:master Apr 6, 2019

soya3129 mentioned this pull request Apr 10, 2019

Common: Consume, proxy and insert metadata from downstream to upstream #5656

Merged

		@@ -1044,9 +1048,29 @@ ConnectionManagerImpl::ActiveStream::commonEncodePrefix(ActiveStreamEncoderFilte

		if (!filter) {


		namespace Envoy {

		// A filter that only allows decodeData() to be called once with fixed data length.

Conversation

soya3129 commented Feb 14, 2019 • edited by alyssawilk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

soya3129 commented Feb 14, 2019

Uh oh!

soya3129 commented Feb 14, 2019

Uh oh!

junr03 commented Feb 14, 2019

Uh oh!

soya3129 commented Feb 14, 2019

Uh oh!

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

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soya3129 commented Feb 21, 2019

Uh oh!

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

stale bot commented Feb 28, 2019

Uh oh!

soya3129 commented Feb 28, 2019

Uh oh!

danzh2010 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

soya3129 commented Feb 14, 2019 •

edited by alyssawilk

Loading