Make cargohold use amazonka instead of aws #1157

mheinzel · 2020-07-02T13:25:10Z

Fixes #876.
Also see https://github.com/zinfra/backend-issues/issues/1481.

This revives #483 with a more recent version of amazonka. Everything seems to work without performance regressions ~~except for problems against Minio with the pre-signed download URLs we generate (more docs). This still needs to be investigated.~~

edit: I found the issue in amazonka and made a fix

Before merging:

make PR against amazonka (Include non-standard port in signed host header brendanhay/amazonka#588)
properly support s3DownloadEndpoint, which is currently ignored
find out whether we actually want to switch to http-client-tls here (and if not, revert that part)

tiago-loureiro

Did some early review with @fisx - we did not manage to review the resumable uploads but already have some feedback and wanted to leave them here already... in general looks great!

tiago-loureiro · 2020-07-09T18:25:53Z

services/cargohold/src/Main.hs

-main = withOpenSSL $ do
-  options <- getOptions desc Nothing defaultPath
-  run options
+main = do


I remember we had a chat about it and mentioned we should probably keep using openSSL but then it was removed in the PR anyway; was it easier to remove it or what was the reason for the change?

@franziskuskiefer do you have an opinion on openssl vs. http://hackage.haskell.org/package/tls?

We had this a while back, and after struggling for a while between "In Haskell with a few good habits, many mistakes cannot be expressed in the language" and "openssl has had so many bugs closed, how many more can there be?", we stuck with openssl.

Ah, I undid the changes removing OpenSSL, but must have missed this one. Interestingly, the code using OpenSSL still worked without the initialization here. Good catch!

For this PR I think it makes sense to stick with OpenSSL for consistency with the rest of the codebase, but yeah, we should think about which library we want to use in general.

tiago-loureiro · 2020-07-09T18:33:50Z

services/cargohold/src/CargoHold/AWS.hs

+instance AWS.MonadAWS Amazon where
+  liftAWS a = view amazonkaEnv >>= flip AWS.runAWS a
+
+mkEnv ::


For future reference, pretty much all our services have a nearly identical copy of this AWS monad (except for the mkEnv), would be nice to at some point generalize it.

services/cargohold/src/CargoHold/S3.hs

tiago-loureiro · 2020-07-09T18:52:37Z

services/cargohold/src/CargoHold/S3.hs

+  signed <-
+    AWS.execute (AWS.useDownloadEndpoint e) $
+      presignURL now (Seconds (fromIntegral ttl)) req
+  return =<< toUri signed


Suggested change

return =<< toUri signed

toUri signed

as part of 877c9a9

tiago-loureiro · 2020-07-09T19:02:00Z

services/cargohold/src/CargoHold/S3.hs

-           return $ statusIsServerError s
-       ]
+parseMIMEType :: Text -> Maybe MIME.Type
+parseMIMEType = MIME.parseMIMEType


Is it worth having this as it's just an alias?

No, will inline it. Same for encodeMIMEType.

services/cargohold/src/CargoHold/S3.hs

tiago-loureiro · 2020-07-09T19:07:42Z

services/cargohold/src/CargoHold/AWS.hs

+execCatch ::
+  (AWSRequest a, AWS.HasEnv r, MonadUnliftIO m, MonadCatch m, MonadThrow m) =>
+  r ->
+  a ->
+  m (Either AWS.Error (Rs a))
+execCatch e cmd =
+  runResourceT . AWST.runAWST e
+    $ AWST.trying AWS._Error
+    $ AWST.send cmd
+
+exec ::
+  (AWSRequest a, AWS.HasEnv r, MonadUnliftIO m, MonadCatch m, MonadThrow m) =>
+  r ->
+  a ->
+  m (Rs a)
+exec e cmd = execCatch e cmd >>= either (throwM . GeneralError) return


These are not used and have functions with the same name on the S3 module; perhaps move those functions here since they are generic and can be used in other modules (although they are not)

