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

feat: LSDV-4972: Optimize the requests for presigning urls #4106

Merged
merged 10 commits into from
May 5, 2023

Conversation

bmartel
Copy link
Contributor

@bmartel bmartel commented Apr 27, 2023

PR fulfills these requirements

  • Commit message(s) and PR title follows the format [fix|feat|ci|chore|doc]: TICKET-ID: Short description of change made ex. fix: DEV-XXXX: Removed inconsistent code usage causing intermittent errors
  • Tests for the changes have been added/updated (for bug fixes/features)
  • Docs have been added/updated (for bug fixes/features)
  • Best efforts were made to ensure docs/code are concise and coherent (checked for spelling/grammatical errors, commented out code, debug logs etc.)
  • Self-reviewed and ran all changes on a local instance (for bug fixes/features)

Change has impacts in these area(s)

(check all that apply)

  • Product design
  • Backend (Database)
  • Backend (API)
  • Frontend

Describe the reason for change

The introduction of the presigned url proxy approach allows for consistent, secure and transparent handling of all task data so the client can safely use the urls embedded in the JSON response without it expiring. This works great for most of the requests made from the application, except for image requests. Image requests are not able to properly cache the response and therefore will always re-request the presigned url endpoint which burdens the server.

What does this fix?

Here we are intercepting the requests made to presign, with a service worker, and cache only the redirect response (not the final response body!) so that the browser can effectively use its own heuristics on the resources presigned url, caching the actual response either in disk/memory cache. This reduces the amounts of requests that hit the server by an order of magnitude, while ensuring the requests are still valid and not being made to an expired presigned url. Because of the way in which Media elements request data, we are skipping Range requests here as there is no need to cache these redirects since it will be making the subsequent requests to the presigned url directly, and not the internal proxy redirect url, and has handling already for recovering from and refreshing presigned urls through the proxy.

What libraries were added/updated?

None.

Does this change affect performance?

Improves performance by removing a pile of redundant requests and making the natural browser cache effective once more.

Does this change affect security?

This is more secure as the only aspect of which is cached is the presigned url which the request would be redirected to, after it is checked fully against the user on the server. This means the url which is user facing is always valid, and never stale, but remains secure at all times.

What alternative approaches were there?

  • Cache the signing in a server side cache. This is non optimal as the cache hits for this application will almost never be cross-user, and would be harder to implement and maintain. Also the effectiveness of this cache is far reduced, as we would still have to check the task/user and project storage anyways for permissions. This is all not necessary when caching the redirect url client-side.

What feature flags were used to cover this change?

  • fflag_fix_all_lsdv_4711_cors_errors_accessing_task_data_short

Does this PR introduce a breaking change?

(check only one)

  • Yes, and covered entirely by feature flag(s)
  • Yes, and covered partially by feature flag(s)
  • No
  • Not sure (briefly explain the situation below)

What level of testing was included in the change?

(check all that apply)

  • e2e
  • integration
  • unit

Which logical domain(s) does this change affect?

Presigned Urls.

@netlify
Copy link

netlify bot commented Apr 27, 2023

Deploy Preview for label-studio-docs-new-theme canceled.

Name Link
🔨 Latest commit ea65e24
🔍 Latest deploy log https://app.netlify.com/sites/label-studio-docs-new-theme/deploys/64550d5b823bf100087ddffd

@github-actions github-actions bot added the feat label Apr 27, 2023
@netlify
Copy link

netlify bot commented Apr 27, 2023

Deploy Preview for heartex-docs canceled.

Name Link
🔨 Latest commit ea65e24
🔍 Latest deploy log https://app.netlify.com/sites/heartex-docs/deploys/64550d5b11b13b00087c8e13

label_studio/core/static/js/sw.js Outdated Show resolved Hide resolved
@bmartel bmartel marked this pull request as draft May 5, 2023 14:28
@bmartel bmartel marked this pull request as ready for review May 5, 2023 14:28
@bmartel bmartel merged commit 6b4bc44 into develop May 5, 2023
28 of 38 checks passed
@makseq makseq deleted the fb-lsdv-4972 branch May 12, 2023 00:05
shayantabatabaee pushed a commit to shayantabatabaee/label-studio that referenced this pull request Sep 19, 2023
…al#4106)

* feat: LSDV-4972: Optimize the requests for presigning urls

* cache the redirect responses only, and set to cleanup the cache periodically to ensure the storage is not flooded

* leave the sw-inert empty in the fetch, so network is used entirely untouched

* linting

* ci: Build frontend

* update sw comments and name of inert to fallback

* inlining Date.now to the expiration check

* ci: Build frontend

---------

Co-authored-by: robot-ci-heartex <robot-ci-heartex@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants