listener: add listener info to the contexts by kyessenov · Pull Request #31067 · envoyproxy/envoy

kyessenov · 2023-11-27T19:56:51Z

Commit Message: Refactor to consolidate listener info and expose it in wider contexts. The concrete issue was that I wanted to use listener properties (direction) in the access logger, but only the filter context has it. By keeping the information in one place, I think we can also trim the large listener proto in the future.
Risk Level: low, internal refactor
Testing: regression
Docs Changes: none
Release Notes: none

Signed-off-by: Kuat Yessenov <kuat@google.com>

wbpcode · 2023-11-28T00:38:49Z

Great job. But I believe the AccessLogFactoryContext will be removed soon. And now FactoryContext is used by the logger factory directly.
Would you like to do the clean up directly? I mean to remove the AccessLogFactoryContext in this patch directly. :)

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov · 2023-11-28T01:20:22Z

Oh, didn't realize the access log factory context is gone. Yes, I can delete it here, since this PR already forces some minor refactors on the code.

Signed-off-by: Kuat Yessenov <kuat@google.com>

wbpcode

Thanks. LGTM overall. Only two comments are added.

zirain · 2023-11-28T10:43:05Z

The concrete issue was that I wanted to use listener properties (direction) in the access logger

IIUC, envoy can expose TrafficDirection to access log with this patch?

kyessenov · 2023-11-28T23:19:30Z

@zirain Yes, the goal is to make istio_stats act like an access logger, which implies the regular access loggers will have access to everything istio_stats uses.

zirain · 2023-11-28T23:44:41Z

@zirain Yes, the goal is to make istio_stats act like an access logger, which implies the regular access loggers will have access to everything istio_stats uses.

that's great, I recall I need this.

Signed-off-by: Kuat Yessenov <kuat@google.com>

wbpcode

Just take a full pass. It's pretty great and I have no much comments to it. Sorry again for the noise of ListenerInfo and ListenerConfig. I think your previous design maybe better.
Thanks so much for your great contribution. And please check the CI.

wbpcode · 2023-11-29T13:42:47Z

 * A configuration for an individual listener.
 */
