[admin] config_dump scaffolding and routes endpoint by jsedgwick · Pull Request #2771 · envoyproxy/envoy

jsedgwick · 2018-03-09T20:52:57Z

Description:
This is a step towards dumping configs in proto form. There is a new config_dump endpoint, which spits out all registered configs. It's pretty-printed JSON for now; obviously we can play with format/serialization.

Anyone can add a config via the ConfigTracker object that hangs off Admin. ConfigTracker makes the whole thing RAII-y, so that the list of config providers is maintained opaquely. This means the end user doesn't have to worry about ownership, capturing this, etc. Contrast with current unenforced pair of addHandler/removeHandler. This sort of managed weak ownership can be useful all over envoy; just in this diff I left a comment where something like ConfigTracker would delete a bunch of fragile code and confusion. I will genericize it in a future diff though, so let's focus on the functionality here and try to punt on extended discussion of this component and its possibilities.

To demonstrate all the structured admin/config dumping scaffolding, I implemented it all for routes. See the data-plane-api PR (envoyproxy/data-plane-api#532) for context. But you can see how other config_dump handlers and, hopefully, all admin endpoints can be turned into structured, arbitrarily consumable proto via this setup.

Risk Level: Medium. New feature on admin subsystem, intended to be minimally intrusive into rest of server

Testing:
New + existing unit. Tested locally with
sudo ./bazel-bin/source/exe/envoy-static -c examples/front-proxy/front-envoy.yaml
Output looks like:
https://gist.github.com/jsedgwick/3a46408a9728ccef9a63d32b4c463c2b

Docs Changes:
I put TODO doxme everywhere I plan to add docs once the code/API is agreed upon. I won't merge this PR until docs are in.

Release Notes:
TODO pending consensus on this PR

Issues:
Makes progress on #2421, #2172

API Changes:]
envoyproxy/data-plane-api#532

Deprecated:
Current routes endpoint should be considered marked for deletion, but I'll do that in a subsequent PR. There should be some nice cleanup in addition to code deletion with this change.

Signed-off-by: James Sedgwick jsedgwick@lyft.com

htuch · 2018-03-09T20:56:43Z

@junr03 can you take first pass? Thanks.

mattklein123

Cool! Few random comments from a quick skim. I did not do a full review. Thanks!

mattklein123 · 2018-03-10T23:00:13Z

include/envoy/router/rds.h

Let's just kill /routes as part of this change. I don't think we need to go through a deprecation cycle for it.

OK. Wasn't intending for a deprecation cycle, rather immediate removal after this change. I do not know what the feeling is around here on PR size from a reviewer's; I was trying to keep it small-ish

The point was also not to delete the old impl before making sure reviewers don't object to the meat of the PR. But I can do so now, no problem.

mattklein123 · 2018-03-10T23:00:54Z

include/envoy/server/config_tracker.h

Use https://github.com/envoyproxy/envoy/blob/master/source/common/common/non_copyable.h

mattklein123 · 2018-03-10T23:01:15Z

include/envoy/server/config_tracker.h

s/= 0/PURE (same elsewhere)

Sure. Curious about that one though, why invent our own keyword? It doesn't save any keystrokes. I understand that pure is an adjective folks might be familiar with, and they absolutely should have made the syntax be = pure;, but it is supposed to stand alongside = default; and = delete;. This way it's more easily confused with const-qualifiers and ref-qualifiers. Syntactically, they're not interchangeable.

mattklein123 · 2018-03-10T23:03:13Z

source/common/router/rds_impl.cc

Why would this ever fail?

Someone stealing the "routes" key for the dump map. See my next comment below.

In general for error handling philosophy I would take a look at: https://github.com/envoyproxy/envoy/blob/master/STYLE.md#error-handling

For this one, I would probably just make this a RELEASE_ASSERT for now to make sure it doesn't get registered over. This is an important part of server functionality so we should just fail hard if someone bungles it with a plugin. We can make the error handling nicer later if this becomes a real issue.

mattklein123 · 2018-03-10T23:07:37Z

source/server/http/admin.cc

don't catch exceptions here. Just crash.

By crash, do you mean RELEASE_ASSERTING or letting the exception escape? Presumably the latter will also crash us, just clarifying your request.

