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

[MM-54232] Implement automatic client registration #38

Merged
merged 1 commit into from
Aug 24, 2023
Merged

Conversation

streamer45
Copy link
Contributor

@streamer45 streamer45 commented Aug 23, 2023

Summary

To make the service play nicely with K8S (or with any other form of load balancing of multiple instances) we added some very hacky logic in mattermost/mattermost-plugin-calls#479 to essentially re-attempt actions due to failed authentication. Not only was this pretty horrible code to implement but also it didn't work particularly well with round-robin load balancers which would distribute each request to a different backend, causing the client to just give up after reaching the maximum failed attempts.

To alleviate these issues and simplify the flow we are adding some functionality to automatically register a client (if missing) upon authentication attempt. This has the huge benefit of removing the need to make separate HTTP requests which could all hit different endpoints. Instead, both registration and authentication are now coupled with whatever action the client is making (e.g. starting a job). This means that a call with valid credentials is guaranteed to succeed, no matter which endpoint it hits.

Related PR

mattermost/mattermost-plugin-calls#501

Ticket Link

https://mattermost.atlassian.net/browse/MM-54232

@streamer45 streamer45 added the 2: Dev Review Requires review by a core committer label Aug 23, 2023
@streamer45 streamer45 requested a review from cpoile August 23, 2023 23:39
@streamer45 streamer45 self-assigned this Aug 23, 2023
@streamer45 streamer45 added this to the v0.3.2 milestone Aug 23, 2023
Copy link
Member

@cpoile cpoile left a comment

Choose a reason for hiding this comment

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

This kind of makes me wonder if there's any point to registering/authentication if self registration is allowed. :)

@cpoile cpoile added 3: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core committer labels Aug 24, 2023
@streamer45
Copy link
Contributor Author

This kind of makes me wonder if there's any point to registering/authentication if self registration is allowed. :)

I think there still is some value in a multi-tenant scenario since you'd like to know which client is authenticating but for self hosted not that much.

@streamer45 streamer45 merged commit b814f1a into master Aug 24, 2023
3 checks passed
@streamer45 streamer45 deleted the MM-54232 branch August 24, 2023 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3: Reviews Complete All reviewers have approved the pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants