feat(helm): support cloud ingress deployment#980
Open
likes1234-bro wants to merge 1 commit intoiflytek:mainfrom
Open
feat(helm): support cloud ingress deployment#980likes1234-bro wants to merge 1 commit intoiflytek:mainfrom
likes1234-bro wants to merge 1 commit intoiflytek:mainfrom
Conversation
|
binfan5 seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
Contributor
Author
Contributor
There was a problem hiding this comment.
Code Review
本次 PR 引入了 Gateway NGINX 作为统一入口,并重构了 Helm Chart 以支持云环境下的 Ingress 部署,这是一个很好的架构改进。通过抽离公共 URL 的生成逻辑到 _helpers.tpl,代码的可维护性得到了提升。新增的 values-aliyun.yaml 和 values-huaweicloud.yaml 也为不同云环境的部署提供了便利。
整体来看,代码质量很高。我只发现一个关于 oauth2JwkSetUri 配置的关键问题,它可能会在启用 TLS 的场景下导致认证失败和安全风险。请查看具体的审查意见。
| value: {{ include "astron-agent.minio.publicUrl" $ | quote }} | ||
| {{- else if eq $key "oauth2JwkSetUri" }} | ||
| - name: {{ upper (snakecase $key) }} | ||
| value: {{ printf "http://%s:%d/.well-known/jwks" (include "astron-agent.casdoor.host" $) ($.Values.casdoor.service.port | int) | quote }} |
Contributor
There was a problem hiding this comment.
OAUTH2_JWK_SET_URI 的值被硬编码为使用 http 协议和内部服务地址。然而,OAUTH2_ISSUER_URI 使用的是公共访问地址(可能为 https)。这会导致几个问题:
- 协议不匹配: 当 Ingress 启用 TLS 时,Issuer URI 是
https,但 JWKS URI 是http。这可能会导致 JWT 验证器因协议不匹配而拒绝验证。 - 安全风险: 在生产环境中,通过非加密的
http获取用于验证签名的公钥存在安全风险。 - 主机不匹配: 验证器可能还会校验 JWKS 的来源主机是否与 Issuer 声明中的主机一致,使用内部主机名
astron-agent-casdoor会导致校验失败。
建议修改此处的逻辑,使用公共 URL 来构建 JWKS URI,以确保协议和主机名的一致性和安全性。Gateway NGINX 已经配置了对 /.well-known/ 路径的代理,因此使用公共地址是可行的。
value: {{ printf "%s/.well-known/jwks" (include "astron-agent.casdoor.publicUrl" $) | quote }}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
概述
本次 PR 对 Helm Chart 进行了云环境部署相关改造,使基于 Ingress 的部署路径可以在云上 Kubernetes 环境中正常使用。
变更内容
验证情况
values-aliyun.yaml执行helm lintvalues-huaweicloud.yaml执行helm lintvalues-aliyun.yaml执行helm templatevalues-huaweicloud.yaml执行helm template说明
可供参考的文档和模板下载链接