Orchestrator and runner #3
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This passes the environment, where the PAT is located, through an environment file instead of a command line argument. This is a security improvement, as it avoids leaking the PAT through the process list. The environment file is automatically removed before the runner process is created, ensuring that it cannot be accessed from within workflows.
Improve logging so the identifier of the runner (locally and at GitHub) is present in each log message to ease identification.
When starting several runners, the same token will be returned. In order to be able to run several runners, they need to be configured from different working directories, and these directories need to contain an entire copy of the runner installation. Arrange for two sub-directories to be created in the working directory: one called `runner` will contain the copy of the installation tree, another called `work` will be where the runner will be "working", e.g. checkout repositories, etc.
WalkthroughThe updates across these files focus on enhancing the infrastructure for GitHub Actions by introducing a new workflow for Docker image builds, refining GitHub runner management through scripts, and improving Docker plugin support. Key improvements include streamlined Docker image building triggered by specific branches, detailed configuration for microVM-based GitHub runners, and enhanced command verification and logging within the scripts. These changes aim to optimize the development and deployment process, ensuring a more efficient and organized CI/CD pipeline. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 19
Configuration used: CodeRabbit UI
Files selected for processing (9)
- .github/workflows/dev.yml (1 hunks)
- base/base.sh (2 hunks)
- docs/NOTES.md (1 hunks)
- lib/common.sh (1 hunks)
- orchestrator.sh (1 hunks)
- runner.sh (1 hunks)
- runner/install.sh (4 hunks)
- runner/runner.sh (9 hunks)
- runner/token.sh (2 hunks)
Additional comments: 23
.github/workflows/dev.yml (1)
- 1-31: The workflow definition correctly sets up two jobs,
build-baseandbuild-main, with dependencies and secrets properly configured. The use of${{ secrets.GITHUB_TOKEN }}for authentication in Docker builds is a secure practice.lib/common.sh (6)
- 42-45: The
usagefunction correctly utilizesKRUNVM_RUNNER_DESCRfor dynamic description. Ensure that all scripts using this function defineKRUNVM_RUNNER_DESCRto avoid empty descriptions.- 49-54: The
check_commandfunction properly verifies the accessibility of commands, enhancing the script's robustness by ensuring necessary dependencies are met before proceeding.- 56-67: The
get_envfunction securely retrieves environment variable values from a file, correctly isolating the sourcing process to prevent variable leakage. This is a good security practice.- 69-72: The
run_krunvmfunction correctly executeskrunvmwith specified arguments usingbuildah unshare, aligning with best practices for containerized environments.- 74-91: The
wait_pathfunction implements a robust mechanism for waiting on a file or directory, including timeout and interval handling. This is useful for synchronization in scripts.- 97-103: The modification to the
_logfunction to dynamically setKRUNVM_RUNNER_BINif not already set is a good enhancement for flexibility in logging. EnsureKRUNVM_RUNNER_BINis appropriately named in all contexts where logging is used.runner/token.sh (2)
- 59-59: The change from
KRUNVM_RUNNER_MAINtoKRUNVM_RUNNER_DESCRfor the description variable aligns with the standardized naming convention across scripts. Ensure consistency in all references.- 90-90: The addition of a check for the
jqcommand before proceeding with token acquisition is a good practice, ensuring the necessary tool is available for JSON parsing.base/base.sh (2)
- 57-57: The update to
KRUNVM_RUNNER_DESCRfor the script description maintains consistency with the naming convention used in other scripts.- 118-118: The installation of
docker-ce-clialongside Docker pluginsdocker-buildx-pluginanddocker-compose-pluginis a significant enhancement, ensuring compatibility and extended functionality for Docker operations within the environment.runner/install.sh (3)
- 57-57: The change to
KRUNVM_RUNNER_DESCRfor the script description aligns with the standardized naming convention across scripts.- 43-43: Changing the installation directory to a versioned path under
../share/runnerimproves organization and version management of the runner installations.- 108-110: Adjusting the download and extraction process to use versioned
.tgzfilenames enhances clarity and version tracking. Removing the deletion of the tar file after extraction is acceptable given the move to a versioned filename, but consider cleanup strategies for older versions to manage disk space.docs/NOTES.md (1)
- 121-141: The added documentation provides clear and detailed information about the organization of GitHub runners, including user details, directory structure, versioned files, and tool cache locations. This enhances understanding and manageability of the runner environment.
runner.sh (2)
- 86-86: The description variable
KRUNVM_RUNNER_DESCRis correctly set to describe the script's purpose, maintaining consistency across the project.- 147-179: The loop logic for continuously starting microVMs to run ephemeral GitHub runners is correctly implemented, including the handling of environment variables and the execution of
krunvm. The use ofrandom_stringfor unique runner IDs and the conditional handling of environment files are well-designed.orchestrator.sh (2)
- 30-30: Validate the output of
command -v -- "$(abspath "$0")"to ensure it returns a valid path before proceeding.- 66-70: The description of
ORCHESTRATOR_ISOLATIONimplies a security feature but does not detail how it's implemented. Ensure the implementation securely handles environment variables without exposing sensitive information.runner/runner.sh (4)
- 53-53: The default value for
RUNNER_USERhas been changed to"runner". Confirm this user exists and has the necessary permissions.- 77-77: Validate the default value for
RUNNER_INSTALLto ensure it points to a valid directory containing the runner binaries.- 82-84: The handling of
RUNNER_ENVFILEshould ensure the file's integrity and security, given it may contain sensitive information.- 375-375: Verify that the
docker_daemonfunction is called only when necessary and that it correctly handles the daemon's lifecycle.
Comment on lines
+319
to
+338
| # Setup variables that would have been missing. These depends on the main | ||
| # variables, so we do it here rather than at the top of the script. | ||
| debug "Setting up missing defaults" | ||
| [ -f "/etc/os-release" ] && . /etc/os-release | ||
| RUNNER_DISTRO=${RUNNER_DISTRO:-"${ID:-"unknown}"}"} | ||
| distro=$(get_env "/etc/os-release" "ID") | ||
| RUNNER_DISTRO=${RUNNER_DISTRO:-"${distro:-"unknown}"}"} | ||
| RUNNER_NAME_PREFIX=${RUNNER_NAME_PREFIX:-"${RUNNER_DISTRO}-krunvm"} | ||
| RUNNER_NAME=${RUNNER_NAME:-"${RUNNER_NAME_PREFIX}-$(random_string)"} | ||
| RUNNER_NAME=${RUNNER_NAME:-"${RUNNER_NAME_PREFIX}-$RUNNER_ID"} | ||
|
|
||
| RUNNER_WORKDIR=${RUNNER_WORKDIR:-"/_work/${RUNNER_NAME}"} | ||
| if [ -n "${ID:-}" ]; then | ||
| if [ -n "${distro:-}" ]; then | ||
| RUNNER_LABELS=${RUNNER_LABELS:-"krunvm,${RUNNER_DISTRO}"} | ||
| else | ||
| RUNNER_LABELS=${RUNNER_LABELS:-"krunvm"} | ||
| fi | ||
|
|
||
| RUNNER_TAR=$(find "$RUNNER_INSTALL" -type f -name "*.tgz" | sort -r | head -n 1) | ||
| if [ -z "$RUNNER_TAR" ]; then | ||
| error "No runner tar file found under $RUNNER_INSTALL" | ||
| fi | ||
|
|
There was a problem hiding this comment.
The setup of missing defaults and construction of the runner URL should validate the values to prevent configuration errors.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add an orchestrator and a runner.
Variables that are runner specific are all called
RUNNER_*in all processes to facilitate passing them across. When starting the "container", the preferred way is to create an .env file with those variables, a file that will be run from within the "entrypoint" of the container, e.g. the process run in the VM. This is a security measure as secrets such as a PAT are not accessible to processes through the process table -- the file is removed as soon as it has been read.The runner is run under the
runneruser and the default is to start podman in emulation mode, making available a/var/run/docker.socksocket file (owned by thedockergroup, of which therunneruser is a member).To ensure that several runners can be start from the same host, the runner implementation (binaries) is copied inside unique directory hierarchies. This is necessary as otherwise token reuse will happen and registration errors will happen.
Summary by CodeRabbit
jqis checked before executing token-related operations, preventing potential errors.