Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

factory context: refactored factory context #31189

Merged
merged 17 commits into from
Dec 8, 2023

Conversation

wbpcode
Copy link
Member

@wbpcode wbpcode commented Dec 6, 2023

Commit Message: factory context: refactored factory context
Additional Description:

  1. removed FactoryContextBase
  2. FactoryContext won't inherit from CommonFactoryContext.

What should I say? Cheers? 😄

Risk Level: mid.
Testing: n/a.
Docs Changes: n/a.
Release Notes: n/a.
Platform Specific Features: n/a.

Signed-off-by: wbpcode <wbphub@live.com>
@wbpcode wbpcode marked this pull request as draft December 6, 2023 04:59
@wbpcode wbpcode marked this pull request as ready for review December 6, 2023 10:35
Signed-off-by: wbpcode <wbphub@live.com>
@wbpcode wbpcode requested a review from abeyad as a code owner December 6, 2023 11:05
Signed-off-by: wbpcode <wbphub@live.com>
Signed-off-by: wbpcode <wbphub@live.com>
Signed-off-by: wbpcode <wbphub@live.com>
Signed-off-by: wbpcode <wbphub@live.com>
Signed-off-by: wbpcode <wbphub@live.com>
@alyssawilk
Copy link
Contributor

/wait on main merge (sorry!)

Signed-off-by: wbpcode <wbphub@live.com>
Signed-off-by: wbpcode <wbphub@live.com>
Signed-off-by: wbpcode <wbphub@live.com>
@wbpcode wbpcode requested a review from soulxu as a code owner December 7, 2023 02:07
Signed-off-by: wbpcode <wbphub@live.com>
@wbpcode wbpcode requested a review from htuch as a code owner December 7, 2023 02:38
Signed-off-by: wbpcode <wbphub@live.com>
@wbpcode wbpcode requested a review from lizan as a code owner December 7, 2023 04:56
Signed-off-by: wbpcode <wbphub@live.com>
@wbpcode wbpcode requested a review from jmarantz as a code owner December 7, 2023 07:23
Signed-off-by: wbpcode <wbphub@live.com>
@wbpcode wbpcode requested a review from zuercher as a code owner December 7, 2023 10:51
Signed-off-by: wbpcode <wbphub@live.com>
@wbpcode
Copy link
Member Author

wbpcode commented Dec 7, 2023

Although we used multiple PRs to do some pre-cleanup, this PR still such big. orz.

@wbpcode
Copy link
Member Author

wbpcode commented Dec 7, 2023

🍻 finally...

Copy link
Contributor

@alyssawilk alyssawilk left a comment

Choose a reason for hiding this comment

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

this change is AMAZING

I'd bump risk to high just because it's a pretty big refactor. Feel free to merge as is and typedef in a follow-up someone else can review as I'm sure merge conflicts will be a lot.

@@ -23,7 +23,7 @@ class BodyFormatter {
content_type_(Http::Headers::get().ContentTypeValues.Text) {}

BodyFormatter(const envoy::config::core::v3::SubstitutionFormatString& config,
Server::GenericFactoryContextImpl context)
Server::Configuration::GenericFactoryContext& context)
Copy link
Contributor

Choose a reason for hiding this comment

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

optional: think it's worth doing a typedef for this one just to avoid the churn here and for folks downstream?

Copy link
Member Author

@wbpcode wbpcode Dec 8, 2023

Choose a reason for hiding this comment

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

Sorry, what typedef?

