Update data-plane-api (core and filter packages) by kyessenov · Pull Request #2495 · envoyproxy/envoy

kyessenov · 2018-01-31T21:27:25Z

title: update data-plane-api

Description:
Update to changes in data-plane-api

Risk Level: Low

Testing:
bazel test.

Docs Changes:
envoyproxy/data-plane-api#452

Release Notes:

Signed-off-by: Kuat Yessenov <kuat@google.com>

mattklein123 · 2018-02-02T22:40:04Z

 GrpcMuxImpl::~GrpcMuxImpl() {
+  // Hack to force linking of the service: https://github.com/google/protobuf/issues/4221
+  envoy::service::discovery::v2::AdsDummy dummy;
+  envoy::service::ratelimit::v2::RateLimitRequest rls_dummy;


We shouldn't need rls_dummy here, right? I don't think it makes sense to include rls.pb.h here. And how come this wasn't an issue previously in general?

I've discovered this by running envoy-static with ADS. It is missing ADS descriptor without this change.

OK, can you remove rls_dummy from here and the rls.pb.h include? It should not be needed in this file.

i am confused here. Why is AdsDummy needed here now? It was able to link properly earlier and why the new package need this? I ran in to the same issue with latest source, that it is not able to find the 'MethodDescriptor' for ADS and bazel clean seems to have resolved that. I may be wrong but Can you double check on this once please?

it links correctly but if you try to call ADS methods, the reflection lookup fails since proto descriptor is missing. there's another PR that does a similar fix, so it is happening consistently. adding a dummy forces linking the proto descriptor.

ok.got it thanks.

mattklein123 · 2018-02-02T22:43:00Z

 // services as well as types that are referenced in Any messages. IF THIS TEST BREAKS YOU
 // HAVE DONE SOMETHING BAD. Consult with the larger dev team on how to handle.
 TEST(ProtoDescriptorTest, BackCompat) {
-  // Hack to force linking of the service: https://github.com/google/protobuf/issues/4221


I'm confused. Without linking to grpc_mux stuff, how does this work now?

You are right, I need it in two places :(

Signed-off-by: Kuat Yessenov <kuat@google.com>

mattklein123 · 2018-02-03T02:07:25Z

 }

 GrpcMuxImpl::~GrpcMuxImpl() {
+  // Hack to force linking of the service: https://github.com/google/protobuf/issues/4221


Sorry, can you add a TODO here to fix this so we can search for it? Also please add a TODO to my new test which I didn't do. Otherwise LGTM.

added todo.

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov · 2018-02-03T03:03:26Z

I'd like to re-run mac build (flaked out). Is there some button in circle to do that?

mattklein123 · 2018-02-03T21:27:02Z

@kyessenov I just kicked it, but typically we will marge without OSX passing if it looks like a flake.

mattklein123 · 2018-02-03T21:34:46Z

@kyessenov sorry you lost the merge race. Can you merge master?

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov · 2018-02-04T00:24:33Z

Merged and fixed.

ramaraochavali · 2018-02-04T04:05:01Z

+GrpcMuxImpl::GrpcMuxImpl(const envoy::api::v2::core::Node& node, Grpc::AsyncClientPtr async_client,
                         Event::Dispatcher& dispatcher,
                         const Protobuf::MethodDescriptor& service_method)
    : node_(node), async_client_(std::move(async_client)), service_method_(service_method) {


should we add some kind of an ASSERT here on service_method_? with out that if the MethodByName is not found in generic pool, it just segfaults.

Merged to keep things moving, tracking this concern at #2528.

htuch

Agree with @ramaraochavali and @mattklein123 that we should make the proto descriptor dynamic lookups more bullet proof, let's merge this one to avoid further drift and we can iterate.

Signed-off-by: Kuat Yessenov <kuat@google.com>

Description: Update the documentation of `find_class` method as it seems to accept `/` and `.` and using `/` should be less confusing as this is what `env->findClass` expects. Risk Level: None Testing: Manual confirmed that the current implementing of tagging works and it does uses `/` as opposed to `.`. Docs Changes: N/A Release Notes: N/A Signed-off-by: Rafał Augustyniak <Augustyniak@users.noreply.github.com> Signed-off-by: JP Simard <jp@jpsim.com>

kyessenov added 5 commits January 31, 2018 13:02

adapt to data-plane-api

6455253

Signed-off-by: Kuat Yessenov <kuat@google.com>

Merge remote-tracking branch 'upstream/master' into control-plane

8b1dbb1

fix format

5306835

Signed-off-by: Kuat Yessenov <kuat@google.com>

fix ads dummy

eabe924

Signed-off-by: Kuat Yessenov <kuat@google.com>

put dummy once

7433b93

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov mentioned this pull request Feb 1, 2018

Upgrade data-plane-api envoyproxy/go-control-plane#39

Merged

alyssawilk assigned htuch Feb 1, 2018

kyessenov added 6 commits February 1, 2018 16:16

merge fix

f916a1e

Signed-off-by: Kuat Yessenov <kuat@google.com>

fix format

1cbd7cd

Signed-off-by: Kuat Yessenov <kuat@google.com>

merge fix

389f325

Signed-off-by: Kuat Yessenov <kuat@google.com>

fix format

f399ef3

Signed-off-by: Kuat Yessenov <kuat@google.com>

Merge remote-tracking branch 'upstream/master' into control-plane

d0e7f5d

update data-plane-api

f2fed0c

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov changed the title ~~Update data-plane-api~~ Update data-plane-api (core and filter packages) Feb 2, 2018

kyessenov added 2 commits February 2, 2018 14:34

missing dependency

92917e5

Signed-off-by: Kuat Yessenov <kuat@google.com>

missing dependency

810d62a

Signed-off-by: Kuat Yessenov <kuat@google.com>

mattklein123 reviewed Feb 2, 2018

View reviewed changes

kyessenov added 2 commits February 2, 2018 15:01

update data-plane-api

6b0b06c

Signed-off-by: Kuat Yessenov <kuat@google.com>

remove rls

edcbde6

Signed-off-by: Kuat Yessenov <kuat@google.com>

mattklein123 previously approved these changes Feb 3, 2018

View reviewed changes

mattklein123 reviewed Feb 3, 2018

View reviewed changes

add todo

b750ef3

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov dismissed mattklein123’s stale review via b750ef3 February 3, 2018 02:17

kyessenov mentioned this pull request Feb 3, 2018

ads: fix MethodDescriptor resolution for ADS #2522

Closed

mattklein123 previously approved these changes Feb 3, 2018

View reviewed changes

merge fix

0b53ac6

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov dismissed mattklein123’s stale review via 0b53ac6 February 3, 2018 23:44

ramaraochavali reviewed Feb 4, 2018

View reviewed changes

htuch approved these changes Feb 4, 2018

View reviewed changes

htuch merged commit c932e50 into envoyproxy:master Feb 4, 2018

This was referenced Feb 5, 2018

Support for external authorization grpc service. #2415

Merged

http: adding 100-Continue support to Envoy #2497

Merged

Shikugawa pushed a commit to Shikugawa/envoy that referenced this pull request Mar 28, 2020

fix bash script (envoyproxy#2495)

e40fdc3

Signed-off-by: Kuat Yessenov <kuat@google.com>

Conversation

kyessenov commented Jan 31, 2018

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!

ramaraochavali Feb 4, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kyessenov commented Feb 3, 2018

Uh oh!

mattklein123 commented Feb 3, 2018

Uh oh!

mattklein123 commented Feb 3, 2018

Uh oh!

kyessenov commented Feb 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ramaraochavali Feb 4, 2018 •

edited

Loading