Skip to content

Bug fixes and features for S3 support #36

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

Merged
merged 13 commits into from
Nov 17, 2022
Merged

Bug fixes and features for S3 support #36

merged 13 commits into from
Nov 17, 2022

Conversation

StarlightIbuki
Copy link
Contributor

@StarlightIbuki StarlightIbuki commented Oct 25, 2022

fix: template incorrect handling of query string
fix: blob param should be raw body
feat: add support for credential from file
fix: escaping for param in uri
fix: handling raw body conflict with body param
fix: crash when no type check designated
fix: missing support for "headers" location
fix: compatibility for S3

@StarlightIbuki StarlightIbuki changed the title Fix/s3 Fix S3 support; fix handling of query string in template path Oct 26, 2022
@StarlightIbuki StarlightIbuki requested a review from Tieske October 26, 2022 03:34
@StarlightIbuki StarlightIbuki force-pushed the fix/s3 branch 2 times, most recently from 257b834 to b0a7d06 Compare November 2, 2022 09:54
@StarlightIbuki StarlightIbuki changed the title Fix S3 support; fix handling of query string in template path Bug fixes and features for S3 support Nov 2, 2022
@StarlightIbuki
Copy link
Contributor Author

We could add a unit test, to mock an HTTP service and inspect the request generated.
To have a determinative result for signatures, we could hook functions to have a constant timestamp.

@StarlightIbuki
Copy link
Contributor Author

Also, the "config".global is pretty slow in my environment, maybe we should address it.

@StarlightIbuki
Copy link
Contributor Author

StarlightIbuki commented Nov 3, 2022

Also, the "config".global is pretty slow in my environment, maybe we should address it.

It turns out to be that the utils.getCurrentRegion() is making request:

local token, err = make_request(IDMS_URI.."/latest/api/token", "PUT", headers)

and the request failed in my env.

So it's better to set AWS_DEFAULT_REGION to avoid sending the request. And I wonder why we don't use AWS_REGION to initialize when AWS_DEFAULT_REGION is not present.

@windmgc
Copy link
Member

windmgc commented Nov 3, 2022

Also, the "config".global is pretty slow in my environment, maybe we should address it.

Are you testing this on your localhost without any related config set(like environment var 'AWS_REGION' or profile config)?
I think it's pretty okay for now not to deal with it because the detect_region will find the correct way if the environment is in the real AWS

So it's better to set AWS_DEFAULT_REGION to avoid sending the request. And I wonder why we don't use AWS_REGION to initialize when AWS_DEFAULT_REGION is not present.

I think the AWS_DEFAULT_REGION is some kind of a fallback value when AWS_REGION is not present, check the
https://github.com/aws/aws-sdk-go/blob/aeeca02c4a3603ec615fa9a011e2e05c9d40e1f8/aws/session/env_config.go#L45-L47

@windmgc
Copy link
Member

windmgc commented Nov 3, 2022

Failed test on AWS environment due to deprecation of putting bucket name in request URI:

{
  body = '<?xml version="1.0" encoding="UTF-8"?>\n<Error><Code>PermanentRedirect</Code><Message>The bucket you are attempting to access must be addressed using the specified endpoint. Please send all future requests to this endpoint.</Message><Endpoint>suikatestbucket.s3.amazonaws.com</Endpoint><Bucket>XXXXXXXXX</Bucket><RequestId>XXXXXXXXXXX</RequestId><HostId>XXXXXXXXXXX</HostId></Error>',
  headers = {
    Connection = "close",
    ["Content-Type"] = "application/xml",
    Date = "Thu, 03 Nov 2022 07:35:20 GMT",
    Server = "AmazonS3",
    ["Transfer-Encoding"] = "chunked",
    ["x-amz-bucket-region"] = "ap-northeast-1",
    ["x-amz-id-2"] = "XXXXXXX",
    ["x-amz-request-id"] = "XXXXXXX",
    <metatable> = {
      __index = <function 1>,
      __newindex = <function 2>,
      normalised = {
        connection = "Connection",
        ["content-type"] = "Content-Type",
        date = "Date",
        server = "Server",
        ["transfer-encoding"] = "Transfer-Encoding",
        ["x-amz-bucket-region"] = "x-amz-bucket-region",
        ["x-amz-id-2"] = "x-amz-id-2",
        ["x-amz-request-id"] = "x-amz-request-id"
      }
    }
  },
  reason = "Moved Permanently",
  status = 301
}nil

Maybe we should do a similar thing https://github.com/aws/aws-sdk-go/blob/aeeca02c4a3603ec615fa9a011e2e05c9d40e1f8/service/s3/host_style_bucket.go#L132-L136

@StarlightIbuki
Copy link
Contributor Author

Failed test on AWS environment due to deprecation of putting bucket name in request URI:

{
  body = '<?xml version="1.0" encoding="UTF-8"?>\n<Error><Code>PermanentRedirect</Code><Message>The bucket you are attempting to access must be addressed using the specified endpoint. Please send all future requests to this endpoint.</Message><Endpoint>suikatestbucket.s3.amazonaws.com</Endpoint><Bucket>XXXXXXXXX</Bucket><RequestId>XXXXXXXXXXX</RequestId><HostId>XXXXXXXXXXX</HostId></Error>',
  headers = {
    Connection = "close",
    ["Content-Type"] = "application/xml",
    Date = "Thu, 03 Nov 2022 07:35:20 GMT",
    Server = "AmazonS3",
    ["Transfer-Encoding"] = "chunked",
    ["x-amz-bucket-region"] = "ap-northeast-1",
    ["x-amz-id-2"] = "XXXXXXX",
    ["x-amz-request-id"] = "XXXXXXX",
    <metatable> = {
      __index = <function 1>,
      __newindex = <function 2>,
      normalised = {
        connection = "Connection",
        ["content-type"] = "Content-Type",
        date = "Date",
        server = "Server",
        ["transfer-encoding"] = "Transfer-Encoding",
        ["x-amz-bucket-region"] = "x-amz-bucket-region",
        ["x-amz-id-2"] = "x-amz-id-2",
        ["x-amz-request-id"] = "x-amz-request-id"
      }
    }
  },
  reason = "Moved Permanently",
  status = 301
}nil

Maybe we should do a similar thing https://github.com/aws/aws-sdk-go/blob/aeeca02c4a3603ec615fa9a011e2e05c9d40e1f8/service/s3/host_style_bucket.go#L132-L136

I strongly doubt if aws-sdk-* are officials. How could they not fix the API template but do a patch like that?

@StarlightIbuki
Copy link
Contributor Author

Should we really calculate the host in v4.lua? @Tieske

@windmgc
Copy link
Member

windmgc commented Nov 3, 2022

Tested with @StarlightIbuki that the old request URI format also works well, so it is our decision whether to "put bucket into host instead of path".

Already tested this PR with SecretsManager:getValue and S3:putObject as well as getObject.

@StarlightIbuki StarlightIbuki force-pushed the fix/s3 branch 2 times, most recently from 89df1f8 to 0dc65b3 Compare November 4, 2022 02:53
@StarlightIbuki
Copy link
Contributor Author

Tested with @StarlightIbuki that the old request URI format also works well, so it is our decision whether to "put bucket into host instead of path".

Already tested this PR with SecretsManager:getValue and S3:putObject as well as getObject.

I made it an option and default to "in host".

@StarlightIbuki StarlightIbuki force-pushed the fix/s3 branch 3 times, most recently from 72ad12f to 9a8a33e Compare November 4, 2022 07:19
Copy link
Contributor

@mayocream mayocream left a comment

Choose a reason for hiding this comment

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

I am good with these changes.
ping @Tieske for another look.

@fffonion
Copy link

We don't have automatic CI and I assume the test suites have been ran locally. Since this
is now a blocker for new release I'm merging this in to unblock upstream PRs. Manual
test is required post feature freeze.

@fffonion fffonion merged commit ae4a939 into main Nov 17, 2022
@fffonion fffonion deleted the fix/s3 branch November 17, 2022 10:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants