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

Refactor SDK's presign URL feature #891

Closed
jasdel opened this issue Nov 11, 2020 · 2 comments · Fixed by #888
Closed

Refactor SDK's presign URL feature #891

jasdel opened this issue Nov 11, 2020 · 2 comments · Fixed by #888
Labels
automation-exempt breaking-change Issue requires a breaking change to remediate. refactor

Comments

@jasdel
Copy link
Contributor

jasdel commented Nov 11, 2020

The v2 SDK should provide an easy way to create a presigned URL for an operation similar to the concept in the v1 SDK. There were a couple issues with the v1 SDK's implementation that the v2 SDK's should avoid.

1.) Presign was available for all API operations regardless if it was actually supported.
2.) Presign was supported for all HTTP methods, even though it was nearly impossible to use non-GET and non-streaming operations.
3.) Presign exposed an Expires behavior for all operations, when this was only valid for Amazon S3 operations.

To address these issues the v2 SDK's presigner design should be generated independently of the API client. And the presigner should be generated with a select subset of supported operations that are known to work, and where it makes sense to have a presigner for.

The work with autofill presign, #799 provided the ground work for generated presign clients for an API. This change also broke out the Expires concept into a conditionally generated option available on the presign clients for APIs that support that behavior.

The PR #888 works to refactor the presign client generator implemented in #799 and export it for for APIs like Amazon S3.

The presign client should be based off of the API client, providing flexable options to initialize it, with optional parameters as needed.

// PresignOptions represents the presign client options
type PresignOptions struct {
	// ClientOptions are list of functional options to mutate client options used by
	// presign client
	ClientOptions []func(*Options)
	// Presigner is the presigner used by the presign url client
	Presigner v4.HTTPPresigner
}

// PresignClient represents the presign url client
type PresignClient struct { /*... */}

// NewPresignClient generates a presign client using provided Client options and
// presign options
func NewPresignClient(options Options, optFns ...func(*PresignOptions)) *PresignClient { /*...*/}

// NewPresignClientWrapper generates a presign client using provided API Client and
// presign options
func NewPresignClientWrapper(c *Client, optFns ...func(*PresignOptions)) *PresignClient {/*...*/}

// NewPresignClientFromConfig generates a presign client using provided AWS config
// and presign options
func NewPresignClientFromConfig(cfg aws.Config, optFns ...func(*PresignOptions)) *PresignClient {/*...*/}

// PresignCopyDBClusterSnapshot will create a AWS sigv4 request presigned URL for the CopyDBClusterSnapshot operation.
func (c *PresignClient) PresignCopyDBClusterSnapshot(ctx context.Context, params *CopyDBClusterSnapshotInput, optFns ...func(*PresignOptions)) (req *v4.PresignedHTTPRequest, err error) {
@jasdel jasdel added refactor feature-request A feature should be added or improved. labels Nov 11, 2020
@jasdel jasdel added this to the v1.0 Release Candidate milestone Nov 11, 2020
@jasdel
Copy link
Contributor Author

jasdel commented Nov 11, 2020

Per #719 the v2 SDK's presigner should ensure that presign URLs created have valid signatures. That issue calls out that v1 and v2 SDK had issue due to query parameter sorting. This should be investigated for the v2 SDK's implementation.

@jasdel jasdel added breaking-change Issue requires a breaking change to remediate. automation-exempt and removed feature-request A feature should be added or improved. labels Nov 11, 2020
@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automation-exempt breaking-change Issue requires a breaking change to remediate. refactor
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant