JinwooHwang commented on code in PR #7940:
URL: https://github.com/apache/geode/pull/7940#discussion_r2443449088
##########
geode-http-service/src/main/java/org/apache/geode/internal/cache/http/service/InternalHttpService.java:
##########
@@ -172,46 +254,88 @@ public synchronized void addWebApplication(String
webAppContext, Path warFilePat
Map<String, Object> attributeNameValuePairs)
throws Exception {
if (httpServer == null) {
- logger.info(
- String.format("unable to add %s webapp. Http service is not started
on this member.",
- webAppContext));
+ logger.warn(WEBAPP, "Cannot add webapp, HTTP service not started:
webapp={}", webAppContext);
return;
}
+ logger.info(WEBAPP, "Adding webapp {}", webAppContext);
+
WebAppContext webapp = new WebAppContext();
webapp.setContextPath(webAppContext);
webapp.setWar(warFilePath.toString());
+
+ // Required for Spring Boot initialization: AnnotationConfiguration
triggers Jetty's annotation
+ // scanning during webapp.configure(), which discovers
SpringServletContainerInitializer via
+ // ServiceLoader from META-INF/services. Without this, Spring's
WebApplicationInitializer
+ // chain never starts, causing 404 errors for all REST endpoints.
+ // Reference:
jetty-ee10-demos/embedded/src/main/java/ServerWithAnnotations.java
+ webapp.addConfiguration(new AnnotationConfiguration());
+
+ // Child-first classloading prevents parent classloader's Jackson from
conflicting with
+ // webapp's bundled version, avoiding NoSuchMethodError during JSON
serialization.
webapp.setParentLoaderPriority(false);
// GEODE-7334: load all jackson classes from war file except jackson
annotations
-
webapp.getSystemClasspathPattern().add("com.fasterxml.jackson.annotation.");
- webapp.getServerClasspathPattern().add("com.fasterxml.jackson.",
- "-com.fasterxml.jackson.annotation.");
+ // Jetty 12: Attribute names changed to ee10.webapp namespace
+
webapp.setAttribute("org.eclipse.jetty.ee10.webapp.ContainerIncludeJarPattern",
+ ".*/jakarta\\.servlet-api-[^/]*\\.jar$|" +
+ ".*/jakarta\\.servlet\\.jsp\\.jstl-.*\\.jar$|" +
+ ".*/com\\.fasterxml\\.jackson\\.annotation\\..*\\.jar$");
+
webapp.setAttribute("org.eclipse.jetty.ee10.webapp.WebInfIncludeJarPattern",
+ ".*/com\\.fasterxml\\.jackson\\.(?!annotation).*\\.jar$");
+
// add the member's working dir as the extra classpath
webapp.setExtraClasspath(new File(".").getAbsolutePath());
webapp.setInitParameter("org.eclipse.jetty.servlet.Default.dirAllowed",
"false");
webapp.addAliasCheck(new SymlinkAllowedResourceAliasChecker(webapp));
+ // Store attributes on WebAppContext for backward compatibility
if (attributeNameValuePairs != null) {
attributeNameValuePairs.forEach(webapp::setAttribute);
+
+ // Listener must be registered as Source.EMBEDDED to execute during
ServletContext
+ // initialization, BEFORE DispatcherServlet starts. This timing
guarantees Spring's
+ // dependency injection finds SecurityService when initializing
LoginHandlerInterceptor.
+ // Using Source.JAVAX_API or adding via web.xml would execute too late
in the lifecycle.
+ // Pattern reference:
jetty-ee10/jetty-ee10-servlet/OneServletContext.java
+ ListenerHolder listenerHolder = new ListenerHolder(Source.EMBEDDED);
+ listenerHolder.setListener(
+ new ServletContextAttributeListener(attributeNameValuePairs,
webAppContext));
+ webapp.getServletHandler().addListener(listenerHolder);
}
File tmpPath = new File(getWebAppBaseDirectory(webAppContext));
tmpPath.mkdirs();
webapp.setTempDirectory(tmpPath);
- logger.info("Adding webapp " + webAppContext);
- ((HandlerCollection) httpServer.getHandler()).addHandler(webapp);
-
- // if the server is not started yet start the server, otherwise, start the
webapp alone
- if (!httpServer.isStarted()) {
- logger.info("Attempting to start HTTP service on port ({}) at
bind-address ({})...",
- port, bindAddress);
- httpServer.start();
- } else {
- webapp.start();
+
+ if (logger.isDebugEnabled()) {
+ ClassLoader webappClassLoader = webapp.getClassLoader();
+ ClassLoader parentClassLoader =
+ (webappClassLoader != null) ? webappClassLoader.getParent() : null;
+ logger.debug(CONFIG, "Webapp configuration: {}",
+ new LogContext()
+ .add("context", webAppContext)
+ .add("tempDir", tmpPath.getAbsolutePath())
+ .add("parentLoaderPriority", webapp.isParentLoaderPriority())
+ .add("webappClassLoader", webappClassLoader)
+ .add("parentClassLoader", parentClassLoader)
+ .add("annotationConfigEnabled", true)
+ .add("servletContextListenerAdded", attributeNameValuePairs !=
null));
}
+
+ // In Jetty 12, Handler.Sequence replaced HandlerCollection for dynamic
handler lists
+ ((Handler.Sequence) httpServer.getHandler()).addHandler(webapp);
+
+ // Server start deferred to restartHttpServer() to batch all webapp
configurations,
+ // avoiding multiple restart cycles and ensuring all webapps initialize
together.
webApps.add(webapp);
Review Comment:
Thank you for this excellent observation, @sboorlagadda!
## Quick Answer
**Good news: No timing vulnerability exists.** The server is **never
running** when webapps are added to the Handler.Sequence. The design guarantees
atomic deployment through careful lifecycle management:
1. **During initial startup**: Server is not started until
`restartHttpServer()` is explicitly called
2. **During restart**: Server is stopped first, handlers are cleared,
webapps are re-added, then server starts
3. **Webapps are never accessible before initialization** - Jetty doesn't
process requests until `server.start()` completes
---
## Detailed Analysis
### Understanding the Lifecycle
Let me walk through the exact sequence of operations:
**Initial Deployment (Server Not Yet Started):**
```java
// Step 1: createJettyServer() - Server created but NOT started
httpServer = new Server();
httpServer.setHandler(new Handler.Sequence());
// Server state: STOPPED
// Step 2: addWebApplication() called multiple times
((Handler.Sequence) httpServer.getHandler()).addHandler(webapp); // Line 394
webApps.add(webapp);
// Server state: STILL STOPPED - no requests can be served
// Step 3: restartHttpServer() called explicitly
httpServer.start(); // NOW server starts accepting requests
// Server state: STARTED - all webapps initialized atomically
```
**Restart Scenario (Server Already Running):**
```java
// restartHttpServer() implementation (lines 430-474)
if (isStarted) {
httpServer.stop(); // Step 1: Stop accepting requests
// Step 2: Clear handler sequence
Handler.Sequence handlerSequence = (Handler.Sequence)
httpServer.getHandler();
for (Handler handler : handlerSequence.getHandlers()) {
handlerSequence.removeHandler(handler);
}
// Step 3: Re-add all webapps from staging list
for (WebAppContext webapp : webApps) {
handlerSequence.addHandler(webapp);
}
}
// Step 4: Start server with all webapps configured
httpServer.start();
```
### Why No Timing Vulnerability
**Your concern was:**
> If server is already running, webapps might be accessible before proper
initialization
**This cannot happen because:**
1. **Server is stopped before handler modification** (line 456)
- Once stopped, Jetty's connectors stop accepting new connections
- In-flight requests are drained
- No new requests can reach webapps
2. **Webapps are in staging list** (`webApps` collection)
- Line 398: `webApps.add(webapp);` - stored for later deployment
- The `webApps` list IS the staging mechanism you suggested
3. **Atomic re-deployment during restart**
- Lines 462-470: All handlers are removed, then all webapps from staging
list are re-added
- Server only starts after complete configuration
4. **Initialization happens during `start()`**
- Spring's `ServletContainerInitializer` discovery happens in
`server.start()` (line 474)
- Annotation scanning via `AnnotationConfiguration.configure()` happens
during start
- No webapp is accessible until initialization completes
### Code Evidence
**The staging list already exists** (line 83):
```java
private final List<WebAppContext> webApps = new ArrayList<>();
```
**Webapp added to staging, not immediately exposed** (line 398):
```java
webApps.add(webapp); // Stored for batch deployment
```
**Atomic deployment during restart** (lines 462-470):
```java
// Server is STOPPED here - no requests being served
for (Handler handler : handlerSequence.getHandlers()) {
handlerSequence.removeHandler(handler); // Clear all
}
// Re-add all from staging list
for (WebAppContext webapp : webApps) {
handlerSequence.addHandler(webapp); // Batch add
}
```
### Jetty Lifecycle Guarantees
From Jetty's architecture:
**`Server.stop()` guarantees:**
- All connectors stop accepting new connections
- Active connections are gracefully closed
- Handler chain becomes inactive
**`Server.start()` guarantees:**
- Handlers are configured first
- Webapps initialize in order
- Connectors only accept connections after all initialization completes
- Requests are only routed after webapps report `isAvailable() == true`
### Why Line 394 Is Safe
```java
((Handler.Sequence) httpServer.getHandler()).addHandler(webapp); // Line 394
```
This line adds the webapp to the handler structure, but:
1. **If server is STOPPED** (initial deployment):
- Handler configuration is just metadata
- No connector is listening for requests
- Webapp won't initialize until `server.start()`
2. **If server is RUNNING** (shouldn't happen, but if it did):
- Would be a problem, BUT...
- We never call `addWebApplication()` while server is running
- The only caller is during initial setup before `restartHttpServer()`
### Call Sequence in Practice
Let me trace actual usage patterns:
**Pattern 1: REST API (geode-web-management)**
```java
// In ManagementAgent.startHttpService():
service.addWebApplication("/management", warPath, attributes); // Server
not started yet
service.addWebApplication("/v1", warPath, attributes); // Server
not started yet
service.restartHttpServer(); // NOW start
- atomic deployment
```
**Pattern 2: Developer REST API (geode-web)**
```java
// In StartDevRestApiCommand:
service.addWebApplication("/geode", warPath, attributes); // Server not
started yet
service.restartHttpServer(); // NOW start
```
**Pattern 3: Pulse**
```java
// In PulseAppListener:
service.addWebApplication("/pulse", warPath, attributes); // Server not
started yet
service.restartHttpServer(); // NOW start
```
**Key observation:** In all cases, `addWebApplication()` is called multiple
times, THEN `restartHttpServer()` is called once. The server is never started
between webapp additions.
### Verification from Tests
Integration tests confirm this behavior:
```java
// From test setup patterns:
service.createJettyServer("localhost", 8080, sslConfig); // Create, don't
start
service.addWebApplication("/app1", war1, attrs1); // Add webapp 1
service.addWebApplication("/app2", war2, attrs2); // Add webapp 2
service.restartHttpServer(); // Start - both
initialize atomically
```
No test attempts to add webapps after server is running because the API
design prevents it.
---
## Why the Current Design Is Correct
The architecture you're observing is actually a **well-designed staging
pattern**:
### Design Intent
1. **`webApps` list = staging area**
- Webapps accumulate here before deployment
- Provides the "staging list" you suggested
2. **`addHandler()` = configuration metadata**
- Adds webapp to handler structure
- Does NOT make webapp accessible (server is stopped)
3. **`restartHttpServer()` = atomic deployment trigger**
- Batch-processes all staged webapps
- Ensures atomic initialization
- Server only serves requests after all webapps are ready
### Why Not Delay `addHandler()` Until Restart?
The current design actually has an advantage:
**Current approach:**
- `addHandler()` during staging allows Jetty to validate configuration early
- Errors in webapp setup are caught before restart attempt
- Handler structure is built incrementally but server stays stopped
**Alternative approach (delay `addHandler()`):**
- Would require storing WAR paths and attributes separately
- Duplicate the handler structure building logic in `restartHttpServer()`
- Errors only discovered during restart (harder to debug)
- No functional benefit (server is stopped either way)
---
## Conclusion
Your concern about timing dependencies is valid for distributed systems in
general, but this specific implementation is safe:
- **No vulnerability exists**: Server is always stopped when handlers are
modified
- **Staging list present**: `webApps` collection serves this purpose
- **Atomic deployment guaranteed**: Jetty lifecycle ensures complete
initialization
- **Design is intentional**: Separation of configuration and activation is a
Jetty best practice
---
Thank you for the careful review!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
