In WSL dev/test mode print localhost instead of 0.0.0.0 in serverListeningMessage #40700
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@gsmet From a new branch because the last PR was from my main branch... |
gsmet
requested changes
May 19, 2024
| @@ -24,7 +24,7 @@ public boolean isDevOrTest() { | |||
| * Returns true if the current launch is the server side of remote dev. | |||
| */ | |||
| public static boolean isRemoteDev() { | |||
| return (current() == DEVELOPMENT) && "true".equals(System.getenv("QUARKUS_LAUNCH_DEVMODE")); | |||
| return (current() == DEVELOPMENT) && System.getenv("QUARKUS_LAUNCH_DEVMODE").equals("true"); | |||
There was a problem hiding this comment.
You shouldn't change this. It looks odd but this is actually the safer pattern as it works even if System.getenv("QUARKUS_LAUNCH_DEVMODE") is null. Your new pattern will fail with a NPE in this case.
You know it once you got bitten by it :).
| * Do not use this during the actual configuration, use options.getHost() there directly instead. | ||
| */ | ||
| private static String getDeveloperFriendlyHostName(HttpServerOptions options) { | ||
| return (isWSL() && LaunchMode.current().isDevOrTest() && options.getHost().equals("0.0.0.0")) ? "localhost" |
There was a problem hiding this comment.
I don't think getHost() can ever return null but I would use the safe pattern here too and have 0.0.0.0 first.
|
Hey @tamaro-skaljic , do you wish to finalize this PR or do you want me to finalize it? |
The previous commit was actually changing the logic for the default case.
gsmet
force-pushed
the
improve-dev-experience-on-wsl
branch
from
1d5003c
to
64bf132
Compare
gsmet
reviewed
Jun 6, 2024
Comment on lines
16
to
36
| if (builder.getDefaultValues().get(QUARKUS_HTTP_HOST) == null) { | ||
| // Sets the default host config value, depending on the launch mode | ||
| if (LaunchMode.isRemoteDev()) { | ||
| // in remote-dev mode we need to listen on all interfaces | ||
| // in remote dev mode, we want to listen on all interfaces | ||
| // to make sure the application is accessible | ||
| builder.withDefaultValue(QUARKUS_HTTP_HOST, ALL_INTERFACES); | ||
| } else if (LaunchMode.current().isDevOrTest()) { | ||
| if (!isWSL()) { | ||
| // in dev mode, we want to listen only on localhost | ||
| // to make sure the app is not accessible from the outside | ||
| builder.withDefaultValue(QUARKUS_HTTP_HOST, LOCALHOST); | ||
| } else { | ||
| // except when using WSL, as otherwise the app wouldn't be accessible from the host | ||
| builder.withDefaultValue(QUARKUS_HTTP_HOST, ALL_INTERFACES); | ||
| } | ||
| } else { | ||
| // In dev-mode we want to only listen on localhost so others on the network cannot connect to the application. | ||
| // However, in WSL this would result in the application not being accessible, | ||
| // so in that case, we launch it on all interfaces. | ||
| builder.withDefaultValue(QUARKUS_HTTP_HOST, | ||
| (LaunchMode.current().isDevOrTest() && !isWSL()) ? "localhost" : ALL_INTERFACES); | ||
| // in all the other cases, we make sure the app is accessible on all the interfaces by default | ||
| builder.withDefaultValue(QUARKUS_HTTP_HOST, ALL_INTERFACES); | ||
| } | ||
| } | ||
| return builder; |
There was a problem hiding this comment.
@dmlloyd could you have a look at this change as it could have some security consequences?
I adjusted it because the previous commit by the OP actually changed the default value and I wanted to make it extra clear (even if a bit more verbose).
The new version should be exactly as what we have in main.
gsmet
approved these changes
Jun 6, 2024
gsmet
added
the
triage/waiting-for-ci
Status for workflow
|
quarkus-bot
bot
removed
the
triage/waiting-for-ci
|
@tamaro-skaljic thanks for raising this and working on a patch! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Like #40497 but with requested changes #40497 (review).