Skip to content

fix: Improve the nacos legacy config checking logic #170

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 1 commit into from
Jun 12, 2025

Conversation

CH3CHO
Copy link
Collaborator

@CH3CHO CH3CHO commented Jun 3, 2025

Background

  1. When using Nacos as the config storage, Higress API Server encrypts all the Secret data before writing to Nacos. A random encryption key will be generated during installation unless explictly provided.
  2. Deleting a Nacos namespace won't clear any config items in it cascadingly.

Current Situtation

User installs Higress to an empty Nacos instance, deletes the "higress-system" namespace from Nacos directly and tries to install Higress again.

During installation, since "higress-system" namespace doesn't exist in Nacos, init.sh won't check if there are any Secret resource existed in Nacos. Then the start-up process will get stuck because the API Server is unable to decrypt existed Secret data using the newly generated encryption key.

Solution

No matter whether the target namespace exists or not in Nacos, init.sh perform the Secret resource check anyway.

Extra Notes

Please be aware that some format issues are fixed in the PR as well.

@CH3CHO CH3CHO requested a review from johnlanni as a code owner June 3, 2025 07:32
Copy link

lingma-agents bot commented Jun 3, 2025

修复Nacos配置检查逻辑以避免加密密钥冲突

变更文件

文件路径 变更说明
compose/scripts/init.sh 1. 在initializeNacos函数中强制检查Nacos命名空间内的Secret配置,即使命名空间不存在也进行检测; 2. 重构存储类型处理逻辑,统一使用`case`分支并新增默认错误提示; 3. 优化Nacos访问令牌获取方式,修复命令执行语法问题; 4. 调整加密密钥生成策略,确保32字节安全随机生成; 5. 增强错误状态码检测,对配置检查失败、命名空间创建失败等场景添加明确退出机制。

时序图

sequenceDiagram
    participant InitScript as init.sh
    InitScript->>InitScript: initializeNacos()
    InitScript->>NacosServer: 检查Nacos服务可用性
    alt 服务就绪
        InitScript->>NacosServer: 验证命名空间存在性
        InitScript->>NacosServer: 检查遗留配置项(secrets.__names__)
        InitScript-->>InitScript: 若检测到配置则终止安装
        InitScript->>NacosServer: 创建缺失的命名空间
    else 超时未就绪
        InitScript-->>InitScript: 抛出错误并退出
    end
    InitScript->>InitScript: 生成加密密钥文件
    InitScript->>InitScript: 启动API服务
Loading

💡 小贴士

与 lingma-agents 交流的方式

📜 直接回复评论
直接回复本条评论,lingma-agents 将自动处理您的请求。例如:

  • 在当前代码中添加详细的注释说明。

  • 请详细介绍一下你说的 LRU 改造方案,并使用伪代码加以说明。

📜 在代码行处标记
在文件的特定位置创建评论并 @lingma-agents。例如:

  • @Lingma-Agent 分析这个方法的性能瓶颈并提供优化建议。

  • @Lingma-Agent 对这个方法生成优化代码。

📜 在讨论中提问
在任何讨论中 @lingma-agents 来获取帮助。例如:

  • @Lingma-Agent 请总结上述讨论并提出解决方案。

  • @Lingma-Agent 请根据讨论内容生成优化代码。

Copy link

@lingma-agents lingma-agents bot left a comment

Choose a reason for hiding this comment

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

🔍 代码评审报告

🎯 评审意见概览

严重度 数量 说明
🔴 Blocker 0 阻断性问题,需立即修复。例如:系统崩溃、关键功能不可用或严重安全漏洞。
🟠 Critical 1 严重问题,高优先级修复。例如:核心功能异常或性能瓶颈影响用户体验。
🟡 Major 1 主要问题,建议修复。例如:非核心功能缺陷或代码维护性较差。
🟢 Minor 1 次要问题,酌情优化。例如:代码格式不规范或注释缺失。

总计: 3 个问题


📋 评审意见详情

💡 单文件建议

