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

Make scheduler configurable on ReactiveAuthenticationManagerAdapter #6852

Merged

Conversation

ttddyy
Copy link
Contributor

@ttddyy ttddyy commented May 9, 2019

Currently, authentication logic will be performed on hardcoded elastic scheduler in ReactiveAuthenticationManagerAdapter.
This needs to be configurable in order to perform the authentication logic in dedicated scheduler.

Also, once authentication is performed, rest of the logic(Controller/Service) also be performed on the published scheduler.
To allow the rest of the logic after authentication to run on different scheduler, adding another optional scheduler to publish on.

This PR is based on the usecase I have described here: reactor/reactor-core#1644 (comment)

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label May 9, 2019
@rwinch
Copy link
Member

rwinch commented May 13, 2019

Thanks for the PR @ttddyy!

Can you please help me better understand what problems you are trying to solve?

@rwinch rwinch added in: core An issue in spring-security-core status: waiting-for-feedback We need additional information before we can continue and removed status: waiting-for-triage An issue we've not yet triaged labels May 13, 2019
@ttddyy
Copy link
Contributor Author

ttddyy commented May 13, 2019

Hi @rwinch

The change has two aspect.

First one is that we want to run the auth logic on controlled executor rather than the elastic one. Currently, it is hardcoded, so I want to make it changeable.

The second point is after auth process has finished, we would like to switch the executor. Currently, since auth logic has switched to the elastic executor, subsequent logic (such as performing controller/service) also continue to be executed on the switched elastic executor.
In my case, after performing auth on the controlled executor, now we want to switch to another executor that is managed differently to perform application logic.
(Switching back may be debatable action to do, according to the discussion on reactor/reactor-core#1644, but having such capability is useful IMO)

@rwinch
Copy link
Member

rwinch commented May 15, 2019

Ok I understand the what, but why are you needing to do this? Specifically, why do you need to switch back? What are you switching back to?

@ttddyy
Copy link
Contributor Author

ttddyy commented May 20, 2019

I would like to publish to another executor that executes application logic. The main reason is to have metrics around the executors that perform auth logic and app logic separately. This way we can get independent metrics for auth and make decision on converting the auth part to full reactive or just use adapter approach.

@rwinch
Copy link
Member

rwinch commented May 21, 2019

Can't you specify the executor for the logic you are executing? Why does this need to be part of Spring Security's code?

@ttddyy
Copy link
Contributor Author

ttddyy commented May 22, 2019

OK, yeah executor for application can be specified outside of spring-security such as another webfilter or so. I'll update the PR to only focus on making auth-manager scheduler modifiable.

@ttddyy ttddyy force-pushed the reactive-auth-adapter-publish-on branch from 44f6761 to 233117f Compare May 22, 2019 00:35
Copy link
Member

@rwinch rwinch left a comment

Choose a reason for hiding this comment

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

Thanks for the fast turnaround! I added one a comment inline.

* Set a scheduler that will be published on to perform the authentication logic.
* @param scheduler a scheduler to be published on
*/
public void setScheduler(Scheduler scheduler) {
Copy link
Member

Choose a reason for hiding this comment

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

Please add a null check and a test to validate the null check

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the PR with null check and test

Currently, authentication logic will be performed on hardcoded elastic
scheduler in ReactiveAuthenticationManagerAdapter.
This commit makes the authentication logic scheduler configureable.
@ttddyy ttddyy force-pushed the reactive-auth-adapter-publish-on branch from 233117f to 6d4f885 Compare May 23, 2019 00:33
@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels May 29, 2019
@ttddyy
Copy link
Contributor Author

ttddyy commented Jun 10, 2019

Hi @rwinch
This PR is ready, can you care to merge?
Thanks,

@rwinch rwinch self-assigned this Jun 12, 2019
@rwinch rwinch added type: enhancement A general enhancement and removed status: feedback-provided Feedback has been provided labels Jun 12, 2019
@rwinch rwinch added this to the 5.2.0.M3 milestone Jun 12, 2019
@rwinch rwinch merged commit 71dc4f3 into spring-projects:master Jun 12, 2019
@rwinch
Copy link
Member

rwinch commented Jun 12, 2019

Thanks for the PR and the ping @ttddyy! This is now merged into master

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core An issue in spring-security-core type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants