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

chore: added proposed solution of for #281 #297

Conversation

andreacavagna01
Copy link
Contributor

Changelog

changed getSession method for AWS Single Sign-On service for resolve #281

Bugfixes

Added solution in order to wait a specific time after getting more Session requests, to prevent TooManyRequest error from AWS

Notes

Still need to discuss with @ericvilla the best solution to solve this.
Need a way to add the wait time and number of integrations not as a magic-string but as parameters.

Thanks to @peteawood for the solution

@andreacavagna01 andreacavagna01 linked an issue Jul 1, 2022 that may be closed by this pull request
@andreacavagna01 andreacavagna01 requested a review from ericvilla July 1, 2022 10:29
@marcovanetti
Copy link
Contributor

AWS SSO has a throttle limit of 20 TPS (Transactions Per Second).

https://docs.aws.amazon.com/singlesignon/latest/userguide/limits.html

the method

  private async getSessions(integrationId: string, accessToken: string, region: string): Promise<SsoRoleSession[]> {
    const accounts: AccountInfo[] = await this.listAccounts(accessToken, region);

    const promiseArray: Promise<SsoRoleSession[]>[] = [];

    accounts.forEach((account) => {
      promiseArray.push(this.getSessionsFromAccount(integrationId, account, accessToken, region));
    });

    return new Promise((resolve, _) => {
      Promise.all(promiseArray).then((sessionMatrix: SsoRoleSession[][]) => {
        resolve(sessionMatrix.flat());
      });
    });
  }

execute a Promise.all that is calling the SSO endpoint listAccountRoles in parallel.

The endpoint request is called with this request

const listAccountRolesRequest: ListAccountRolesRequest = {
  accountId: accountInfo.accountId,
  accessToken,
  maxResults: 30, // TODO: find a proper value
};

so each call to the endpoint is reiterated a number of times equal to accountRoles.length / 30.

Since listAccountRoles is called in parallel, the final number of times that the endpoint is queried is near accounts.length * (accountRoles.length / 30) that can become very high with a high number of accounts or a a high number of roles.

Looking at the code in the pull request, getSessionsFromAccount (that is calling the SSO endpoints) is executed every 5 seconds and at the end all the results are collected by the Promise.all. The solution is probably working and way better than the previous code, but it is neither optimal nor gives any guarantee of not exceeding the TPS threshold (e.g. with a high number of roles).

Possible solutions IMHO:

  • Increasing or setting to undefined the maxResults in the ListAccountRolesRequest can limit the number of requests executed due to the pagination.

  • Wrapping the ssoPortal.listAccountRoles call inside a queue that implements an ad-hoc throttling using the limit reported the AWS docs (20 TPS). When we are exceeding this limit, the next calls remain in the queue without being called, until the first calls terminate. If we keep this queue async, other external methods remain untouched.

  • Each request (inside the queue logic) should be however included in an exponential backoff mechanism, in order to avoid problems with multiple Leapp apps running behind a NAT with the same IP address.

I hope that these solutions can reach, without surpassing, the maximum allowed TPS limit and so maximize the speed of the sync process.

ericvilla and others added 3 commits July 13, 2022 14:30
…yrequestsexception

* master: (164 commits)
  chore: fixed latest changelog
  chore(release): release desktop app v0.13.0
  chore(release): release desktop app v0.13.0
  chore(release): release cli v0.1.14
  chore(release): release cli v0.1.14
  chore: updated cli version and documentation
  chore(release): release core v0.1.114
  test: fixed azure-persistence-service os compatibility
  test: fixed azure-presistence-service tests on windows os
  chore(release): release core v0.1.114
  fix: desktop app package.json dependencies versions
  docs: reviewd and corrected some typos in the docs
  fix: _workspaceVersion is a number and not a string
  fix: introduced a new independent workspaceVersion
  feat: reintroduced, refactored and partially tested retro compatibility service
  chore: removed delete button for azure in command session
  chore: added linux build test pipeline
  chore: test windows online pipeline
  chore: bump core version for local testing
  chore: added new azure integration demo video
  ...

# Conflicts:
#	packages/core/src/services/integration/aws-sso-integration-service.ts
@marcovanetti
Copy link
Contributor

marcovanetti commented Jul 14, 2022

We created a ThrottleService with the aim to delay each request to a minimum time of 1/TPS.
Attached screenshots show the differences between the old implementation and the new one with the ThrottleService.
For the moment TPS was se to 20 in the constants. In case of further errors or more users using Leapp behind a NAT, the constant can be reduced and an exponential backoff retry system can be implemented.
Before
before

After
after

@marcovanetti marcovanetti merged commit 764b291 into master Jul 14, 2022
@ericvilla ericvilla deleted the 281-aws-sso-synclogin-receiving-429-toomanyrequestsexception branch August 5, 2022 10:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AWS SSO Sync/Login receiving 429 - TooManyRequestsException
3 participants