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

Add gRPC reverse proxy server example #5722

Merged
merged 23 commits into from
Jun 26, 2024
Merged

Conversation

eottabom
Copy link
Contributor

@eottabom eottabom commented Jun 4, 2024

Motivation:

Add gRPC reverse proxy server example

Modifications:

Add gRPC reverse proxy server example with testcontainers.

Result:

@CLAassistant
Copy link

CLAassistant commented Jun 4, 2024

CLA assistant check
All committers have signed the CLA.

@eottabom eottabom force-pushed the feature/ISSUE-2353 branch 2 times, most recently from 387a425 to effa17f Compare June 4, 2024 06:33
@trustin
Copy link
Member

trustin commented Jun 5, 2024

Could you check the build failures and make sure the tests are not flaky?

- fix: Modify armeria server port
@@ -16,6 +16,7 @@ dependencies {
testImplementation libs.awaitility
testImplementation libs.junit5.jupiter.api
testImplementation libs.protobuf.java.util
testImplementation libs.testcontainers.junit.jupiter
Copy link
Member

Choose a reason for hiding this comment

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

Please create a separate project for the example you're writing. Don't make an example do more than one thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, am I right to understand?
Are you telling me to create and use a grpc-reverse-proxy project in the sample directory?

@BeforeAll
static void startServer() {
server = Server.builder()
.http(18080)
Copy link
Member

Choose a reason for hiding this comment

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

Please use an ephemeral port to avoid port collision


@Container
private static GenericContainer<?> envoy = new GenericContainer<>("envoyproxy/envoy:v1.22.0")
.withExposedPorts(9090)
Copy link
Member

Choose a reason for hiding this comment

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

Please use a random unused port to avoid port collision as much as possible. Use PortUtil.unusedTcpPort.

- Remove reverse proxy in grpc project
- Add reverse proxy project
- Add reverse proxy example
@eottabom
Copy link
Contributor Author

It does not appear to be a problem caused by overlapping ports.
Let me find the cause again.

@eottabom
Copy link
Contributor Author

build-self-hosted-unsafe-jdk-21-snapshot-blockhound

I don't think there's a docker on the ci server

java.util.concurrent.CompletionException: java.lang.IllegalStateException: Could not find a valid Docker environment. Please see logs and check configuration

- Add try/catch CompletionException
Comment on lines 24 to 25
private static final int serverPort = PortUtil.unusedTcpPort();
private static final int backendServerPort = PortUtil.unusedTcpPort();
Copy link
Member

Choose a reason for hiding this comment

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

In an example, we should use the fixed ports. I'd expect tests use unused random ports though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I modified it to use a fixed port.

9424ddb

@BeforeAll
static void startServer() {
server = Server.builder()
.http(serverPort)
Copy link
Member

Choose a reason for hiding this comment

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

Specify 0 and let the OS choose the port number?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I modified it to use port 0.
ce6ab18

- test: Using server port 0
- fix: Modify default port
- test: Apply checkstyle
@jrhee17
Copy link
Contributor

jrhee17 commented Jun 14, 2024

Pushed some small commits 🙇 Let me know if any changes don't make sense

@jrhee17 jrhee17 added this to the 1.30.0 milestone Jun 14, 2024
Copy link
Contributor

@jrhee17 jrhee17 left a comment

Choose a reason for hiding this comment

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

👍 👍 👍

Copy link
Member

@minwoox minwoox left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!
Could you also run this script to see if everything works correctly?
https://github.com/line/armeria-examples/blob/main/update-examples.sh
You might run this command: (I supposed armeria and armeria-examples are in the same parent directory)
./update-examples.sh 1.29.0 ../armeria

examples/grpc-envoy/build.gradle Show resolved Hide resolved
class GrpcEnvoyProxyTest {

// the port envoy binds to within the container
private static final int envoyPort = 10000;
Copy link
Member

Choose a reason for hiding this comment

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

nit:

Suggested change
private static final int envoyPort = 10000;
private static final int ENVOY_PORT = 10000;

Comment on lines 36 to 43
private static Server startBackendServer(int serverPort) {
return Server.builder()
.http(serverPort)
.service(GrpcService.builder()
.addService(new HelloService())
.build())
.build();
}
Copy link
Member

Choose a reason for hiding this comment

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

nit: style

Suggested change
private static Server startBackendServer(int serverPort) {
return Server.builder()
.http(serverPort)
.service(GrpcService.builder()
.addService(new HelloService())
.build())
.build();
}
private static Server startBackendServer(int serverPort) {
return Server.builder()
.http(serverPort)
.service(GrpcService.builder()
.addService(new HelloService())
.build())
.build();
}

- Apply code style
- Add dependencies
- Modify variable name
@@ -48,5 +48,5 @@ static_resources:
- endpoint:
address:
socket_address:
address: host.testcontainers.internal
address: host.docker.internal
Copy link
Member

Choose a reason for hiding this comment

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

host.docker.internal doesn't work on Linux. I think we should revert this change.
See envoyproxy/java-control-plane#303

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was merging because of a crash, but I forgot the file. I fixed it!

@eottabom
Copy link
Contributor Author

Looks great, thanks! Could you also run this script to see if everything works correctly? https://github.com/line/armeria-examples/blob/main/update-examples.sh You might run this command: (I supposed armeria and armeria-examples are in the same parent directory) ./update-examples.sh 1.29.0 ../armeria

This script is not working properly.

The following error occurs
Is there a dependency I need to add?
image

@minwoox
Copy link
Member

minwoox commented Jun 19, 2024

This script is not working properly.

Let me see what happened. 😉

@minwoox
Copy link
Member

minwoox commented Jun 19, 2024

Let me see what happened. 😉

It works correctly. The problem was just that we needed to convert the libs.testcontainers.junit.jupiter to the actual names in the script like:
-pe "s/libs.testcontainers.junit.jupiter/'org.testcontainers:junit-jupiter:$TESTCONTAINER_VERSION'/g;" \

It will be handled when we release the next version so you don't have to worry about it. 😉

Copy link
Member

@minwoox minwoox left a comment

Choose a reason for hiding this comment

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

Thanks a lot, @eottabom! 😄

@eottabom
Copy link
Contributor Author

Rather, I think learned a lot. Thank you. 😄

Copy link
Member

@trustin trustin left a comment

Choose a reason for hiding this comment

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

👍 🙇

Copy link
Contributor

@ikhoon ikhoon left a comment

Choose a reason for hiding this comment

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

Looks a useful example! 🙇‍♂️🙇‍♂️

@ikhoon ikhoon merged commit f0ec1ba into line:main Jun 26, 2024
13 of 15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add gRPC reverse proxy server example
6 participants