I think you mentioned the desire to be able to dynamically hook in plugins and so on, so I was coding defensively. Is the stance going to be that we won't try to recover from any exception thrown by someone's plugin? It seems that segfaults and such aside, we don't want the rest of envoy to go down when you curl an admin endpoint (i.e. at some unpredictable time after start) because of something outside of this repo's control. So swallowing/logging would be fine here from that point of view because for this code the exception is recoverable.

Just asking about the philosophy here, can definitely change this and just crash.

Per above take a look at the philosophy document. Our general philosophy is fail hard when possible, make it nicer later if needed. In this case I would just let the exceptions propagate which will crash the server hard (for now).

mattklein123 · 2018-03-10T23:07:50Z

source/server/http/admin.cc

I would just ASSERT this

See above, except even softer case since they're just failing to follow an API, not letting an exception escape. I am just picturing everything humming along fine and then someone is firefighting and consuming admin and crashes all their instances because it turns out their logging function didn't return the right thing.

Yup that's a bug and should be tested and fixed. I would fail hard here for now. RELEASE_ASSERT if you like over normal ASSERT.

mattklein123 · 2018-03-10T23:08:39Z

source/server/http/admin.h

This TODO isn't actionable. Either make it more detailed or delete.

I'll do the refactor in this PR since it's actionable for me and most of this code is changing anyways.

mattklein123 · 2018-03-10T23:10:59Z

source/server/http/config_tracker_impl.cc

Normal ASSERT

OK. Again curious about crashing philosophy. Up above, you want to crash, but here, in code we totally own, you don't? If this invariant is violated, ConfigTracker is broken and doesn't do its job.

See philosophy. For normal programming errors that should never happen always use ASSERT.

Still need to switch this to ASSERT

mattklein123 · 2018-03-10T23:11:59Z

include/envoy/server/config_tracker.h

nit: generally we do typedef over using

Sure, will change.

As you are probably aware, they're not always exactly the same, and if this gets genericized I might go back to using for some stuff it interacts better with templates. IMO it's also nicer to unify these aliases with using aliases for namespace members but whatever.

mattklein123 · 2018-03-10T23:14:55Z

source/common/router/rds_impl.cc

Sorry I don't follow why this is necessary. Please remove the word "jank" and make the TODO more clear on why the thing is being done and how it will be fixed in the future. I haven't looked through in detail but it seems odd to me that we need to do this lazy clearing at all.

(Better, just fix it now if a better ownership model would make the code cleaner. This is not a very difficult change).

I will just change it now. Only had to leave it for the old "routes/" implementation which I'll remove in this PR per your request above.

jsedgwick · 2018-03-11T15:16:54Z

Thanks for the comments Matt. Tomorrow, I will remove the current "/routes" implementation and do other refactors mentioned above.

@htuch

