feat: Support Debian-based distros #62
Conversation
WalkthroughThis change updates the Possibly related PRs
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
userdata.sh.tmpl(2 hunks)
🔇 Additional comments (2)
userdata.sh.tmpl (2)
6-18: LGTM: Clean package manager detection logic.The implementation correctly detects apt-get availability and falls back to dnf, which aligns well with the PR objective of supporting Debian-based distributions.
118-118:✅ Verification successful
Verify the official Tailscale install script handles service management.
The change simplifies Tailscale installation by using the official script, but the previous implementation explicitly enabled and started the service via
systemctl. Ensure the official script handles this properly.Please verify that the official Tailscale install script properly enables and starts the tailscaled service:
🏁 Script executed:
#!/bin/bash # Check what the official Tailscale install script does regarding service management curl -fsSL https://tailscale.com/install.sh | grep -A 5 -B 5 "systemctl\|service"Length of output: 2240
Service management confirmed in the official Tailscale installer
I’ve verified that the upstream install script handles enabling and starting the
tailscaledservice across all supported platforms:
- Debian/Ubuntu (APT):
Installstailscaleand, ifAPT_SYSTEMCTL_START=true, runssystemctl enable --now tailscaled systemctl start tailscaled- RHEL/CentOS/Fedora (yum, dnf, tdnf):
Always runssystemctl enable --now tailscaled- SUSE (zypper), Arch (pacman):
Likewise usessystemctl enable --now tailscaled- FreeBSD (pkg):
Usesservice tailscaled enable service tailscaled start- Alpine (apk):
Usesrc-update add tailscale rc-service tailscale startSince the official script already manages service enablement and startup, replacing the manual
systemctlcalls with the curl-and-sh approach is safe.
| echo "Installing utilities..." | ||
| retry_command "$INSTALL_CMD dnf-utils" 5 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistent utility installation between package managers.
For apt-get systems, the script installs curl and wget, but for dnf systems it only installs dnf-utils. This inconsistency could cause issues if later parts of the script expect these utilities to be available.
Ensure consistent utility installation:
# Install utilities.
echo "Installing utilities..."
- retry_command "$INSTALL_CMD dnf-utils" 5
+ retry_command "$INSTALL_CMD curl wget dnf-utils" 5📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| echo "Installing utilities..." | |
| retry_command "$INSTALL_CMD dnf-utils" 5 | |
| # Install utilities. | |
| echo "Installing utilities..." | |
| retry_command "$INSTALL_CMD curl wget dnf-utils" 5 |
🤖 Prompt for AI Agents
In userdata.sh.tmpl around lines 99 to 100, the script installs curl and wget
for apt-get systems but only installs dnf-utils for dnf systems, causing
inconsistency. Modify the dnf installation command to also include curl and wget
along with dnf-utils to ensure these utilities are consistently installed across
package managers.
| arch=arm64 | ||
| ;; | ||
| esac | ||
| retry_command "wget https://amazoncloudwatch-agent.s3.amazonaws.com/$distro/$arch/latest/amazon-cloudwatch-agent.deb" 5 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate distro ID before using in URL construction.
The script extracts the distro ID directly from /etc/os-release without validation, which could lead to malformed URLs or security issues if the distro ID contains unexpected characters.
Add validation for the distro ID:
distro=$(grep '^ID=' /etc/os-release | cut -d'=' -f2)
+ # Remove quotes and validate distro ID
+ distro=$(echo "$distro" | tr -d '"' | grep -E '^[a-z0-9]+$')
+ if [ -z "$distro" ]; then
+ echo "Could not determine valid distro ID"
+ exit 1
+ fi
arch=$(uname -m)📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| retry_command "wget https://amazoncloudwatch-agent.s3.amazonaws.com/$distro/$arch/latest/amazon-cloudwatch-agent.deb" 5 | |
| distro=$(grep '^ID=' /etc/os-release | cut -d'=' -f2) | |
| # Remove quotes and validate distro ID | |
| distro=$(echo "$distro" | tr -d '"' | grep -E '^[a-z0-9]+$') | |
| if [ -z "$distro" ]; then | |
| echo "Could not determine valid distro ID" | |
| exit 1 | |
| fi | |
| arch=$(uname -m) | |
| retry_command "wget https://amazoncloudwatch-agent.s3.amazonaws.com/$distro/$arch/latest/amazon-cloudwatch-agent.deb" 5 |
🤖 Prompt for AI Agents
In userdata.sh.tmpl at line 94, the distro ID used in the wget URL is extracted
without validation, risking malformed URLs or security issues. Add validation
logic to ensure the distro ID contains only expected characters (e.g.,
alphanumeric, dashes) before using it in the URL. If the validation fails,
handle the error appropriately or set a default safe value to prevent unsafe URL
construction.
| distro=$(grep '^ID=' /etc/os-release | cut -d'=' -f2) | ||
| arch=$(uname -m) | ||
| case "$arch" in | ||
| x86_64) | ||
| arch=amd64 | ||
| ;; | ||
| *) | ||
| arch=arm64 | ||
| ;; | ||
| esac |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve architecture detection for better compatibility.
The current architecture mapping is overly simplistic and may cause issues with other architectures like i386, s390x, or ppc64le.
Consider using a more comprehensive architecture mapping:
- case "$arch" in
- x86_64)
- arch=amd64
- ;;
- *)
- arch=arm64
- ;;
- esac
+ case "$arch" in
+ x86_64)
+ arch=amd64
+ ;;
+ aarch64|arm64)
+ arch=arm64
+ ;;
+ *)
+ echo "Unsupported architecture: $arch"
+ exit 1
+ ;;
+ esac📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| distro=$(grep '^ID=' /etc/os-release | cut -d'=' -f2) | |
| arch=$(uname -m) | |
| case "$arch" in | |
| x86_64) | |
| arch=amd64 | |
| ;; | |
| *) | |
| arch=arm64 | |
| ;; | |
| esac | |
| distro=$(grep '^ID=' /etc/os-release | cut -d'=' -f2) | |
| arch=$(uname -m) | |
| case "$arch" in | |
| x86_64) | |
| arch=amd64 | |
| ;; | |
| aarch64|arm64) | |
| arch=arm64 | |
| ;; | |
| *) | |
| echo "Unsupported architecture: $arch" | |
| exit 1 | |
| ;; | |
| esac |
🤖 Prompt for AI Agents
In userdata.sh.tmpl around lines 84 to 93, the architecture detection is too
simplistic, only mapping x86_64 to amd64 and everything else to arm64, which can
cause issues with other architectures like i386, s390x, or ppc64le. Update the
case statement to include explicit mappings for common architectures such as
i386 to 386, s390x to s390x, ppc64le to ppc64le, and keep amd64 for x86_64,
while defaulting to arm64 only if no other match is found. This will improve
compatibility across different systems.
There was a problem hiding this comment.
@cmam appreciate you opening a PR and trying to upstream these changes. We haven't had the need to run this module on any other distros so this hasn't come up before.
I'm hesitant to accept these changes. It substantially increases the footprint of what we would need to test to verify that this module is working correctly. Also, bash that runs on the host machine is also hard to test -- we definitely don't have a setup for testing this today. Those two pieces make me feel like we're boiling the ocean with this type of change, because someone using Arch linux could come by next week and want to support their OS / package manager.
I think what I'd rather see is that we have an examples/debian/ that shows how to use this module with a different user_data.sh.tmpl script and then a callout in our README that directs debian users there. Then it's less likely that we're taking on potentially hard-to-test changes like this and we're still providing a path for folks like yourself who might have OS requirements in their org.
Make sense? Do you mind updating this PR to be an example rather than trying to handle non-Amazon Linux distros?
| # Setup Tailscale | ||
| echo "Enabling and starting tailscaled service..." | ||
| systemctl enable --now tailscaled | ||
|
|
There was a problem hiding this comment.
Ah from your comment in PR description, it sounds like the new install script that you're invoking does this step?
There was a problem hiding this comment.
Indeed @Gowiem, if you check the install script provided by Tailscale, they cover the service registration (https://tailscale.com/install.sh).
@Gowiem Thank you for taking the time to look at the PR and provide comments.
Fair enough, I thought that you might want to extend the "out-of-the-box" image coverage, but since the module exposes the "user_data" variable, it makes sense to just provide more examples of this as you say.
Sure, I will make some changes and come back. |
|
@Gowiem Hello again, as I tried to make the example, I realized that we can't have a user-data "drop-in" exactly, because the module uses |
what
why
references
Summary by CodeRabbit
apt-getordnf) for installing required packages.