Conversation
This adds an integration test checking `amazonka-s3` against MinIO. To
execute the test just run the following:
```
$ nix-build tests/s3.nix
...
s3: must succeed:
echo 'Hello World!' > ./some-file
export AWS_ACCESS_KEY_ID="BKIKJAA5BMMU2RHO6IBB"
export AWS_SECRET_ACCESS_KEY="V7f1CwQqAcwo80UEIJEjc5gVQUSSx5ohQ9GSrr12"
amazonka-s3-test-app ./some-file http://s3:9000 my-bucket some-file
s3 # > > > > > [ 11.524753] minio[818]: {"deploymentid":"0de4a2b3-eaf1-46a0-8ec7-2e706e68d3b4","level":"ERROR","errKind":"MINIO","time":"2022-07-09T18:07:28.335404358Z","api":{"name":"SYSTEM","args":{}},"error":{"message":"EOF (*errors.errorString)","source":["cmd/handler-utils.go:55:cmd.parseLocationConstraint()","cmd/auth-handler.go:344:cmd.checkRequestAuthTypeCredential()","cmd/auth-handler.go:296:cmd.checkRequestAuthType()","cmd/bucket-handlers.go:719:cmd.objectAPIHandlers.PutBucketHandler()","net/http/server.go:2047:http.HandlerFunc.ServeHTTP()"]}}
s3 # amazonka-s3-test-app: ServiceError (ServiceError' {_serviceErrorAbbrev = Abbrev {fromAbbrev = "S3"}, _serviceErrorStatus = Status {statusCode = 400, statusMessage = "Bad Request"}, _serviceErrorHeaders = [("Accept-Ranges","bytes"),("Content-Length","371"),("Content-Security-Policy","block-all-mixed-content"),("Content-Type","application/xml"),("Server","MinIO"),("Strict-Transport-Security","max-age=31536000; includeSubDomains"),("Vary","Origin"),("Vary","Accept-Encoding"),("X-Amz-Bucket-Region","us-east-1"),("X-Amz-Request-Id","17003B77BD220CF2"),("X-Content-Type-Options","nosniff"),("X-Xss-Protection","1; mode=block"),("Date","Sat, 09 Jul 2022 18:07:28 GMT")], _serviceErrorCode = ErrorCode "MalformedXML", _serviceErrorMessage = Just (ErrorMessage {fromErrorMessage = "The XML you provided was not well-formed or did not validate against our published schema."}), _serviceErrorRequestId = Just (RequestId {fromRequestId = "17003B77BD220CF2"})})
```
|
That ^ commit extends the test with capturing the network packets using So you can see a packet containing the HTTP request You can also see the MinIO response What's at fault here? Amazonka? MinIO? Or am I missing something? |
|
I'm suspicious of the fact that, even though the name of the bucket is specified in the PutObject'
{ contentLength = Nothing
, objectLockMode = Nothing
, expires = Nothing
, grantReadACP = Nothing
, sSECustomerAlgorithm = Nothing
, sSECustomerKey = Nothing
, requestPayer = Nothing
, grantWriteACP = Nothing
, bucketKeyEnabled = Nothing
, websiteRedirectLocation = Nothing
, grantRead = Nothing
, storageClass = Nothing
, sSECustomerKeyMD5 = Nothing
, sSEKMSKeyId = Nothing
, grantFullControl = Nothing
, contentEncoding = Nothing
, tagging = Nothing
, contentMD5 = Nothing
, objectLockRetainUntilDate = Nothing
, metadata = fromList []
, sSEKMSEncryptionContext = Nothing
, cacheControl = Nothing
, contentLanguage = Nothing
, objectLockLegalHoldStatus = Nothing
, acl = Nothing
, contentDisposition = Nothing
, expectedBucketOwner = Nothing
, serverSideEncryption = Nothing
, contentType = Nothing
, bucket = BucketName "my-bucket"
, key = ObjectKey "some-file"
, body = Hashed HashedStream
{ sha256 = 03 ba204e50d126e4674c005e04d82e84c21366780af1f43bd54a37816b6ab340
, length = 13
}
}that bucket name is not included in the HTTP request to MinIO: Maybe that explains why MinIO is not expecting to receive an object at URL |
|
If I look at examples of how to put an object to MinIO I see the request path needs to begin with the bucket name. So in our example we need the request to be: I'll dive into the amazonka source code to see why the bucket name is omitted. |
|
I found the root cause: the default request of a The right solution is probably to only apply the |
|
@endgame, @akshaymankar basvandijk/amazonka@put-to-minio-malformed-xml...put-to-minio-malformed-xml-fix-attempt-1 tries to fix #760 by adding the field A user can then override this field in the environment by setting it to defEnv <- Amazonka.newEnv Amazonka.Auth.fromKeysEnv
let env =
flip Amazonka.override defEnv $
(Amazonka.serviceEndpoint .~ endpoint)
. (Amazonka.serviceRewriteS3VHost .~ False)Then This doesn't work unfortunately since the Possible solutionTo fix this I think the class AWSService a where
service :: Proxy a -> Serviceand have instances for all request types like for example: instance AWSService PutObject where
service _proxy = Amazonka.S3.Types.defaultServicethen the class AWSRequest a where
...
request :: Service -> a -> Request aThe instance for instance Core.AWSRequest PutObject where
type AWSResponse PutObject = PutObjectResponse
- request =
+ request srv =
Request.expectHeader
Prelude.. Request.s3vhost
- Prelude.. Request.putBody defaultService
+ Prelude.. Request.putBody srv
- configureRequest :: AWSRequest a => Env' withAuth -> a -> Request a
+ configureRequest :: forall a. (AWSRequest a, AWSService a) => Env' withAuth -> a -> Request a
configureRequest env x =
let overrides = envOverride env
+ srv = appEndo (getDual overrides) $ service (Proxy :: Proxy a)
- in request x & requestService %~ appEndo (getDual overrides)
+ in request srv xWe could then also consider dropping the |
|
This seems overwrought; can you not just set the amazonka/lib/amazonka/src/Amazonka/HTTP.hs Lines 157 to 160 in 2646624 |
I tried that:
But:
Hence my suggestion for that "possible solution". |
This implements a change suggested by this comment: brendanhay#797 (comment) as part of a fix to the following issue: brendanhay#760 This modifies the code generation for AWS endpoints so that requests which make other requests all share the same (passed-in) `Service`. This is done by making `request` take an extra `Service` argument. This also adds a new parent class of `AWSRequest`, called `AWSService`, which defines the default service used for each request (this being `defaultService`.) These are some breaking changes, but they are required to allow modified `Service`s to be used in complex requests. This is necessary to allow VHost rewriting to be turned off for local mocking and other use cases at the `Service` level. Co-authored-by: Bas van Dijk <v.dijk.bas@gmail.com>
Originally "Non-working attempt to fix the test failure in PR brendanhay#797" This adds a new field to `Service` which controls if requests to the service should have their URLs rewritten using `s3vhost`. This is particularly relevant for mocks of S3 which don't support having the bucket name in the hostname (e.g. because they are bound to `localhost`, which isn't a domain and can't have a subdomain.) This field is defaulted to `True` to preserve the current behaviour for all services. Co-authored-by: Isaac van Bakel <isaac@supercede.com>
|
FYI @basvandijk your fix is now implemented over on our branch here: https://github.com/SupercedeTech/amazonka/commits/temp-vhost-fix I've tested it for our use case (using Moto as an S3 mock) and it works as expected. One particular issue of note was this line, which I believe is the cause for #760, in part. Because the service overrides are originally applied after the request is built, the call to The fix involving |
This implements a change suggested by this comment: brendanhay#797 (comment) as part of a fix to the following issue: brendanhay#760 This modifies the code generation for AWS endpoints so that requests which make other requests all share the same (passed-in) `Service`. This is done by making `request` take an extra `Service` argument. This also adds a new parent class of `AWSRequest`, called `AWSService`, which defines the default service used for each request (this being `defaultService`.) These are some breaking changes, but they are required to allow modified `Service`s to be used in complex requests. This is necessary to allow VHost rewriting to be turned off for local mocking and other use cases at the `Service` level. Co-authored-by: Bas van Dijk <v.dijk.bas@gmail.com> # Conflicts: # gen/src/Gen/AST/Data/Syntax.hs
Originally "Non-working attempt to fix the test failure in PR brendanhay#797" This adds a new field to `Service` which controls if requests to the service should have their URLs rewritten using `s3vhost`. This is particularly relevant for mocks of S3 which don't support having the bucket name in the hostname (e.g. because they are bound to `localhost`, which isn't a domain and can't have a subdomain.) This field is defaulted to `True` to preserve the current behaviour for all services. Co-authored-by: Isaac van Bakel <isaac@supercede.com>
|
We're also experiencing the issue with What are the next steps regarding wrt. the patch proposed by @ivb-supercede? |
This implements a change suggested by this comment: brendanhay#797 (comment) as part of a fix to the following issue: brendanhay#760 This modifies the code generation for AWS endpoints so that requests which make other requests all share the same (passed-in) `Service`. This is done by making `request` take an extra `Service` argument. This also adds a new parent class of `AWSRequest`, called `AWSService`, which defines the default service used for each request (this being `defaultService`.) These are some breaking changes, but they are required to allow modified `Service`s to be used in complex requests. This is necessary to allow VHost rewriting to be turned off for local mocking and other use cases at the `Service` level. Co-authored-by: Bas van Dijk <v.dijk.bas@gmail.com>
Originally "Non-working attempt to fix the test failure in PR brendanhay#797" This adds a new field to `Service` which controls if requests to the service should have their URLs rewritten using `s3vhost`. This is particularly relevant for mocks of S3 which don't support having the bucket name in the hostname (e.g. because they are bound to `localhost`, which isn't a domain and can't have a subdomain.) This field is defaulted to `True` to preserve the current behaviour for all services. Co-authored-by: Isaac van Bakel <isaac@supercede.com>
Originally "Non-working attempt to fix the test failure in PR brendanhay#797" This adds a new field to `Service` which controls if requests to the service should have their URLs rewritten using `s3vhost`. This is particularly relevant for mocks of S3 which don't support having the bucket name in the hostname (e.g. because they are bound to `localhost`, which isn't a domain and can't have a subdomain.) This field is defaulted to `True` to preserve the current behaviour for all services. Co-authored-by: Isaac van Bakel <isaac@supercede.com>
5 participants
This adds a reproducible test case for issue #796 "PutObject request failing on MinIO because of malformed XML" by adding an integration test checking
amazonka-s3against MinIO. To execute the test just run the following: