Skip to content

fix: cni-plugins backward compatibility - continue #7881

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.

Sign up for GitHub

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
djsly merged 11 commits into from
Feb 16, 2026
Merged

fix: cni-plugins backward compatibility - continue #7881

Changes from 4 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
7790e27
fix: cni-plugins backward compatibility - continue
fseldow Feb 16, 2026
cb01226
Update parts/linux/cloud-init/artifacts/cse_install.sh
fseldow Feb 16, 2026
94299c6
f
fseldow Feb 16, 2026
2ac15ec
ff
fseldow Feb 16, 2026
daae386
data
fseldow Feb 16, 2026
d04709c
mini change
fseldow Feb 16, 2026
c5c3618
data
fseldow Feb 16, 2026
67f45bf
word
fseldow Feb 16, 2026
680e1c2
data
fseldow Feb 16, 2026
3b6d89a
update
fseldow Feb 16, 2026
cf37a68
data
fseldow Feb 16, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 43 additions & 2 deletions parts/linux/cloud-init/artifacts/cse_install.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,8 +138,8 @@ installContainerRuntime() {
installFixedCNI() {
# Old versions of VHDs will not have components.json. If it does not exist, we will try our best to download the hardcoded version for CNI here during provisioning.
# Network Isolated Cluster / Bring Your Own ACR will not work with a vhd that requires a hardcoded CNI download.
if [ ! -f "$COMPONENTS_FILEPATH" ] || [ -z "$(jq -r '.Packages[] | select(.name == "containernetworking-plugins") | .name' < $COMPONENTS_FILEPATH)" ]; then
echo "WARNING: no containernetworking-plugins components present falling back to hard coded download of 1.4.1"
if [ ! -f "$COMPONENTS_FILEPATH" ] || [ -z "$(jq -r '.Packages[] | select(.name == "containernetworking-plugins" or .name == "cni-plugins") | .name' < $COMPONENTS_FILEPATH)" ]; then
echo "WARNING: no containernetworking-plugins or cni-plugins component present, falling back to hard coded download of 1.4.1"
# handles amd64 and arm64 via CPU_ARCH
if [ -z "${CPU_ARCH:-}" ]; then
CPU_ARCH="$(getCPUArch)"
Expand All @@ -148,6 +148,47 @@ installFixedCNI() {
extract_tarball "${CNI_DOWNLOADS_DIR}/refcni.tar.gz" "$CNI_BIN_DIR"
return
fi
#always just use what is listed in components.json so we don't have to sync.
if [ -z "$(jq -r '.Packages[] | select(.name == "containernetworking-plugins") | .name' < $COMPONENTS_FILEPATH)" ]; then
cniPackage=$(jq ".Packages" "$COMPONENTS_FILEPATH" | jq ".[] | select(.name == \"cni-plugins\")") || exit $ERR_CNI_VERSION_INVALID

#CNI doesn't really care about this but wanted to reuse updatePackageVersions which requires it.
os=${UBUNTU_OS_NAME}
if [ -z "$UBUNTU_RELEASE" ]; then
os=${OS}
os_version="current"
fi
os_version="${UBUNTU_RELEASE}"
Copy link

Copilot AI Feb 16, 2026

Choose a reason for hiding this comment

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

The os_version assignment logic has a bug. Line 161 unconditionally overwrites the os_version set in line 159. This means when UBUNTU_RELEASE is empty (non-Ubuntu systems), os_version will be set to an empty string instead of "current". The logic should match the pattern used in installContainerdWithComponentsJson (lines 53-59) where os_version is only set to UBUNTU_RELEASE when it's not empty. The corrected logic should be:

os=${UBUNTU_OS_NAME}
os_version="${UBUNTU_RELEASE}"
if [ -z "$UBUNTU_RELEASE" ]; then
    os=${OS}
    os_version="current"
fi
Suggested change
if [ -z "$UBUNTU_RELEASE" ]; then
os=${OS}
os_version="current"
fi
os_version="${UBUNTU_RELEASE}"
os_version="${UBUNTU_RELEASE}"
if [ -z "$UBUNTU_RELEASE" ]; then
os=${OS}
os_version="current"
fi

Copilot uses AI. Check for mistakes.
if isMarinerOrAzureLinux "${OS}" && [ "${IS_KATA}" = "true" ]; then
os=${MARINER_KATA_OS_NAME}
fi
Copy link

Copilot AI Feb 16, 2026

Choose a reason for hiding this comment

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

The logic in line 162-163 only handles the Kata case for setting os to MARINER_KATA_OS_NAME, but it doesn't handle the Azure Linux Kata case (AZURELINUX_KATA_OS_NAME). The containerd installation function (lines 62-67) checks both isMariner and isAzureLinux separately for Kata. This code should follow the same pattern:

if isMarinerOrAzureLinux "${OS}" && [ "${IS_KATA}" = "true" ]; then
    os=${MARINER_KATA_OS_NAME}
fi
if isAzureLinux "${OS}" && [ "${IS_KATA}" = "true" ]; then
    os=${AZURELINUX_KATA_OS_NAME}
fi

Note that isMarinerOrAzureLinux returns true for both Mariner and Azure Linux, so the current code sets os to MARINER_KATA_OS_NAME even when OS is Azure Linux, which is incorrect.

Suggested change
fi
fi
if isAzureLinux "${OS}" && [ "${IS_KATA}" = "true" ]; then
os=${AZURELINUX_KATA_OS_NAME}
fi

Copilot uses AI. Check for mistakes.
updatePackageVersions "${cniPackage}" "${os}" "${os_version}" "${OS_VARIANT}"

#should change to ne
Copy link

Copilot AI Feb 16, 2026

Choose a reason for hiding this comment

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

Unclear comment. Line 167 says "#should change to ne" but it's not clear what "ne" means or what should be changed. This appears to be an incomplete thought or TODO that should either be completed or removed.

Suggested change
#should change to ne

Copilot uses AI. Check for mistakes.
# shellcheck disable=SC3010
if [[ ${#PACKAGE_VERSIONS[@]} -gt 1 ]]; then
echo "WARNING: containerd package versions array has more than one element. Installing the last element in the array."
Copy link

Copilot AI Feb 16, 2026

Choose a reason for hiding this comment

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

The error message refers to "containerd" but this code is installing CNI plugins. The message should say "CNI plugins package versions array has more than one element" instead of "containerd package versions array has more than one element".

Suggested change
echo "WARNING: containerd package versions array has more than one element. Installing the last element in the array."
echo "WARNING: CNI plugins package versions array has more than one element. Installing the last element in the array."

Copilot uses AI. Check for mistakes.
exit $ERR_CONTAINERD_VERSION_INVALID
fi
packageVersion=${PACKAGE_VERSIONS[0]}
Copy link

Copilot AI Feb 16, 2026

Choose a reason for hiding this comment

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

Missing validation for empty PACKAGE_VERSIONS array or value. The code directly accesses PACKAGE_VERSIONS[0] without checking if the array is empty or contains the special value. Following the pattern in installContainerdWithComponentsJson (lines 77-80), this code should include a check like:

if [[ ${#PACKAGE_VERSIONS[@]} -eq 0 || ${PACKAGE_VERSIONS[0]} == "<SKIP>" ]]; then
    echo "INFO: cni-plugins package versions array is either empty or the first element is <SKIP>. Skipping cni-plugins installation."
    return 0
fi

Without this check, the script will fail with an unbound variable error or use an invalid version if the array is empty.

Copilot uses AI. Check for mistakes.

# Is there a ${arch} variable I can use instead of the iff
Copy link

Copilot AI Feb 16, 2026

Choose a reason for hiding this comment

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

Typo in comment: "iff" should be "if". The comment currently reads "Is there a ${arch} variable I can use instead of the iff" but should say "Is there a ${arch} variable I can use instead of the if".

Suggested change
# Is there a ${arch} variable I can use instead of the iff
# Is there a ${arch} variable I can use instead of the if

Copilot uses AI. Check for mistakes.
if [ "$(isARM64)" -eq 1 ]; then
CNI_DIR_TMP="cni-plugins-linux-arm64-v${packageVersion}"
else
CNI_DIR_TMP="cni-plugins-linux-amd64-v${packageVersion}"
fi

if [ -d "$CNI_DOWNLOADS_DIR/${CNI_DIR_TMP}" ]; then
#not clear to me when this would ever happen. assume its related to the line above Latest VHD should have the untar, older should have the tgz.
mv ${CNI_DOWNLOADS_DIR}/${CNI_DIR_TMP}/* $CNI_BIN_DIR
else
echo "CNI tarball should already be unzipped by components.json"
exit $ERR_CNI_VERSION_INVALID
fi

chown -R root:root $CNI_BIN_DIR
fi
Copy link

Copilot AI Feb 16, 2026

Choose a reason for hiding this comment

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

Logic error: This code assumes "cni-plugins" package is cached in the VHD downloads directory, but according to the VHD build script (vhdbuilder/packer/install-dependencies.sh:335-473), there is NO case handler for "cni-plugins", which means it's never downloaded or extracted during VHD build. The test file (vhdbuilder/packer/test/linux-vhd-content-test.sh:236-238) explicitly states: "cni-plugins is a special case that it is still in components.json for backward compatibility but we don't cache it."

This means the check at line 182 will always fail (directory doesn't exist), and the code will always exit with ERR_CNI_VERSION_INVALID at line 187. The entire code block (lines 152-191) appears to be attempting to handle a "cni-plugins" entry that was never installed on the VHD, making it non-functional.

If the intent is backward compatibility with old VHDs that have "containernetworking-plugins" but new CSE scripts that reference "cni-plugins", the logic is backwards. The code should check if "cni-plugins" exists in components.json AND was actually cached on the VHD during build. Since "cni-plugins" is never cached, this entire code path is broken.

Copilot generated this review using guidance from repository custom instructions.
}

installNetworkPlugin() {
Expand Down
Loading