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.

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

Suggest setting quarkus.security.ldap.cache.enabled #46688

Merged
merged 2 commits into from
Mar 14, 2025
Merged

Conversation

t1
Copy link
Contributor

@t1 t1 commented Mar 10, 2025

As discussed in #46507

This comment has been minimized.

Copy link

github-actions bot commented Mar 10, 2025

🙈 The PR is closed and the preview is expired.

<3> The URL used by our test resource. Tests may leverage `LdapServerTestResource` provided by Quarkus as link:{quickstarts-blob-url}/security-ldap-quickstart/src/test/java/org/acme/elytron/security/ldap/ElytronLdapExtensionTestResources.java[we do] in the test coverage of the example application.
<2> The URL used by our test resource. Tests may leverage `LdapServerTestResource` provided by Quarkus as link:{quickstarts-blob-url}/security-ldap-quickstart/src/test/java/org/acme/elytron/security/ldap/ElytronLdapExtensionTestResources.java[we do] in the test coverage of the example application.
<3> `{0}` is substituted by the `uid`.
<4> Without this configuration, every request to your service will cause an additional roundtrip to the LDAP server. Therefore, it's a common practice to cache these result to improve performance, but the tradeoff is that there will be a delay between changes in the LDAP getting effective in your service. The default cache max-age is `60s`.
Copy link
Member

Choose a reason for hiding this comment

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

Thanks @t1, this information will be useful.
However, I'd like to propose to turn into a Note, that would inform users that by default, LDAP request is made each time, and you can set this property if you'd like to optimize, etc

The reason I propose it is because with this current text we essentially recommend enabling the cache, in which case it would be reasonable for users to ask, why do we have to configure it ourselves?

The note, with pros and cons, would make it more obvious why users have to make this decision

@sberyozkin
Copy link
Member

@t1 @michalvavrik

By the way, how is this cache managed ?

@michalvavrik
Copy link
Member

@t1 @michalvavrik

By the way, how is this cache managed ?

Elytron does that, not us. On the Quarkus side, the feautre is about creating cache security realm, see 4dc83f9#diff-536222a00f1e17a090ff0327620053fa1db46827f4491350fa42809c80243a67 for details.

@sberyozkin
Copy link
Member

Thanks @michalvavrik, I'm assuming it is memory based by default.
I believe the proposed note should link to Elytron docs for users to know what options are available

Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

I took a stab at it.

I didn't link to the Elytron doc as all the properties are exposed from Quarkus, you can't tweak it further than what Quarkus exposes.

I made it a TIP and added more information.

Copy link

quarkus-bot bot commented Mar 14, 2025

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit 0f6940a.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

@gsmet gsmet merged commit 3afb8be into quarkusio:main Mar 14, 2025
5 checks passed
@gsmet
Copy link
Member

gsmet commented Mar 14, 2025

Thanks for going the extra mile and creating the PR!

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.

4 participants