Skip to content

swift: stop using loopback for direct responses#24402

Merged
alyssawilk merged 1 commit intoenvoyproxy:mainfrom
alyssawilk:remove_loopback_requirement
Dec 8, 2022
Merged

swift: stop using loopback for direct responses#24402
alyssawilk merged 1 commit intoenvoyproxy:mainfrom
alyssawilk:remove_loopback_requirement

Conversation

@alyssawilk
Copy link
Copy Markdown
Contributor

@alyssawilk alyssawilk commented Dec 6, 2022

This changes swift's direct responses from being proxied upstream and handled by the "fake" (real) listener, to being handled locally, and bypassing the "do not forward local replies" logic in the onLocalReply classes

Commit Message: na/a
Additional Description: n/a
Risk Level: medium (mobile only)
Testing: existing tests
part of #24428

@repokitteh-read-only
Copy link
Copy Markdown

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: #24402 was opened by alyssawilk.

see: more, trace.

@repokitteh-read-only
Copy link
Copy Markdown

CC @envoyproxy/coverage-shephards: FYI only for changes made to (test/per_file_coverage.sh).
envoyproxy/coverage-shephards assignee is @RyanTheOptimist
CC @envoyproxy/mobile-maintainers: FYI only for changes made to (mobile/).
envoyproxy/mobile-maintainers assignee is @abeyad

🐱

Caused by: #24402 was opened by alyssawilk.

see: more, trace.

@alyssawilk alyssawilk force-pushed the remove_loopback_requirement branch 3 times, most recently from 6227056 to 0ef0598 Compare December 7, 2022 20:45
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
@alyssawilk alyssawilk force-pushed the remove_loopback_requirement branch from 0ef0598 to 4ff414b Compare December 7, 2022 22:04
@alyssawilk alyssawilk changed the title Remove loopback requirement swift: stop using loopback for direct responses Dec 7, 2022
@alyssawilk alyssawilk marked this pull request as ready for review December 7, 2022 22:06
@alyssawilk
Copy link
Copy Markdown
Contributor Author

cc @jpsim

- name: remote_service
domains: ["*"]
routes:
#{fake_remote_responses}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

oh, I thought that we would be able to remove a bigger portion of this fake listener as part of this step. What's preventing us from doing that (apologies if you have explained it in the community sync meeting)?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I was going to do that as a follow-up after I removed listener support so we can make sure it doesn't break Lyft tests first :-)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

so the plan is land this, then #24363 (working on a c-opt for legacy tests like the kotlin tests) then remove the listener entirely

@alyssawilk alyssawilk merged commit 6040d0b into envoyproxy:main Dec 8, 2022
@alyssawilk alyssawilk deleted the remove_loopback_requirement branch April 5, 2023 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants