+
Skip to content

Allow host:port syntax for --debug in kc.sh #39924

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

Merged
merged 10 commits into from
Jul 3, 2025
Merged

Conversation

Mirciulica15
Copy link
Contributor

Loosen the --debug argument parsing in kc.sh to accept full host:port addresses (e.g. 0.0.0.0:8787 or *:8787) in addition to plain port numbers. This enables JDWP to bind to all container interfaces when running Keycloak in Docker, without requiring manual JAVA_OPTS overrides or script edits.

Closes #38924

Loosen the --debug argument parsing in kc.sh to accept full host:port
addresses (e.g. 0.0.0.0:8787 or *:8787) in addition to plain port
numbers. This enables JDWP to bind to all container interfaces when
running Keycloak in Docker, without requiring manual JAVA_OPTS overrides
or script edits.

Closes keycloak#38924

Signed-off-by: mircea.talu <talumircea13@gmail.com>
@shawkins
Copy link
Contributor

shawkins commented May 27, 2025

Thank you for the pr @Mirciulica15. I believe this looks good. Can you address:

  • kc.bat handling
  • what about someone using an ipv6 address ::1 - would that require detecting / handling addresses enclosed in [] with an optional port?
  • do you see a good place to add the new support to the documentation?

Signed-off-by: mircea.talu <talumircea13@gmail.com>
@Mirciulica15
Copy link
Contributor Author

@shawkins Added support for IPv6, I will later look for a good place to put this in the docs.

As for the kc.bat script, I'd like to test my changes before committing. kc.sh was relatively easy to test by volume mounting in the container via

podman run -v "$(pwd)/kc.sh:/opt/keycloak/bin/kc.sh:ro" -p 8080:8080 -p 8087:8087 localhost/keycloak-fix-address:local start-dev --debug [::0]:8087

Do you have any advice for testing kc.bat? Thanks!

@shawkins
Copy link
Contributor

Do you have any advice for testing kc.bat? Thanks!

I've locally used wine, but it doesn't always faithfully reproduce. cc @Pepo48 do you have any advise on windows testing?

We can always try to have someone on the keycloak team round out the pr for the kc.bat handling once we're set on the kc.sh logic.

Signed-off-by: mircea.talu <talumircea13@gmail.com>
Signed-off-by: mircea.talu <talumircea13@gmail.com>
…sses

Signed-off-by: mircea.talu <talumircea13@gmail.com>
Signed-off-by: mircea.talu <talumircea13@gmail.com>
@shawkins
Copy link
Contributor

shawkins commented Jun 2, 2025

Thank you for the update. Hopefully updating kc.bat won't be so bad now.

@Pepo48
Copy link
Contributor

Pepo48 commented Jun 2, 2025

@Mirciulica15 cc:@shawkins with regards to testing kc.bat I'm afraid that I can only offer the obvious options - windows bare metal, VM or eventually a container (for non UI test cases).

We typically use mix of all - win-server VMs in our internal pipelines and containers in Github CI for the testsuite execution; and I can certainly test the PR even on my physical machine, as Steve already mentioned.

@Mirciulica15
Copy link
Contributor Author

Mirciulica15 commented Jun 3, 2025

@Pepo48 I managed to get a hold of a Windows machine for testing, but I am facing several issues while trying to implement the regexes in the .bat script. For example, a one-liner like the one below works perfectly when I test it in my terminal, but always fails within the kc.bat script:

echo 127.0.0.1 | findstr /R /X "^[0-9][0-9]*\.[0-9][0-9]*\.[0-9][0-9]*\.[0-9][0-9]*$" >nul && echo validIPv4 || echo notIPv4

I've tried changing the file encoding according to certain threads I have found, but the issue persists. Have you encountered this issue previously?

EDIT: The kc.bat script appears to become increasingly long and complex, especially with these latest changes. Echoing @shawkins's previous concerns about the complexity in the .bat script, do you think that migrating it to a more modern/maintainable alternative could be considered?

@Pepo48
Copy link
Contributor

Pepo48 commented Jun 5, 2025

@Mirciulica15 I guess that by a modern alternative you mean PowerShell, right?

It's not on our radar right now, since there hasn't been a demand for it. The main concern is about maintainability - we don't want to maintain two separate scripts for one platform. On top of that, there might be still some users, who are running their instances within environments, where PowerShell is not/can't be installed.

In other words, guaranteed availability + non-existing demand for a change are the main reasons, why the bat script is still used.
There is probably a higher chance that both scripts will be replaced by a native Quarkus launcher in the future.

Regardless, feel free to open a discussion if you consider it to be important. It may raise some attention from the community and get some serious consideration.

@Pepo48
Copy link
Contributor

Pepo48 commented Jun 5, 2025

