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

support swift tempURLs #37

Merged
merged 1 commit into from
May 1, 2024

Conversation

anshrupani
Copy link
Contributor

swift tempURLs are not compatible with the S3 API calls in s3cli.

Therefore, we implement a signed URL provider which determines which provider to delegate the signed URL call to, the s3 provider or the swift provider.

We also implement the swift client to extend the signed URL functionality for swift/openstack compatibility.

@jpalermo jpalermo requested review from a team, selzoc and lnguyen and removed request for a team April 25, 2024 14:53
Comment on lines -117 to +122
signedURL, err := blobstoreClient.Sign(objectID, action, expiration)
signedURL, err := signURLProvider.Sign(objectID, action, expiration)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious why there is a conditional in client.NewSignURLProvider() which can return different clients, rather than having client.S3Blobstore.Sign() use the contents of config.S3Cli (which it already has access to) to conditionally choose the signing method?

What I have in mind would look something like:

func (client *S3Blobstore) Sign(objectID string, action string, expiration time.Duration) (string, error) {
	if client.s3cliConfig.SwiftAuthAccount != "" {
		return client.swiftSign(objectID, action, expiration)
	}

	return client.s3Sign(objectID, action, expiration)
}

func (client *S3Blobstore) s3Sign(objectID string, action string, expiration time.Duration) (string, error) {
	// implementation
}

func (client *S3Blobstore) swiftSign(objectID string, action string, expiration time.Duration) (string, error) {
	// implementation
}

Something like this would add swift signing behavior without adding a different signing-only client, and hides the details behind client.S3Blobstore.Sign().

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would not mix up the S3 client code with Swift specific implementations because I wouldn't expect Swift Code in that client.
So I do prefer making it obvious like the current implementation does. But that's more a question of taste ...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I was confused about the origin of Swift in this case. I thought it was an extension of S3 in some way.

I agree that the Swift code doesn't belong on the client.S3Blobstore struct.

I do think that client.New() should return a generic client which has a an interface that matches that of client.S3Blobstore and has a Sign() implementation that is similar in spirit to my example above but instead calls out to whichever internal entity supports AWS S3, or Swift when Sign() is called.

func (c *GenericS3Blobstore) Sign(objectID string, action string, expiration time.Duration) (string, error) {
	if client.s3cliConfig.SwiftAuthAccount != "" {
		return c.someEntityWhichSupportsSwift.Sign(objectID, action, expiration)
	}

	return c.someEntityWhichSupportsAwsS3.Sign(objectID, action, expiration)
}

The GenericS3Blobstore should export an interface which matches:

type Blobstore interface {
	Put(src io.ReadSeeker, dest string) error
	Delete(dest string) error
	Exists(dest string) (bool, error)
	Sign(objectID string, action string, expiration time.Duration) (string, error)
}

The code in main shouldn't know, or need to know that Sign() can have multiple implementations depending on what config was passed. The result of calling client.New() should be an entity which satisfies the interface above and performs operations like Sign() based on the config is passed to New()

}
}

