Skip to content

ads: fix MethodDescriptor resolution for ADS#2522

Closed
joeyb wants to merge 2 commits intoenvoyproxy:masterfrom
joeyb:fix-ads-descriptor
Closed

ads: fix MethodDescriptor resolution for ADS#2522
joeyb wants to merge 2 commits intoenvoyproxy:masterfrom
joeyb:fix-ads-descriptor

Conversation

@joeyb
Copy link
Copy Markdown

@joeyb joeyb commented Feb 3, 2018

Envoy currently segfaults if ADS is being used because resolution of the envoy.service.discovery.v2.AggregatedDiscoveryService.StreamAggregatedResources method fails. This fix uses the same hack as descriptor_test.cc to force linking so that the method is resolvable.

Risk Level: Low

Testing: Manual

Docs Changes: N/A

@kyessenov

@joeyb joeyb force-pushed the fix-ads-descriptor branch 2 times, most recently from a9652a8 to 1f6d8cc Compare February 3, 2018 18:55
joeyb added 2 commits February 3, 2018 14:09
Envoy currently segfaults if ADS is being used because resolution of the `envoy.service.discovery.v2.AggregatedDiscoveryService.StreamAggregatedResources` method fails. This fix uses the same hack as `descriptor_test.cc` to force linking so that the method is resolvable.

Signed-off-by: Joey Bratton <jbratton@salesforce.com>
Signed-off-by: Joey Bratton <jbratton@salesforce.com>
@joeyb joeyb force-pushed the fix-ads-descriptor branch from fc4e0a9 to 602de76 Compare February 3, 2018 19:09
@kyessenov
Copy link
Copy Markdown
Contributor

Sorry, I hit the same issue, and have another dummy in #2495

@joeyb
Copy link
Copy Markdown
Author

joeyb commented Feb 3, 2018

Ok, cool. I'll close this one in favor of that PR.

@joeyb joeyb closed this Feb 3, 2018
@joeyb joeyb deleted the fix-ads-descriptor branch February 3, 2018 19:20
@mattklein123
Copy link
Copy Markdown
Member

@kyessenov @htuch I'm not thrilled that there is no great way to test for this. I wonder if we should have a RELEASE_ASSERT in main_common which basically does the same thing as the test I wrote?

@htuch
Copy link
Copy Markdown
Member

htuch commented Feb 3, 2018

@mattklein123 yes, we could do that, maybe just factor that code out to somewhere in server. It's a cheap check and will give us consistency.

jpsim pushed a commit that referenced this pull request Nov 28, 2022
Signed-off-by: GitHub Action <noreply@github.com>
Signed-off-by: JP Simard <jp@jpsim.com>
jpsim pushed a commit that referenced this pull request Nov 29, 2022
Signed-off-by: GitHub Action <noreply@github.com>
Signed-off-by: JP Simard <jp@jpsim.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants