cleanup common factory context using by wbpcode · Pull Request #31072 · envoyproxy/envoy

wbpcode · 2023-11-28T01:26:06Z

Commit Message: cleanup common factory context using
Additional Description:

Part of factory context refactoring and clean up. This PR clean up all remaining CommonFactoryContext using.

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

wbpcode · 2023-11-28T01:55:49Z

Note: The target of this patch is not to remove all CommonFactoryContext, but to ensure all position that take CommonFactoryContext as parameter will use ServerFactoryContext as argument rather than using FactoryContext.

Then, we can break the inheritance relationship between CommonFactoryContext and FactoryContext. And in the future, the CommonFactoryContext could be treated as a limited ServerFactoryContext.

Based on above target, this cleanup did different things for different scenarios.

For the CommonFactoryContext that always be used in the listener scope, we will change it to FactoryContext directly. This is safe and have no any side effect. See the request id extension for an example.
For the ComonFactoryContext that always be used in the server scope, we do nothing to it. It's basically OK. See the cluster specifer plugin https://github.com/envoyproxy/envoy/blob/main/envoy/router/cluster_specifier_plugin.h#L38 as an example.
For the ComonFactoryContext that may be used in both listener scope and server scope, I updated code base to always use ServerFactoryContext as the argument. This may bring potential risk because a ServerFactoryContext powered CommonFactoryContext has different scope(), initManager(), etc. with FactoryContext powered CommonFactoryContext. The lucky thing is that we never use these methods in our code base so it's safe to update it directly. This still may effect third-party forks (Although I think the possibility is very low because only the Formatter and CustomResponse will be effected and these extension point rarely use methods like scope(), etc.).

Signed-off-by: wbpcode <wbphub@live.com>

wbpcode · 2023-11-28T03:43:13Z

+/**
+ * Factory context for configuration parsing and initiation. This context contains server factory
+ * context reference and a validation visitor from uncertain context (server, listener, cluster or
+ * others).
+ */
+class ConfigFactoryContext {
+public:
+  virtual ~ConfigFactoryContext() = default;
+
+  /**
+   * @return ServerFactoryContext which lifetime is no shorter than the server and provides
+   *         access to the server's resources.
+   */
+  virtual ServerFactoryContext& getServerFactoryContext() const PURE;
+
+  /**
+   * @return ProtobufMessage::ValidationVisitor& validation visitor for configuration messages.
+   */
+  virtual ProtobufMessage::ValidationVisitor& messageValidationVisitor() const PURE;
+};


Formatter extensions maybe used in both server scope and listener scope. We should use different for message validation visitors in different scopes.

See #31073 for more context.

wbpcode · 2023-11-28T03:44:13Z

-  virtual Http::RequestIDExtensionSharedPtr
-  createExtensionInstance(const Protobuf::Message& config, CommonFactoryContext& context) PURE;
+  virtual Http::RequestIDExtensionSharedPtr createExtensionInstance(const Protobuf::Message& config,
+                                                                    FactoryContext& context) PURE;


This change is complete safe because the request id extension is only used in the listener scope. We change the CommonFactoryContext here to FactoryContext directly.

wbpcode · 2023-11-28T03:57:19Z

-  auto connectivity_manager = Network::ConnectivityManagerFactory{context}.get();
+  auto connectivity_manager =
+      Network::ConnectivityManagerFactory{context.getServerFactoryContext()}.get();


This bring a possible logic change. The messageValidationVistior() of server factory context will be used always by the DnsCacheImpl. This is wrong, see See #31073 for more detail.

I will revise this.

wbpcode · 2023-11-28T04:01:13Z

      auto typed_config = Envoy::Config::Utility::translateAnyToFactoryConfig(
          formatter.typed_config(), context.messageValidationVisitor(), *factory);
-      auto parser = factory->createCommandParserFromProto(*typed_config, context);
+      auto parser =
+          factory->createCommandParserFromProto(*typed_config, context.getServerFactoryContext());


This bring potential logic change to third party forks (with very low possibility). Because scope(), initManager(), etc methods are different in the FactoryContext and ServerFactoryContext.

But lucky we never use it in our current formatter extensions. (metadata and req_without_query formatters didn't use any context and cel formatter use singleton context only.)

wbpcode · 2023-11-28T04:02:19Z


  BodyFormatter(const envoy::config::core::v3::SubstitutionFormatString& config,
-                Server::Configuration::CommonFactoryContext& context)
+                Server::Configuration::FactoryContext& context)


Safe because the BodyFormatter only be used in the listener scope.

sorry shouldn't thinks in the listener scope have a genericfactorycontext with stats and init manager scoped to the listener?

Yeah, generic factory context is enough for this scenario. I use FactoryContext just for convenience. I will update it.

wbpcode · 2023-11-28T04:03:37Z

 std::vector<Value>
 Config::parse(const Protobuf::RepeatedPtrField<FilterStateValueProto>& proto_values,
-              Server::Configuration::CommonFactoryContext& context) const {
+              Server::Configuration::ConfigFactoryContext& context) const {


The dependency to the Formatter result in this change.

Signed-off-by: wbpcode <wbphub@live.com>

alyssawilk · 2023-11-28T18:32:17Z

hm, regarding (3) I thought the plan long term was to have a FactoryContext class which had
serverFactoryContext()
scope()
initManager()

as the latter two are the only things overridden. For (3) I wonder if we should create that class now, and have a simple wrapper converter so folks can still use those? Want to get thoughts before digging in.

alyssawilk · 2023-11-28T18:32:26Z

/retest

alyssawilk · 2023-11-28T22:43:21Z

/wait on CI

wbpcode · 2023-11-29T00:33:24Z

as the latter two are the only things overridden.

In fact, ~~drainDecison()~~ and messageValidationVisitor() also should be overridden. 🤣

For (3) I wonder if we should create that class now, and have a simple wrapper converter so folks can still use those? Want to get thoughts before digging in.

I hesitate now. I cannot image in which case the developers may use socpe() or initManager() in Formatter. But we did change something.

wbpcode · 2023-11-29T00:47:22Z

Ok, let's create a GenericFactoryContext for these scenarios. And if third party forks extended these extension points in the (3), a compile error will be throwed and they know they need update their implementation.

Signed-off-by: wbpcode <wbphub@live.com>

…lean-common-factory-context

Signed-off-by: wbpcode <wbphub@live.com>

alyssawilk

almost there!
/wait

alyssawilk · 2023-11-29T16:01:02Z


  BodyFormatter(const envoy::config::core::v3::SubstitutionFormatString& config,
-                Server::Configuration::CommonFactoryContext& context)
+                Server::Configuration::FactoryContext& context)


sorry shouldn't thinks in the listener scope have a genericfactorycontext with stats and init manager scoped to the listener?

alyssawilk · 2023-11-29T17:53:53Z

+  // TODO(wbpcode): these is a potential bug of message validation. The validation visitor
+  // of server context should not be used here directly. But this is bug is not introduced
+  // by this PR and will be fixed in the future.
+  Server::GenericFactoryContextImpl generic_context(context, context.messageValidationVisitor());


mm, if this may be a problem shouldn't we not switch to server factory context here? I'd think if we wanted to inherit validation we'd need to keep this a generic.

alyssawilk · 2023-11-29T17:54:33Z

  LocalResponsePolicy(const envoy::extensions::http::custom_response::local_response_policy::v3::
                          LocalResponsePolicy& config,
-                      Server::Configuration::CommonFactoryContext& context);
+                      Server::Configuration::ServerFactoryContext& context);


In general passing Generic makes more sense than server in case folks want (properly scoped) stats down the road WDYT?

Actually, in previous implementation, the backend of CommonFactoryContext here will always be a ServerFactoryContext.

As I said in the TODO, this potential bug is not introduced by this patch. The LocalResponsePolicy will be created in the createPolicy method which will always take a ServerFactoryContext.

This patch just wrappered the ServerFactoryContext to a GenericFactoryContext (for Formatter usage) and doesn't change any logic or behavior here.

See:

Extensions::HttpFilters::CustomResponse::PolicySharedPtr LocalResponseFactory::createPolicy(const Protobuf::Message& config, Envoy::Server::Configuration::ServerFactoryContext& context, Stats::StatName) { const auto& local_response_config = MessageUtil::downcastAndValidate<const LocalResponsePolicyProto&>( config, context.messageValidationVisitor()); return std::make_shared<LocalResponsePolicy>(local_response_config, context); }

Although I think we should fix this bug (this is why I left a TODO and #31073), but the fix also a behavior change. I don't want introduce any additional behavior changes (except we have no choice) in this big PR.

But if you still think to fix it here make sense, I also can update it. :)

Signed-off-by: wbpcode <wbphub@live.com>

wbpcode · 2023-11-30T06:05:40Z

        "//source/extensions/clusters/strict_dns:strict_dns_cluster_lib",
        "//source/extensions/load_balancing_policies/cluster_provided:config",
        "//source/extensions/load_balancing_policies/least_request:config",
-        "//source/extensions/load_balancing_policies/maglev:config",
        "//source/extensions/load_balancing_policies/random:config",
-        "//source/extensions/load_balancing_policies/ring_hash:config",
        "//source/extensions/load_balancing_policies/round_robin:config",
-        "//source/extensions/load_balancing_policies/subset:config",


Seems this patch add some more symbols and result in coverage linking failure. I removed some unnecessary extensions here to make the CI green. orz.

Signed-off-by: wbpcode <wbphub@live.com>

wbpcode · 2023-11-30T14:33:03Z

Coverage.... But at least I know how to fix it... let me do it tomorrow.

Signed-off-by: wbpcode <wbphub@live.com>

wbpcode · 2023-12-02T08:45:14Z

finally......

wbpcode requested review from RyanTheOptimist, abeyad, alyssawilk, htuch, kyessenov, mattklein123 and yanavlasov as code owners November 28, 2023 01:26

wbpcode assigned alyssawilk Nov 28, 2023