以下是文件级别的代码建议,聚焦于代码的可读性、可维护性和潜在问题。
📜 compose/scripts/init.sh (3 💬)

🚀 跨文件建议

以下是对代码架构和设计的综合分析,聚焦于跨文件交互、系统一致性和潜在优化空间。
🔍 1. 敏感信息传输风险:Nacos认证参数以明文方式通过命令行传递

在获取Nacos访问令牌时,使用curl命令直接传递NACOS_USERNAME和NACOS_PASSWORD环境变量。虽然curl使用POST方式发送数据,但命令行参数可能被记录到系统日志中,导致敏感信息泄露风险。建议采用更安全的认证方式(如API密钥轮换或环境变量加密存储),或通过中间代理层进行参数处理。

📌 关键代码:

+    NACOS_ACCESS_TOKEN="$(curl -s ... --data-urlencode "username=${NACOS_USERNAME}" --data-urlencode "password=${NACOS_PASSWORD}" ...)"

⚠️ 潜在风险: 环境变量泄露可能导致Nacos服务器被非法访问,敏感配置数据可能被窃取

🔍 2. 配置检查逻辑未进行函数封装导致可维护性下降

Nacos命名空间配置检查逻辑(如检测secrets.__names__是否存在)直接内嵌在initializeNacos函数中,未进行模块化封装。这将导致代码冗余和维护困难,当后续需要调整检查逻辑时需修改多处代码。建议将配置检查逻辑封装为独立函数,并提供可复用的错误处理模块。

📌 关键代码:

if [ $statusCode -eq 200 ]; then ... fi

⚠️ 潜在风险: 代码维护成本增加,逻辑变更时容易引入不一致性

🔍 3. HTTP请求增加对初始化性能产生潜在影响

新增的配置检查步骤(如检测现有配置项、创建命名空间)引入额外的curl请求。在高延迟网络环境下或大规模部署场景中,这些新增的HTTP调用可能导致初始化时间显著增加。建议通过批量请求或缓存机制优化请求流程。

📌 关键代码:

curl -s "${NACOS_SERVER_URL}/v1/console/namespaces..."

⚠️ 潜在风险: 初始化超时风险增加,影响系统部署成功率

🔍 4. 未验证依赖工具的存在性可能导致执行失败

脚本直接使用curl和jq命令,但未检查这些工具是否存在于目标执行环境中。在某些受限环境或最小化安装系统中,可能导致命令未找到错误。建议在脚本开头添加依赖检测机制,或通过包管理器确保工具可用性。

📌 关键代码:

openssl enc -base64...

⚠️ 潜在风险: 在缺少curl或jq的环境中脚本无法执行,导致部署失败


💡 小贴士

与 lingma-agents 交流的方式

📜 直接回复评论
直接回复本条评论,lingma-agents 将自动处理您的请求。例如:

  • 在当前代码中添加详细的注释说明。

  • 请详细介绍一下你说的 LRU 改造方案,并使用伪代码加以说明。

📜 在代码行处标记
在文件的特定位置创建评论并 @lingma-agents。例如:

  • @Lingma-Agent 分析这个方法的性能瓶颈并提供优化建议。

  • @Lingma-Agent 对这个方法生成优化代码。

📜 在讨论中提问
在任何讨论中 @lingma-agents 来获取帮助。例如:

  • @Lingma-Agent 请总结上述讨论并提出解决方案。

  • @Lingma-Agent 请根据讨论内容生成优化代码。

Copy link
Contributor

@johnlanni johnlanni left a comment

Choose a reason for hiding this comment

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

LGTM

@CH3CHO CH3CHO force-pushed the fix/nacos-enc-key branch from 4e7a23d to 0ac551b Compare June 12, 2025 07:11
Copy link
Contributor

@johnlanni johnlanni left a comment

Choose a reason for hiding this comment

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

LGTM

@johnlanni johnlanni merged commit fda0055 into higress-group:main Jun 12, 2025
2 checks passed
@CH3CHO CH3CHO deleted the fix/nacos-enc-key branch June 12, 2025 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