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.

Sign up for GitHub

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

Jump to bottom

Add Lazy cache for originalhost #1766

Merged
merged 3 commits into from
Apr 30, 2024
Merged

Add Lazy cache for originalhost #1766

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9510bbe
Lazy cache originalhost
Apr 18, 2024
811e370
Update toString function
Apr 19, 2024
542b81b
Update unittests
Apr 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions zuul-core/src/main/java/com/netflix/zuul/message/http/HttpRequestMessageImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,7 @@ public class HttpRequestMessageImpl implements HttpRequestMessage {
private String reconstructedUri = null;
private String pathAndQuery = null;
private String infoForLogging = null;
private String originalHost = null;

private static final SocketAddress UNDEFINED_CLIENT_DEST_ADDRESS = new SocketAddress() {
@Override
Expand Down Expand Up @@ -510,7 +511,10 @@ protected String generateInfoForLogging() {
@Override
public String getOriginalHost() {
try {
return getOriginalHost(getHeaders(), getServerName());
if (originalHost == null) {
originalHost = getOriginalHost(getHeaders(), getServerName());
Copy link
Collaborator

Choose a reason for hiding this comment

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

The compute intensive part here should be the parseHostHeader logic.
Consider the option to just cache that part and not the X-Forwarded-Host part.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

X-Forwarded-Host part is called everytime before the parseHostHeader and it loops on all existing headers to check for the x-forwarded-host using headers.getFirst before attempting to call the parseHostHeader. Doesn't sound efficient. I am tempted to cache that as well.

Copy link
Collaborator

Choose a reason for hiding this comment

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

hmm, it does iterate through the list, ugh.

}
return originalHost;
} catch (URISyntaxException e) {
throw new IllegalArgumentException(e);
}
Expand Down Expand Up @@ -662,7 +666,7 @@ protected String _reconstructURI() {

String scheme = getOriginalScheme().toLowerCase();
uri.append(scheme);
uri.append(URI_SCHEME_SEP).append(getOriginalHost(getHeaders(), getServerName()));
uri.append(URI_SCHEME_SEP).append(getOriginalHost());

int port = getOriginalPort();
if ((URI_SCHEME_HTTP.equals(scheme) && 80 == port) || (URI_SCHEME_HTTPS.equals(scheme) && 443 == port)) {
Expand All @@ -674,7 +678,7 @@ protected String _reconstructURI() {
uri.append(getPathAndQuery());

return uri.toString();
} catch (URISyntaxException e) {
} catch (IllegalArgumentException e) {
// This is not really so bad, just debug log it and move on.
LOG.debug("Error reconstructing request URI!", e);
return "";
Expand All @@ -700,7 +704,8 @@ public String toString() {
+ inboundRequest + ", parsedCookies="
+ parsedCookies + ", reconstructedUri='"
+ reconstructedUri + '\'' + ", pathAndQuery='"
+ pathAndQuery + '\'' + ", infoForLogging='"
+ pathAndQuery + '\'' + "/ originalHost='"
+ originalHost + '\'' + ", infoForLogging='"
+ infoForLogging + '\'' + '}';
}
}
28 changes: 28 additions & 0 deletions zuul-core/src/test/java/com/netflix/zuul/message/http/HttpRequestMessageImplTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,6 +305,34 @@ void testPathAndQuery_immutable() {
assertEquals("/blah", request.getPathAndQuery());
}

@Test
void testGetOriginalHost_immutable() {
HttpQueryParams queryParams = new HttpQueryParams();
Headers headers = new Headers();
headers.add("Host", "blah.netflix.com");
request = new HttpRequestMessageImpl(
new SessionContext(),
"HTTP/1.1",
"POST",
"/some/where",
queryParams,
headers,
"192.168.0.2",
"https",
7002,
"localhost",
new SocketAddress() {},
true);

// Check it's the same value 2nd time.
assertEquals("blah.netflix.com", request.getOriginalHost());
assertEquals("blah.netflix.com", request.getOriginalHost());

// Update the Host header value and ensure the result didn't change.
headers.set("Host", "testOriginalHost2");
assertEquals("blah.netflix.com", request.getOriginalHost());
}

@Test
void testGetOriginalHost() {
HttpQueryParams queryParams = new HttpQueryParams();
Expand Down
Loading