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

refactor: Alibaba Cloud API #5294

Merged
merged 4 commits into from
Sep 20, 2024
Merged

Conversation

PMExtra
Copy link
Contributor

@PMExtra PMExtra commented Sep 19, 2024

#5205 (comment)

@Neilpang 这是基于以上讨论的初步修改版本,尚未测试,只是先看下这个重构思路是否符合预期。

Copy link

Welcome
First thing: don't send PR to the master branch, please send to the dev branch instead.
Please make sure you've read our DNS API Dev Guide and DNS-API-Test.
Then reply on this message, otherwise, your code will not be reviewed or merged.
We look forward to reviewing your Pull request shortly ✨
注意: 必须通过了 DNS-API-Test 才会被 review. 无论是修改, 还是新加的 dns api, 都必须确保通过这个测试.

return 1
fi

# shellcheck disable=SC2034
Ali_API="https://cdn.aliyuncs.com/"
Copy link
Member

Choose a reason for hiding this comment

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

两边不要定义重名的变量. 这个改个名.
然后修改 _ali_rest() 把api传进去就好了.

@Neilpang
Copy link
Member

基本没啥问题, 继续.

@PMExtra
Copy link
Contributor Author

PMExtra commented Sep 19, 2024

@Neilpang 你看下现在这个模式可以吗?参考每个 rest 之前通过一个函数设置 query 的方式,在同一个函数中设置 API的 endpoint

@PMExtra
Copy link
Contributor Author

PMExtra commented Sep 19, 2024

@Neilpang 我跑了 DNS-API 的测试,发现部分用例(FreeBSD和OpenBSD)会添加值为 acmeTestTxtRecord_ 的TXT记录(后面没有随机字符了,就是到下划线就结束了),而且这个记录似乎没有被自动删除。导致我不得不人工清理解析记录,然后重跑测试才能通过。

日志如下:

  [Thu Sep 19 10:00:36 UTC 2024] _sub_domain='acmetestXyzRandomName.test'
  [Thu Sep 19 10:00:36 UTC 2024] _domain='acme.jubeat.net'
  [Thu Sep 19 10:00:36 UTC 2024] Add record
  [Thu Sep 19 10:00:37 UTC 2024] GET
  [Thu Sep 19 10:00:37 UTC 2024] url='https://alidns.aliyuncs.com/?Signature=P6mHxOtNnmImFNytWBWutcnFSS0%3D&AccessKeyId=***&Action=AddDomainRecord&DomainName=acme.jubeat.net&Format=json&RR=acmetestXyzRandomName.test&SignatureMethod=HMAC-SHA1&SignatureNonce=1726740036N&SignatureVersion=1.0&Timestamp=2024-09-19T10%3A00%3A36Z&Type=TXT&Value=acmeTestTxtRecord_&Version=2015-01-09'
  [Thu Sep 19 10:00:37 UTC 2024] timeout=
  [Thu Sep 19 10:00:37 UTC 2024] _CURL='curl --silent --dump-header /root/.acme.sh/http.header  -L  -g '
  [Thu Sep 19 10:00:38 UTC 2024] ret='0'
  [Thu Sep 19 10:00:38 UTC 2024] response='{"RequestId":"8831E7B6-DAFD-5C7E-A847-1302F6BAB45B","HostId":"alidns.aliyuncs.com","Code":"DomainRecordDuplicate","Message":"The DNS record already exists.","Recommend":"https://api.aliyun.com/troubleshoot?q=DomainRecordDuplicate&product=Alidns&requestId=8831E7B6-DAFD-5C7E-A847-1302F6BAB45B"}'
  [Thu Sep 19 10:00:38 UTC 2024] The DNS record already exists.
  Run Failed

我觉得是测试脚本的问题,这个错误看起来和我当前的改动没有关系,而且在这之前有大量的测试用例都能成功通过。

事实上我也把我当前的分支拉取到正式环境中进行了一次 force renew 测试,能够正确签发并部署新证书到阿里云CDN。

@Neilpang
Copy link
Member

