@vmuzikar - This change adds a new "tools" command. I consider the naming well-picked, and I'm overall happy with the change.

If you agree with the naming of the command, I'll review the rest of the pull request will then finally approve it. Thanks!

On startup and reload, the server automatically picks up the .bloom file when present, falling back to plaintext if the file is missing or corrupt.

Does the server need to have some understanding of what denylist state corresponds to a given .bloom state? That is should it continue to use the .bloom file even if the .text file has changed?

To prevent this ambiguity, should the changes here require the user to instead configure the provider to use the .bloom file, rather than the .text file? Then simply fail if the .bloom file does not exist?

Change detection watches the .bloom file when present, so the server reloads correctly when the denylist is updated and re-precomputed.

It's probably out-of-scope for this pr / issue, but I'm wondering If not present, and if we have a good way to ensure that the .bloom file is up-to-date, should the server be creating a .bloom file in our temp directory for reuse?

New kc.sh tools build-password-denylist command generates a .bloom file alongside the plaintext denylist.

This is straight-forward with a single node, but for use in a cluster are we going to show or recommend using a shared volume / job to do this?

On startup and reload, the server automatically picks up the .bloom file when present, falling back to plaintext if the file is missing or corrupt.

Does the server need to have some understanding of what denylist state corresponds to a given .bloom state? That is should it continue to use the .bloom file even if the .text file has changed?

To prevent this ambiguity, should the changes here require the user to instead configure the provider to use the .bloom file, rather than the .text file? Then simply fail if the .bloom file does not exist?

Change detection watches the .bloom file when present, so the server reloads correctly when the denylist is updated and re-precomputed.

It's probably out-of-scope for this pr / issue, but I'm wondering If not present, and if we have a good way to ensure that the .bloom file is up-to-date, should the server be creating a .bloom file in our temp directory for reuse?

New kc.sh tools build-password-denylist command generates a .bloom file alongside the plaintext denylist.

This is straight-forward with a single node, but for use in a cluster are we going to show or recommend using a shared volume / job to do this?

Thanks for the review @shawkins! Let me know if the below points for your three questions above makes sense.

1. Stale .bloom detection
   Good catch - I can add a warning log at load time when the .txt is newer than the .bloom, so users are alerted to regenerate it. Would that be an agreeable solution?

2. Auto-detect vs explicit config
   I intentionally kept the auto-detect approach to avoid a breaking change, existing users need zero reconfiguration. The .bloom file is purely an opt-in performance optimization. Users who don't generate one are unaffected. Requiring explicit config would expose this complexity to everyone, even those with small denylists who never need it. WDYT?

3. Clustering
   The .bloom file follows the exact same deployment model as the existing plaintext file, there's no cluster-specific handling today either. Each node independently loads from its configured path. If operators already have a shared volume or image-baked file for the .txt, they simply do the same for .bloom. No new clustering complexity is introduced. Correct me if I'm wrong!

1. Seems reasonable given your answer to the second point.

2. I was thinking along the lines of a boolean option that would instruct the provider to expect .bloom files instead. But if you want to be able to support mixed scenarios, where some denylists have .bloom files while others are just the .txt files, then that doesn't work.

3. You are correct that this is not really a new concern - let's see if this could be a production readiness team follow-up Ideally we'd address keycloak operator usage as well. Although without proper mount support we may not want to show how this yet. cc @ahus1 @vmuzikar

Would it be simpler if we just let the user configure any filename? If the file ends with .bloom, we load it as a serialized bloom filter. Otherwise, we read it as a text file. That way the user has no doubt about what file ended up being loaded, or monitored for updates. Realm admins don't need to look into Keycloak logs for warnings about discrepancies, and there's no need to compare modification dates.

It starts to feel to me that creating the association around basename (denylist.{txt,bloom}) just brings complications with no big gains over treating all files as independent and letting the user load two different file formats.

Would it be simpler if we just let the user configure any filename? If the file ends with .bloom, we load it as a serialized bloom filter. Otherwise, we read it as a text file. That way the user has no doubt about what file ended up being loaded, or monitored for updates. Realm admins don't need to look into Keycloak logs for warnings about discrepancies, and there's no need to compare modification dates.

It starts to feel to me that creating the association around basename (denylist.{txt,bloom}) just brings complications with no big gains over treating all files as independent and letting the user load two different file formats.

@shawkins : This idea looks more straightforward and simpler to me.. WDYT?

@shawkins : This idea looks more straightforward and simpler to me.. WDYT?

It's good with me, but it seemed like a requirement here was not to require realm level changes.

@shawkins : This idea looks more straightforward and simpler to me.. WDYT?

It's good with me, but it seemed like a requirement here was not to require realm level changes.

@shawkins the explicit file type approach matches my requirement perfectly, I was looking for flexibility to opt in, and that's exactly what this gives. Admins who need the performance benefit simply configure denylist.bloom, others continue with denylist.txt as before. If you are okay with the approach, I can update the PR accordingly. It feels like being explicit about what is needed, avoids a lot of confusions. People who continue to use the current approach anyways do not need any realm level changes. But yes, others need to specify the filename, but isn't that better than the ambiguity?

If you are okay with the approach, I can update the PR accordingly

No objection from me, I'd prefer to be explicit.

If you are okay with the approach, I can update the PR accordingly

No objection from me, I'd prefer to be explicit.

@shawkins @tsaarni : I have made the changes as we have discussed so far, let me know if this looks okay now! -- dropped the basename association so .bloom and plaintext are fully independent files (server checks extension only, no cross-file watching), and added -o/--output to build-password-denylist CLI for a custom output path and bloom file name.

The External links check has been continuously failing whenever it is run, and the error is unrelated to the current changes. I pushed this separate PR #48682 to add the problematic link to ignored links. @shawkins in case that makes sense, feel free to review

@kfaseela Thank you for the contribution.

From the CLI perspective, this looks good.

As for the container deployments concerns, this is also good as is from my perspective. We're not introducing any new concept here, deny list file was there even before, this just adds a new format. Any enhancements for container/operator use cases would be a follow-up.

+1 also for requiring the users to explicitly set the .bloom file in the Realm config.

@kfaseela Thank you for the contribution.

From the CLI perspective, this looks good.

As for the container deployments concerns, this is also good as is from my perspective. We're not introducing any new concept here, deny list file was there even before, this just adds a new format. Any enhancements for container/operator use cases would be a follow-up.

+1 also for requiring the users to explicitly set the .bloom file in the Realm config.

Thanks @vmuzikar ! Good to know that you are in agreement with the current implementation. Would wait for @shawkins and @ahus1 review now!