Skip to content

test: creating a test listener filter and moving the xds test over to it#20571

Merged
alyssawilk merged 3 commits intoenvoyproxy:mainfrom
alyssawilk:test_listener_filter
Mar 30, 2022
Merged

test: creating a test listener filter and moving the xds test over to it#20571
alyssawilk merged 3 commits intoenvoyproxy:mainfrom
alyssawilk:test_listener_filter

Conversation

@alyssawilk
Copy link
Copy Markdown
Contributor

@alyssawilk alyssawilk commented Mar 29, 2022

Part of #9953

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

see: more, trace.

@alyssawilk alyssawilk changed the title test: creating a test listener filter and moving the xds test over to test: creating a test listener filter and moving the xds test over to it Mar 29, 2022
it.

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
@alyssawilk alyssawilk force-pushed the test_listener_filter branch from 4bdee74 to aaef467 Compare March 29, 2022 17:50
}

ProtobufTypes::MessagePtr createEmptyConfigProto() override {
return ProtobufTypes::MessagePtr{new Envoy::ProtobufWkt::Struct()};
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.

Given #20397 and upcoming #20049 , can we add a real protobuf for this test filter?

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 mean it's not needed here, so would it make sense to add it as a follow up over in one of those PRs?

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.

oh whoops, I was looking at the second and not the first. :-/

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.

Yeah, we have #20476 for general fix-ups.

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
alpn_.clear();
return Network::FilterStatus::Continue;
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We got a new method for ListenerFilter

virtual FilterStatus onData(Network::ListenerFilterBuffer& buffer) PURE;

Just need a empty implement of onData

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.

Oh thanks for the heads up - github hadn't picked up what would have broken CI :-)

std::string name() const override { return "envoy.filters.listener.test"; }
};

absl::Mutex TestListenerFilter::alpn_lock_;
Copy link
Copy Markdown
Member

@soulxu soulxu Mar 30, 2022

Choose a reason for hiding this comment

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

Another way is to set the alpn as a parameter for this filter config, then we can get per-instance alpn value. But yes, it needs more code probably.

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 think that doesn't work as we want one filter which returns different alpn values for different connections. But anyway the goal is to just have those tests pass without a filter some folks don't want to include so I think simple (with crummy locks) is fine :-)

@alyssawilk alyssawilk marked this pull request as ready for review March 30, 2022 14:09
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Copy link
Copy Markdown
Contributor

@adisuissa adisuissa left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@alyssawilk alyssawilk merged commit 2757eb9 into envoyproxy:main Mar 30, 2022
ravenblackx pushed a commit to ravenblackx/envoy that referenced this pull request Jun 8, 2022
@alyssawilk alyssawilk deleted the test_listener_filter branch August 4, 2022 01:08
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.

4 participants