[CKS] script to generate cks images with cilium as default CNI#12619
[CKS] script to generate cks images with cilium as default CNI#12619ewerton-silva00 wants to merge 2 commits intoapache:mainfrom
Conversation
|
Congratulations on your first Pull Request and welcome to the Apache CloudStack community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/cloudstack/blob/main/CONTRIBUTING.md)
|
|
@blueorangutan package |
|
@sureshanaparti a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #12619 +/- ##
============================================
+ Coverage 17.90% 17.94% +0.04%
- Complexity 16094 16165 +71
============================================
Files 5938 5939 +1
Lines 532864 533015 +151
Branches 65192 65218 +26
============================================
+ Hits 95392 95649 +257
+ Misses 426793 426638 -155
- Partials 10679 10728 +49
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✖️ debian ✔️ suse15. SL-JID 16769 |
There was a problem hiding this comment.
Pull request overview
Adds a new utility script to generate CloudStack Kubernetes Service (CKS) “binaries ISO” images that bundle Kubernetes components plus manifests/images needed to run with Cilium as the default CNI, addressing the requested Cilium support in #9304.
Changes:
- Introduces
create-kubernetes-binaries-iso-with-cilium.shto build an ISO with Kubernetes binaries, CNI/crictl, addons/manifests, and pre-pulled container images. - Generates the Cilium manifest via Helm with a set of default Cilium configuration values.
- Updates image collection/pulling logic to include images referenced by generated Cilium and Dashboard manifests.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| sudo $PKG_MGR --assumeyes remove docker-common docker container-selinux docker-selinux docker-engine | ||
| sudo $PKG_MGR --assumeyes install lvm2 device-mapper device-mapper-persistent-data device-mapper-event device-mapper-libs device-mapper-event-libs | ||
| sudo $PKG_MGR --assumeyes install http://mirror.centos.org/centos/7/extras/x86_64/Packages/container-selinux-2.107-3.el7.noarch.rpm | ||
| sudo $PKG_MGR --assumeyes install containerd.io | ||
| elif [ -f /etc/debian_version ] || command -v apt-get > /dev/null 2>&1; then |
There was a problem hiding this comment.
RedHat install path downloads a container-selinux RPM from a CentOS 7 x86_64 URL. This will fail (and possibly install an incompatible package) on aarch64/arm64, even though the script advertises arm support. Consider selecting the correct arch/repo or using distro-provided packages/repos instead of a hard-coded x86_64 RPM URL.
shwstppr
left a comment
There was a problem hiding this comment.
@ewerton-silva00 can you please check comments from Copilot and look into failing pre-commit GHA?
|
@shwstppr, yes. I am reviewing the points reported by Copilot. I will make the necessary adjustments, test, and submit the changes. |
* etcd download: use arch variable instead of hard-coded amd64 Replace hard-coded linux-amd64 in etcd tarball download URL and output filename with linux- to support both amd64 and arm64 architectures. * container-selinux: install from distro repos instead of CentOS 7 mirror Remove hard-coded CentOS 7 x86_64 RPM URL for container-selinux and install directly from the system's configured repositories, which handles architecture and version automatically. * build_name: fix dead-code fallback default Check whether parameter is empty before constructing build_name, so the fallback default setup--.iso is actually reachable. * CNI download: fail on HTTP errors for both URL attempts Use curl -f on both primary and legacy CNI download URLs and exit with a clear error if neither succeeds, instead of only checking for 404 on the first attempt. * temp directory: use mktemp and trap for cleanup Replace fixed /tmp/iso with mktemp -d to avoid collisions between concurrent runs, and register a trap EXIT to ensure cleanup on failure or early exit.
|
@shwstppr and @DaanHoogland, could you please review this Pull Request again? |
| echo "Downloading etcd ${ETCD_VERSION}..." | ||
| curl -sS -L "https://github.com/etcd-io/etcd/releases/download/${ETCD_VERSION}/etcd-${ETCD_VERSION}-linux-${ARCH}.tar.gz" -o "${etcd_dir}/etcd-linux-${ARCH}.tar.gz" | ||
|
|
||
| mkisofs -o "${output_dir}/${build_name}" -J -R -l "${iso_dir}" No newline at end of file |
There was a problem hiding this comment.
| mkisofs -o "${output_dir}/${build_name}" -J -R -l "${iso_dir}" | |
| mkisofs -o "${output_dir}/${build_name}" -J -R -l "${iso_dir}" | |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if [[ $(echo "${2} $VAL" | awk '{print ($1 < $2)}') == 1 ]]; then | ||
| curl -sSL "https://raw.githubusercontent.com/kubernetes/kubernetes/${RELEASE}/build/debs/kubelet.service" | sed "s:/usr/bin:/opt/bin:g" > "${kubelet_service_file}" | ||
| else | ||
| curl -sSL "https://raw.githubusercontent.com/shapeblue/cloudstack-nonoss/main/cks/kubelet.service" | sed "s:/usr/bin:/opt/bin:g" > "${kubelet_service_file}" | ||
| fi |
There was a problem hiding this comment.
The curl | sed > file pipelines can silently succeed even if curl fails (because the pipeline exit status is from sed unless pipefail is set). With set -e alone, this can leave empty/partial kubelet.service (and similarly 10-kubeadm.conf) in the ISO without failing the script. Consider enabling set -o pipefail and/or avoiding pipelines by downloading to a temp file and transforming it after a successful curl --fail.
| if [[ $(echo "${2} $VAL" | awk '{print ($1 < $2)}') == 1 ]]; then | ||
| curl -sSL "https://raw.githubusercontent.com/kubernetes/kubernetes/${RELEASE}/build/debs/kubelet.service" | sed "s:/usr/bin:/opt/bin:g" > "${kubelet_service_file}" |
There was a problem hiding this comment.
The Kubernetes version comparison echo "${2} $VAL" | awk '{print ($1 < $2)}' is not a reliable semver comparison (e.g., 1.9.0 is treated as 1.9 and compares as greater than 1.18.0). This can select the wrong source for kubelet.service/10-kubeadm.conf. Use a version-aware compare (e.g., sort -V, dpkg --compare-versions, or parsing major/minor/patch) instead of numeric awk.
| exit 1 | ||
| fi | ||
|
|
||
| helm repo add cilium https://helm.cilium.io/ > /dev/null 2>&1 || true |
There was a problem hiding this comment.
helm repo add ... || true suppresses failures (e.g., network/DNS issues), which can turn an actionable error into a later, less-clear failure during helm template. It’s better to either let helm repo add fail normally or only ignore the specific “already exists” case (e.g., by checking helm repo list).
| helm repo add cilium https://helm.cilium.io/ > /dev/null 2>&1 || true | |
| # Add the Cilium Helm repository only if it is not already configured | |
| if ! helm repo list 2>/dev/null | awk 'NR>1 {print $1}' | grep -qx "cilium"; then | |
| helm repo add cilium https://helm.cilium.io/ > /dev/null 2>&1 | |
| fi |
| curl -sS -L "https://github.com/etcd-io/etcd/releases/download/${ETCD_VERSION}/etcd-${ETCD_VERSION}-linux-${ARCH}.tar.gz" -o "${etcd_dir}/etcd-linux-${ARCH}.tar.gz" | ||
|
|
There was a problem hiding this comment.
This curl -sS -L download won’t fail the script on HTTP errors (e.g., 404/500) because --fail isn’t used; the ISO could be generated with an HTML error page instead of the etcd tarball. Add --fail/--fail-with-body (and optionally validate the archive) so the script reliably stops on download problems.
| curl -sS -L "https://github.com/etcd-io/etcd/releases/download/${ETCD_VERSION}/etcd-${ETCD_VERSION}-linux-${ARCH}.tar.gz" -o "${etcd_dir}/etcd-linux-${ARCH}.tar.gz" | |
| curl -sS -L --fail "https://github.com/etcd-io/etcd/releases/download/${ETCD_VERSION}/etcd-${ETCD_VERSION}-linux-${ARCH}.tar.gz" -o "${etcd_dir}/etcd-linux-${ARCH}.tar.gz" | |
| # Validate that the downloaded etcd archive is a valid tar.gz | |
| if ! tar -tzf "${etcd_dir}/etcd-linux-${ARCH}.tar.gz" > /dev/null; then | |
| echo "ERROR: Downloaded etcd archive is invalid or corrupted." | |
| exit 1 | |
| fi |
| ARCH="arm64" | ||
| ARCH_SUFFIX="aarch64" | ||
| else | ||
| echo "ERROR: ARCH must be 'x86_64' or 'aarch64'." |
There was a problem hiding this comment.
The ARCH validation error message is inconsistent with the accepted values. The script accepts amd64/arm64 as aliases, but the error only mentions x86_64/aarch64, which can confuse users troubleshooting invalid input. Update the message (and/or the usage text) to reflect all accepted values.
| echo "ERROR: ARCH must be 'x86_64' or 'aarch64'." | |
| echo "ERROR: ARCH must be one of: x86_64, amd64, aarch64, or arm64." |
| if [[ $(echo "${2} $VAL" | awk '{print ($1 < $2)}') == 1 ]]; then | ||
| curl -sSL "https://raw.githubusercontent.com/kubernetes/kubernetes/${RELEASE}/build/debs/kubelet.service" | sed "s:/usr/bin:/opt/bin:g" > "${kubelet_service_file}" | ||
| else | ||
| curl -sSL "https://raw.githubusercontent.com/shapeblue/cloudstack-nonoss/main/cks/kubelet.service" | sed "s:/usr/bin:/opt/bin:g" > "${kubelet_service_file}" | ||
| fi | ||
|
|
||
| echo "Downloading 10-kubeadm.conf ${RELEASE}..." | ||
| kubeadm_conf_file="${working_dir}/10-kubeadm.conf" | ||
| touch "${kubeadm_conf_file}" | ||
| if [[ $(echo "${2} $VAL" | awk '{print ($1 < $2)}') == 1 ]]; then | ||
| curl -sSL "https://raw.githubusercontent.com/kubernetes/kubernetes/${RELEASE}/build/debs/10-kubeadm.conf" | sed "s:/usr/bin:/opt/bin:g" > "${kubeadm_conf_file}" | ||
| else | ||
| curl -sSL "https://raw.githubusercontent.com/shapeblue/cloudstack-nonoss/main/cks/10-kubeadm.conf" | sed "s:/usr/bin:/opt/bin:g" > "${kubeadm_conf_file}" | ||
| fi |
There was a problem hiding this comment.
These curl calls fetch kubelet.service and 10-kubeadm.conf directly from the shapeblue GitHub repository main branch at ISO build time and embed them as systemd unit/config files that will later be executed with root privileges. Because they are pinned only to a mutable branch and no checksum or signature verification is performed, a compromise of that repository (or its branch) would silently inject arbitrary code into all clusters built with this script. To reduce supply-chain risk, vendor these files into this repository or pin them to immutable commits/tags and validate their integrity before packaging them into the ISO.
…a default CNI
Description
This PR fixes #9304.
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
The script in question generates CloudStack Kubernetes Service (CKS) images using Cilium as the default CNI.
Additionally, the other components are up-to-date.
You can also use ready-made images from my repository.. See: https://download.suanuvem.io/cks/