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 custom s3 api endpoints #677

Merged
merged 7 commits into from Jan 9, 2018

Conversation

Projects
None yet
7 participants
@easel
Contributor

easel commented Dec 30, 2017

Rebased #619 against latest master.

@easel

This comment has been minimized.

Show comment
Hide comment
@easel

easel Dec 30, 2017

Contributor

I've added some documentation and adjusted the s3 integration tests to work against Minio. Aside from this patch, getting minio to pass the tests requires falling back to url encoding the responses from the upload calls.

The regular test suite still passes for me, and unless I'm missing something I don't believe any of the changes should change the code path unless an endpoint url is specified. Happy to address any other feedback.

@takashima0411 @ktoso any thoughts?

Contributor

easel commented Dec 30, 2017

I've added some documentation and adjusted the s3 integration tests to work against Minio. Aside from this patch, getting minio to pass the tests requires falling back to url encoding the responses from the upload calls.

The regular test suite still passes for me, and unless I'm missing something I don't believe any of the changes should change the code path unless an endpoint url is specified. Happy to address any other feedback.

@takashima0411 @ktoso any thoughts?

@ktoso

This comment has been minimized.

Show comment
Hide comment
@ktoso

ktoso Dec 30, 2017

Member

Sorry for the delay, family time back home until New Years :)
Will review then!

Member

ktoso commented Dec 30, 2017

Sorry for the delay, family time back home until New Years :)
Will review then!

@easel

This comment has been minimized.

Show comment
Hide comment
@easel

easel Jan 1, 2018

Contributor

Thanks @ktoso, happy New Year!

Contributor

easel commented Jan 1, 2018

Thanks @ktoso, happy New Year!

@ennru ennru added the p:aws-s3 label Jan 2, 2018

@ennru

This comment has been minimized.

Show comment
Hide comment
@ennru

ennru Jan 3, 2018

Member

Does this replace #619 ? Please close it if so.

Member

ennru commented Jan 3, 2018

Does this replace #619 ? Please close it if so.

@easel

This comment has been minimized.

Show comment
Hide comment
@easel

easel Jan 3, 2018

Contributor

Yes, it's includes all of #619 but includes fixes, additional capabilities and tests. I didn't create #619 so I don't believe I can update it or delete it, hence the new PR.

Contributor

easel commented Jan 3, 2018

Yes, it's includes all of #619 but includes fixes, additional capabilities and tests. I didn't create #619 so I don't believe I can update it or delete it, hence the new PR.

@francisdb

This comment has been minimized.

Show comment
Hide comment
@francisdb

francisdb Jan 3, 2018

Contributor

Please keep the AwsS3IntegrationSpec as the same previous refactor to S3IntegrationSpec was no longer doing proper validation of all AWS S3 cases.

I personally think it's better to have a specific test for each service as they each have their edge cases and url path issues.

Contributor

francisdb commented Jan 3, 2018

Please keep the AwsS3IntegrationSpec as the same previous refactor to S3IntegrationSpec was no longer doing proper validation of all AWS S3 cases.

I personally think it's better to have a specific test for each service as they each have their edge cases and url path issues.

@easel

This comment has been minimized.

Show comment
Hide comment
@easel

easel Jan 3, 2018

Contributor

The AWSS3IntegrationSpec is still there, I just lifted the test methods up to a trait so I could inject the differing config for this patch. The tests are all identical to before and they all pass against minio -- as far as I am concerned if a provider claims to be s3 compatible it should pass the same integration suite.

Contributor

easel commented Jan 3, 2018

The AWSS3IntegrationSpec is still there, I just lifted the test methods up to a trait so I could inject the differing config for this patch. The tests are all identical to before and they all pass against minio -- as far as I am concerned if a provider claims to be s3 compatible it should pass the same integration suite.

@francisdb

This comment has been minimized.

Show comment
Hide comment
@francisdb

francisdb Jan 3, 2018

Contributor

Seems I was a bit too fast, it's the S3NoMock test that is totally broken and that was why I re-introduced an old version as the AWSS3IntegrationSpec.

Contributor

francisdb commented Jan 3, 2018

Seems I was a bit too fast, it's the S3NoMock test that is totally broken and that was why I re-introduced an old version as the AWSS3IntegrationSpec.

@easel

This comment has been minimized.

Show comment
Hide comment
@easel

easel Jan 3, 2018

Contributor

Yeah, I couldn't figure that one out at all =p The AWSS3IntegrationSpec worked ok, I just added some delete calls to make it idempotent.

Contributor

easel commented Jan 3, 2018

Yeah, I couldn't figure that one out at all =p The AWSS3IntegrationSpec worked ok, I just added some delete calls to make it idempotent.

@ennru

This comment has been minimized.

Show comment
Hide comment
@ennru

ennru Jan 8, 2018

Member

@easel and @francisdb can you get this one ready for merge and release? I'd like to get 0.16 out on Thursday.

Member

ennru commented Jan 8, 2018

@easel and @francisdb can you get this one ready for merge and release? I'd like to get 0.16 out on Thursday.

@francisdb

This comment has been minimized.

Show comment
Hide comment
@francisdb

francisdb Jan 8, 2018

Contributor

francisdb approved these changes 5 days ago

just added some small remarks

Contributor

francisdb commented Jan 8, 2018

francisdb approved these changes 5 days ago

just added some small remarks

@takashima0411

This comment has been minimized.

Show comment
Hide comment
@takashima0411

takashima0411 Jan 9, 2018

@easel sorry for delay, and thank you for the patches.
Minio seems good 👍

takashima0411 commented Jan 9, 2018

@easel sorry for delay, and thank you for the patches.
Minio seems good 👍

@easel

This comment has been minimized.

Show comment
Hide comment
@easel

easel Jan 9, 2018

Contributor

Thanks all for the feedback. I've rebased against master and adjusted things per earlier comments.

  • Removed the remaining references to baseUrl
  • Default to a commented out endpoint-url key, and added tests that ensure missing keys and resetting to null both are handled as None
  • Switched java options to scala.Option.empty(), which compiles. I don't use java api's anymore so I can't say if it's a better solution personally but the code is cleaner!
Contributor

easel commented Jan 9, 2018

Thanks all for the feedback. I've rebased against master and adjusted things per earlier comments.

  • Removed the remaining references to baseUrl
  • Default to a commented out endpoint-url key, and added tests that ensure missing keys and resetting to null both are handled as None
  • Switched java options to scala.Option.empty(), which compiles. I don't use java api's anymore so I can't say if it's a better solution personally but the code is cleaner!
@ennru

ennru approved these changes Jan 9, 2018

LGTM.

@ennru ennru merged commit e474a4a into akka:master Jan 9, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
typesafe-cla-validator All users have signed the CLA
Details

@ennru ennru added this to the 0.16 milestone Jan 9, 2018

@ennru

This comment has been minimized.

Show comment
Hide comment
@ennru

ennru Jan 9, 2018

Member

Fixes #181

Thank you for you contribution! Keep going!

Member

ennru commented Jan 9, 2018

Fixes #181

Thank you for you contribution! Keep going!

@easel

This comment has been minimized.

Show comment
Hide comment
@easel

easel Jan 9, 2018

Contributor

Thanks @ennru !

Contributor

easel commented Jan 9, 2018

Thanks @ennru !

@easel easel deleted the easel:s3-entrypoint branch Jan 9, 2018

@ennru

This comment has been minimized.

Show comment
Hide comment
@ennru

ennru Jan 9, 2018

Member

Cheers. We 'll release 0.16 on Thursday.

Member

ennru commented Jan 9, 2018

Cheers. We 'll release 0.16 on Thursday.

@l15k4

This comment has been minimized.

Show comment
Hide comment
@l15k4

l15k4 Jan 19, 2018

Thanks guys, it seems to be working with https://github.com/findify/s3mock but I still cannot figure out why multipart upload doesn't work on https://github.com/jubos/fake-s3 ...

I set it this way :

      endpointUrl = Some(s"http://localhost:$randomPort"),
      pathStyleAccess = true
akka.http.scaladsl.unmarshalling.Unmarshaller$UnsupportedContentTypeException: Unsupported Content-Type, supported: application/xml, application/octet-stream
	at akka.http.scaladsl.unmarshalling.Unmarshaller$UnsupportedContentTypeException$.apply(Unmarshaller.scala:158)
	at akka.http.scaladsl.unmarshalling.Unmarshaller$EnhancedFromEntityUnmarshaller$.$anonfun$forContentTypes$3(Unmarshaller.scala:114)
	at akka.http.scaladsl.unmarshalling.Unmarshaller$$anon$1.apply(Unmarshaller.scala:58)
	at akka.http.scaladsl.unmarshalling.Unmarshaller$EnhancedUnmarshaller$.$anonfun$mapWithInput$3(Unmarshaller.scala:91)
	at akka.http.scaladsl.unmarshalling.Unmarshaller$$anon$1.apply(Unmarshaller.scala:58)
	at akka.http.scaladsl.unmarshalling.Unmarshaller.$anonfun$transform$3(Unmarshaller.scala:23)
	at akka.http.scaladsl.unmarshalling.Unmarshaller$$anon$1.apply(Unmarshaller.scala:58)
	at akka.http.scaladsl.unmarshalling.Unmarshal.to(Unmarshal.scala:25)
	at akka.stream.alpakka.s3.impl.S3Stream.$anonfun$initiateMultipartUpload$3(S3Stream.scala:216)

I think that fake-s3 requires this to be set on the aws java client :

com.amazonaws.services.s3.S3ClientOptions.Builder#disableChunkedEncoding

l15k4 commented Jan 19, 2018

Thanks guys, it seems to be working with https://github.com/findify/s3mock but I still cannot figure out why multipart upload doesn't work on https://github.com/jubos/fake-s3 ...

I set it this way :

      endpointUrl = Some(s"http://localhost:$randomPort"),
      pathStyleAccess = true
akka.http.scaladsl.unmarshalling.Unmarshaller$UnsupportedContentTypeException: Unsupported Content-Type, supported: application/xml, application/octet-stream
	at akka.http.scaladsl.unmarshalling.Unmarshaller$UnsupportedContentTypeException$.apply(Unmarshaller.scala:158)
	at akka.http.scaladsl.unmarshalling.Unmarshaller$EnhancedFromEntityUnmarshaller$.$anonfun$forContentTypes$3(Unmarshaller.scala:114)
	at akka.http.scaladsl.unmarshalling.Unmarshaller$$anon$1.apply(Unmarshaller.scala:58)
	at akka.http.scaladsl.unmarshalling.Unmarshaller$EnhancedUnmarshaller$.$anonfun$mapWithInput$3(Unmarshaller.scala:91)
	at akka.http.scaladsl.unmarshalling.Unmarshaller$$anon$1.apply(Unmarshaller.scala:58)
	at akka.http.scaladsl.unmarshalling.Unmarshaller.$anonfun$transform$3(Unmarshaller.scala:23)
	at akka.http.scaladsl.unmarshalling.Unmarshaller$$anon$1.apply(Unmarshaller.scala:58)
	at akka.http.scaladsl.unmarshalling.Unmarshal.to(Unmarshal.scala:25)
	at akka.stream.alpakka.s3.impl.S3Stream.$anonfun$initiateMultipartUpload$3(S3Stream.scala:216)

I think that fake-s3 requires this to be set on the aws java client :

com.amazonaws.services.s3.S3ClientOptions.Builder#disableChunkedEncoding
@bandrzejczak

This comment has been minimized.

Show comment
Hide comment
@bandrzejczak

bandrzejczak Apr 7, 2018

@l15k4 Yes it does. While using standard S3 client I had to disable chunked encoding to make it work with fake-s3. It still allows for sending your document in multiple chunks though, right?

bandrzejczak commented Apr 7, 2018

@l15k4 Yes it does. While using standard S3 client I had to disable chunked encoding to make it work with fake-s3. It still allows for sending your document in multiple chunks though, right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment