Remove the XXX_Server resource requirement

[jamezp]

We should attempt to remove the ${project.name}_Server resource requirement. It's a big awkward from a user perspective to have to ensure one end point always gets invoked before another endpoint.

I would think we could use a filter of some sort to handle this. We may have to set the servlet context on the ResteasyContext, but IMO that should be a last resort. I feel we could use a ContainerRequestFilter to set a property on the request itself.

If this cannot be removed, we really should remove the start and stop end points as they should not be publicly exposed.

[liweinan]

I can confirm removing the XXX_Server does no harm to the service to work when WildFly gRPC Subsystem is present. To verify this, I removed the XXX_Server from my generated war:

weli@192:~/w/g/target|main⚡?
➤ zip -d grpcToRest.example.grpc-1.0.1.Final-SNAPSHOT.war 'WEB-INF/classes/org/greet/Greet_Server$2.class'                                                                                                                                                                                  22:20:38
deleting: WEB-INF/classes/org/greet/Greet_Server$2.class
weli@192:~/w/g/target|main⚡?
➤ zip -d grpcToRest.example.grpc-1.0.1.Final-SNAPSHOT.war 'WEB-INF/classes/org/greet/Greet_Server$1.class'                                                                                                                                                                                  22:21:00
deleting: WEB-INF/classes/org/greet/Greet_Server$1.class
weli@192:~/w/g/target|main⚡?
weli@192:~/w/g/target|main⚡?
➤ zip -d grpcToRest.example.grpc-1.0.1.Final-SNAPSHOT.war 'WEB-INF/classes/org/greet/Greet_Server.class'                                                                                                                                                                                    22:21:11
deleting: WEB-INF/classes/org/greet/Greet_Server.class

And then I deployed this updated WAR into my local Galleon provisioned WildFly dist with gRPC subsystem installed:

And then I use this branch to verify that the gRPC service is up:

• https://github.com/liweinan/grpcToRest.example/compare/verify_no_XXX_server?expand=1

Here is the test result:

So I guess it's safe to remove the XXX_Server classes from the auto-generated project, @ronsigal wdyt?

[ronsigal]

Heyyy @liweinan. Well, I have a few comments.

1. One of the reasons for XXX_Server is the startGRPC() method, which lets you start a gRPC server outside of WildFly with the grpc subsystem.

2. Another reason was the use of XXX_Server.getServletContext() in XXXServiceGrpcImpl. I was using an initial call to XXX_Server.startContext() to grab a ServletContext that I could use for some of the HttpServletRequest methods. Lately I've changed that to GrpcHttpServletDispatcher.getServletContext("GrpcServlet") [still a work in progress], but I haven't committed anything, so I'm guessing you would still see XXX_Server.getServletContext(). Is that true? So, I'm not sure how things are working for you. I'm confused.

3. I have a new use for XXX_Server. I'm using the initial call to grab an HttpServletRequest that I can use for the HttpServletRequest security methods like login():

    public static HttpServletRequest getHttpServletRequest(String name) {
        return requests.get(name);
    }
    
    @Path("context")
    @GET
    public String startContext(@Context HttpServletRequest request, @Context ServletConfig servletConfig) throws Exception {
        servletContextMap.put(servletConfig.getServletName(), request.getServletContext());
        requests.put(servletConfig.getServletName(), ResteasyContext.getContextData(HttpServletRequest.class));

        HttpServletRequest r = (HttpServletRequest)ResteasyContext.getContextData(HttpServletRequest.class);
        return "Got " + this + ": " + servletConfig.getServletName() + " servletContext";
    }

Of course, you would have no way of knowing that. ;-)

So, I don't think we can remove it. But I would like to understand what's going on in item 2.

-Ron

[jamezp]

Could we use a ServletContextListener to register the context name? I would think that's doable.

I think we definitely need to remove the generated startGRPC end point, e.g. /start. This could potentially be used for a DDoS or thread exhaustion attack. The generated method looks like:

@Path("start")
@GET
public String startGRPC(@Context HttpServletRequest request) throws Exception {
    servletContext = request.getServletContext();
    final CC1_Server server = new CC1_Server();
    new Thread() {
        @SuppressWarnings({"deprecation", "removal"})
        public void run() {
            try {
                if (System.getSecurityManager() == null) {
                    server.start();
                } else {
                    AccessController.doPrivileged((PrivilegedExceptionAction<Void>) () -> {server.start(); return null;});
                }
                logger.info("started gRPC server on port " + PORT);
                server.blockUntilShutdown();
            } catch (Exception e) {
                e.printStackTrace();
            }
        }
    }.start();
    return "Starting gRPC server on port " + PORT;
}

If it just set loop to keep hitting that end point I could start too many threads and just crash the server potentially. We should also not be running it privileged. If we wanted something like this the we should require the correct permissions.

[liweinan]

@jamezp I'll verify the suggested solution.

[ronsigal]

This could potentially be used for a DDoS or thread exhaustion attack.

Excellent point

[ronsigal]

There's a variable "inWildfly" in the pom.xml that comes out of the archetype than can change some of the results. We could update ServerGrpcGenerator to output startGRPC() only if inWildfly is false.

[jamezp]

There's a variable "inWildfly" in the pom.xml that comes out of the archetype than can change some of the results. We could update ServerGrpcGenerator to output startGRPC() only if inWildfly is false.

IMO we don't want that method exposed to any endpoint. I assume we want this to be usable in Tomcat and the like as well. It would be a security issue there too.

What I think we need is a way to register the resources like we do in the gprc subsystem. I'm not sure how this would be done yet. I need to learn more about gRPC to give an idea for this :) One option might be a simple API, something like:

public class ServerBuilder {
    public ServerBuilder addService(final BindableService service) {
        // add
    }
    public Server build() {
        // create the server
    }
}

Or, maybe better, we require the user to create the gRPC server and we have an API that just returns a list of the BindableService's we generate.

[ronsigal]

I assume we want this to be usable in Tomcat

Yeah, I've been pretty WildFly-centric. E.g., I don't have any non-WildFly tests yet. But, yeah, you're right. Btw, the startGRPC() method dates from the time before the existence of the grpc subsystem.

[liweinan]

After some learning I understand the class file is generated by ServerGrpcGenerator.java:

weli@192:~/w/resteasy-grpc|main✓
➤ grep -rn 'startGRPC' *                                                                                                                      00:42:32
grpc-bridge/src/main/java/dev/resteasy/grpc/bridge/generator/ServerGrpcGenerator.java:109:                    .startBlock("public String startGRPC(@Context HttpServletRequest request) throws Exception {")
weli@192:~/w/resteasy-grpc|main✓

And it's used by ServiceGrpcExtender:

// Write the prefix_Server
final ServerGrpcGenerator generator = new ServerGrpcGenerator(Path.of(generatedSourcePath), packageName,
        root);
generator.generate();

For the startGRPC method itself, maybe adding a flag can solve the problem:

    final AtomicBoolean grpcStarted = new AtomicBoolean(false);

    @Path("start")
    @GET
    public String startGRPC(@Context HttpServletRequest request) throws Exception {
        if (grpcStarted.getAndSet(true)) {
            servletContext = request.getServletContext();
            final Greet_Server server = new Greet_Server();
            new Thread() {
                @SuppressWarnings({"deprecation", "removal"})
                public void run() {
                    try {
                        if (System.getSecurityManager() == null) {
                            server.start();
                        } else {
                            AccessController.doPrivileged((PrivilegedExceptionAction<Void>) () -> {
                                server.start();
                                return null;
                            });
                        }
                        logger.info("started gRPC server on port " + PORT);
                        server.blockUntilShutdown();
                    } catch (Exception e) {
                        e.printStackTrace();
                    }
                }
            }.start();
            return "Starting gRPC server on port " + PORT;
        } else {
            return "gRPC server already started on port " + PORT;
        }
    }

...

    @Path("stop")
    @GET
    public void stopGRPC() throws Exception {
        logger.info("stopping gRPC server on port " + PORT);
        stop();
        grpcStarted.set(false);
    }

[liweinan]

