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

optionally turn off 'S3Disable100Continue' via url query parameter #3228

Merged
merged 4 commits into from
Feb 17, 2023

Conversation

shichanglin5
Copy link
Contributor

问题:
aws-sdk-go 客户端上传文件到 s3 时,如果文件大于 2m 会自动加上请求头 Expect: '100-Continue',但http规范 Expect 的值是 '100-continue' (大小写不一样)

如果请求发生重试,aws-sdk-go 会讲 Expect header 请求头用作签名,由于上面大小写问题,如果对象存储在验签时将 '100-continue' 作为 expect 的值,那么会导致签名校验失败

考虑上面原因 以及 juicefs 实际上传 s3 的都是分块文件比较下,所以这里将 expect 请求头禁用

@CLAassistant
Copy link

CLAassistant commented Feb 11, 2023

CLA assistant check
All committers have signed the CLA.

@davies
Copy link
Contributor

davies commented Feb 11, 2023

可以怎么复现这个问题?

@codecov-commenter
Copy link

codecov-commenter commented Feb 11, 2023

Codecov Report

Base: 55.70% // Head: 55.54% // Decreases project coverage by -0.17% ⚠️

Coverage data is based on head (a7945c4) compared to base (6d7c905).
Patch coverage: 0.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3228      +/-   ##
==========================================
- Coverage   55.70%   55.54%   -0.17%     
==========================================
  Files         142      142              
  Lines       33048    33145      +97     
==========================================
  Hits        18410    18410              
- Misses      12698    12788      +90     
- Partials     1940     1947       +7     
Impacted Files Coverage Δ
cmd/mount.go 55.01% <0.00%> (-0.34%) ⬇️
pkg/meta/dump.go 75.48% <ø> (ø)
pkg/object/s3.go 40.65% <0.00%> (-0.57%) ⬇️
cmd/umount.go 26.51% <0.00%> (-27.73%) ⬇️
pkg/vfs/fill.go 65.21% <0.00%> (-2.46%) ⬇️
pkg/meta/sql.go 58.70% <0.00%> (-0.36%) ⬇️
pkg/chunk/disk_cache.go 73.53% <0.00%> (-0.31%) ⬇️
pkg/meta/redis.go 66.25% <0.00%> (-0.08%) ⬇️
cmd/objbench.go 58.67% <0.00%> (ø)
... and 21 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@shichanglin5
Copy link
Contributor Author

可以怎么复现这个问题?

选型:
juicefs 版本:最新版本即可
元数据选型:不限
对象存储选型:seaweedfs (最新版本即可)

步骤:

  1. 部署 seaweedfs (参数指定了只能分配1个1MB 的 volume,上传文件会由于seaweedfs空间不足而响应 500 状态码)
weed -v=1 server -ip.bind=0.0.0.0 -master.volumeSizeLimitMB=1 -master.volumePreallocate=false -master=true -volume=true -filer=true -s3=true -s3.config=identity.json -iam=false -webdav=false -volume.max=1

同目录下需要两个配置文件
filer.toml

[mysql2]  # or memsql, tidb
enabled = true
createTable = """
  CREATE TABLE IF NOT EXISTS `%s` (
    dirhash BIGINT,
    name VARCHAR(1000) BINARY,
    directory TEXT BINARY,
    meta LONGBLOB,
    PRIMARY KEY (dirhash, name)
  ) DEFAULT CHARSET=utf8;
"""
hostname = "localhost"
port = 3306
username = "root"
password = "6789"
database = "s3"              # create or use an existing database
connection_max_idle = 2
connection_max_open = 100
connection_max_lifetime_seconds = 0
interpolateParams = false
# if insert/upsert failing, you can disable upsert or update query syntax to match your RDBMS syntax:
enableUpsert = true
upsertQuery = """INSERT INTO `%s` (dirhash,name,directory,meta) VALUES(?,?,?,?) ON DUPLICATE KEY UPDATE meta = VALUES(meta)"""

identity.json

{
  "identities": [
    {
      "name": "admin",
      "accountId": "admin",
      "credentials": [
        {
          "access_key": "AKIARVTQO2DJNBALAG2Y",
          "secret_key": "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"
        }
      ],
      "actions": [
        "Admin",
        "Read",
        "Write",
        "List"
      ]
    }
  ]
}
  1. 创建 bucket : jfs

image

  1. 在 nginx 配置 s3 域名
    我们是在通过域名运行 juicefs benchmark 时发现这个问题,当 seaweedfs 磁盘容量满了过后,对于文件上传请求会相应 500状态码,juicefs使用的 aws-sdk-go 的 s3 客户端判断如果是服务端 500 的话,就会重试请求,重试请求时会把 Expect: '100-Continue' (大写)放到签名内容进行签名,而 nginx 默认不会转发 Expect 请求头,当 seaweedfs 解析 Authentication header的时候,发现客户端签名用到了Expect header,但由于 nginx 没有透传于是使用 http 规范的 '100-continue' (小写)作为默认值放到签名内容,因此签名内容不一致,导致最终签名不一致,客户端接收到 403 状态码

  2. 执行 mount

  3. 在 mount 目录下执行命令 (写文件,aws请求s3时请求体大于2M才会添加 Expect 头)

dd if=/dev/zero of=50m bs=10m count=5
  1. 观察日志会发现出现以下内容:

wecom-temp-163132-88c8921cbadb7a08a281efb2e7de4081

@shichanglin5
Copy link
Contributor Author

问题复现三个条件:

  1. 上传文件,请求体大于 2M (满足这个才会添加 Expect:'100-Continue' )
  2. 对象存储由于各种原因返回 500 (这样的 aws-sdk-go 会重试,重试就会将 Expect 放入签名内容)
  3. nginx 转发文件上传请求时不透传 Expect header (nginx默认行为,不透传 seaweedfs 就会使用 '100-continue')

@shichanglin5
Copy link
Contributor Author

aws/aws-sdk-go#4722

@davies
Copy link
Contributor

davies commented Feb 12, 2023

明白了。如果默认去掉 100-continue, 有什么不好的呢?感觉是当没有授权时,可能会浪费带宽?对方访问正常(aws 的 s3 服务),可以用 objbench 测试下有没有性能影响。

目前看起来只对某些s3后端有影响,可能通过 url query 来可选地关闭它比较合适。

@shichanglin5
Copy link
Contributor Author

明白了。如果默认去掉 100-continue, 有什么不好的呢?感觉是当没有授权时,可能会浪费带宽?对方访问正常(aws 的 s3 服务),可以用 objbench 测试下有没有性能影响。

目前看起来只对某些s3后端有影响,可能通过 url query 来可选地关闭它比较合适。

的确有可能会浪费带宽如果没有授权

如果可选的话当然更好,怎么 url query 来控制的?

@davies
Copy link
Contributor

davies commented Feb 14, 2023

endpoint 是 一个 url ,可以参考 tikv.go 里解析 query 的做法

@shichanglin5
Copy link
Contributor Author

endpoint 是 一个 url ,可以参考 tikv.go 里解析 query 的做法

好办法,你看看有什么需要修改的吗

pkg/object/s3.go Outdated Show resolved Hide resolved
@shichanglin5 shichanglin5 changed the title s3 disable expect header optionally turn off 'disable100Continue' via url query parameter Feb 16, 2023
@shichanglin5 shichanglin5 changed the title optionally turn off 'disable100Continue' via url query parameter optionally turn off 'S3Disable100Continue' via url query parameter Feb 16, 2023
@davies davies merged commit 9942d53 into juicedata:main Feb 17, 2023
@SandyXSD SandyXSD mentioned this pull request Mar 28, 2023
@shichanglin5 shichanglin5 deleted the fix_expect_header branch April 20, 2023 06:49
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.

None yet

4 participants