It was not quite as easy as copying them, because of the module dependencies (Cargohold.AWS doesn't know about App and Env), but here it is: 959adbc

fisx · 2020-07-09T19:18:30Z

services/cargohold/package.yaml

  - byteable >=0.1
  - base16-bytestring >=0.1
+  - blaze-builder >=0.3
+  - binary >=0.8


Nit-pick: I think the only thing used from binary is toLazyByteString, which I think is just a re-export from bytestring.

Good spot. We also do this in a few other places, so I cleaned them up, too.
0bfad02

fisx

Given that the resume functionality (which is not used by any client btw) is somewhat covered in the test suite, I'll take your word for that the changes are valid.

But does the V3 in the tests stand for the signature version, and do we need to cover V4 as well? I am missing a lot of context here. :)

fisx · 2020-07-10T07:05:26Z

services/cargohold/src/Main.hs

-main = withOpenSSL $ do
-  options <- getOptions desc Nothing defaultPath
-  run options
+main = do


@franziskuskiefer do you have an opinion on openssl vs. http://hackage.haskell.org/package/tls?

We had this a while back, and after struggling for a while between "In Haskell with a few good habits, many mistakes cannot be expressed in the language" and "openssl has had so many bugs closed, how many more can there be?", we stuck with openssl.

tiago-loureiro · 2020-07-10T08:13:10Z

But does the V3 in the tests stand for the signature version, and do we need to cover V4 as well? I am missing a lot of context here. :)

V3 is the Cargohold API version, nothing to do with signatures

franziskuskiefer · 2020-07-10T11:56:54Z

@franziskuskiefer do you have an opinion on openssl vs. http://hackage.haskell.org/package/tls?
We had this a while back, and after struggling for a while between "In Haskell with a few good habits, many mistakes cannot be expressed in the language" and "openssl has had so many bugs closed, how many more can there be?", we stuck with openssl.

I generally prefer not using openssl for multiple reasons. But that haskell package doesn't appear to be used a lot, which is crucial for a TLS lib (most errors are found while using it).
I'm happy to look into what we could generally use on the BE for TLS instead of openssl. But we should probably handle that separately.

* just takes all amazonka packages from the pinned fork. they depend on core, so need to be rebuilt anyways. * also drops elb, redshift, route53

mheinzel · 2020-07-10T14:42:59Z

I addressed all comments and now rebased to resolve conflicts. Looking forward to more feedback!

tiago-loureiro

Small note: I added an extra commit with a little comment a few extra config options explaining how you could test with cloudfront too and updated the file on S3

This reverts commit e570f2e.

* provide AWS_REGION variable to cargohold Now with cargohold using V4 signatures (wireapp/wire-server#1157), the region is part of the Authorization header and needs to configured correctly. We already do that for the other services, so I just copied the code from there. * provide AWS env vars to cargohold integration We do this for all the other servers, too, but I'm actually not sure why. The integration tests themselves don't call AWS. We should probably just remove it (+ from the other services).

* squash #483 * remove all usages of aws * support s3DownloadEndpoint config * stack.yaml: pin updated fork of amazonka The update to stack.yaml just takes all amazonka packages from the pinned fork. They depend on core, so need to be rebuilt anyways. We also drop elb, redshift, route53 * depend on OpenSSL again This is mostly done for consistency with the other services. If we want to change it, we should change it everywhere. * remove unnecessary dependencies on binary and blaze-builder * Add comment on how to use cloudfront Co-authored-by: Tiago Loureiro <[email protected]>

mheinzel force-pushed the mheinzel/cargohold-use-amazonka branch 2 times, most recently from 19a002c to c25d4ba Compare July 6, 2020 10:54

mheinzel changed the title ~~[WIP] Make cargohold use amazonka instead of aws~~ Make cargohold use amazonka instead of aws Jul 6, 2020

mheinzel force-pushed the mheinzel/cargohold-use-amazonka branch from 338b372 to f9abcc5 Compare July 7, 2020 09:47

mheinzel marked this pull request as ready for review July 7, 2020 12:33

mheinzel mentioned this pull request Jul 7, 2020

is the code using S3 support V4 signature #876

Closed

1 task

tiago-loureiro suggested changes Jul 9, 2020

View reviewed changes

fisx reviewed Jul 9, 2020

View reviewed changes

fisx reviewed Jul 10, 2020

View reviewed changes

mheinzel added 16 commits July 10, 2020 16:34

squash wire-server#483

99c8863

don't make test harder/longer here

efa9a89

TODO!

87182c7

remove all usages of aws

6f4a108

update amazonka master

ddb8a57

use local amazonka-core

613a0ba

stack.yaml: pin updated fork of amazonka

1d63eb1

* just takes all amazonka packages from the pinned fork. they depend on core, so need to be rebuilt anyways. * also drops elb, redshift, route53

support s3DownloadEndpoint config

b3f564e

cleanup

d342bbd

depend on OpenSSL again

669b40d

simplify Error type

ba744f7

formatting

652358a

inline utilities

ac5112b

remove unnecessary dependencies on binary and blaze-builder

f6e1929

re-add forgotten OpenSSL initialization

b0c8469

move exec and execCatch

0ecdbfe

mheinzel force-pushed the mheinzel/cargohold-use-amazonka branch from 959adbc to 0ecdbfe Compare July 10, 2020 14:42

Add comment on how to use cloudfront

bde835d

tiago-loureiro force-pushed the mheinzel/cargohold-use-amazonka branch from 4a5a869 to bde835d Compare July 10, 2020 20:49

tiago-loureiro approved these changes Jul 10, 2020

View reviewed changes

mheinzel merged commit e570f2e into develop Jul 13, 2020

mheinzel deleted the mheinzel/cargohold-use-amazonka branch July 13, 2020 08:43

fisx mentioned this pull request Jul 13, 2020

Release 2020/07/13 #1164

Merged

fisx added a commit that referenced this pull request Jul 13, 2020

Revert "Make cargohold use amazonka instead of aws (#1157)"

704182f

This reverts commit e570f2e.

fisx added a commit that referenced this pull request Jul 14, 2020

Revert "Make cargohold use amazonka instead of aws (#1157)"

a43a215

This reverts commit e570f2e.

mheinzel mentioned this pull request Jul 14, 2020

Provide AWS_REGION variable to cargohold wireapp/wire-server-deploy#314

Merged

fisx mentioned this pull request Jul 29, 2020

Release 2020-07-29 #1182

Merged

obstenu mentioned this pull request Sep 9, 2020

Cargohold v2.88.0 won't upload objects to s3 because of BadDigest error #1203

Open

mheinzel mentioned this pull request Sep 15, 2020

Cargohold: Log more about AWS errors #1205

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make cargohold use amazonka instead of aws #1157

Make cargohold use amazonka instead of aws #1157

mheinzel commented Jul 2, 2020 •

edited

Loading

tiago-loureiro left a comment

tiago-loureiro Jul 9, 2020

fisx Jul 10, 2020

mheinzel Jul 10, 2020

mheinzel Jul 10, 2020

tiago-loureiro Jul 9, 2020

tiago-loureiro Jul 9, 2020

mheinzel Jul 10, 2020

tiago-loureiro Jul 9, 2020

mheinzel Jul 10, 2020

mheinzel Jul 10, 2020

tiago-loureiro Jul 9, 2020

mheinzel Jul 10, 2020

fisx Jul 9, 2020

mheinzel Jul 10, 2020

fisx left a comment

fisx Jul 10, 2020

tiago-loureiro commented Jul 10, 2020

franziskuskiefer commented Jul 10, 2020

mheinzel commented Jul 10, 2020

tiago-loureiro left a comment

Make cargohold use amazonka instead of aws #1157

Make cargohold use amazonka instead of aws #1157

Conversation

mheinzel commented Jul 2, 2020 • edited Loading

tiago-loureiro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fisx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiago-loureiro commented Jul 10, 2020

franziskuskiefer commented Jul 10, 2020

mheinzel commented Jul 10, 2020

tiago-loureiro left a comment

Choose a reason for hiding this comment

mheinzel commented Jul 2, 2020 •

edited

Loading