func (client *SwiftBlobstore) SignedURL(action string, objectID string, expiration time.Duration) (string, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method should be private

Comment on lines -117 to +122
signedURL, err := blobstoreClient.Sign(objectID, action, expiration)
signedURL, err := signURLProvider.Sign(objectID, action, expiration)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would not mix up the S3 client code with Swift specific implementations because I wouldn't expect Swift Code in that client.
So I do prefer making it obvious like the current implementation does. But that's more a question of taste ...

Copy link
Member

@lnguyen lnguyen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@aramprice aramprice merged commit c6fc710 into cloudfoundry:main May 1, 2024
3 of 5 checks passed
aramprice added a commit that referenced this pull request May 2, 2024
max-soe added a commit to max-soe/community that referenced this pull request Jun 20, 2024
# Foundational Infrastructure: VM deployment lifecycle (BOSH) Contributions
### PRs Commented on/Reviewed:
- 2024-05-02T00:33:35Z: [Refactor the `client` package](cloudfoundry/bosh-s3cli#38)
### Issues that may be relevant:
- 2023-04-28T11:54:45Z: [BOSH Agent not reachable due to issues with blobstore settings](cloudfoundry/bosh-aws-cpi-release#149)
- 2023-05-15T09:53:53Z: [Update CONTRIBUTING.md](cloudfoundry/bosh#2443)
- 2023-05-16T08:07:09Z: [support instance tags and include relevant specs](cloudfoundry/bosh#2444)
- 2023-05-24T05:01:42Z: [Prevent gcc version check for non xenial stemcells](cloudfoundry/bosh-azure-cpi-release#685)
- 2023-05-31T17:49:02Z: [Support instance group tags](cloudfoundry/bosh#2448)
- 2023-07-24T14:07:16Z: [Efficient development with the bosh-agent](cloudfoundry/bosh-agent#312)
- 2023-08-14T14:17:13Z: [Enable support for verify-ca SSL mode for cases when hostname verification is not possible/required](cloudfoundry/bosh#2461)
- 2023-08-14T14:20:56Z: [Enable support for verify-ca SSL mode for cases when hostname verification is not possible/required](cloudfoundry/bosh#2462)
- 2023-08-17T11:42:28Z: [Use config properties with hyphens to prevent verification clashes in the agent](cloudfoundry/bosh-azure-storage-cli#9)
- 2023-08-17T11:53:04Z: [Unifying azure storage cli config across director and agent to prevent conflicts](cloudfoundry/bosh#2465)
- 2023-11-14T12:56:35Z: [Truncate downloaded blobs](cloudfoundry/bosh-azure-storage-cli#13)
- 2023-11-16T10:19:43Z: [Adding disaster recovery documentation for bosh in case of AZ outages](cloudfoundry/docs-bosh#815)
- 2023-12-18T14:27:56Z: [set server-side timeout for GET requests](cloudfoundry/bosh-azure-storage-cli#14)
- 2024-02-09T16:27:17Z: [retry CPI calls in case of SSL_read errors as well](cloudfoundry/bosh-azure-cpi-release#689)
- 2024-02-16T12:56:19Z: [fix failing unit tests](cloudfoundry/bosh-azure-cpi-release#690)
- 2024-04-24T12:44:52Z: [Revert ssl read workaround](cloudfoundry/bosh-azure-cpi-release#694)
- 2024-04-24T15:23:13Z: [support swift tempURLs](cloudfoundry/bosh-s3cli#37)
- 2024-05-02T13:58:07Z: [fix swift tempURLs and add unit/integration tests](cloudfoundry/bosh-s3cli#39)
### Code contributions:
- 2022-11-15T15:17:30Z: [Initial client offering Put functionality (cloudfoundry#2)](cloudfoundry/bosh-azure-storage-cli@9deb735)
- 2022-11-16T07:40:28Z: [initial get functionality (cloudfoundry#3)](cloudfoundry/bosh-azure-storage-cli@77df9a7)
- 2022-11-17T15:17:26Z: [initial exists functionality (cloudfoundry#6)](cloudfoundry/bosh-azure-storage-cli@e82b78b)
- 2023-05-23T05:52:41Z: [prevent gcc version check for fast_jsonparser for bionic and jammy stemcells](cloudfoundry/bosh-azure-cpi-release@dfc9402)
- 2023-05-24T05:05:08Z: [prevent gcc version check for fast_jsonparser for bionic and jammy stemcells](cloudfoundry/bosh-azure-cpi-release@dfc9402)
- 2023-07-24T13:57:00Z: [Efficient development with the bosh-agent](cloudfoundry/bosh-agent@3bba88a)
- 2023-07-24T14:09:00Z: [Efficient development with the bosh-agent](cloudfoundry/bosh-agent@3bba88a)
- 2023-08-14T14:06:32Z: [Enable support for verify-full SSL mode for cases when hostname verification is not possible/required](cloudfoundry/bosh@45a062f)
- 2023-08-17T11:39:50Z: [Use config properties with hyphens to prevent verification clashes in the agent](cloudfoundry/bosh-azure-storage-cli@1c46f43)
- 2023-08-17T11:45:44Z: [Unifying azure storage cli config across director and agent to prevent conflicts](cloudfoundry/bosh@d5cc637)
- 2023-11-16T10:12:44Z: [Adding disaster recovery documentation for bosh in case of AZ outages](cloudfoundry/docs-bosh@3427377)
- 2023-12-06T20:07:15Z: [updated docs](cloudfoundry/docs-bosh@7be327e)
- 2023-12-06T20:48:22Z: [updated docs](cloudfoundry/docs-bosh@7be327e)
- 2023-12-18T14:21:43Z: [set server-side timeout for GET requests](cloudfoundry/bosh-azure-storage-cli@759862b)
- 2023-12-18T15:09:12Z: [include comment for reasoning](cloudfoundry/bosh-azure-storage-cli@749bf5a)
- 2023-12-19T09:10:09Z: [add timeout for PUT requests as well](cloudfoundry/bosh-azure-storage-cli@22372ec)
- 2023-12-21T12:49:26Z: [include further changes and comments](cloudfoundry/docs-bosh@95ad588)
- 2023-12-21T14:02:00Z: [include further changes and comments](cloudfoundry/docs-bosh@95ad588)
- 2024-02-09T16:16:21Z: [retry CPI calls in case of SSL_read errors as well](cloudfoundry/bosh-azure-cpi-release@f291148)
- 2024-02-13T12:49:12Z: [fixed typo](cloudfoundry/bosh-azure-cpi-release@d15f44a)
- 2024-02-15T12:27:32Z: [make the changes more readable](cloudfoundry/bosh-azure-cpi-release@f7dd849)
- 2024-02-16T12:54:10Z: [fix failing unit tests](cloudfoundry/bosh-azure-cpi-release@28f4ab3)
- 2024-04-17T14:08:21Z: [support swift tempURLs](cloudfoundry/bosh-s3cli@c6fc710)
- 2024-04-24T12:07:12Z: [Revert "fix failing unit tests"](cloudfoundry/bosh-azure-cpi-release@7a2e45f)
- 2024-04-24T12:07:31Z: [Revert "Remove retry logic"](cloudfoundry/bosh-azure-cpi-release@1a6dad7)
- 2024-04-24T12:39:47Z: [Revert "make the changes more readable"](cloudfoundry/bosh-azure-cpi-release@913ec21)
- 2024-04-24T12:40:07Z: [Revert "Ignore EOF Error during ssl_read"](cloudfoundry/bosh-azure-cpi-release@7e8817c)
- 2024-04-24T12:40:22Z: [Revert "fixed typo"](cloudfoundry/bosh-azure-cpi-release@b3f1c99)
- 2024-04-24T12:40:33Z: [Revert "retry CPI calls in case of SSL_read errors as well"](cloudfoundry/bosh-azure-cpi-release@0b1b2bc)
- 2024-05-02T13:43:27Z: [fix swift tempURLs and add unit/integration tests](cloudfoundry/bosh-s3cli@d3389dd)
- 2024-05-02T14:01:48Z: [fix swift tempURLs and add unit/integration tests](cloudfoundry/bosh-s3cli@d3389dd)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

4 participants