This verification is wrong. I tried to stop the WildFly server, changed the port in test, and disable the beforeTest setup, and test is still passing, it's really weird:

I've put my test code here:

https://github.com/liweinan/grpcToRest.example/blob/main/src/test/java/org/jboss/resteasy/grpc/server/GreetingTest.java

It passes no matter I start WildFly server or not, and I confirmed no other server is running(The port is even not listening for connection).

I can confirm removing the XXX_Server does no harm to the service to work when WildFly gRPC Subsystem is present. To verify this, I removed the XXX_Server from my generated war:

weli@192:~/w/g/target|main⚡?
➤ zip -d grpcToRest.example.grpc-1.0.1.Final-SNAPSHOT.war 'WEB-INF/classes/org/greet/Greet_Server$2.class'                                                                                                                                                                                  22:20:38
deleting: WEB-INF/classes/org/greet/Greet_Server$2.class
weli@192:~/w/g/target|main⚡?
➤ zip -d grpcToRest.example.grpc-1.0.1.Final-SNAPSHOT.war 'WEB-INF/classes/org/greet/Greet_Server$1.class'                                                                                                                                                                                  22:21:00
deleting: WEB-INF/classes/org/greet/Greet_Server$1.class
weli@192:~/w/g/target|main⚡?
weli@192:~/w/g/target|main⚡?
➤ zip -d grpcToRest.example.grpc-1.0.1.Final-SNAPSHOT.war 'WEB-INF/classes/org/greet/Greet_Server.class'                                                                                                                                                                                    22:21:11
deleting: WEB-INF/classes/org/greet/Greet_Server.class

And then I deployed this updated WAR into my local Galleon provisioned WildFly dist with gRPC subsystem installed:

And then I use this branch to verify that the gRPC service is up:

• https://github.com/liweinan/grpcToRest.example/compare/verify_no_XXX_server?expand=1

Here is the test result:

So I guess it's safe to remove the `XXX_Server` classes from the auto-generated project, @ronsigal wdyt?

[ronsigal]

For the startGRPC method itself, maybe adding a flag can solve the problem:

I think that's good. Note @jamezp's comment: "We should also not be running it privileged."

It passes no matter I start WildFly server or not

Wow, I have no idea. I'll try to run it this afternoon.

[liweinan]

@ronsigal In addition I have done more researches to this, and I tried to start the gRPC server inside WildFly:

➤ curl http://localhost:8080/grpcToRest.example.grpc-0.0.1-SNAPSHOT/grpcToJakartaRest/grpcserver/start                                                                                                                                                                                      00:49:42

And the server started in WildFly:

00:20:22,791 INFO  [org.greet.Greet_Server] (Thread-146) Server started, listening on 8082
00:20:22,792 INFO  [org.greet.Greet_Server] (Thread-146) started gRPC server on port 8082

I have a question, what is this server's relationship with the gRPC subsystem that listens on 9555, I assume they provide the similar functions right?

[ronsigal]

@liweinan, yes, the start method is meant for situations where the grpc subsystems isn't available, either in WildFly without the grpc layer or in some other server. Note that I haven't done any experiments with tomcat or other servers.

[ronsigal]

@liweinan, panic is over!! ;-) Try this:

    @Test
    public void testGreeting() {
        channelPlaintext = ManagedChannelBuilder.forTarget("localhost:9556")
                .usePlaintext()
                .intercept(new ClientLogInterceptor()).
                build();
        blockingStub = GreetServiceGrpc.newBlockingStub(channelPlaintext);

        GeneralEntityMessage.Builder builder = GeneralEntityMessage.newBuilder();
        GeneralEntityMessage gem = builder.setURL("http://localhost:xxx/greet/Bill").build();
        try {
            GeneralReturnMessage grm = blockingStub.greet(gem);
        	System.out.println("OK!!");
            dev_resteasy_example_grpc_greet___Greeting greeting = grm.getDevResteasyExampleGrpcGreetGreetingField();
            Assert.assertEquals("hello, Bill1", greeting.getS());
        } catch (StatusRuntimeException e) {
            //
        	System.out.println("ooops!!");
        }
    }