Copilot commented on code in PR #14070:
URL: https://github.com/apache/apisix/pull/14070#discussion_r4355677187
##########
.github/workflows/build.yml:
##########
@@ -183,7 +204,8 @@ jobs:
- name: Linux Script
env:
TEST_FILE_SUB_DIR: ${{ matrix.test_dir }}
- run: sudo -E ./ci/${{ matrix.os_name }}_runner.sh script
+ # sudo-rs (Ubuntu 26.04) ignores -E, so pass what the script needs
explicitly
+ run: sudo -E TEST_FILE_SUB_DIR="$TEST_FILE_SUB_DIR" ./ci/${{
matrix.os_name }}_runner.sh script
Review Comment:
Passing env vars via `sudo VAR=... cmd` can be rejected depending on sudoers
settings/implementation (it often requires `SETENV`). To make this portable
across sudo variants, invoke via `sudo env
TEST_FILE_SUB_DIR=\"$TEST_FILE_SUB_DIR\" ...` or use `sudo
--preserve-env=TEST_FILE_SUB_DIR ...` (when supported).
##########
utils/install-dependencies.sh:
##########
@@ -63,25 +63,33 @@ function install_dependencies_with_yum() {
function install_dependencies_with_apt() {
# add OpenResty source
sudo apt-get update
- sudo apt-get -y install software-properties-common wget lsb-release gnupg
patch
- wget -qO - https://openresty.org/package/pubkey.gpg | sudo apt-key add -
+ sudo apt-get -y install wget lsb-release gnupg patch
+ sudo mkdir -p /etc/apt/keyrings
+ wget -qO - https://openresty.org/package/pubkey.gpg | sudo gpg --dearmor
--yes -o /etc/apt/keyrings/openresty.gpg
Review Comment:
The keyring file permissions can be affected by the executing user's umask;
if it ends up not readable by the `_apt` user, `apt-get update` may fail. After
writing `/etc/apt/keyrings/openresty.gpg`, explicitly `chmod 0644` (or
otherwise ensure it is world-readable) to make apt behavior deterministic.
##########
utils/install-dependencies.sh:
##########
@@ -63,25 +63,33 @@ function install_dependencies_with_yum() {
function install_dependencies_with_apt() {
# add OpenResty source
sudo apt-get update
- sudo apt-get -y install software-properties-common wget lsb-release gnupg
patch
- wget -qO - https://openresty.org/package/pubkey.gpg | sudo apt-key add -
+ sudo apt-get -y install wget lsb-release gnupg patch
+ sudo mkdir -p /etc/apt/keyrings
+ wget -qO - https://openresty.org/package/pubkey.gpg | sudo gpg --dearmor
--yes -o /etc/apt/keyrings/openresty.gpg
arch=$(uname -m | tr '[:upper:]' '[:lower:]')
arch_path=""
if [[ $arch == "arm64" ]] || [[ $arch == "aarch64" ]]; then
arch_path="arm64/"
fi
- if [[ "${1}" == "ubuntu" ]]; then
- sudo add-apt-repository -y "deb
http://openresty.org/package/${arch_path}ubuntu $(lsb_release -sc) main"
- elif [[ "${1}" == "debian" ]]; then
- # add-apt-repository on Debian 12 writes an empty list file for a plain
- # deb line, so the repository never makes it into apt
- echo "deb http://openresty.org/package/${arch_path}debian
$(lsb_release -sc) openresty" \
- | sudo tee /etc/apt/sources.list.d/openresty.list
+ codename=$(lsb_release -sc)
+ repo_url="http://openresty.org/package/${arch_path}${1}";
Review Comment:
The OpenResty repository is configured over plain HTTP, which allows MITM
tampering of repository metadata (even though packages are signed, transport
security still matters for reliability and downgrade/availability risks).
Prefer HTTPS for `repo_url` (and for the `wget --spider` check) if the endpoint
supports it.
##########
utils/install-dependencies.sh:
##########
@@ -63,25 +63,33 @@ function install_dependencies_with_yum() {
function install_dependencies_with_apt() {
# add OpenResty source
sudo apt-get update
- sudo apt-get -y install software-properties-common wget lsb-release gnupg
patch
- wget -qO - https://openresty.org/package/pubkey.gpg | sudo apt-key add -
+ sudo apt-get -y install wget lsb-release gnupg patch
+ sudo mkdir -p /etc/apt/keyrings
+ wget -qO - https://openresty.org/package/pubkey.gpg | sudo gpg --dearmor
--yes -o /etc/apt/keyrings/openresty.gpg
arch=$(uname -m | tr '[:upper:]' '[:lower:]')
arch_path=""
if [[ $arch == "arm64" ]] || [[ $arch == "aarch64" ]]; then
arch_path="arm64/"
fi
- if [[ "${1}" == "ubuntu" ]]; then
- sudo add-apt-repository -y "deb
http://openresty.org/package/${arch_path}ubuntu $(lsb_release -sc) main"
- elif [[ "${1}" == "debian" ]]; then
- # add-apt-repository on Debian 12 writes an empty list file for a plain
- # deb line, so the repository never makes it into apt
- echo "deb http://openresty.org/package/${arch_path}debian
$(lsb_release -sc) openresty" \
- | sudo tee /etc/apt/sources.list.d/openresty.list
+ codename=$(lsb_release -sc)
+ repo_url="http://openresty.org/package/${arch_path}${1}";
+ # OpenResty publishes packages for a new Ubuntu release some time after it
+ # comes out (e.g. 26.04). Only openresty-zlib-dev and openresty-pcre-dev
are
+ # needed from it, and they are self-contained under /usr/local/openresty,
+ # so fall back to the latest LTS repository it has.
+ if [[ "${1}" == "ubuntu" ]] && ! wget -q --spider
"${repo_url}/dists/${codename}/Release"; then
+ codename=noble
fi
+ component=main
+ if [[ "${1}" == "debian" ]]; then
+ component=openresty
+ fi
+ echo "deb [signed-by=/etc/apt/keyrings/openresty.gpg] ${repo_url}
${codename} ${component}" \
Review Comment:
The OpenResty repository is configured over plain HTTP, which allows MITM
tampering of repository metadata (even though packages are signed, transport
security still matters for reliability and downgrade/availability risks).
Prefer HTTPS for `repo_url` (and for the `wget --spider` check) if the endpoint
supports it.
##########
utils/linux-install-luarocks.sh:
##########
@@ -72,3 +72,9 @@ luarocks config variables.OPENSSL_DIR ${OPENSSL_PREFIX}
luarocks config variables.OPENSSL_LIBDIR ${OPENSSL_PREFIX}/lib
luarocks config variables.OPENSSL_INCDIR ${OPENSSL_PREFIX}/include
luarocks config variables.YAML_DIR /usr
+
+# lrexlib-pcre needs PCRE1, which newer distributions (Ubuntu 26.04, Debian 13)
+# no longer ship; use the copy bundled with APISIX runtime instead.
+if [ -f ${OPENRESTY_PREFIX}/pcre/include/pcre.h ]; then
+ luarocks config variables.PCRE_DIR ${OPENRESTY_PREFIX}/pcre
+fi
Review Comment:
Shell variables should be quoted to avoid word-splitting/globbing issues.
Quote `${OPENRESTY_PREFIX}` in both the `-f` check and the `luarocks config`
invocation (and consider quoting the whole path arguments consistently in this
script).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
