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

Feat: Let the ingress-gateway uses the same port as the business service as a proxy for layer 7 traffic. #318

Merged
merged 10 commits into from
Mar 25, 2023

Conversation

panniyuyu
Copy link
Contributor

Let the ingress-gateway uses the same port as the business service as a proxy for layer 7 traffic.

panniyuyu@gmail.com and others added 4 commits February 24, 2023 10:17
Copy link
Member

@Xunzhuo Xunzhuo left a comment

Choose a reason for hiding this comment

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

Overall looks good to me, some nits. Thanks.

plugin/metaprotocol/generator.go Outdated Show resolved Hide resolved
plugin/metaprotocol/generator.go Outdated Show resolved Hide resolved
test/e2e/metaprotocol-gateway/metaprotocol_test.go Outdated Show resolved Hide resolved
test/e2e/metaprotocol-gateway/testdata/metarouter-v1.yaml Outdated Show resolved Hide resolved
test/e2e/metaprotocol-gateway/testdata/metarouter-v2.yaml Outdated Show resolved Hide resolved
test/e2e/metaprotocol-gateway/testdata/thrift-sample.yaml Outdated Show resolved Hide resolved
test/e2e/metaprotocol-gateway/testdata/virtualservice.yaml Outdated Show resolved Hide resolved
test/e2e/metaprotocol-gateway/hello.thrift Outdated Show resolved Hide resolved
@panniyuyu
Copy link
Contributor Author

OK, i will address these suggestions later. Thanks for reviewing. @Xunzhuo

Signed-off-by: panniyuyu@gmail.com <panniyuyu@gmail.com>
Copy link
Member

@zhaohuabing zhaohuabing left a comment

Choose a reason for hiding this comment

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

Could you please add the gateway e2e test to the github workflow?

@zhaohuabing
Copy link
Member

Thank you, @panniyuyu, for contributing this incredibly useful feature! I just wanted to let you know that since this PR is relatively large, it may take me some time to review and fully understand all of the changes. However, I really appreciate your hard work and am excited to see how this feature can improve our projec

@panniyuyu
Copy link
Contributor Author

Could you please add the gateway e2e test to the github workflow?

Sure, i will finish it later.

panniyuyu@gmail.com added 2 commits March 21, 2023 16:11
Signed-off-by: panniyuyu@gmail.com <panniyuyu@gmail.com>
Signed-off-by: panniyuyu@gmail.com <panniyuyu@gmail.com>
@zhaohuabing
Copy link
Member

zhaohuabing commented Mar 22, 2023

@panniyuyu Great job on the PR! I have reviewed it and everything looks good to me. Have you been able to identify the root cause of the failed gateway test? Once you have fixed the gateway test, yamllint errors, and golang lint errors, we will be able to merge your changes into the master branch.

Copy link
Member

@Xunzhuo Xunzhuo left a comment

Choose a reason for hiding this comment

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

Side comment, we need a document to decribe this new feature, thanks.

@zhaohuabing
Copy link
Member

Side comment, we need a document to decribe this new feature, thanks.

The documentation is not a blocker and can be added separately through another PR.

panniyuyu@gmail.com added 2 commits March 24, 2023 20:26
Signed-off-by: panniyuyu@gmail.com <panniyuyu@gmail.com>
Signed-off-by: panniyuyu@gmail.com <panniyuyu@gmail.com>
@panniyuyu
Copy link
Contributor Author

I have aready fixed the gateway test, yamllint errors. But i have no idea about golang lint errors. Could you please do me a favor? @zhaohuabing

@zhaohuabing
Copy link
Member

I have aready fixed the gateway test, yamllint errors. But i have no idea about golang lint errors. Could you please do me a favor? @zhaohuabing

golang lint checks the order of import packages. This should be fixed with:

goimports -local github.com/aeraki-mesh/aeraki ./...

Signed-off-by: panniyuyu@gmail.com <panniyuyu@gmail.com>
@zhaohuabing zhaohuabing merged commit 94f9ed0 into aeraki-mesh:master Mar 25, 2023
@zhaohuabing
Copy link
Member

This PR is the implementation of #303

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.

3 participants