…nt (#532) See envoyproxy/envoy#2771 for context I will add documentation once the interfaces and placement of these things is settled. Current location (new admin package) was agreed upon as a good start with @htuch but i don't feel strongly about that or naming. Signed-off-by: James Sedgwick <jsedgwick@lyft.com>

jsedgwick · 2018-03-16T18:54:20Z

oh wtf

mattklein123 · 2018-03-16T19:52:50Z

Per new release note policy please add a release note with any applicable feature docs to https://github.com/envoyproxy/data-plane-api/blob/master/docs/root/intro/version_history.rst. Thank you!

mattklein123 · 2018-03-17T18:59:08Z

@jsedgwick LMK when this is ready to review again (unclear on current state). No rush. Thanks.

jsedgwick · 2018-03-18T01:17:20Z

Yup it's ready.

On Sat, Mar 17, 2018 at 2:59 PM Matt Klein ***@***.***> wrote: @jsedgwick <https://github.com/jsedgwick> LMK when this is ready to review again (unclear on current state). No rush. Thanks. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2771 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAc4Ih3wUaZKVVs7llHTTw-i_-EMmZm2ks5tfVz-gaJpZM4Sk2kM> .

-- *James Sedgwick* Software Engineer, Networking Infrastructure 973.738.0737 <+19737380737> [image: Lyft] <http://www.lyft.com/>

mattklein123 · 2018-03-19T02:06:04Z

@jsedgwick tests are failing and there are merge conflicts. Can you fix?

jsedgwick · 2018-03-19T18:05:03Z

@mattklein123 rebased and fixed builds, should be good to go

mattklein123

Very nice. Some small comments otherwise looks great.

mattklein123 · 2018-03-20T01:06:08Z

include/envoy/server/admin.h

mattklein123 · 2018-03-20T01:18:46Z

include/envoy/router/rds.h

I think you can return a const envoy::api::v2::RouteConfiguration& here. You can use default_instance() in places where you need a reference to a default configuration.

mattklein123 · 2018-03-20T01:20:12Z

include/envoy/router/route_config_provider_manager.h

const envoy::api::v2::RouteConfiguration&. Also can we add some API docs for this new function?

Will add docs.

Re: pass by const& vs value, this isn't explicitly addressed in our/G's style guide? I'm sure you already know the reasons I like it by value but I wouldn't fight about it save perf-sensitive paths, which this obviously isn't.

We should flesh it out. In general the existing code follows roughly the following guidelines.

"For small objects, pass by value. For large objects, prefer passing by const-reference. If it's clear that move semantics will yield better performance, pass by rvalue reference with an explicit && operator. The reason for the explicit rvalue reference indication is that the call site then must either pass a temporary or use std::move() which makes the intention more clear." (The latter part is already covered when discussing unique_ptr, etc.).

I, for one, try to always pass by value when possible (and move() to take advantage) aside from primitives. This is reflexive enough for me that I want to get ahead of whether you/other maintainers care enough about this that I have to change that.

So, to formalize my take:

For cheap-copy objects, e.g. primitives and some POD, pass by value

If the function needs its own copy of the object for any reason, pass by value

Otherwise, pass by const reference

The additional restrictions you've laid out above, while fine for existing code, don't really make sense to me going forward. They complicate things for the caller instead of giving the caller the option of whether they care about perf. Passing by const& precludes taking advantage of move semantics, and passing by && precludes anything but rvalues. Passing by value gives you both.

It's obviously possible to have the internet's millionth long discussion about this but I also don't see it as something where consistency is critical, like with many extant conventions. In any event let's take it off this PR if necessary.

This is reflexive enough for me that I want to get ahead of whether you/other maintainers care enough about this that I have to change that.

Yes, we are care about consistency.

It's obviously possible to have the internet's millionth long discussion about this but I also don't see it as something where consistency is critical, like with many extant conventions. In any event let's take it off this PR if necessary.

The expectation is that in general consistency is followed unless there is a good reason not to, which in this case is a matter of opinion. Happy to discuss further offline but at this point I'm going to consider this thread closed and assume that you will be consistent in this area. Thanks!

mattklein123 · 2018-03-20T01:20:55Z

include/envoy/server/config_tracker.h

nit: /** (same elsewhere if applicable). We should probably just check this with the linter at some point.

Yeah, oops. Don't we just use vanilla clang-format? Or would we have control over this somewhere.

/ worth filing an issue? By my count there are roughly 15 occurrences in trunk atm

Sure, it's not a big deal, just noticed it.

mattklein123 · 2018-03-20T01:21:13Z

include/envoy/server/config_tracker.h

mattklein123 · 2018-03-20T01:22:47Z

source/common/router/rds_impl.cc

Can you add a small comment here on why RELEASE_ASSERT (basically just hard checking that someone doesn't add a random "routes" entry)

mattklein123 · 2018-03-20T01:24:28Z

source/common/router/rds_impl.cc

I'm still a little bit unclear on why the dance in here is required. Can you add some comments?

The code is paranoid about static route objects going away (they are shared_ptrs that we don't own, claims of static permanence or not). So this cleans out expired weak_ptrs simultaneously with accumulating the shared_ptrs for the caller. I will add comments.

mattklein123 · 2018-03-20T01:25:17Z

source/common/router/rds_impl.cc

Surprised it doesn't just cast correctly when you return config_dump? Oh well.

Yeah this was a wtf moment for me, this is what broke the Mac build the first time around. I tried so hard to resist the temptation, but I just went down a deep rabbit hole on why it's not compiling and have had no luck pinning down an answer yet. It really seems like clang or libc++ is misbehaving though.

Don't worry about it. Was just curious.

mattklein123 · 2018-03-20T01:26:47Z

source/server/http/config_tracker_impl.cc

Still need to switch this to ASSERT

mattklein123 · 2018-03-20T01:27:27Z

source/server/http/config_tracker_impl.h

nit: period end of sentence. Same elsewhere.

htuch

overall, I like where this is heading, thanks for taking this on. I still would like to ultimately have the final dump be closer to the ADS delivered config (i.e. replayable by a management server), but this gets us most of the way there, we can iterate on the final dump format later once we have this merged.

htuch · 2018-03-19T22:21:29Z

include/envoy/server/config_tracker.h

Nit: "is also stored with"

htuch · 2018-03-21T02:40:12Z

include/envoy/server/config_tracker.h

I would actually drop the above two points, they confused me more as a reader than I think they helped. I would replace it with just "This has the advantage of simplifying callback lifetime and ownership management."

htuch · 2018-03-21T02:40:53Z

include/envoy/server/config_tracker.h

I think you can also drop the last sentence here, most Envoy developers will know this to be the case.

htuch · 2018-03-21T02:43:22Z

include/envoy/server/config_tracker.h

Envoy style is just {} instead of default. I know there are valid arguments for default, but consistency is a powerful argument as well. If you feel strongly about doing default, maybe change it everywhere in a distinct PR.

Nah, don't feel strongly, this isn't one of the rare cases where it makes a difference. It should be in STYLE.md though, I'll make a note to add it.

htuch · 2018-03-21T02:45:38Z

include/envoy/server/config_tracker.h

const std::string& key

htuch · 2018-03-21T02:47:00Z

include/envoy/server/config_tracker.h

Nit: prefer for consistency to stick to Envoy typedef std:::unique_ptr<EntryOwner> EntryOwnerPtr. Consistency is a strong argument, if we want to change how this is done in Envoy, best to do it everywhere in a separate PR.

as with default above, sure & I will add a discouragement of using in STYLE.md

htuch · 2018-03-21T03:00:34Z

source/common/router/rds_impl.cc

Do we just keep accumulating here if we don't do a GC sweep in getStaticRouteConfigProviders() ?

Yup. I didn't stress too much because A. these are static and there should be a fixed number and B. I was planning on separately refactoring so the weak_ptr backflips are unnecessary

Yeah agreed. Thanks for the comments in the other function, much more clear what is going on here. This seems fine to me given the use case.

htuch · 2018-03-21T03:01:33Z

source/common/router/rds_impl.cc

This should probably be MergeFrom.

Can you clarify? I can't tell how to use MergeFrom here from the PB docs.

Something like static_configs->Add()->MergeFrom(provider->configAsProto()) I think is how it is typically done IIRC.

htuch · 2018-03-21T03:02:05Z

source/common/router/rds_impl.cc

This should probably be MergeFrom.

htuch

LGTM modulo comments and build fixes.

htuch · 2018-03-22T22:41:19Z

source/common/router/rds_impl.cc

I think this falls into the ASSERT category, it's an internal programming invariant rather than a statement about the runtime system. RELEASE_ASSERT is largely used for unrecoverable environmental issues such as OOM or some unexpected syscall behavior that should "never happen".

@htuch FWIW I recommended using RELEASE_ASSERT here because it's theoretically possible for a plugin to register a config tracker entry with the name "routes" which would be pretty confusing. This would at least crash in all builds. I don't feel very strongly either way.

Fair enough, I would buy the argument that a plugin is an external environment factor that we can't control for as an internal invariant.

mattklein123 · 2018-03-23T00:05:13Z

source/server/http/admin.cc

Sorry I didn't notice this till just now, but per the docs this should only respond to a POST. Can we fix that? Since we are probably going to have more POST methods in the future (cc @jmarantz) it might be worth it to build this into the registration system somehow, but up to you.

+1 on making mutations respond only to POSTs. But is this method a mutation? It doesn't look like it, based on that it's a const method and it looks like it's not mutating config_tracker_. Which docs are you talking about? I couldn't find a reference to POST in the linked data-frame API PR.

Note in admin.h:

struct UrlHandler { const std::string prefix_; const std::string help_text_; const HandlerCb handler_; const bool removable_; const bool mutates_server_state_; }

This last 'false' in the initializer for /config_dump should not mutate server state, and therefore should be fine as a GET. That bool can be used to check for POST. Currently it is used to determine whether to render the HTML table entry as a simple a-tag link or as a button with a form that POSTs.

Oops sorry I was crossing streams with #2837. This comment applies there. :)

ok following up there.

jmarantz · 2018-03-23T00:17:11Z

source/server/http/admin.cc

+1 on making mutations respond only to POSTs. But is this method a mutation? It doesn't look like it, based on that it's a const method and it looks like it's not mutating config_tracker_. Which docs are you talking about? I couldn't find a reference to POST in the linked data-frame API PR.

Note in admin.h:

struct UrlHandler { const std::string prefix_; const std::string help_text_; const HandlerCb handler_; const bool removable_; const bool mutates_server_state_; }

This last 'false' in the initializer for /config_dump should not mutate server state, and therefore should be fine as a GET. That bool can be used to check for POST. Currently it is used to determine whether to render the HTML table entry as a simple a-tag link or as a button with a form that POSTs.

jmarantz · 2018-03-23T00:19:54Z

source/server/http/admin.cc

nit combine these two lines:
ProtobufTypes::MessagePtr message = key_callback_pair.second();

jsedgwick · 2018-03-23T20:56:47Z

PTAL

mattklein123 · 2018-03-23T21:05:17Z

@jsedgwick build is still broken.

jsedgwick · 2018-03-24T02:01:49Z

@mattklein123 ah, variable that is only referenced in an ASSERT, didn't built -c opt locally... frustrating...

Should be ok now. PTAL

jsedgwick · 2018-03-24T17:56:29Z

BTW I think our ASSERT should be something like do { (void)sizeof(CONDITION); } while(0) when disabled, instead of just making CONDITION disappear.

htuch · 2018-03-25T12:21:05Z

@jsedgwick yeah, that ASSERT implementation would be a nice improvement, maybe either file a PR or issue for it?

htuch

LGTM, just one last thing.

htuch · 2018-03-25T12:38:54Z

source/server/http/config_tracker_impl.h

Why is this only a weak_ptr? The map itself doesn't need to be weak does it? Style guide prefers simple ownership semantics where possible https://google.github.io/styleguide/cppguide.html#Ownership_and_Smart_Pointers.

Here's my strawman suggestion. Can we just assume ConfigTracker is going to outlive all EntryOwners? It should be effectively a singleton across the server, I think.

Your second question answers your first - I wanted either to be able to outlive the other but for the map belong to ConfigTracker.

You're totally right with your strawman but as I intimated a few times in this PR I was planning on reusing ConfigTracker in situations where we couldn't guarantee it will effectively be a singleton.

Side note: +1 on simple ownership semantics, but that's exactly why I like to hide more complicated ownership situations behind simpler interfaces when they come up, as in this PR.

In general my preference is to keep it simple unless the more complicated code is actually needed right now, but in the interest of moving this along I don't feel that strongly about it. Will defer to @htuch on it.

Either way, this PR needs a master merge now.

@jsedgwick Ack. Looks like https://github.com/envoyproxy/envoy/pull/2771/files#diff-a112a0982616fac9024fc1c8e42f0661R175 is one such place, but the map is strongly owned there. If that's the only one, I'd say just change to shared_ptr for now and leave a TODO, that way we can see how it plays out.

jsedgwick · 2018-03-25T14:02:54Z

@htuch I'll file a PR for ASSERT().
re: your last comment, I'm going to leave it as is per my reply above

jsedgwick · 2018-03-25T19:31:29Z

This is an implementation detail. It's not an exposed API. Let's merge.

On Sun, Mar 25, 2018 at 12:21 PM Matt Klein ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In source/server/http/config_tracker_impl.h <#2771 (comment)>: > +namespace Server { + +/** + * Implementation of ConfigTracker. + */ +class ConfigTrackerImpl : public ConfigTracker { +public: + EntryOwnerPtr add(const std::string& key, Cb cb) override; + const CbsMap& getCallbacksMap() const override; + +private: + std::shared_ptr<CbsMap> map_{std::make_shared<CbsMap>()}; + + class EntryOwnerImpl : public ConfigTracker::EntryOwner { + public: + EntryOwnerImpl(std::weak_ptr<CbsMap> map_weak, std::string key); In general my preference is to keep it simple unless the more complicated code is actually needed right now, but in the interest of moving this along I don't feel that strongly about it. Will defer to @htuch <https://github.com/htuch> on it. Either way, this PR needs a master merge now. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2771 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAc4Iv3Km86rGT7KTZxZneM6zvmd_gT0ks5th8P-gaJpZM4Sk2kM> .

-- *James Sedgwick* Software Engineer, Networking Infrastructure 973.738.0737 <+19737380737> [image: Lyft] <http://www.lyft.com/>

jsedgwick · 2018-03-26T20:58:02Z

@htuch changed, PTAL

mattklein123 · 2018-03-27T00:12:50Z

@jsedgwick you need to merge master. Thanks!

Signed-off-by: James Sedgwick <jsedgwick@lyft.com>

htuch

Awesome, thanks.

jsedgwick · 2018-03-27T03:10:30Z

rebased on master

@htuch

…nt (#532) See envoyproxy/envoy#2771 for context I will add documentation once the interfaces and placement of these things is settled. Current location (new admin package) was agreed upon as a good start with @htuch but i don't feel strongly about that or naming. Signed-off-by: James Sedgwick <jsedgwick@lyft.com>

htuch assigned junr03 and htuch Mar 9, 2018

jsedgwick mentioned this pull request Mar 9, 2018

[envoy/admin] Scaffolding for structured admin and config_dump endpoint envoyproxy/data-plane-api#532

Merged

mattklein123 reviewed Mar 10, 2018

View reviewed changes

jsedgwick mentioned this pull request Mar 13, 2018

bazel: better support for temporarily using a local dependency repository #2781

Merged

mattklein123 reviewed Mar 20, 2018

View reviewed changes

htuch suggested changes Mar 21, 2018

View reviewed changes

htuch reviewed Mar 22, 2018

View reviewed changes

mattklein123 reviewed Mar 23, 2018

View reviewed changes

jmarantz reviewed Mar 23, 2018

View reviewed changes

mattklein123 mentioned this pull request Mar 23, 2018

admin: add /runtime_modify endpoint and make runtime work without fs backing #2837

Merged

htuch reviewed Mar 25, 2018

View reviewed changes

James Sedgwick added 18 commits March 26, 2018 18:54

structured admin

716beb0

Signed-off-by: James Sedgwick <jsedgwick@lyft.com>

rebase on dpapi

dff9375

Signed-off-by: James Sedgwick <jsedgwick@lyft.com>

remove /routes handler

bd30807

Signed-off-by: James Sedgwick <jsedgwick@lyft.com>

address comments pt. 1

8d16a46

Signed-off-by: James Sedgwick <jsedgwick@lyft.com>

address comments pt2

53c9a55

Signed-off-by: James Sedgwick <jsedgwick@lyft.com>

fix tests + format

c903ef8

Signed-off-by: James Sedgwick <jsedgwick@lyft.com>

docs

8244e30

Signed-off-by: James Sedgwick <jsedgwick@lyft.com>

remove RdsRouteConfigProvider

dc2d26a

Signed-off-by: James Sedgwick <jsedgwick@lyft.com>

missed a spot

60db1d1

Signed-off-by: James Sedgwick <jsedgwick@lyft.com>

fix build

9665ebc

Signed-off-by: James Sedgwick <jsedgwick@lyft.com>

format

0ce7460

Signed-off-by: James Sedgwick <jsedgwick@lyft.com>

fix mac build

2e7a454

Signed-off-by: James Sedgwick <jsedgwick@lyft.com>

address more comments

dfc08fd

Signed-off-by: James Sedgwick <jsedgwick@lyft.com>

change a signature

da1ea92

Signed-off-by: James Sedgwick <jsedgwick@lyft.com>

Harvey's comments

98032b3

Signed-off-by: James Sedgwick <jsedgwick@lyft.com>

address comments

a05c51f

Signed-off-by: James Sedgwick <jsedgwick@lyft.com>

fix opt build

a8a23aa

Signed-off-by: James Sedgwick <jsedgwick@lyft.com>

changed shared_ptr to weak_ptr

9074dbb

Signed-off-by: James Sedgwick <jsedgwick@lyft.com>

htuch approved these changes Mar 27, 2018

View reviewed changes

htuch merged commit 2429797 into envoyproxy:master Mar 27, 2018

Conversation

jsedgwick commented Mar 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

htuch commented Mar 9, 2018

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!

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!

jsedgwick commented Mar 11, 2018

Uh oh!

jsedgwick commented Mar 16, 2018

Uh oh!

mattklein123 commented Mar 16, 2018

Uh oh!

mattklein123 commented Mar 17, 2018

Uh oh!

jsedgwick commented Mar 18, 2018 via email

Uh oh!

mattklein123 commented Mar 19, 2018

Uh oh!

jsedgwick commented Mar 19, 2018

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!

jsedgwick commented Mar 9, 2018 •

edited

Loading