我一会修测试

@Neilpang
Copy link
Member

修了 , 你再试试

@PMExtra
Copy link
Contributor Author

PMExtra commented Sep 20, 2024

@Neilpang https://github.com/PMExtra/acme.sh/actions/runs/10937323435

Actions 测试完全通过了,实际部署我也验证过了,如果没有别的问题,我认为可以合入了。

Ali_API="https://cdn.aliyuncs.com/"
# Load dnsapi/dns_ali.sh to reduce the duplicated codes
# https://github.com/acmesh-official/acme.sh/pull/5205#issuecomment-2357867276
dnsapi_ali="$(_findHook "" "$_SUB_FOLDER_DNSAPI" dns_ali)"
Copy link
Member

Choose a reason for hiding this comment

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

把这段加载代码放到 ali_cdn_deploy 函数里面去, 这样 _findHook 第一个参数可以传 "$_cdomain"

#################### Private functions below ##################################
#################### Alibaba Cloud common functions below ####################

_prepare_ali_credentials() {
Copy link
Member

Choose a reason for hiding this comment

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

这个函数的所有调用地方, 都没有判返回值. 如果出错了,没法中止

@Neilpang Neilpang merged commit 114eb62 into acmesh-official:dev Sep 20, 2024
43 checks passed
@PMExtra
Copy link
Contributor Author

PMExtra commented Sep 20, 2024

@Neilpang 随机串为空的问题似乎没有完全解决,在MacOS测试上又出现了。https://github.com/PMExtra/acme.sh/actions/runs/10954656359/job/30418361264

@Neilpang
Copy link
Member

貌似域名已经随机了. 你检查一下是不是别的问题.

@PMExtra
Copy link
Contributor Author

PMExtra commented Sep 20, 2024

https://github.com/acmesh-official/acmetest/blob/master/letest.sh#L1504-L1507

acmetestXyzRandomName 是一个常量,没有真的做到 random 。

测试过后会残留一些解析记录,其中问题最大的就是这个没有后缀的 acmeTestTxtRecord_

image

@PMExtra
Copy link
Contributor Author

PMExtra commented Sep 20, 2024

我确认过,我修改后的 dns_ali 至少在大多数情况下 dns_ali_rm 函数是工作正常的。

我试过正常走一遍 issue 或者 --renew --force 的流程,执行成功是不会残留 TXT 记录的。

这些残留的TXT记录应该是只在 acmetest 下出现。

@PMExtra
Copy link
Contributor Author

PMExtra commented Sep 20, 2024

从 API 审计日志来看,似乎每个测试都是调用了三次 Add 但是只调用两次 Delete 。

image

我再看看 job 的日志。

@Neilpang
Copy link
Member

这个名字不会随机: acmetestXyzRandomName
只是对应的txt 内容随机. 这是为了测试 域名重复, 但 txt 不同的多条记录的.

@PMExtra
Copy link
Contributor Author

PMExtra commented Sep 20, 2024

我感觉现在有三个问题:

  1. macOS 下,仍然会发生 tr: Illegal byte sequence。(虽然我自己在 macOS 上实测 LC_CTYPE=C 可以解决问题,但不知道为什么 GitHub Actions 里面仍然发生)
  2. OpenBSD 系统中,head 命令不支持 -c 参数。
  3. 不知道为什么,https://github.com/acmesh-official/acmetest/blob/master/letest.sh#L1507 的删除操作似乎没有执行。

对于 1 和 2,要不考虑一下 dd if=/dev/urandom bs=1 count=9 2>/dev/null | base64 | tr '+/' '-_'

事实上 tr 可能是多余的,根据 RFC1464

All printable ASCII characters are permitted in the attribute value

所以 dnsapi 应当正确处理包含符号的 TXT 记录,对特殊符号进行转义或者URL编码。

对于 3 我还在找原因。

@Neilpang
Copy link
Member

可以

@PMExtra PMExtra deleted the refactor/ali_api branch September 23, 2024 07:20
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.

2 participants