Skip to content

build: Moving more downstream code into the listener extension#24495

Merged
alyssawilk merged 3 commits intoenvoyproxy:mainfrom
alyssawilk:more_followup
Dec 15, 2022
Merged

build: Moving more downstream code into the listener extension#24495
alyssawilk merged 3 commits intoenvoyproxy:mainfrom
alyssawilk:more_followup

Conversation

@alyssawilk
Copy link
Copy Markdown
Contributor

@alyssawilk alyssawilk commented Dec 12, 2022 •
edited
Loading

Risk Level: low
Testing: n/a
Docs Changes: n/a
Release Notes: n/a

part of envoyproxy/envoy-mobile#2711

@alyssawilk
build: Moving more downstream code out of Envoy Mobile
2a66c97 
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
@repokitteh-read-only
Copy link
Copy Markdown

repokitteh-read-only Bot commented Dec 12, 2022

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #24495 was opened by alyssawilk.

see: more, trace.

@alyssawilk
Merge branch 'main' into more_followup
f85f2b0 
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
@alyssawilk
fix contrib
18a9bbc 
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
@alyssawilk alyssawilk changed the title build: Moving more downstream code out of Envoy Mobile build: Moving more downstream code into the listener extension Dec 13, 2022
@alyssawilk alyssawilk marked this pull request as ready for review December 13, 2022 18:06
soulxu
soulxu reviewed Dec 14, 2022
View reviewed changes
Comment thread source/server/server.cc
if (factory) {
return factory->createConnectionHandler(dispatcher, absl::nullopt);
}
ENVOY_LOG_MISC(debug, "Unable to find envoy.connection_handler.default factory");
Copy link
Copy Markdown
Member

@soulxu soulxu Dec 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be an assertion? since it should never happen? as I understand, we won't make this configurable.

Copy link
Copy Markdown
Contributor Author

@alyssawilk alyssawilk Dec 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should never happen in upstream Envoy. it can in Envoy mobile if you misconfigure things.

Copy link
Copy Markdown
Contributor

@snowp snowp Dec 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would that be due to failing to register the factory? Or is there some config that drives this?

In general EM does crash on missing registration, though I guess just logging might be better?

Copy link
Copy Markdown
Contributor

@Augustyniak Augustyniak Jan 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is hit in Envoy Mobile which makes mobile application crash after we call addDirectResponse on TestEngineBuilder (as it adds a listener) (iOS only API). Not sure whether it crashes in any other cases but in general this method always returns nullptr.

soulxu
soulxu reviewed Dec 14, 2022
View reviewed changes
Comment thread source/server/worker_impl.cc
return factory->createConnectionHandler(dispatcher, index);
}
ENVOY_LOG_MISC(debug, "Unable to find envoy.connection_handler.default factory");
return nullptr;
Copy link
Copy Markdown
Member

@soulxu soulxu Dec 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

snowp
snowp approved these changes Dec 15, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@snowp snowp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code move lgtm

@alyssawilk alyssawilk merged commit d78e0c8 into envoyproxy:main Dec 15, 2022
alyssawilk added a commit to alyssawilk/envoy that referenced this pull request Dec 15, 2022
@alyssawilk
Revert "build: Moving more downstream code into the listener extension (
fb8ee95 
envoyproxy#24495)"

This reverts commit d78e0c8.
@alyssawilk alyssawilk mentioned this pull request Dec 15, 2022
Merged
alyssawilk added a commit that referenced this pull request Dec 15, 2022
@alyssawilk
Revert "build: Moving more downstream code into the listener extensio… (
7d26203 
#24554)

Revert "build: Moving more downstream code into the listener extension (#24495)"

This reverts commit d78e0c8.

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
@alyssawilk alyssawilk mentioned this pull request Dec 15, 2022
Merged
alyssawilk added a commit that referenced this pull request Jan 3, 2023
@alyssawilk
build: Moving more downstream code into the listener extension #24495
2705949 
Risk Level: low
Testing: n/a
Docs Changes: n/a
Release Notes: n/a

part of envoyproxy/envoy-mobile#2711

