label-studio icon indicating copy to clipboard operation
label-studio copied to clipboard

fix: LEAP-1404: Support multiple import storages per provider

Open jpantzlaff opened this issue 1 year ago • 4 comments

PR fulfills these requirements

  • [x] 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
  • [x] Tests for the changes have been added/updated (for bug fixes/features)
  • [ ] Docs have been added/updated (for bug fixes/features)
  • [x] Best efforts were made to ensure docs/code are concise and coherent (checked for spelling/grammatical errors, commented out code, debug logs etc.)
  • [x] 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)
  • [x] Backend (API)
  • [ ] Frontend

Describe the reason for change

What does this fix?

Within a single project, if more than one import storage (i.e. bucket) is added for the same cloud provider (e.g. S3 or GCS), authentication errors can occur when requesting signed URLs.

What is the new behavior?

The bucket and prefix of the imported object are now considered when choosing which import storage to use for signing.

What is the current behavior?

The first import storage added is used for all presigning requests from that provider. In other words, bucket and prefix settings are not honored.

What libraries were added/updated?

None

Does this change affect performance?

No

Does this change affect security?

No

What alternative approaches were there?

Not applicable

What feature flags were used to cover this change?

Not applicable

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)
  • [x] 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?

(for bug fixes/features, be as precise as possible. ex. Authentication, Annotation History, Review Stream etc.)

jpantzlaff avatar Aug 16 '24 21:08 jpantzlaff

Deploy request for label-studio-docs-new-theme pending review.

Visit the deploys page to approve it

Name Link
Latest commit a471d495680a370589801fc9276840bf13839af9

netlify[bot] avatar Aug 16 '24 21:08 netlify[bot]

Deploy request for heartex-docs pending review.

Visit the deploys page to approve it

Name Link
Latest commit a471d495680a370589801fc9276840bf13839af9

netlify[bot] avatar Aug 16 '24 21:08 netlify[bot]

@jpantzlaff Thank you for this great PR! Is it possible to add tests there?

makseq avatar Aug 26 '24 16:08 makseq

@makseq Sorry for the delay, I got pulled into other work. Should be ready to review now. I had to update quite a few existing test assertions, just to make the bucket/container names match the existing mocks.

jpantzlaff avatar Sep 06 '24 19:09 jpantzlaff