-
Notifications
You must be signed in to change notification settings - Fork 394
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
<fix>[conf]: Isolating ansible with python venv #1347
base: master
Are you sure you want to change the base?
Conversation
Since there will be manually installed python pacakges at the system level, this may cause ansible installnation failed due to conflicts. Therefore, install ansible into a new python virtual environment at /var/lib/zstack/virtualenv/zstacksys. Resolves: ZSTAC-56494 Change-Id: I6a6165736a62686b61736c6963787765636c7670
Walkthrough本次更新主要涉及 Changes
TipsChat with CodeRabbit Bot (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Files selected for processing (2)
- conf/tools/install.sh (1 hunks)
- core/src/main/java/org/zstack/core/ansible/AnsibleFacadeImpl.java (2 hunks)
Additional comments: 1
core/src/main/java/org/zstack/core/ansible/AnsibleFacadeImpl.java (1)
- 244-258: 在
run
函数中,激活虚拟环境的命令是通过源激活脚本来完成的,这是标准做法。但是,需要确保在执行 Ansible 命令之前,虚拟环境被正确激活,并且环境变量没有被不当地修改。
placePip703(); | ||
placeAnsible4100(); | ||
|
||
ShellUtils.run(String.format("if ! sudo ansible --version | grep -q 'core 2.11.12'; then " + | ||
"if grep -i -s -E 'centos|rocky' /etc/system-release; then " + | ||
"sudo yum remove -y ansible; " + | ||
"elif grep -i -s ubuntu /etc/issue; then " + | ||
"sudo apt-get --assume-yes remove ansible; " + | ||
"else echo \"Warning: can't remove ansible from unknown platform\"; " + | ||
placeAnsible196(); | ||
|
||
ShellUtils.run(String.format( | ||
"NEED_INSTALL=false; " + | ||
"if [ -d /var/lib/zstack/virtualenv/zstacksys ]; then " + | ||
". /var/lib/zstack/virtualenv/zstacksys/bin/activate; " + | ||
"if ! ansible --version | grep -q 'core 2.11.12'; then " + | ||
"deactivate; " + | ||
"NEED_INSTALL=true; " + | ||
"fi; " + | ||
"sudo pip uninstall -y ansible; " + | ||
"sudo pip install -i file://%s --trusted-host localhost -I setuptools==39.2.0; " + | ||
"sudo pip install -i file://%s --trusted-host localhost -I ansible==4.10.0; " + | ||
"fi", AnsibleConstant.PYPI_REPO, AnsibleConstant.PYPI_REPO), false); | ||
"else " + | ||
"NEED_INSTALL=true; "+ | ||
"fi; " + | ||
"if $NEED_INSTALL; then " + | ||
"sudo bash -c 'rm -rf /var/lib/zstack/virtualenv/zstacksys && virtualenv /var/lib/zstack/virtualenv/zstacksys --python=python2.7; "+ | ||
". /var/lib/zstack/virtualenv/zstacksys/bin/activate; "+ | ||
"pip install -i file://%s --trusted-host localhost -I setuptools==39.2.0; "+ | ||
"pip install -i file://%s --trusted-host localhost -I ansible==4.10.0'; "+ | ||
"fi" , AnsibleConstant.PYPI_REPO, AnsibleConstant.PYPI_REPO), false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
在 init
函数中,检查 Ansible 版本的逻辑似乎是硬编码的,这可能会在未来的更新中导致问题。此外,脚本中使用 sudo
可能会带来安全风险。建议使用配置文件或环境变量来管理 Ansible 版本,以及审查 sudo
的使用,确保它是必要的并且安全。
- if ! ansible --version | grep -q 'core 2.11.12'; then
+ # 应该使用配置文件或环境变量来管理版本
- sudo bash -c 'rm -rf /var/lib/zstack/virtualenv/zstacksys && virtualenv /var/lib/zstack/virtualenv/zstacksys --python=python2.7;
+ # 审查 sudo 的使用,确保它是必要的并且安全
Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
placePip703(); | |
placeAnsible4100(); | |
ShellUtils.run(String.format("if ! sudo ansible --version | grep -q 'core 2.11.12'; then " + | |
"if grep -i -s -E 'centos|rocky' /etc/system-release; then " + | |
"sudo yum remove -y ansible; " + | |
"elif grep -i -s ubuntu /etc/issue; then " + | |
"sudo apt-get --assume-yes remove ansible; " + | |
"else echo \"Warning: can't remove ansible from unknown platform\"; " + | |
placeAnsible196(); | |
ShellUtils.run(String.format( | |
"NEED_INSTALL=false; " + | |
"if [ -d /var/lib/zstack/virtualenv/zstacksys ]; then " + | |
". /var/lib/zstack/virtualenv/zstacksys/bin/activate; " + | |
"if ! ansible --version | grep -q 'core 2.11.12'; then " + | |
"deactivate; " + | |
"NEED_INSTALL=true; " + | |
"fi; " + | |
"sudo pip uninstall -y ansible; " + | |
"sudo pip install -i file://%s --trusted-host localhost -I setuptools==39.2.0; " + | |
"sudo pip install -i file://%s --trusted-host localhost -I ansible==4.10.0; " + | |
"fi", AnsibleConstant.PYPI_REPO, AnsibleConstant.PYPI_REPO), false); | |
"else " + | |
"NEED_INSTALL=true; "+ | |
"fi; " + | |
"if $NEED_INSTALL; then " + | |
"sudo bash -c 'rm -rf /var/lib/zstack/virtualenv/zstacksys && virtualenv /var/lib/zstack/virtualenv/zstacksys --python=python2.7; "+ | |
". /var/lib/zstack/virtualenv/zstacksys/bin/activate; "+ | |
"pip install -i file://%s --trusted-host localhost -I setuptools==39.2.0; "+ | |
"pip install -i file://%s --trusted-host localhost -I ansible==4.10.0'; "+ | |
"fi" , AnsibleConstant.PYPI_REPO, AnsibleConstant.PYPI_REPO), false); | |
placePip703(); | |
placeAnsible196(); | |
ShellUtils.run(String.format( | |
"NEED_INSTALL=false; " + | |
"if [ -d /var/lib/zstack/virtualenv/zstacksys ]; then " + | |
". /var/lib/zstack/virtualenv/zstacksys/bin/activate; " + | |
"# 应该使用配置文件或环境变量来管理版本 " + | |
"if ! ansible --version | grep -q 'core 2.11.12'; then " + | |
"deactivate; " + | |
"NEED_INSTALL=true; " + | |
"fi; " + | |
"else " + | |
"NEED_INSTALL=true; "+ | |
"fi; " + | |
"if $NEED_INSTALL; then " + | |
"# 审查 sudo 的使用,确保它是必要的并且安全 " + | |
"sudo bash -c 'rm -rf /var/lib/zstack/virtualenv/zstacksys && virtualenv /var/lib/zstack/virtualenv/zstacksys --python=python2.7; "+ | |
". /var/lib/zstack/virtualenv/zstacksys/bin/activate; "+ | |
"pip install -i file://%s --trusted-host localhost -I setuptools==39.2.0; "+ | |
"pip install -i file://%s --trusted-host localhost -I ansible==4.10.0'; "+ | |
"fi" , AnsibleConstant.PYPI_REPO, AnsibleConstant.PYPI_REPO), false); |
conf/tools/install.sh
Outdated
elif [ $tool = 'zstack-sys' ]; then | ||
SYS_VIRENV_PATH=/var/lib/zstack/virtualenv/zstacksys | ||
NEED_INSTALL=false | ||
if [ -d $SYS_VIRENV_PATH ]; then | ||
. $SYS_VIRENV_PATH/bin/activate | ||
if ! ansible --version | grep -q 'core 2.11.12.3'; then | ||
deactivate | ||
NEED_INSTALL=true | ||
fi | ||
else | ||
NEED_INSTALL=true | ||
fi | ||
if $NEED_INSTALL; then | ||
rm -rf $SYS_VIRENV_PATH && virtualenv $SYS_VIRENV_PATH --python=python2.7 || exit 1 | ||
. $SYS_VIRENV_PATH/bin/activate | ||
cd $cwd | ||
pip install -i $pypi_path --trusted-host localhost --ignore-installed setuptools==39.2.0 || exit 1 | ||
pip install -i $pypi_path --trusted-host localhost --ignore-installed ansible==4.10.0 || exit 1 | ||
|
||
cat > /usr/bin/ansible << EOF | ||
#! /bin/sh | ||
VIRTUAL_ENV=/var/lib/zstack/virtualenv/zstacksys | ||
if [ ! -d $VIRTUAL_ENV ]; then | ||
echo "Need to install zstacksys before using it" | ||
exit 1 | ||
fi | ||
|
||
LANG=en_US.UTF-8 | ||
LC_ALL=en_US.utf8 | ||
export LANG LC_ALL | ||
. ${VIRTUAL_ENV}/bin/activate | ||
|
||
ansible \$@ | ||
EOF | ||
chmod +x /usr/bin/ansible | ||
|
||
cat > /usr/bin/ansible-playbook << EOF | ||
#! /bin/sh | ||
VIRTUAL_ENV=/var/lib/zstack/virtualenv/zstacksys | ||
if [ ! -d $VIRTUAL_ENV ]; then | ||
echo "Need to install zstacksys before using it" | ||
exit 1 | ||
fi | ||
|
||
LANG=en_US.UTF-8 | ||
LC_ALL=en_US.utf8 | ||
export LANG LC_ALL | ||
. ${VIRTUAL_ENV}/bin/activate | ||
|
||
ansible-playbook \$@ | ||
EOF | ||
chmod +x /usr/bin/ansible-playbook | ||
fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
在安装 Ansible 时,脚本直接写入 /usr/bin
,这可能需要 root 权限。如果脚本没有以适当的权限运行,这可能会导致安全问题或失败。建议检查脚本是否以 root 权限运行,或者提供一个更安全的方式来创建这些可执行脚本,例如使用系统的包管理器或将它们放在非全局路径中。
conf/tools/install.sh
Outdated
NEED_INSTALL=true | ||
fi | ||
if $NEED_INSTALL; then | ||
rm -rf $SYS_VIRENV_PATH && virtualenv $SYS_VIRENV_PATH --python=python2.7 || exit 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
脚本使用 rm -rf
来删除现有的虚拟环境,这是一个破坏性操作,如果路径中包含错误或变量未设置,可能会非常危险。建议在执行此操作之前添加更多的检查,以确保路径是正确的,并且变量已经设置。
脚本使用了 virtualenv --python=python2.7
,这表明它依赖于 Python 2.7。由于 Python 2.7 已经达到生命周期的终点,这可能是一个潜在的问题。如果可能,建议将脚本迁移到 Python 3,以确保长期的兼容性和安全性。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
删除之前检查什么呢
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MatheMatrix 在执行删除操作之前,应该检查 SYS_VIRENV_PATH
变量确实已经被设置,并且不是指向根目录(/
)或用户的主目录(~
)。这可以通过添加一些条件语句来实现,例如:
if [ -n "$SYS_VIRENV_PATH" ] && [ "$SYS_VIRENV_PATH" != "/" ] && [ "$SYS_VIRENV_PATH" != "~" ]; then
rm -rf $SYS_VIRENV_PATH
else
echo "Dangerous SYS_VIRENV_PATH set: '$SYS_VIRENV_PATH'. Aborting."
exit 1
fi
这样可以防止脚本在变量为空或指向关键系统目录时执行危险的删除操作。
Since there will be manually installed python pacakges at the system level, this may cause ansible installnation failed due to conflicts. Therefore, install ansible into a new python virtual environment at /var/lib/zstack/virtualenv/zstacksys.
Resolves: ZSTAC-56494
Change-Id: I6a6165736a62686b61736c6963787765636c7670
Summary by CodeRabbit
新功能
setuptools
的安装。改进