@Mirciulica15 regarding the regex, my initial guess it that this may be caused by Windows findstr - it has known issues with the end anchor $ when used with piped input.

A simple demonstration of this limitation would be:
image
If you remove $ sign, it should work.

Unfortunately, in order to validate that nothing comes after the expected pattern I haven't found any other way than just a string manipulation:

@echo off
set "ip=127.0.0.1"
echo %ip% | findstr /R "^[0-9][0-9]*\.[0-9][0-9]*\.[0-9][0-9]*\.[0-9][0-9]*" >nul
if %errorlevel% equ 0 (
    for /f "tokens=1-4 delims=." %%a in ("%ip%") do (
        if "%ip%" equ "%%a.%%b.%%c.%%d" (echo validIPv4) else (echo notIPv4)
    )
) else (
    echo notIPv4
)

(findstr /X means exact match, you should not need it since anchors are used)

Let me know if it solves something for you. Thanks!

@shawkins
Copy link
Contributor

Approved the workflow run for the kc.sh change to validate it's working as expected.

Hopefully kc.bat won't be too burdensome to update.

@mabartos
Copy link
Contributor

mabartos commented Jun 18, 2025

@Mirciulica15 Please fetch the latest changes from the main branch, as it should resolve the Windows issues in the CI. Thanks!

EDIT: Ahh, ok... I was probably able to do that :)

mabartos and others added 3 commits June 18, 2025 15:34
Signed-off-by: mircea.talu <talumircea13@gmail.com>
Signed-off-by: mircea.talu <talumircea13@gmail.com>
@Mirciulica15
Copy link
Contributor Author

Sorry for the delay guys, I've been busy with my final exams these past few weeks.

Thanks for the tips @Pepo48, I worked on the kc.bat script a bit today and implemented the logic, trying to stay as close to kc.sh as possible. I tried the %errorlevel% approach, but I decided to use a different method with boolean operator chaining. I did some local tests and it seems to work, but I will take another look later today. Let me know if this looks like the right path to go down to.

Signed-off-by: mircea.talu <talumircea13@gmail.com>
@Mirciulica15
Copy link
Contributor Author

Thank you for the pr @Mirciulica15. I believe this looks good. Can you address:

  • kc.bat handling
  • what about someone using an ipv6 address ::1 - would that require detecting / handling addresses enclosed in [] with an optional port?
  • do you see a good place to add the new support to the documentation?

@shawkins I have added the new support for the --debug parameter in quarkus/CONTRIBUTING.md, this is the best place where I've seen this parameter detailed. Let me know if it looks ok, thanks!

@shawkins
Copy link
Contributor

@shawkins I have added the new support for the --debug parameter in quarkus/CONTRIBUTING.md, this is the best place where I've seen this parameter detailed. Let me know if it looks ok, thanks!

That looks good to me. Thank you!

@shawkins shawkins self-requested a review June 19, 2025 16:11
@shawkins shawkins requested review from Pepo48, vmuzikar and mabartos June 19, 2025 16:11
@shawkins
Copy link
Contributor

@vmuzikar @Pepo48 @mabartos I don't have a problem with this getting into 26.3, can you have a look?

Copy link
Contributor

@vmuzikar vmuzikar left a comment

Choose a reason for hiding this comment

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

LGTM. @Mirciulica15 Thank you for the contribution!

Copy link
Contributor

Choose a reason for hiding this comment

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

@Pepo48 Can you please check the kc.bat changes?

@Pepo48
Copy link
Contributor

Pepo48 commented Jul 3, 2025

@Mirciulica15 thanks for the contribution! I just rechecked the Windows part - syntactically, everything looks good. Functionality-wise, I was able to run Keycloak in the debug mode with a specific debug address, attach the debugger, and hit the expected breakpoint, so the behavior is as intended.

Therefore, I’m approving the PR. Great job!

cc: @vmuzikar @shawkins

@shawkins shawkins merged commit c315762 into keycloak:main Jul 3, 2025
79 checks passed
@Mirciulica15
Copy link
Contributor Author

Thanks a lot, @shawkins @Pepo48 @vmuzikar @mabartos ! This was my first ever contribution to an open source project, and you made it a very pleasant learning experience!

Looking forward to helping with other things in the future! 🎩

@liamjones
Copy link

As the person who was originally bitten by the issue and reported it, thank you for working on this @Mirciulica15!

@shawkins
Copy link
Contributor

shawkins commented Jul 5, 2025

This was my first ever contribution to an open source project, and you made it a very pleasant learning experience!

Congratulations, you did a great job with feadback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

--debug does not work with docker container version of Keycloak
6 participants
点击 这是indexloc提供的php浏览器服务,不要输入任何密码和下载