cleanup all common factory context

67c3c79

Signed-off-by: wbpcode <wbphub@live.com>

wbpcode force-pushed the dev-clean-common-factory-context branch from 4a5af94 to 67c3c79 Compare November 28, 2023 03:10

wbpcode commented Nov 28, 2023

View reviewed changes

fix visitor

00dcd6a

Signed-off-by: wbpcode <wbphub@live.com>

wbpcode requested a review from lizan as a code owner November 28, 2023 04:30

wbpcode added 8 commits November 28, 2023 05:56

minor update

7433084

Signed-off-by: wbpcode <wbphub@live.com>

fix building

8d8059d

Signed-off-by: wbpcode <wbphub@live.com>

fix format

c5db28d

Signed-off-by: wbpcode <wbphub@live.com>

fix test

3be03b1

Signed-off-by: wbpcode <wbphub@live.com>

fix building again

a0f6a5e

Signed-off-by: wbpcode <wbphub@live.com>

fix test

ce48c68

Signed-off-by: wbpcode <wbphub@live.com>

add more cost

e6d9d33

Signed-off-by: wbpcode <wbphub@live.com>

hate keyboard of mac

34c440d

Signed-off-by: wbpcode <wbphub@live.com>

repokitteh-read-only Bot added the waiting label Nov 28, 2023

new generic factory context

9e7316f

Signed-off-by: wbpcode <wbphub@live.com>

wbpcode added 9 commits November 29, 2023 08:26

workaround

ade383b

Signed-off-by: wbpcode <wbphub@live.com>

again fix building

6921aee

Signed-off-by: wbpcode <wbphub@live.com>

Merge branch 'main' of https://github.com/envoyproxy/envoy into dev-c…

6f08fef

…lean-common-factory-context

fix spelling

9f99b67

Signed-off-by: wbpcode <wbphub@live.com>

fix test

e44c807

Signed-off-by: wbpcode <wbphub@live.com>

fix test one by one

d5713ac

Signed-off-by: wbpcode <wbphub@live.com>

fix test

96186e7

Signed-off-by: wbpcode <wbphub@live.com>

should be final commit

3a58af6

Signed-off-by: wbpcode <wbphub@live.com>

fix...

8964ad9

Signed-off-by: wbpcode <wbphub@live.com>

alyssawilk reviewed Nov 29, 2023

View reviewed changes

repokitteh-read-only Bot added the waiting label Nov 29, 2023

wbpcode added 3 commits November 30, 2023 02:33

address comments

cfa6b22

Signed-off-by: wbpcode <wbphub@live.com>

Generic

2ed941d

resolve coverage problem

6874e2a

Signed-off-by: wbpcode <wbphub@live.com>

repokitteh-read-only Bot removed the waiting label Nov 30, 2023

wbpcode commented Nov 30, 2023

View reviewed changes

fix test

a65c759

Signed-off-by: wbpcode <wbphub@live.com>

wbpcode added 6 commits December 1, 2023 02:59

try to improve coverage

1106ade

Signed-off-by: wbpcode <wbphub@live.com>

minor update

8d7e366

Signed-off-by: wbpcode <wbphub@live.com>

save file

58f3a62

Signed-off-by: wbpcode <wbphub@live.com>

remove unexpected updates

a25c98d

Signed-off-by: wbpcode <wbphub@live.com>

fix building

f4698b8

Signed-off-by: wbpcode <wbphub@live.com>

coverage

358f910

Signed-off-by: wbpcode <wbphub@live.com>

wbpcode mentioned this pull request Dec 3, 2023

update all getServerFactoryContext to serverFactoryContext #31150

Merged

alyssawilk approved these changes Dec 4, 2023

View reviewed changes

wbpcode merged commit 5bc7a8c into envoyproxy:main Dec 4, 2023

wbpcode deleted the dev-clean-common-factory-context branch December 4, 2023 22:12

wbpcode mentioned this pull request Dec 8, 2023

factory context: refactored factory context #31189

Merged

Conversation

wbpcode commented Nov 28, 2023

Uh oh!

wbpcode commented Nov 28, 2023

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!

wbpcode Nov 28, 2023 • 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!

wbpcode Nov 30, 2023 • 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!

alyssawilk commented Nov 28, 2023

Uh oh!

alyssawilk commented Nov 28, 2023

Uh oh!

alyssawilk commented Nov 28, 2023

Uh oh!

wbpcode commented Nov 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wbpcode commented Nov 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alyssawilk left a comment

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!

wbpcode Nov 30, 2023 • 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!

wbpcode Nov 30, 2023 • 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!

wbpcode commented Nov 30, 2023

Uh oh!

wbpcode commented Dec 2, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

wbpcode Nov 28, 2023 •

edited

Loading

wbpcode Nov 30, 2023 •

edited

Loading

wbpcode commented Nov 29, 2023 •

edited

Loading

wbpcode commented Nov 29, 2023 •

edited

Loading

wbpcode Nov 30, 2023 •

edited

Loading

wbpcode Nov 30, 2023 •

edited

Loading