#24495 now with merge fix

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
@Augustyniak Augustyniak mentioned this pull request Jan 4, 2023
Merged
Augustyniak added a commit that referenced this pull request Jan 4, 2023
@Augustyniak
core: add missing extension registration (#24750)
d2fe5cc 
Commit Message: A follow up fixes to #24495. Without these fixes application crashes after `addDirectResponse` call on `TestEngineBuilder` (iOS only api) as the `handler_` in https://github.com/envoyproxy/envoy/blob/93ea91f42ef749a4560a4596174402a5aad8ee65/source/server/worker_impl.cc#L61 is `nullptr`.
Additional Description: The question is whether without the added registration calls application may crash even without the `addDirectResponse` call. cc @alyssawilk. Asking since Lyft has pushed #24495 to prod pipeline already and the question is whether we need to cherry-pick the fix from this PR now.
Risk Level: Low
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional [API Considerations](https://github.com/envoyproxy/envoy/blob/main/api/review_checklist.md):]

Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
ohadvano pushed a commit to ohadvano/envoy that referenced this pull request Jan 5, 2023
@alyssawilk
build: Moving more downstream code into the listener extension envoyp…
404a87e 
…roxy#24495

Risk Level: low
Testing: n/a
Docs Changes: n/a
Release Notes: n/a

part of envoyproxy/envoy-mobile#2711

envoyproxy#24495 now with merge fix

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Ohad Vano <ohadvano@microsoft.com>
ohadvano pushed a commit to ohadvano/envoy that referenced this pull request Jan 5, 2023
@Augustyniak
core: add missing extension registration (envoyproxy#24750)
888b74a 
Commit Message: A follow up fixes to envoyproxy#24495. Without these fixes application crashes after `addDirectResponse` call on `TestEngineBuilder` (iOS only api) as the `handler_` in https://github.com/envoyproxy/envoy/blob/93ea91f42ef749a4560a4596174402a5aad8ee65/source/server/worker_impl.cc#L61 is `nullptr`.
Additional Description: The question is whether without the added registration calls application may crash even without the `addDirectResponse` call. cc @alyssawilk. Asking since Lyft has pushed envoyproxy#24495 to prod pipeline already and the question is whether we need to cherry-pick the fix from this PR now.
Risk Level: Low
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional [API Considerations](https://github.com/envoyproxy/envoy/blob/main/api/review_checklist.md):]

Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
Signed-off-by: Ohad Vano <ohadvano@microsoft.com>
VishalDamgude pushed a commit to freshworks-oss/envoy that referenced this pull request Feb 1, 2023
@alyssawilk @VishalDamgude
build: Moving more downstream code into the listener extension envoyp…
eeb1657 
…roxy#24495

Risk Level: low
Testing: n/a
Docs Changes: n/a
Release Notes: n/a

part of envoyproxy/envoy-mobile#2711

envoyproxy#24495 now with merge fix

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: VishalDamgude <vishal.damgude@freshworks.com>
VishalDamgude pushed a commit to freshworks-oss/envoy that referenced this pull request Feb 1, 2023
@Augustyniak @VishalDamgude
core: add missing extension registration (envoyproxy#24750)
0d8f985 
Commit Message: A follow up fixes to envoyproxy#24495. Without these fixes application crashes after `addDirectResponse` call on `TestEngineBuilder` (iOS only api) as the `handler_` in https://github.com/envoyproxy/envoy/blob/93ea91f42ef749a4560a4596174402a5aad8ee65/source/server/worker_impl.cc#L61 is `nullptr`.
Additional Description: The question is whether without the added registration calls application may crash even without the `addDirectResponse` call. cc @alyssawilk. Asking since Lyft has pushed envoyproxy#24495 to prod pipeline already and the question is whether we need to cherry-pick the fix from this PR now.
Risk Level: Low
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional [API Considerations](https://github.com/envoyproxy/envoy/blob/main/api/review_checklist.md):]

Signed-off-by: Rafal Augustyniak <raugustyniak@lyft.com>
Signed-off-by: VishalDamgude <vishal.damgude@freshworks.com>
@alyssawilk alyssawilk deleted the more_followup branch April 5, 2023 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ggreenway ggreenway Awaiting requested review from ggreenway ggreenway is a code owner

@kyessenov kyessenov Awaiting requested review from kyessenov kyessenov is a code owner

@adisuissa adisuissa Awaiting requested review from adisuissa adisuissa is a code owner

3 more reviewers

@soulxu soulxu soulxu left review comments

@Augustyniak Augustyniak Augustyniak left review comments

@snowp snowp snowp approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@snowp snowp

@mattklein123 mattklein123

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@alyssawilk @soulxu @snowp @Augustyniak @mattklein123