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

Auth: Decouple client and hook registration #85084

Merged
merged 3 commits into from Apr 4, 2024

Conversation

kalleep
Copy link
Contributor

@kalleep kalleep commented Mar 25, 2024

What is this feature?
Currently all client and hook registrations happens in the service provider factory. This prevent us from injecting this authn into other services that needs it due to circular injections. We decided early on to move the registrations into their respected service but never got around to it.

As a first step I created a new component that depends on authn.Service and all other services. The only think this component does is register clients and hooks and then return an empty struct. I then use the backgroudservice injection hack to make sure this is run on startup.

Which issue(s) does this PR fix?:

Fixes #

Special notes for your reviewer:

Please check that:

  • It works as expected from a user's perspective.
  • If this is a pre-GA feature, it is behind a feature toggle.
  • The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

@kalleep kalleep added the no-changelog Skip including change in changelog/release notes label Mar 25, 2024
@kalleep kalleep added this to the 11.0.x milestone Mar 25, 2024
@kalleep kalleep requested a review from Jguer March 25, 2024 14:52
@kalleep kalleep self-assigned this Mar 25, 2024
@kalleep kalleep requested review from a team as code owners March 25, 2024 14:52
@kalleep kalleep requested review from papagian, zserge and mildwonkey and removed request for a team March 25, 2024 14:52
@ashharrison90 ashharrison90 modified the milestones: 11.0.x, 11.1.x Mar 26, 2024
Copy link
Contributor

@IevaVasiljeva IevaVasiljeva left a comment

Choose a reason for hiding this comment

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

LGTM with a small comment about fixing logging.

pkg/services/authn/authnimpl/registration.go Outdated Show resolved Hide resolved
@kalleep kalleep force-pushed the authn/client-hook-registration-component branch 2 times, most recently from 2224b21 to a6c3fa6 Compare March 27, 2024 14:01
Copy link
Contributor

@Jguer Jguer left a comment

Choose a reason for hiding this comment

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

if we can hold just a bit to merge OBO, it would save me a lengthy rebase

@kalleep
Copy link
Contributor Author

kalleep commented Mar 28, 2024

if we can hold just a bit to merge OBO, it would save me a lengthy rebase

That is why I have not merged this one yet. I already have caused some conflicts for you 🙈

@alexanderzobnin alexanderzobnin changed the title Auth: Decouple client and hook registratiom Auth: Decouple client and hook registration Mar 28, 2024
@alexanderzobnin
Copy link
Contributor

We still have problems here I think. Authn service depends on usagestats.Service and login.AuthInfoService and they are depend on accesscontrol service. Which makes it impossible to inject authn service into access control one.

@kalleep kalleep force-pushed the authn/client-hook-registration-component branch from a6c3fa6 to 7be6270 Compare April 3, 2024 08:38
@kalleep
Copy link
Contributor Author

kalleep commented Apr 3, 2024

@alexanderzobnin I managed to get rid of usage stats as a dependency but the other problem is AuthInfoService depends on SecretService that depends on UsageStats. Maybe the proper fix if to remove accesscontrol.Service as a dependecy in usage stats. The only think it does is register a fixed role and we can probably just move that declaration and registration into access control package instead, WDYT?

@kalleep kalleep force-pushed the authn/client-hook-registration-component branch from 0b7ae57 to 7be6270 Compare April 3, 2024 09:14
@kalleep kalleep force-pushed the authn/client-hook-registration-component branch from 952dda2 to 66ce6a4 Compare April 4, 2024 07:05
@kalleep kalleep merged commit 504870f into main Apr 4, 2024
11 checks passed
@kalleep kalleep deleted the authn/client-hook-registration-component branch April 4, 2024 07:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/backend no-changelog Skip including change in changelog/release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants