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

generic proxy: move generic proxy from contrib into extensions #34892

Merged
merged 15 commits into from
Jul 9, 2024

Conversation

wbpcode
Copy link
Member

@wbpcode wbpcode commented Jun 25, 2024

Commit Message: generic proxy: move generic proxy from contrib into extensions
Additional Description:

The generic proxy is designed as a framework to empower the developers to develop new L7 proxy for various L7 protocols.
It's be used for multiple different private protocols now in the production env of our users. And recently, I complete the last part of generic proxy: the filter chain support to the variable length stream. And except the private protocols, we use the generic proxy implement the kafka proxing and pulsar proxing and conditional traffic routing. (part of these works are contributed back to the envoy).

After an offline discussion with other maintainers, I prepare to move the generic proxy to the extensions now.

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

wbpcode added 3 commits June 25, 2024 08:34
Signed-off-by: wbpcode <wbphub@live.com>
Signed-off-by: wbpcode <wbphub@live.com>
Signed-off-by: wbpcode <wbphub@live.com>
Copy link

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @adisuissa
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #34892 was opened by wbpcode.

see: more, trace.

wbpcode added 2 commits June 25, 2024 14:19
Signed-off-by: wbpcode <wbphub@live.com>
Signed-off-by: wbpcode <wbphub@live.com>
Copy link
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.

API changes LGTM, other than the comment I've passed to Harvey.
Left a minor comment on the codeowners file.

CODEOWNERS Show resolved Hide resolved
@@ -16,7 +16,7 @@ import "validate/validate.proto";
option java_package = "io.envoyproxy.envoy.extensions.filters.network.generic_proxy.action.v3";
option java_outer_classname = "ActionProto";
option java_multiple_files = true;
option go_package = "github.com/envoyproxy/go-control-plane/contrib/envoy/extensions/filters/network/generic_proxy/action/v3;actionv3";
option go_package = "github.com/envoyproxy/go-control-plane/envoy/extensions/filters/network/generic_proxy/action/v3;actionv3";
Copy link
Contributor

Choose a reason for hiding this comment

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

cc @htuch will this impact go based control planes that provided a contrib?
IMHO as this is contrib it may be ~ok to break, but just wondering whether we should reconsider the impact of moving an extension from contrib to extensions in the future.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not worried about that since it was contrib (it will have build consequences though).

Signed-off-by: wbpcode <wbphub@live.com>
@wbpcode
Copy link
Member Author

wbpcode commented Jun 26, 2024

/coverage

Copy link

Coverage for this Pull Request will be rendered here:

https://storage.googleapis.com/envoy-pr/34892/coverage/index.html

The coverage results are (re-)rendered each time the CI envoy-presubmit (check linux_x64 coverage) job completes.

🐱

Caused by: a #34892 (comment) was created by @wbpcode.

see: more, trace.

Signed-off-by: wbpcode <wbphub@live.com>
@wbpcode
Copy link
Member Author

wbpcode commented Jun 27, 2024

/coverage

Copy link

Coverage for this Pull Request will be rendered here:

https://storage.googleapis.com/envoy-pr/34892/coverage/index.html

The coverage results are (re-)rendered each time the CI envoy-presubmit (check linux_x64 coverage) job completes.

🐱

Caused by: a #34892 (comment) was created by @wbpcode.

see: more, trace.

wbpcode added 3 commits June 27, 2024 14:35
Signed-off-by: wbpcode <wbphub@live.com>
Signed-off-by: wbpcode <wbphub@live.com>
Signed-off-by: wbpcode <wbphub@live.com>
@wbpcode
Copy link
Member Author

wbpcode commented Jun 28, 2024

/retest

1 similar comment
@wbpcode
Copy link
Member Author

wbpcode commented Jun 28, 2024

/retest

Copy link
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 api

contrib/contrib_build_config.bzl Outdated Show resolved Hide resolved
@repokitteh-read-only repokitteh-read-only bot removed the api label Jul 1, 2024
wbpcode added 2 commits July 4, 2024 07:35
Signed-off-by: wbpcode <wbphub@live.com>
…hanks

Signed-off-by: wbpcode <wbphub@live.com>
@wbpcode wbpcode assigned soulxu and unassigned adisuissa Jul 4, 2024
@soulxu
Copy link
Member

soulxu commented Jul 8, 2024

It LGTM, but it seems the CI still not happy

@soulxu
Copy link
Member

soulxu commented Jul 8, 2024

/wait

Signed-off-by: wbpcode <wbphub@gmail.com>
@wbpcode
Copy link
Member Author

wbpcode commented Jul 8, 2024

Just for confirmation, this is normal extension, so I think one approval from maintainer is enough? cc @alyssawilk

@alyssawilk
Copy link
Contributor

Generally I'd say senior maintainer approval for initial import but AFIK you have Matt's thumbs up on slack so yeah I'd say any approval is fine.

Copy link
Member

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

@wbpcode wbpcode merged commit 0dbd441 into envoyproxy:main Jul 9, 2024
51 of 52 checks passed
@wbpcode wbpcode deleted the dev-move-generic-proxy branch July 9, 2024 03:36
alyssawilk added a commit to alyssawilk/envoy that referenced this pull request Jul 9, 2024
envoyproxy#34892)"

This reverts commit 0dbd441.

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
@alyssawilk
Copy link
Contributor

was prechecks broken prior to this PR? I'm seeing it fail on main now where it wasn't before my last sync, but it could have been a numbe ro fPRs

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.

None yet

5 participants