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

golang: L4 network extension #26173

Merged
merged 31 commits into from
Jun 26, 2023
Merged

Conversation

antJack
Copy link
Contributor

@antJack antJack commented Mar 20, 2023

Commit Message: l4 golang extension
Additional Description: allow using golang to write envoy network filter
Risk Level: low
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

@repokitteh-read-only
Copy link

Hi @antJack, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #26173 was opened by antJack.

see: more, trace.

@alyssawilk
Copy link
Contributor

/wait on main merge and CI

@antJack antJack force-pushed the l4-golang-extension branch 2 times, most recently from ff25b3d to cd1a5f0 Compare March 23, 2023 07:25
@antJack antJack force-pushed the l4-golang-extension branch 2 times, most recently from d27a841 to 0b4a454 Compare March 28, 2023 08:40
@lizan lizan marked this pull request as draft March 28, 2023 13:14
Copy link
Member

@phlax phlax left a comment

Choose a reason for hiding this comment

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

thanks for working on this @antJack - looks very cool

i will do a proper docs review when its ready

/docs

just from skimming through the PR - im wondering is the L4 extension replacing the L7 one ?

if not im wondering about whether we should have a separate sandbox for it

contrib/golang/filters/go/go.mod Outdated Show resolved Hide resolved
@repokitteh-read-only
Copy link

Docs for this Pull Request will be rendered here:

https://storage.googleapis.com/envoy-pr/26173/docs/index.html

The docs are (re-)rendered each time the CI envoy-presubmit (precheck docs) job completes.

🐱

Caused by: a #26173 (review) was submitted by @phlax.

see: more, trace.

@antJack
Copy link
Contributor Author

antJack commented Mar 30, 2023

The L4 extension is not going to replace the L7 one. The different between them is just like the different between envoy's Http::StreamFilter and Network::Filter.

And we do need a separate sandbox for L4, I'll work on this.

@antJack antJack force-pushed the l4-golang-extension branch 5 times, most recently from 0f89f2e to f0995b0 Compare April 18, 2023 15:03
@antJack
Copy link
Contributor Author

antJack commented Apr 20, 2023

/retest

@repokitteh-read-only
Copy link

Retrying Azure Pipelines:
Check envoy-presubmit isn't fully completed, but will still attempt retrying.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #26173 (comment) was created by @antJack.

see: more, trace.

@antJack
Copy link
Contributor Author

antJack commented Apr 20, 2023

/retest

@repokitteh-read-only
Copy link

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #26173 (comment) was created by @antJack.

see: more, trace.

@antJack
Copy link
Contributor Author

antJack commented Apr 22, 2023

/retest

@repokitteh-read-only
Copy link

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #26173 (comment) was created by @antJack.

see: more, trace.

@antJack
Copy link
Contributor Author

antJack commented Apr 23, 2023

/retest

@repokitteh-read-only
Copy link

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #26173 (comment) was created by @antJack.

see: more, trace.

Signed-off-by: yongjie.yyj <yeyeyongjie@gmail.com>
@phlax
Copy link
Member

phlax commented Jun 1, 2023

great, thanks for updating

cc @wbpcode for code review

@antJack
Copy link
Contributor Author

antJack commented Jun 1, 2023

I've placed the sandbox here: antJack#1

once this pr is ok, I will re-open another one into the envoyproxy:main

@phlax
Copy link
Member

phlax commented Jun 1, 2023

once this pr is ok, I will re-open another one into the envoyproxy:main

cool, feel free to open it WIP based on this PR - im happy to start reviewing

@antJack
Copy link
Contributor Author

antJack commented Jun 2, 2023

cool, feel free to open it WIP based on this PR - im happy to start reviewing

done, I open up a wip with sandbox here: #27762

Signed-off-by: yongjie.yyj <yeyeyongjie@gmail.com>
@wbpcode
Copy link
Member

wbpcode commented Jun 5, 2023

This is a huge PR orz. So, I think I can only review it on the weekend. orz

cc @StarryVae could you help me to take a first pass when you have free time?

@StarryVae
Copy link
Contributor

yeah, thanks.

@ravenblackx
Copy link
Contributor

Mostly commenting because this is out of review SLO (but I see that's been explained already).

But also, might be worth addressing the merge conflicts while you wait for reviewer availability.

Signed-off-by: yongjie.yyj <yeyeyongjie@gmail.com>
Signed-off-by: yongjie.yyj <yeyeyongjie@gmail.com>
Signed-off-by: yongjie.yyj <yeyeyongjie@gmail.com>
@antJack
Copy link
Contributor Author

antJack commented Jun 12, 2023

@wangfakang @doujiang24 @phlax @wbpcode @StarryVae @ravenblackx

Could you please help take a look at this huge PR again? I would be very grateful for your help.

Signed-off-by: yongjie.yyj <yeyeyongjie@gmail.com>
Signed-off-by: yongjie.yyj <yeyeyongjie@gmail.com>
Signed-off-by: yongjie.yyj <yeyeyongjie@gmail.com>
Signed-off-by: yongjie.yyj <yeyeyongjie@gmail.com>
@antJack
Copy link
Contributor Author

antJack commented Jun 16, 2023

/retest

@doujiang24
Copy link
Member

@wbpcode @phlax I think it's good enough for a first iteration, maybe we could merge it and improve it later?
Now, @antJack have to resolve conflicts again and again, since l7 is also iterating quickly.

@phlax
Copy link
Member

phlax commented Jun 22, 2023

@mattklein123 would you mind taking a pass at this ?

@mattklein123 mattklein123 merged commit 63d4fb0 into envoyproxy:main Jun 26, 2023
asheryerm pushed a commit to asheryerm/envoy that referenced this pull request Jul 5, 2023
Signed-off-by: yongjie.yyj <yeyeyongjie@gmail.com>
Signed-off-by: asheryer <asheryer@amazon.com>
reskin89 pushed a commit to reskin89/envoy that referenced this pull request Jul 11, 2023
Signed-off-by: yongjie.yyj <yeyeyongjie@gmail.com>
Signed-off-by: Ryan Eskin <ryan.eskin89@protonmail.com>
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.