-
Notifications
You must be signed in to change notification settings - Fork 978
Add VirtualHost support for random ports (#6410) #6603
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
base: main
Are you sure you want to change the base?
Changes from 5 commits
941415c
80d4811
4c4f766
8a3b41a
4786e0e
3004ed6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -138,6 +138,9 @@ final class DefaultServerConfig implements ServerConfig { | |
| private final ServerMetrics serverMetrics; | ||
| private final Function<? super String, ? extends EventLoopGroup> bossGroupFactory; | ||
|
|
||
| @Nullable | ||
| private volatile Int2ObjectMap<VirtualHost> actualPortToVirtualHost; | ||
|
|
||
| @Nullable | ||
| private String strVal; | ||
|
|
||
|
|
@@ -332,8 +335,9 @@ private static Mapping<String, VirtualHost> buildDomainMapping(VirtualHost defau | |
| // Set virtual host definitions and initialize their domain name mapping. | ||
| final DomainMappingBuilder<VirtualHost> mappingBuilder = new DomainMappingBuilder<>(defaultVirtualHost); | ||
| for (VirtualHost h : virtualHosts) { | ||
| if (h.port() > 0) { | ||
| if (h.port() > 0 || h.serverPort() != null) { | ||
| // A port-based virtual host will be handled by buildDomainAndPortMapping(). | ||
| // A ServerPort-based virtual host (with port 0) will be resolved at runtime. | ||
| continue; | ||
| } | ||
| mappingBuilder.add(h.hostnamePattern(), h); | ||
|
|
@@ -398,6 +402,26 @@ void setServer(Server server) { | |
| this.server = requireNonNull(server, "server"); | ||
| } | ||
|
|
||
| /** | ||
| * Builds the O(1) lookup map from actual bound ports to VirtualHosts. | ||
| * This should be called after all ports are bound. | ||
| */ | ||
| void buildActualPortMapping() { | ||
| final Int2ObjectMap<VirtualHost> mapping = new Int2ObjectOpenHashMap<>(); | ||
| for (VirtualHost vh : virtualHosts) { | ||
| final ServerPort sp = vh.serverPort(); | ||
| if (sp != null && sp.localAddress().getPort() == 0) { | ||
| final int actualPort = sp.actualPort(); | ||
| if (actualPort > 0) { | ||
| mapping.put(actualPort, vh); | ||
| } | ||
| } | ||
| } | ||
| if (!mapping.isEmpty()) { | ||
| actualPortToVirtualHost = mapping; | ||
| } | ||
| } | ||
|
Comment on lines
409
to
420
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: # Find ServerBuilder and virtualHost method implementations
find . -name "ServerBuilder.java" -type f | head -5Repository: line/armeria Length of output: 123 🏁 Script executed: # Search for virtualHost method with ServerPort parameter
rg -n "virtualHost.*ServerPort" --type=java -B2 -A10 | head -100Repository: line/armeria Length of output: 8958 🏁 Script executed: # Search for ServerPort class definition
find . -name "ServerPort.java" -type fRepository: line/armeria Length of output: 120 🏁 Script executed: # Look for port validation in ServerBuilder context
rg -n "ServerPort.*port.*0|ephemeral" --type=java -B2 -A2 | head -80Repository: line/armeria Length of output: 8867 🏁 Script executed: # Check ServerPort implementation to understand the actual port handling
cat core/src/main/java/com/linecorp/armeria/server/ServerPort.javaRepository: line/armeria Length of output: 12854 🏁 Script executed: # Look at buildDomainMapping to understand the full context
sed -n '330,350p' core/src/main/java/com/linecorp/armeria/server/DefaultServerConfig.javaRepository: line/armeria Length of output: 1158 🏁 Script executed: # Check if there's any validation in ServerBuilder that would prevent non-ephemeral ServerPort
sed -n '1880,1920p' core/src/main/java/com/linecorp/armeria/server/ServerBuilder.javaRepository: line/armeria Length of output: 1940 🏁 Script executed: # Look for any documentation or comments about ServerPort usage constraints
rg -n "ServerPort.*ephemeral|virtualHost.*ServerPort" --type=java --type=mdx | grep -i "comment\|javadoc\|ephemeral\|doc" | head -20Repository: line/armeria Length of output: 83 🏁 Script executed: # Check if there's any validation added in ServerBuilder for ephemeral ports
rg -n "virtualHost.*ServerPort|port.*0|ephemeral" core/src/main/java/com/linecorp/armeria/server/ServerBuilder.java -B3 -A3 | head -60Repository: line/armeria Length of output: 2365 🏁 Script executed: # Check if buildActualPortMapping is tested with non-ephemeral ports
rg -n "buildActualPortMapping|ServerPort.*[89][0-9]{2,}" core/src/test/java/com/linecorp/armeria/server/ -B2 -A2 | head -80Repository: line/armeria Length of output: 1843 🏁 Script executed: # Check what happens after buildActualPortMapping - how is actualPortToVirtualHost used?
sed -n '450,470p' core/src/main/java/com/linecorp/armeria/server/DefaultServerConfig.javaRepository: line/armeria Length of output: 907 🏁 Script executed: # Look for any VirtualHostBuilder validation that might restrict ServerPort to ephemeral
rg -n "class VirtualHostBuilder" core/src/main/java/com/linecorp/armeria/server/ -A 30 | head -80Repository: line/armeria Length of output: 3644 🏁 Script executed: # Check if there are any test cases that try to use non-ephemeral ServerPort with virtualHost()
rg -n "virtualHost.*new ServerPort.*[89]" core/src/test/java/com/linecorp/armeria/server/ -B2 -A5Repository: line/armeria Length of output: 38 🏁 Script executed: # Check the VirtualHostBuilder constructor to see if it validates ServerPort
sed -n '145,180p' core/src/main/java/com/linecorp/armeria/server/VirtualHostBuilder.javaRepository: line/armeria Length of output: 1332 🏁 Script executed: # Look for the build() method of VirtualHostBuilder to see if it creates VirtualHost with validation
rg -n "VirtualHostBuilder.*build\(\)|new VirtualHost" core/src/main/java/com/linecorp/armeria/server/VirtualHostBuilder.java -A 5 | head -60Repository: line/armeria Length of output: 2308 🏁 Script executed: # Get the full build() method from VirtualHostBuilder
sed -n '1400,1530p' core/src/main/java/com/linecorp/armeria/server/VirtualHostBuilder.javaRepository: line/armeria Length of output: 7353 🏁 Script executed: # Check if there's validation for ephemeral port requirement for ServerPort-based VirtualHosts
rg -n "serverPort.*port.*0|ephemeral.*ServerPort|build\(\)" core/src/main/java/com/linecorp/armeria/server/VirtualHostBuilder.java -B3 -A3 | head -100Repository: line/armeria Length of output: 3314 🏁 Script executed: # Check the VirtualHost constructor to see if it validates serverPort
rg -n "class VirtualHost|VirtualHost(" core/src/main/java/com/linecorp/armeria/server/VirtualHost.java -A 2 | head -50Repository: line/armeria Length of output: 166 🏁 Script executed: # Check if there are any integration tests that would catch the unreachable VirtualHost issue
rg -n "virtualHost.*ServerPort.*8080|virtualHost.*new ServerPort.*[1-9]" core/src/test/java/com/linecorp/armeria/server/PortBasedVirtualHostTest.java -B2 -A5Repository: line/armeria Length of output: 38 🏁 Script executed: # Check VirtualHost constructor
rg -n "public VirtualHost\(" core/src/main/java/com/linecorp/armeria/server/VirtualHost.java -A 30 | head -80Repository: line/armeria Length of output: 38 🏁 Script executed: # Search for any validation logic in VirtualHost that checks serverPort
rg -n "serverPort.*port.*0|checkArgument.*serverPort|checkState.*serverPort" core/src/main/java/com/linecorp/armeria/server/VirtualHost.java -B2 -A2Repository: line/armeria Length of output: 38 🏁 Script executed: # Check the actual port in the comment about ServerPort-based virtual hosts
sed -n '1860,1900p' core/src/main/java/com/linecorp/armeria/server/ServerBuilder.javaRepository: line/armeria Length of output: 1793
The Javadoc and design explicitly state this API is for "random port (port 0)", and Add a port validation check in 🤖 Prompt for AI Agents |
||
|
|
||
| @Override | ||
| public List<ServerPort> ports() { | ||
| return ports; | ||
|
|
@@ -425,6 +449,14 @@ public VirtualHost findVirtualHost(String hostname) { | |
|
|
||
| @Override | ||
| public VirtualHost findVirtualHost(String hostname, int port) { | ||
| final Int2ObjectMap<VirtualHost> portMapping = actualPortToVirtualHost; | ||
| if (portMapping != null) { | ||
| final VirtualHost vh = portMapping.get(port); | ||
| if (vh != null) { | ||
| return vh; | ||
| } | ||
| } | ||
|
|
||
| if (virtualHostAndPortMapping == null) { | ||
| return defaultVirtualHost; | ||
| } | ||
|
|
@@ -437,6 +469,7 @@ public VirtualHost findVirtualHost(String hostname, int port) { | |
| return virtualHost; | ||
| } | ||
| } | ||
|
|
||
| // No port-based virtual host is configured. Look for named-based virtual host. | ||
| final Mapping<String, VirtualHost> nameBasedMapping = virtualHostAndPortMapping.get(-1); | ||
| assert nameBasedMapping != null; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,6 +17,7 @@ | |
| package com.linecorp.armeria.server; | ||
|
|
||
| import static com.google.common.base.Preconditions.checkArgument; | ||
| import static com.google.common.base.Preconditions.checkState; | ||
| import static com.linecorp.armeria.common.SessionProtocol.HTTP; | ||
| import static com.linecorp.armeria.common.SessionProtocol.HTTPS; | ||
| import static com.linecorp.armeria.common.SessionProtocol.PROXY; | ||
|
|
@@ -68,6 +69,13 @@ static long nextPortGroup() { | |
| private int hashCode; | ||
| @Nullable | ||
| private ServerPortMetric serverPortMetric; | ||
| @Nullable | ||
| private final ServerPort originalServerPort; | ||
|
|
||
| /** | ||
| * The actual port after binding. -1 means not set yet. | ||
| */ | ||
| private volatile int actualPort = -1; | ||
|
|
||
| @Nullable | ||
| private String strVal; | ||
|
|
@@ -113,6 +121,15 @@ public ServerPort(InetSocketAddress localAddress, Iterable<SessionProtocol> prot | |
| * will choose the same port number for them, rather than allocating two ephemeral ports. | ||
| */ | ||
| ServerPort(InetSocketAddress localAddress, Iterable<SessionProtocol> protocols, long portGroup) { | ||
| this(localAddress, protocols, portGroup, null); | ||
| } | ||
|
|
||
| /** | ||
| * Creates a new {@link ServerPort} with a reference to the original {@link ServerPort}. | ||
| * This constructor is used internally when binding to an ephemeral port. | ||
| */ | ||
| ServerPort(InetSocketAddress localAddress, Iterable<SessionProtocol> protocols, long portGroup, | ||
| @Nullable ServerPort originalServerPort) { | ||
| // Try to resolve the localAddress if not resolved yet. | ||
| if (requireNonNull(localAddress, "localAddress").isUnresolved()) { | ||
| try { | ||
|
|
@@ -127,6 +144,7 @@ public ServerPort(InetSocketAddress localAddress, Iterable<SessionProtocol> prot | |
| this.localAddress = localAddress; | ||
| this.protocols = checkProtocols(protocols); | ||
| this.portGroup = portGroup; | ||
| this.originalServerPort = originalServerPort; | ||
|
|
||
| if (localAddress instanceof DomainSocketAddress) { | ||
| comparisonStr = ((DomainSocketAddress) localAddress).authority() + '/' + protocols; | ||
|
|
@@ -228,6 +246,46 @@ long portGroup() { | |
| return portGroup; | ||
| } | ||
|
|
||
| /** | ||
| * Returns the actual port this {@link ServerPort} is bound to. | ||
| * If the port was configured as 0 (ephemeral) and has been bound, returns the actual bound port. | ||
| * Otherwise, returns the configured port from {@link #localAddress()}. | ||
| */ | ||
| @UnstableApi | ||
| public int actualPort() { | ||
| final int actualPort = this.actualPort; | ||
| if (actualPort > 0) { | ||
| return actualPort; | ||
| } | ||
| return localAddress.getPort(); | ||
| } | ||
|
|
||
| /** | ||
| * Sets the actual port after binding. | ||
| * This is only allowed when the configured port is 0 (ephemeral) and hasn't been set yet. | ||
| * | ||
| * @param actualPort the actual bound port | ||
| * @throws IllegalArgumentException if actualPort is not positive | ||
| * @throws IllegalStateException if the configured port is not 0 or actualPort was already set | ||
| */ | ||
| void setActualPort(int actualPort) { | ||
| checkArgument(actualPort > 0, "actualPort: %s (expected: > 0)", actualPort); | ||
| checkState(localAddress.getPort() == 0, | ||
| "Cannot set actualPort for non-ephemeral port: %s", localAddress.getPort()); | ||
| checkState(this.actualPort == -1, | ||
| "actualPort is already set to %s", this.actualPort); | ||
| this.actualPort = actualPort; | ||
| } | ||
|
|
||
| /** | ||
| * Returns the original {@link ServerPort} that this port was created from, | ||
| * or {@code null} if this is an original port. | ||
| */ | ||
| @Nullable | ||
| ServerPort originalServerPort() { | ||
|
||
| return originalServerPort; | ||
| } | ||
|
|
||
| @Nullable | ||
| ServerPortMetric serverPortMetric() { | ||
| return serverPortMetric; | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -84,6 +84,8 @@ public final class VirtualHost { | |||||
| private final String hostnamePattern; | ||||||
| private final int port; | ||||||
| @Nullable | ||||||
| private final ServerPort serverPort; | ||||||
| @Nullable | ||||||
| private final SslContext sslContext; | ||||||
| @Nullable | ||||||
| private final TlsProvider tlsProvider; | ||||||
|
|
@@ -112,6 +114,7 @@ public final class VirtualHost { | |||||
| private final Function<RoutingContext, RequestId> requestIdGenerator; | ||||||
|
|
||||||
| VirtualHost(String defaultHostname, String hostnamePattern, int port, | ||||||
| @Nullable ServerPort serverPort, | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This change seems to introduce a compilation error in |
||||||
| @Nullable SslContext sslContext, | ||||||
| @Nullable TlsProvider tlsProvider, | ||||||
| @Nullable TlsEngineType tlsEngineType, | ||||||
|
|
@@ -142,6 +145,7 @@ public final class VirtualHost { | |||||
| this.hostnamePattern = hostnamePattern; | ||||||
| } | ||||||
| this.port = port; | ||||||
| this.serverPort = serverPort; | ||||||
| this.sslContext = sslContext; | ||||||
| this.tlsProvider = tlsProvider; | ||||||
| this.tlsEngineType = tlsEngineType; | ||||||
|
|
@@ -182,7 +186,8 @@ VirtualHost withNewSslContext(SslContext sslContext) { | |||||
| ReferenceCountUtil.release(sslContext); | ||||||
| throw new IllegalStateException("Cannot set a new SslContext when TlsProvider is set."); | ||||||
| } | ||||||
| return new VirtualHost(originalDefaultHostname, originalHostnamePattern, port, sslContext, null, | ||||||
| return new VirtualHost(originalDefaultHostname, originalHostnamePattern, port, serverPort, | ||||||
| sslContext, null, | ||||||
| tlsEngineType, serviceConfigs, fallbackServiceConfig, | ||||||
| RejectedRouteHandler.DISABLED, host -> accessLogger, defaultServiceNaming, | ||||||
| defaultLogName, requestTimeoutMillis, maxRequestLength, verboseResponses, | ||||||
|
|
@@ -309,9 +314,15 @@ void setServerConfig(ServerConfig serverConfig) { | |||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Returns the default hostname of this virtual host. | ||||||
| * Returns the name of the default host. | ||||||
| */ | ||||||
| public String defaultHostname() { | ||||||
| if (serverPort != null && serverPort.localAddress() != null) { | ||||||
|
||||||
| if (serverPort != null && serverPort.localAddress() != null) { | |
| if (serverPort != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for catching this. You're right, localAddress() returns a final field that is initialized with requireNonNull in the constructor, so it can never be null. I'll remove the unnecessary null check.
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit; could this be cached? Depending on the call path, this may be called for every request. Ditto for hostnamePattern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the suggestion! Since actualPort is fixed after server start, caching the computed string would avoid unnecessary allocation on repeated calls. I'll add cached fields and compute them lazily on the first access after the port is bound. 👍👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we defer building domain and port mapping until the actual port is resolved instead of adding a new maping?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point. Deferring
virtualHostAndPortMappinguntil after ports are bound would let us unify the two maps into one. The tradeoff is thatvirtualHostAndPortMappingis currentlyfinaland would need to becomevolatile, and thebuildDomainAndPortMapping()logic would need to incorporate actual port resolution. I'm happy to refactor if you prefer a unified approach.