Skip to content
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(kasmvnc): optimize KasmVNC deployment script #329

Merged
merged 31 commits into from
Oct 30, 2024
Merged
Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
a8cc861
Refactor KasmVNC Deployment
djarbz Oct 24, 2024
8213a57
Make Prettier happy
djarbz Oct 24, 2024
6d6e0dd
Run KasmVNC server as coder user
djarbz Oct 24, 2024
c59ba95
Coder supports ssl now and remove sudo requirement
djarbz Oct 24, 2024
bb7d438
Require sudo to install only
djarbz Oct 24, 2024
10a86bd
Re-add SSL Req
djarbz Oct 24, 2024
46bbcb9
Alert KasmVNC started successfully
djarbz Oct 24, 2024
30e6bed
Remove unneeded shellcheck directive
djarbz Oct 24, 2024
4d831b4
Override kasm ssl certificate directives
djarbz Oct 24, 2024
9c3904d
Handle Fedora Distro Version
djarbz Oct 24, 2024
4a72b2e
Improve readability fo amd64 check
djarbz Oct 24, 2024
a2d8e72
Bash Array for download command
djarbz Oct 24, 2024
026a5bc
Don't mention distro
djarbz Oct 24, 2024
4fc9f6d
Bracket Consistency
djarbz Oct 24, 2024
d418c81
Naming cleanup
djarbz Oct 24, 2024
c4f88fa
cat vs tee
djarbz Oct 24, 2024
52ba74c
Update wording
djarbz Oct 24, 2024
d619e65
Print logs on failure
djarbz Oct 24, 2024
eff921e
Portability of checking server status
djarbz Oct 24, 2024
eb974cb
dnf localinstall
djarbz Oct 24, 2024
f3a0f98
Show up to 10 lines of log
djarbz Oct 24, 2024
ab96d93
Cleanup Package Install
djarbz Oct 25, 2024
ebc57a1
We don't use SSL Certificates
djarbz Oct 25, 2024
86b48dd
Prefer /etc instead of /home for Kasm Config
djarbz Oct 25, 2024
15e3ec2
Prettier formatting
djarbz Oct 25, 2024
41baf48
Do not overwrite custom user config
djarbz Oct 25, 2024
7d7c7e8
Prettier Format
djarbz Oct 25, 2024
f35b535
Fix Typo
djarbz Oct 25, 2024
a0373c0
Cleanup architecture mapping
djarbz Oct 25, 2024
ccf299b
Prettier Format
djarbz Oct 25, 2024
ef4f704
Merge branch 'main' into kasmVNC_optimize
matifali Oct 30, 2024
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
2 changes: 1 addition & 1 deletion kasmvnc/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ Automatically install [KasmVNC](https://kasmweb.com/kasmvnc) in a workspace, and
```tf
module "kasmvnc" {
source = "registry.coder.com/modules/kasmvnc/coder"
version = "1.0.22"
version = "1.0.23"
agent_id = coder_agent.example.id
desktop_environment = "xfce"
}
Expand Down
2 changes: 1 addition & 1 deletion kasmvnc/main.tf
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ resource "coder_script" "kasm_vnc" {
script = templatefile("${path.module}/run.sh", {
PORT : var.port,
DESKTOP_ENVIRONMENT : var.desktop_environment,
VERSION : var.kasm_version
KASM_VERSION : var.kasm_version
})
run_on_start = true
}
Expand Down
218 changes: 116 additions & 102 deletions kasmvnc/run.sh
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#!/usr/bin/env bash

#!/bin/bash
# Exit on error, undefined variables, and pipe failures
set -euo pipefail

# Function to check if vncserver is already installed
check_installed() {
Expand All @@ -14,166 +15,179 @@ check_installed() {

# Function to download a file using wget, curl, or busybox as a fallback
download_file() {
local url=$1
local output=$2
if command -v wget &> /dev/null; then
wget $url -O $output
elif command -v curl &> /dev/null; then
curl -fsSL $url -o $output
local url="$1"
local output="$2"
local download_tool

if command -v curl &> /dev/null; then
# shellcheck disable=SC2034
download_tool=(curl -fsSL)
elif command -v wget &> /dev/null; then
# shellcheck disable=SC2034
download_tool=(wget -q -O-)
elif command -v busybox &> /dev/null; then
busybox wget -O $output $url
# shellcheck disable=SC2034
download_tool=(busybox wget -O-)
else
echo "Neither wget, curl, nor busybox is installed. Please install one of them to proceed."
echo "ERROR: No download tool available (curl, wget, or busybox required)"
exit 1
fi
}

# Function to install kasmvncserver for debian-based distros
install_deb() {
local url=$1
download_file $url /tmp/kasmvncserver.deb
sudo apt-get update
DEBIAN_FRONTEND=noninteractive sudo apt-get install --yes -qq --no-install-recommends --no-install-suggests /tmp/kasmvncserver.deb
sudo adduser $USER ssl-cert
rm /tmp/kasmvncserver.deb
# shellcheck disable=SC2288
"$${download_tool[@]}" "$url" > "$output" || {
echo "ERROR: Failed to download $url"
exit 1
}
}

# Function to install kasmvncserver for Oracle 8
install_rpm_oracle8() {
local url=$1
download_file $url /tmp/kasmvncserver.rpm
sudo dnf config-manager --set-enabled ol8_codeready_builder
sudo dnf install oracle-epel-release-el8 -y
sudo dnf localinstall /tmp/kasmvncserver.rpm -y
sudo usermod -aG kasmvnc-cert $USER
rm /tmp/kasmvncserver.rpm
# Add user to group using available commands
add_user_to_group() {
local user="$1"
local group="$2"

if command -v usermod &> /dev/null; then
sudo usermod -aG "$group" "$user"
elif command -v adduser &> /dev/null; then
sudo adduser "$user" "$group"
else
echo "ERROR: At least one of 'adduser' or 'usermod' is required"
exit 1
fi
}

# Function to install kasmvncserver for CentOS 7
install_rpm_centos7() {
djarbz marked this conversation as resolved.
Show resolved Hide resolved
# Function to install kasmvncserver for debian-based distros
install_deb() {
local url=$1
download_file $url /tmp/kasmvncserver.rpm
sudo yum install epel-release -y
sudo yum install /tmp/kasmvncserver.rpm -y
sudo usermod -aG kasmvnc-cert $USER
rm /tmp/kasmvncserver.rpm
download_file "$url" /tmp/kasmvncserver.deb
# Define the directory to check
CACHE_DIR="/var/lib/apt/lists/partial"
# Check if the directory exists and was modified in the last 60 minutes
if [[ ! -d "$CACHE_DIR" ]] || ! find "$CACHE_DIR" -mmin -60 -print -quit &> /dev/null; then
echo "Stale package cache, updating..."
# Update package cache with a 300-second timeout for dpkg lock
sudo apt-get -o DPkg::Lock::Timeout=300 -qq update
fi
DEBIAN_FRONTEND=noninteractive sudo apt-get -o DPkg::Lock::Timeout=300 install --yes -qq --no-install-recommends --no-install-suggests /tmp/kasmvncserver.deb
Copy link
Member

Choose a reason for hiding this comment

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

Out of curiosity, what packages are we avoiding installing here by not pulling in the rec/suggestions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure, I left that in from the original script.
It's working for me, so as of right now, I don't think anything important is missing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI, doesn't look like anything important.

Suggested packages:
  pciutils
Recommended packages:
  libgl1-amber-dri mesa-vulkan-drivers | vulkan-icd

add_user_to_group "$USER" ssl-cert
rm /tmp/kasmvncserver.deb
}

# Function to install kasmvncserver for rpm-based distros
install_rpm() {
local url=$1
download_file $url /tmp/kasmvncserver.rpm
sudo rpm -i /tmp/kasmvncserver.rpm
download_file "$url" /tmp/kasmvncserver.rpm
sudo dnf localinstall /tmp/kasmvncserver.rpm
rm /tmp/kasmvncserver.rpm
}

# Function to install kasmvncserver for Alpine Linux
install_alpine() {
local url=$1
download_file $url /tmp/kasmvncserver.tgz
download_file "$url" /tmp/kasmvncserver.tgz
tar -xzf /tmp/kasmvncserver.tgz -C /usr/local/bin/
rm /tmp/kasmvncserver.tgz
}

# Detect system information
distro=$(grep "^ID=" /etc/os-release | awk -F= '{print $2}')
version=$(grep "^VERSION_ID=" /etc/os-release | awk -F= '{print $2}' | tr -d '"')
arch=$(uname -m)
if [[ ! -f /etc/os-release ]]; then
djarbz marked this conversation as resolved.
Show resolved Hide resolved
echo "ERROR: Cannot detect OS: /etc/os-release not found"
exit 1
fi

# shellcheck disable=SC1091
source /etc/os-release
distro="$ID"
distro_version="$VERSION_ID"
codename="$VERSION_CODENAME"
arch="$(uname -m)"
if [[ "$ID" == "ol" ]]; then
distro="oracle"
distro_version="$${distro_version%%.*}"
elif [[ "$ID" == "fedora" ]]; then
distro_version="$(grep -oP '\(\K[\w ]+' /etc/fedora-release | tr '[:upper:]' '[:lower:]' | tr -d ' ')"
fi

echo "Detected Distribution: $distro"
echo "Detected Version: $version"
echo "Detected Version: $distro_version"
echo "Detected Codename: $codename"
echo "Detected Architecture: $arch"

# Map arch to package arch
if [[ "$arch" == "x86_64" ]]; then
if [[ "$distro" == "ubuntu" || "$distro" == "debian" || "$distro" == "kali" ]]; then
arch="amd64"
else
arch="x86_64"
fi
elif [[ "$arch" == "aarch64" || "$arch" == "arm64" ]]; then
if [[ "$distro" == "ubuntu" || "$distro" == "debian" || "$distro" == "kali" ]]; then
arch="arm64"
else
arch="aarch64"
fi
else
echo "Unsupported architecture: $arch"
exit 1
fi
case "$arch" in
x86_64)
if [[ "$distro" =~ ^(ubuntu|debian|kali)$ ]]; then
arch="amd64"
fi
;;
aarch64 | arm64)
[[ "$distro" =~ ^(ubuntu|debian|kali)$ ]] && arch="arm64" || arch="aarch64"
Copy link
Member

Choose a reason for hiding this comment

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

Same as above, except I also wonder if we actually want to enforce the overwrite from arm64 to aarch64 when distro isn't one of the three?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For x86_64, it probably doesn't need to overwrite.

For aarch64 or arm64, do we need to do anything other than just match?
I would imagine that each distro would report the correct version for itself?

Copy link
Member

@mafredri mafredri Oct 25, 2024

Choose a reason for hiding this comment

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

Doesn't look like arm64 case in aarch64 | arm64) is ever applicable:

❯ for container in ubuntu debian kalilinux/kali-rolling alpine fedora opensuse/leap container-registry.oracle.com/os/oraclelinux:9-slim; do for plat in linux/arm64 linux/amd64; do docker run -it --rm --platform $plat $container /bin/sh -c 'echo $(grep ^NAME= /etc/os-release): $(uname -m)'; done; done 2>/dev/null
NAME="Ubuntu": aarch64
NAME="Ubuntu": x86_64
NAME="Debian GNU/Linux": aarch64
NAME="Debian GNU/Linux": x86_64
NAME="Kali GNU/Linux": aarch64
NAME="Kali GNU/Linux": x86_64
NAME="Alpine Linux": aarch64
NAME="Alpine Linux": x86_64
NAME="Fedora Linux": aarch64
NAME="Fedora Linux": x86_64
NAME="openSUSE Leap": aarch64
NAME="openSUSE Leap": x86_64
NAME="Oracle Linux Server": aarch64
NAME="Oracle Linux Server": x86_64

So we can essentially use the same logic as above for x86_64, default=aarch64, for debian based, use arm64 instead (verified from kasm releases that all others are aarch64).

;;
*)
echo "ERROR: Unsupported architecture: $arch"
exit 1
;;
esac

# Check if vncserver is installed, and install if not
if ! check_installed; then
echo "Installing KASM version: ${VERSION}"
# Check for sudo (required)
if ! command -v sudo &> /dev/null; then
echo "ERROR: Required command 'sudo' not found"
exit 1
fi

base_url="https://github.com/kasmtech/KasmVNC/releases/download/v${KASM_VERSION}"

echo "Installing KASM version: ${KASM_VERSION}"
case $distro in
ubuntu | debian | kali)
case $version in
"20.04")
install_deb "https://github.com/kasmtech/KasmVNC/releases/download/v${VERSION}/kasmvncserver_focal_${VERSION}_$${arch}.deb"
;;
"22.04")
install_deb "https://github.com/kasmtech/KasmVNC/releases/download/v${VERSION}/kasmvncserver_jammy_${VERSION}_$${arch}.deb"
;;
"24.04")
install_deb "https://github.com/kasmtech/KasmVNC/releases/download/v${VERSION}/kasmvncserver_noble_${VERSION}_$${arch}.deb"
;;
*)
echo "Unsupported Ubuntu/Debian/Kali version: $${version}"
exit 1
;;
esac
bin_name="kasmvncserver_$${codename}_${KASM_VERSION}_$${arch}.deb"
install_deb "$base_url/$bin_name"
;;
oracle)
if [[ "$version" == "8" ]]; then
install_rpm_oracle8 "https://github.com/kasmtech/KasmVNC/releases/download/v${VERSION}/kasmvncserver_oracle_8_${VERSION}_$${arch}.rpm"
else
echo "Unsupported Oracle version: $${version}"
exit 1
fi
;;
centos)
if [[ "$version" == "7" ]]; then
install_rpm_centos7 "https://github.com/kasmtech/KasmVNC/releases/download/v${VERSION}/kasmvncserver_centos_core_${VERSION}_$${arch}.rpm"
else
install_rpm "https://github.com/kasmtech/KasmVNC/releases/download/v${VERSION}/kasmvncserver_centos_core_${VERSION}_$${arch}.rpm"
fi
oracle | fedora | opensuse)
bin_name="kasmvncserver_$${distro}_$${distro_version}_${KASM_VERSION}_$${arch}.rpm"
djarbz marked this conversation as resolved.
Show resolved Hide resolved
install_rpm "$base_url/$bin_name"
Copy link
Member

Choose a reason for hiding this comment

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

We probably need both install_rpm and install_dnf. I just tried the opensuse/leap image and it has neither dnf nor yum, only zypper and rpm.

I'm not sure if this is a script meant for installing the kasmvnc built package or something else, but this at least gives hints for the expectation per distro: https://github.com/kasmtech/KasmVNC/blob/3a8517d7dc461eaccc7ed8e3d3b155e233426fc8/builder/scripts/common.sh#L22-L29

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps in the install_rpm function, we have a switch case to select based on what tooling is available?
Similar to the download function?

Priority:

  1. dnf
  2. zypper
  3. yum
  4. rpm

;;
alpine)
if [[ "$version" == "3.17" || "$version" == "3.18" || "$version" == "3.19" || "$version" == "3.20" ]]; then
install_alpine "https://github.com/kasmtech/KasmVNC/releases/download/v${VERSION}/kasmvnc.alpine_$${version}_$${arch}.tgz"
else
echo "Unsupported Alpine version: $${version}"
exit 1
fi
;;
fedora | opensuse)
install_rpm "https://github.com/kasmtech/KasmVNC/releases/download/v${VERSION}/kasmvncserver_$${distro}_$${version}_${VERSION}_$${arch}.rpm"
bin_name="kasmvnc.alpine_$${distro_version//./}_$${arch}.tgz"
install_alpine "$base_url/$bin_name"
;;
*)
echo "Unsupported distribution: $${distro}"
echo "Unsupported distribution: $distro"
exit 1
;;
esac
else
echo "vncserver already installed. Skipping installation."
fi

# Coder port-forwarding from dashboard only supports HTTP
sudo bash -c "cat > /etc/kasmvnc/kasmvnc.yaml <<EOF
cat << EOF > "$HOME/.vnc/kasmvnc.yaml"
Copy link
Member

Choose a reason for hiding this comment

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

I just realized we're unconditionally overwriting the users kasmvnc configuration here.

We should switch back to writing this to /etc since it will still apply to the user. That'll allow the user to provide their own settings in dotfiles.

KasmVNC is configured via YAML based configurations. The server level configuration is at /etc/kasmvnc/kasmvnc.yaml. Edits to this file apply to all users. Individual users can override server global configurations by specifying them in their configuration file at ~/.vnc/kasmvnc.yaml.

(emphasis mine.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Regarding arch mapping.

Is this what you are thinking?

case "$arch" in
  x86_64)
    if [[ "$distro" =~ ^(ubuntu|debian|kali)$ ]]; then
      arch="amd64"
    fi
    ;;
  aarch64)
    if [[ "$distro" =~ ^(ubuntu|debian|kali)$ ]]; then
      arch="arm64"
    fi
    ;;
  arm64)
    :  # This is effectively a noop
    ;;
  *)
    echo "ERROR: Unsupported architecture: $arch"
    exit 1
    ;;
esac

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My concern with the /etc kasmvnc.yaml is the requirement for sudo and when using the official KasmWeb images, they have some settings preconfigured.

We could create a check for if sudo is available or not and handle differently.
Pseudo-code

if sudo
  if /etc/kasmvnc/kasmvnc.yaml exists
    Merge yaml with our config taking precedence?
  else
    write our config
else
  Merge yaml with our config taking precedence?

I'm thinking we need to enforce our config as it is what is needed for KasmVNC to work with coder.

Copy link
Member

@mafredri mafredri Oct 25, 2024

Choose a reason for hiding this comment

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

Regarding arch mapping.

Yes, except in my testing the noop for arm64 isn't necessary at all.

My concern with the /etc kasmvnc.yaml is the requirement for sudo and when using the official KasmWeb images, they have some settings preconfigured.

In what situations can KasmVNC be installed if sudo isn't available? I suppose KasmWeb images are the only case since it's preinstalled?

I think explicit overwrite of config in /etc is fine, they're using our module for a reason. Down the line we may want to think about introducing more KasmVNC settings and that means we again want to overwrite the yaml again. Merging yaml seems like a cumbersome process requiring additional tools for manipulation.

If we fallback to writing the config in user home, we should check it's presence and only write if it's missing. If we detect an existing config we can print a warning that if it doesn't work, consider deleting the file and restarting the workspace.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, except in my testing the noop for arm64 isn't necessary at all.
I'm just thinking of future, don't want to fail out if it is reported as such?

If we use a KasmWeb image, we do not have sudo access.
This means that we cannot write to the /etc config file and must write to the home directory.
I think we should default to /etc if sudo is available, but fallback to /home with a warning message in case the user is trying to figure out why their /home config might not be taking effect.

Copy link
Member

Choose a reason for hiding this comment

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

If we use a KasmWeb image, we do not have sudo access.

Yeah, makes sense.

warning message in case the user is trying to figure out why their /home config might not be taking effect

Do you mean overwriting + warning or not overwriting + warning here? I'm still of the opinion that overwrite is not acceptable in user home due to the sheer amount of configuration options that are available. Wouldn't want to get in the way of what they want to do or to destroy all their work: https://www.kasmweb.com/kasmvnc/docs/latest/configuration.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added a commit to handle this with /etc as default and a warning if we need to fallback to /home for the config.

network:
protocol: http
websocket_port: ${PORT}
ssl:
require_ssl: false
pem_certificate:
pem_key:
udp:
public_ip: 127.0.0.1
EOF"
EOF

# This password is not used since we start the server without auth.
# The server is protected via the Coder session token / tunnel
# and does not listen publicly
echo -e "password\npassword\n" | vncpasswd -wo -u $USER
echo -e "password\npassword\n" | vncpasswd -wo -u "$USER"

# Start the server
printf "🚀 Starting KasmVNC server...\n"
sudo -u $USER bash -c "vncserver -select-de ${DESKTOP_ENVIRONMENT} -disableBasicAuth" > /tmp/kasmvncserver.log 2>&1 &
vncserver -select-de "${DESKTOP_ENVIRONMENT}" -disableBasicAuth > /tmp/kasmvncserver.log 2>&1 &
pid=$!

# Wait for server to start
sleep 5
grep -v '^[[:space:]]*$' /tmp/kasmvncserver.log | tail -n 10
if ps -p $pid | grep -q "^$pid"; then
echo "ERROR: Failed to start KasmVNC server. Check full logs at /tmp/kasmvncserver.log"
exit 1
fi
printf "🚀 KasmVNC server started successfully!\n"