The GenericFactoryContextImpl was just introduced in the #31072 (3 days ago), So, I think the change from GenericFactoryContextImpl to Server::Configuration::GenericFactoryContext&won't effect anyone.
(I used GenericFactoryContextImpl in the #31072 because I hope the FactoryContext could be used to construct a GenericFactoryContextImpl implicitly, then we can reduce unnecessary code change in lots of postions. But now, after this patch, FactoryContext inherit from GenericFactoryContext, so GenericFactoryContextImpl is unnecessary now.)

@@ -17,7 +17,7 @@ namespace Common {
namespace DynamicForwardProxy {

absl::StatusOr<std::shared_ptr<DnsCacheImpl>> DnsCacheImpl::createDnsCacheImpl(
const Server::Configuration::GenericFactoryContext& context,
Server::Configuration::GenericFactoryContext& context,
Copy link
Contributor

Choose a reason for hiding this comment

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

why the change to non-const?

Copy link
Member Author

@wbpcode wbpcode Dec 8, 2023

Choose a reason for hiding this comment

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

Because the scope() method and initManager() method are non-const in the FactoryContext.

An I hope FactoryContext could inherit from GenericFactoryContext but doesn't change the method signatures in the FactoryContext. So, I finally update the method signatures of GenericFactoryContext (because it's only be used in limited positions now.)

I believe this is safe because almost all context is used in non-const mode.

@wbpcode
Copy link
Member Author

wbpcode commented Dec 8, 2023

Let's me check it last time and then land it.

@wbpcode wbpcode merged commit 887e472 into envoyproxy:main Dec 8, 2023
105 checks passed
@wbpcode wbpcode deleted the dev-refactory-last-factory-context branch December 8, 2023 02:22
eric846 pushed a commit to envoyproxy/nighthawk that referenced this pull request Dec 14, 2023
- change `context.runtime()` to `context.serverFactoryContext().runtime()` in source/server/http_dynamic_delay_filter_config.cc due to envoyproxy/envoy#31189 and implemented PANIC methods in NighthawkServerFactoryContext
- replace `@envoy//source/exe:envoy_common_lib_with_external_headers` with `@envoy//source/exe:all_extensions_lib_with_external_headers` due to envoyproxy/envoy#31109
- replace `@envoy//source/exe:main_common_lib_with_external_headers` with `@envoy//source/exe:main_common_with_all_extensions_lib_with_external_headers` due to a combination of envoyproxy/envoy#30924 and envoyproxy/envoy#31109 causing unit test `UtilityAddressResolutionTest, AddressResolution` failed because `envoy.network.dns_resolver.cares` was not linked in 
- lower `upstream_cx_rx_bytes_total` counter threshold in `test_http_h2` integration test.
- disable oghttp2 in Nighthawk which was enabled by default in Envoy in envoyproxy/envoy@b58d312
- sync files

Signed-off-by: Qin Qin <qqin@google.com>
jbohanon added a commit to solo-io/envoy-gloo that referenced this pull request Mar 1, 2024
soloio-bulldozer bot pushed a commit to solo-io/envoy-gloo that referenced this pull request Mar 12, 2024
* bump envoy-fork, fix extensions

* fix FactoryContext from envoyproxy/envoy#31189

* more build fixes

* fix MockFactoryContext

* use xds pkg for unified matcher instead of udpa

* export the build options

* another ServerFactoryContext

* use typed_config for lambda filter test

* runtime flag...?

* runtime flag...as a string...?

* use nullopt instead of server factory context. why? not sure yet

* put the server factory context back and try a different runtime flag per envoyproxy/envoy#31135

* nuke le asan

* uhh

* use libcurl, patch out virtual call to ServerFactoryContext::clusterManager()

* forgot the patch d'oh

* ls on the out dirs

* undo commenting creds

* add a build dir

* derp

* what the heck, respect my vars bro

* WHY AREN'T WE USING THE RIGHT ENV VARS

* look at me. i am the captain now.

* shhh, it can't see you if you don't move

* well this is nasty but we're getting somewhere

* alt+f4

* asan

* lower jobs oom-killed

* try some more goofiness with the optref

* ci shenanigans

* server factory context as nullopt

* ci nonsense

* getting closer

* cleanup

* remove hard-coding override in initialize()

* bump to 1.29.2

* bump to merged

---------

Co-authored-by: Nathan Fudenberg <nathan.fudenberg@solo.io>
sayboras added a commit to sayboras/proxy that referenced this pull request Mar 23, 2024
Relates: envoyproxy/envoy#30765
Relates: envoyproxy/envoy#31189

Signed-off-by: Tam Mach <tam.mach@cilium.io>
sayboras added a commit to sayboras/proxy that referenced this pull request Mar 23, 2024
Relates: envoyproxy/envoy#30765
Relates: envoyproxy/envoy#31189

Signed-off-by: Tam Mach <tam.mach@cilium.io>
sayboras added a commit to sayboras/proxy that referenced this pull request Mar 25, 2024
Relates: envoyproxy/envoy#30765
Relates: envoyproxy/envoy#31189

Signed-off-by: Tam Mach <tam.mach@cilium.io>
sayboras added a commit to sayboras/proxy that referenced this pull request Apr 2, 2024
Relates: envoyproxy/envoy#30765
Relates: envoyproxy/envoy#31189

Signed-off-by: Tam Mach <tam.mach@cilium.io>
sayboras added a commit to sayboras/proxy that referenced this pull request Apr 5, 2024
Relates: envoyproxy/envoy#30765
Relates: envoyproxy/envoy#31189

Signed-off-by: Tam Mach <tam.mach@cilium.io>
sayboras added a commit to sayboras/proxy that referenced this pull request Apr 6, 2024
Relates: envoyproxy/envoy#30765
Relates: envoyproxy/envoy#31189

Signed-off-by: Tam Mach <tam.mach@cilium.io>
sayboras added a commit to sayboras/proxy that referenced this pull request Apr 16, 2024
Relates: envoyproxy/envoy#30765
Relates: envoyproxy/envoy#31189

Signed-off-by: Tam Mach <tam.mach@cilium.io>
sayboras added a commit to sayboras/proxy that referenced this pull request Apr 16, 2024
Relates: envoyproxy/envoy#30765
Relates: envoyproxy/envoy#31189

Signed-off-by: Tam Mach <tam.mach@cilium.io>
sayboras added a commit to sayboras/proxy that referenced this pull request Apr 16, 2024
Relates: envoyproxy/envoy#30765
Relates: envoyproxy/envoy#31189

Signed-off-by: Tam Mach <tam.mach@cilium.io>
sayboras added a commit to sayboras/proxy that referenced this pull request Apr 17, 2024
Relates: envoyproxy/envoy#30765
Relates: envoyproxy/envoy#31189

Signed-off-by: Tam Mach <tam.mach@cilium.io>
github-merge-queue bot pushed a commit to cilium/proxy that referenced this pull request Apr 17, 2024
Relates: envoyproxy/envoy#30765
Relates: envoyproxy/envoy#31189

Signed-off-by: Tam Mach <tam.mach@cilium.io>
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.

2 participants