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

Add PreSignURL method #255

Open
wants to merge 33 commits into
base: master
Choose a base branch
from
Open

Add PreSignURL method #255

wants to merge 33 commits into from

Conversation

EngHabu
Copy link
Contributor

@EngHabu EngHabu commented Mar 25, 2022

Add PreSignURL API on container interface and implement it for S3, Azure and Google.
fixes #213

Signed-off-by: Haytham Abuelfutuh <[email protected]>
Signed-off-by: Haytham Abuelfutuh <[email protected]>
Signed-off-by: Haytham Abuelfutuh <[email protected]>
Signed-off-by: Haytham Abuelfutuh <[email protected]>
Signed-off-by: Haytham Abuelfutuh <[email protected]>
@EngHabu EngHabu marked this pull request as ready for review March 25, 2022 22:52
@EngHabu EngHabu mentioned this pull request Mar 25, 2022
EngHabu and others added 22 commits March 25, 2022 21:08
Signed-off-by: Haytham Abuelfutuh <[email protected]>
Rename module to flyteorg/stow
Support ContentMD5 as an optional param
Signed-off-by: Sean Lin <[email protected]>
Signed-off-by: Haytham Abuelfutuh <[email protected]>
Signed-off-by: Haytham Abuelfutuh <[email protected]>
combine write and update to one request
Signed-off-by: Gopal K. Vashishtha <[email protected]>
Signed-off-by: Gopal K. Vashishtha <[email protected]>
Signed-off-by: Gopal K. Vashishtha <[email protected]>
Signed-off-by: Gopal K. Vashishtha <[email protected]>
Signed-off-by: Gopal K. Vashishtha <[email protected]>
Signed-off-by: Gopal K. Vashishtha <[email protected]>
Signed-off-by: Gopal K. Vashishtha <[email protected]>
* adding baseUrl to get signed URl

Signed-off-by: Gopal K. Vashishtha <[email protected]>

* default correctly

Signed-off-by: Gopal K. Vashishtha <[email protected]>

* adding base url in Containers

Signed-off-by: Gopal K. Vashishtha <[email protected]>

* renaming

Signed-off-by: Gopal K. Vashishtha <[email protected]>

* Revert "renaming"

This reverts commit 05c7cb3.

Signed-off-by: Gopal K. Vashishtha <[email protected]>

* renaming to non-exported function

Signed-off-by: Gopal K. Vashishtha <[email protected]>

---------

Signed-off-by: Gopal K. Vashishtha <[email protected]>
* A refactor of the `azure` module to support authenticating via either Azure AD or shared keys. This required a larger-than-expected series of changes because the Azure SDK now very different than the one stow used before. This is an early commit to get changes out there for discussion. Included here is:

- An upgrade of the Azure SDK used for the project (needed to support AD auth). This, unfortunately also brought in a whole series if indirect upgrades so testing will be needed.
- Support for authenticating via either Azure AD or Shared keys
- Removal of several azure config values (ConfigAPIVersion, ConfigUseHTTPS) and addition of ConfigUploadConcurrency.
- Removal of the multi-part upload code for Azure, this is now included in the SDK
- Addition of new presigned url tests into the general stow test/ module.
- Addition of a new terraform module to stand up an azure storage account for testing.

Signed-off-by: Terence Kent <[email protected]>

* fix misspellings (doh)

Signed-off-by: Terence Kent <[email protected]>

* Update azure/config.go

Accepting len(var)==0 vs var==""

Co-authored-by: Haytham Abuelfutuh <[email protected]>
Signed-off-by: Terence Kent <[email protected]>

* Update azure/config.go

Accepting improved error message

Co-authored-by: Haytham Abuelfutuh <[email protected]>
Signed-off-by: Terence Kent <[email protected]>

* Changes before moving PR from draft. No impact to tests or functionality at this point:

- Removed all inline context.Background() calls when using Azure SDK methods. Now set an explicit `ctx` variable at the top of each function.
- Added comments around the clock skew code, and moved all clock skews to 15 minutes (the azure-recommended value). Also left a comment explaining the decision.
- Made the use of removed config values an error state for validation. Added a test to ensure this happens.
- Moved some errant logging in the `checkMetadata` test method to only show up in inequality, to keep test output clean.

Signed-off-by: Terence Kent <[email protected]>

* Missed that the previous version of the Azure implementation silently ignored failures due to creating an already-existing container! The code from the old SDK to ignore "already exists" was preserved, but not working with the new SDK.

- Updated the check to work with the new SDK.
-  Added a general test to confirm this behavior.

Note: It's debatable if this should be the default behavior for all stow implementations. However, it seems like whatever the desired behavior is, it should be consistent across all implementations.
Signed-off-by: Terence Kent <[email protected]>

---------

Signed-off-by: Terence Kent <[email protected]>
Co-authored-by: Terence Kent <[email protected]>
Co-authored-by: Haytham Abuelfutuh <[email protected]>
EngHabu and others added 5 commits October 24, 2023 15:31
* Add extra header to signed url

Signed-off-by: Kevin Su <[email protected]>

* make metadata optional

Signed-off-by: Kevin Su <[email protected]>

* nit

Signed-off-by: Kevin Su <[email protected]>

* Add metadata to PresignRequestParams

Signed-off-by: Kevin Su <[email protected]>

* lint

Signed-off-by: Kevin Su <[email protected]>

* lint

Signed-off-by: Kevin Su <[email protected]>

---------

Signed-off-by: Kevin Su <[email protected]>
* Update azure sdk / mod tidy

 - Resolves vuln CVE-2024-35255

Signed-off-by: ddl-ebrown <[email protected]>

* Fix test compile errors

 - These were caught by go vet. How does CI even run?

Signed-off-by: ddl-ebrown <[email protected]>

---------

Signed-off-by: ddl-ebrown <[email protected]>
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.

Support presigned URLs
8 participants