fix: simplify debug handling and remove the 0.0.0.0 default #43574
Conversation
closes: keycloak#43160 Signed-off-by: Steve Hawkins <[email protected]>
|
@Pepo48 I'm not quite getting the windows changes right. Initially I'm just checking for debug with the next token starting with a digit or [, then later checking if it looks like it needs a port. |
| DEBUG_MODE="${KC_DEBUG:-${DEBUG:-false}}" | ||
| DEBUG_PORT="${KC_DEBUG_PORT:-${DEBUG_PORT:-8787}}" | ||
| DEBUG_MODE="${KC_DEBUG_MODE:-${DEBUG:-false}}" | ||
| DEBUG_ADDRESS="${KC_DEBUG_PORT:-${DEBUG_PORT:-8787}}" |
There was a problem hiding this comment.
I wonder if we should really rely on the default behaviour here with the address. Maybe we could add the explicit 127.0.0.1:8787 as the default instead of just the port?
There was a problem hiding this comment.
I think we can trust the JRE behavior.
There was a problem hiding this comment.
Do you see any downsides if we state the address explicitly? Feels safer to me.
There was a problem hiding this comment.
From what we have documented there wouldn't be a downside to specify localhost:8787. From a jdwp perspective there could eventually be other ways to affect the default address, which this would prevent.
I'm now thinking that adding the handling for specifying the address only, without the port, was a mistake as it will futher deviates from the jdwp handling and adds a little more complexity to our scripts. If there are no objections, I'll remove that with this pr as well.
There was a problem hiding this comment.
I'm wondering if we should even rely so much on this kind of debug logic. JAVA_OPTS, JAVA_OPTS_APPEND are perfectly capable of accomodating the options.
https://docs.oracle.com/en/java/javase/21/docs/specs/jpda/conninv.html - it looks like users may want to control virtual thread listing, the allow list if using a publically accessible address, etc. Maybe we should just update the doc to provide an example of using an environment variable.
There is probably even a case to be made for handling -X arguments in the same way we are handling -D.
@keycloak/cloud-native WDYT?
There was a problem hiding this comment.
@vmuzikar are you ok with leaving localhost off, or do you want it on? I can add that, but it will need similar handling as to what was removed for supporting ip only - that is check for a number only after parsing so that it's applied to both the env and the cli cases.
Co-authored-by: Václav Muzikář <[email protected]> Signed-off-by: Steven Hawkins <[email protected]>
Signed-off-by: Steve Hawkins <[email protected]>
Pepo48
left a comment
There was a problem hiding this comment.
@shawkins I tried to simplify quite complex IP/port detection logic (hopefully I am not simplifying it too much. I added a short explanation underneath each suggestion. Let me know if everything does make sense and if it really does what you wanted to achieve.
I already did some tests, but very brief ones, I can test it thoroughly tomorrow, based on feedback from you.
Thanks for the PR, I think the provided changes are step into the right direction!
Co-authored-by: Peter Zaoral <[email protected]> Signed-off-by: Steven Hawkins <[email protected]>
|
Thank you for the help @Pepo48, this looks ready to go now. |
Pepo48
left a comment
There was a problem hiding this comment.
LGTM. On Windows I specifically tested the default behaviour (nothing defined), then with custom port, full address and with IPv6 format as well. Everything worked as expected, including the precedence. I could connect a debugger successfully in each case.
Thanks, @shawkins!
|
@vmuzikar Do you want to take a look at it? |
…#43574) * fix: simplify debug handling and remove the 0.0.0.0 default closes: keycloak#43160 Signed-off-by: Steve Hawkins <[email protected]> * Update quarkus/dist/src/main/content/bin/kc.sh Co-authored-by: Václav Muzikář <[email protected]> Signed-off-by: Steven Hawkins <[email protected]> * removing the ability to specify just the ip Signed-off-by: Steve Hawkins <[email protected]> * Apply suggestions from code review Co-authored-by: Peter Zaoral <[email protected]> Signed-off-by: Steven Hawkins <[email protected]> --------- Signed-off-by: Steve Hawkins <[email protected]> Signed-off-by: Steven Hawkins <[email protected]> Co-authored-by: Václav Muzikář <[email protected]> Co-authored-by: Peter Zaoral <[email protected]> (cherry picked from commit 9e98f2b)
…#43574) * fix: simplify debug handling and remove the 0.0.0.0 default closes: keycloak#43160 Signed-off-by: Steve Hawkins <[email protected]> * Update quarkus/dist/src/main/content/bin/kc.sh Co-authored-by: Václav Muzikář <[email protected]> Signed-off-by: Steven Hawkins <[email protected]> * removing the ability to specify just the ip Signed-off-by: Steve Hawkins <[email protected]> * Apply suggestions from code review Co-authored-by: Peter Zaoral <[email protected]> Signed-off-by: Steven Hawkins <[email protected]> --------- Signed-off-by: Steve Hawkins <[email protected]> Signed-off-by: Steven Hawkins <[email protected]> Co-authored-by: Václav Muzikář <[email protected]> Co-authored-by: Peter Zaoral <[email protected]>
closes: #43160
Changes:
@Pepo48 can you please validate the windows logic as well?