-class ListenerConfig {
+class ListenerConfig : public ListenerInfo {


if it's because the lifetime problem, then I think you previous design is reasonable. (sorry

No worries, reverted. I kept the change to remove listenerConfig from ListenerFactoryContext and isQuicListener from FactoryContext.

wbpcode · 2023-11-29T14:01:45Z

-    ON_CALL(context_, listenerMetadata()).WillByDefault(ReturnRef(listener_metadata_));
+    ON_CALL(context_, listenerInfo()).WillByDefault(ReturnRef(listener_info_));
+    ON_CALL(listener_info_, metadata()).WillByDefault(ReturnRef(listener_metadata_));


ON_CALL(context_.listener_info_, ...); is enough. We needn't create a local listener info again.

This reverts commit a63a39d. Signed-off-by: Kuat Yessenov <kuat@google.com>

Signed-off-by: Kuat Yessenov <kuat@google.com>

wbpcode

LGTM. Thanks. cc Hi, @alyssawilk, could you also take a look when you have free time in case I missed something, thanks.

soulxu · 2023-11-30T07:55:24Z


+class ListenerInfoImpl : public Network::ListenerInfo {
+public:
+  explicit ListenerInfoImpl(const envoy::config::listener::v3::Listener config)


nit, this can be a reference 'const envoy::config::listener::v3::Listener& config'

Good catch, thanks.

alyssawilk

Very nice clean up!

alyssawilk · 2023-11-30T19:36:19Z

+  /**
+   * @return ListenerInfo description of the listener.
+   */
+  virtual const Network::ListenerInfo& listenerInfo() const PURE;


I like the consolidation but it feels like a regression to add more listener info into the non-listener-named factory context. I suspect it'd be a complete PITA to move this accessor down to the ListenerFactoryContext but have we tried to see how bad it would be?

We'd have to change the signature of network filters at the very least - HCM needs isQuic check which needs the listener info. I suspect most of the uses for FactoryContext are actually ListenerFactoryContext. What is called ListenerFactoryContext is only used by listener filters.

I suspect most of the uses for FactoryContext are actually ListenerFactoryContext. What is called ListenerFactoryContext is only used by listener filters.

Yeah, actually almost all FactoryContext is a listener factory context that provide isQuicListener, listenerMetadata, listenerTypedMetadata, etc. methods.

Maybe we can just rename it to ListenerFactoryContext in the future with a simple name replacement, if we want.

alyssawilk · 2023-11-30T19:37:02Z

-   */
-  virtual const Network::ListenerConfig& listenerConfig() const PURE;
-};
+class ListenerFactoryContext : public virtual FactoryContext {};


ooc if we keep this and it has no functions can it just be typeddefd to FactoryContext or does that cause inheritance problems?

It's still painful with mocks/fakes. I thought it's better to delete it then - do you want to try that?

I think we can just clean up it with a new PR.

alyssawilk · 2023-11-30T19:39:57Z

-  createListenerFilterFactories();
-  validateFilterChains();
-  buildFilterChains();
+  buildUdpListenerFactory(config, parent_.server_.options().concurrency());


mild preference for leaving these function definitions as-is and adding a protected accessor config() which returns info->config()

I thought it would be good to delete config() accessor long term because storing the proto as-is can be expensive with large listeners or Wasm. It's only needed for config dump which can be optimized to recreate the proto like it's done in some other places.

alyssawilk · 2023-11-30T19:44:44Z

 namespace Envoy {
 namespace Server {

+class EmptyListenerInfo : public Network::ListenerInfo {


why not just create a listener info impl with minimal contents?

Didn't want to bring a dependency on extensions...

admin already depends on listener no? and listenre is moving back to core code soon. we could alternately factor it out into a minimal library? I'm just not a fan of one-off admin classes but I can look the other way if it's a pain =P

Sure, if we're OK sharing the code in listener_manager, I deleted this dummy class.

alyssawilk · 2023-11-30T19:45:15Z

-envoy::config::core::v3::TrafficDirection FactoryContextImpl::direction() const {
-  return config_.traffic_direction();
-}
+const Network::ListenerInfo& FactoryContextImpl::listenerInfo() const { return listener_info_; }


nice clean up

Signed-off-by: Kuat Yessenov <kuat@google.com>

soulxu

LGTM, thanks

Signed-off-by: Kuat Yessenov <kuat@google.com>

wbpcode · 2023-12-05T01:50:31Z

CI still requires a check.

wbpcode · 2023-12-05T01:50:38Z

/wait

kyessenov · 2023-12-05T03:40:46Z

Hmmm, might be something unrelated https://dev.azure.com/cncf/envoy/_build/results?buildId=157018&view=logs&jobId=8c169225-0ae8-53bd-947f-07cb81846cb5&j=8c169225-0ae8-53bd-947f-07cb81846cb5&t=d1a98671-b7ba-5fbf-f06c-ff337c010df4.

/retest

Signed-off-by: Kuat Yessenov <kuat@google.com>

wbpcode · 2023-12-06T01:53:42Z

/retest

wbpcode · 2023-12-06T01:55:48Z

(Recently, we also are refactoring factory context related things which result in some conflictions. Sorry for that. There are still last one PR.)

zirain · 2023-12-07T01:20:04Z

 * without reaching to Envoy maintainers first.
 */
-class ListenerAccessLogFactoryContext : public virtual CommonFactoryContext {
+class FactoryContext : public virtual CommonFactoryContext {


@kyessenov need to update the comment of this class? it looks like a little mismatch?

kyessenov added 4 commits November 22, 2023 23:24

wip

b7664a5

Signed-off-by: Kuat Yessenov <kuat@google.com>

Merge remote-tracking branch 'upstream/main' into add_listener_info

bc92ad6

listener: consolidate listener info and expose it wider

164def2

Signed-off-by: Kuat Yessenov <kuat@google.com>

fix typo

91e66cd

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov requested review from adisuissa, alyssawilk, ggreenway, htuch, lizan, mattklein123, soulxu and wbpcode as code owners November 27, 2023 20:16

kyessenov added 2 commits November 27, 2023 22:46

fix

6a47c4b

Signed-off-by: Kuat Yessenov <kuat@google.com>

fix

4948a31

Signed-off-by: Kuat Yessenov <kuat@google.com>

wbpcode self-assigned this Nov 28, 2023

kyessenov added 2 commits November 28, 2023 01:16

Merge remote-tracking branch 'upstream/main' into add_listener_info

fba6815

delete access log factory context

c294fd9

Signed-off-by: Kuat Yessenov <kuat@google.com>

add coverage

4b199a8

Signed-off-by: Kuat Yessenov <kuat@google.com>

wbpcode reviewed Nov 28, 2023

View reviewed changes

Comment thread envoy/network/listener.h

Comment thread envoy/server/factory_context.h Outdated

kyessenov added 2 commits November 29, 2023 04:31

Merge remote-tracking branch 'upstream/main' into add_listener_info

447f743

simplify

a63a39d

Signed-off-by: Kuat Yessenov <kuat@google.com>

wbpcode reviewed Nov 29, 2023

View reviewed changes

kyessenov added 2 commits November 29, 2023 16:49

Revert "simplify"

ef742ee

This reverts commit a63a39d. Signed-off-by: Kuat Yessenov <kuat@google.com>

remove isquiclistener and listenerconfig

4e0b3e7

Signed-off-by: Kuat Yessenov <kuat@google.com>

wbpcode previously approved these changes Nov 30, 2023

View reviewed changes

wbpcode assigned alyssawilk and unassigned wbpcode Nov 30, 2023

soulxu reviewed Nov 30, 2023

View reviewed changes

alyssawilk reviewed Nov 30, 2023

View reviewed changes

alyssawilk added the waiting label Nov 30, 2023

change to reference

da3b77c

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov dismissed wbpcode’s stale review via da3b77c November 30, 2023 20:29

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

soulxu previously approved these changes Dec 1, 2023

View reviewed changes

kyessenov added 2 commits December 4, 2023 19:01

Merge remote-tracking branch 'upstream/main' into add_listener_info

4136c78

review

e83052b

Signed-off-by: Kuat Yessenov <kuat@google.com>

kyessenov dismissed soulxu’s stale review via e83052b December 4, 2023 19:21

kyessenov added 2 commits December 4, 2023 19:27

oops

cd15501

Signed-off-by: Kuat Yessenov <kuat@google.com>

merge fix

b348f9a

Signed-off-by: Kuat Yessenov <kuat@google.com>

repokitteh-read-only Bot added the waiting label Dec 5, 2023

kyessenov added 2 commits December 5, 2023 18:03

merge fix

2d697ae

Signed-off-by: Kuat Yessenov <kuat@google.com>

format

f1c5aa6

Signed-off-by: Kuat Yessenov <kuat@google.com>

repokitteh-read-only Bot removed the waiting label Dec 5, 2023

fix

108c714

Signed-off-by: Kuat Yessenov <kuat@google.com>

alyssawilk approved these changes Dec 6, 2023

View reviewed changes

alyssawilk merged commit 148fd48 into envoyproxy:main Dec 6, 2023

zirain mentioned this pull request Dec 6, 2023

Update envoy with fixes istio/proxy#5194

Merged

zirain reviewed Dec 7, 2023

View reviewed changes

wbpcode mentioned this pull request Dec 8, 2023

fix potential risk of multiple listener info objects for single listener #31227

Merged

Conversation

kyessenov commented Nov 27, 2023

Uh oh!

wbpcode commented Nov 28, 2023

Uh oh!

kyessenov commented Nov 28, 2023

Uh oh!

wbpcode left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

zirain commented Nov 28, 2023

Uh oh!

kyessenov commented Nov 28, 2023

Uh oh!

zirain commented Nov 28, 2023

Uh oh!

wbpcode left a comment

Choose a reason for hiding this comment

Uh oh!

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 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!

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soulxu left a comment

Choose a reason for hiding this comment

Uh oh!

wbpcode commented Dec 5, 2023

Uh oh!

wbpcode commented Dec 5, 2023

Uh oh!

kyessenov commented Dec 5, 2023

Uh oh!

wbpcode commented Dec 6, 2023

Uh oh!

wbpcode commented Dec 6, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers