fix(docker): Multi does not launch a second Jenkins controller anymore. #264
Conversation
|
@coderabbitai review |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThe changes in this pull request involve modifications to the Changes
Poem
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
dockerfiles/agent-discovery/find-name.sh (1)
82-86: Consider consolidating duplicate HTTP checks.The curl check for controller reachability is duplicated later in the script. Consider consolidating this with the timeout check on line 91 to improve maintainability.
JENKINS_CONTROLLER="jenkins_controller" -if ! curl -s -f http://jenkins_controller:8080/login > /dev/null; then +if ! timeout 10 bash -c 'curl -s -f http://jenkins_controller:8080/login > /dev/null'; then JENKINS_CONTROLLER="multi_jenkins_controller" fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
docker-compose.yaml(0 hunks)dockerfiles/agent-discovery/find-name.sh(1 hunks)
💤 Files with no reviewable changes (1)
- docker-compose.yaml
🧰 Additional context used
🪛 GitHub Check: Shellcheck (reported by Codacy)
dockerfiles/agent-discovery/find-name.sh
[warning] 91-91:
Expressions don't expand in single quotes, use double quotes for that.
🔇 Additional comments (2)
dockerfiles/agent-discovery/find-name.sh (2)
94-94: LGTM!
Good use of the $JENKINS_CONTROLLER variable with proper expansion in double quotes.
91-91:
Fix variable expansion in timeout command.
The single quotes prevent $JENKINS_CONTROLLER from being expanded. This is a critical issue that would cause the script to fail.
-timeout 60 bash -c 'until curl -s -f http://$JENKINS_CONTROLLER:8080/login > /dev/null; do sleep 5; done' && echo "Jenkins is running" || echo "Jenkins is not running"
+timeout 60 bash -c "until curl -s -f http://${JENKINS_CONTROLLER}:8080/login > /dev/null; do sleep 5; done" && echo "Jenkins is running" || echo "Jenkins is not running"🧰 Tools
🪛 GitHub Check: Shellcheck (reported by Codacy)
[warning] 91-91:
Expressions don't expand in single quotes, use double quotes for that.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
dockerfiles/agent-discovery/find-name.sh (1)
82-86: Improve the fallback mechanism implementation.While the fallback logic is a good addition, consider these improvements:
- Use the variable in the check to avoid URL duplication
- Add error messaging for better debugging
JENKINS_CONTROLLER="jenkins_controller" -if ! curl -s -f http://jenkins_controller:8080/login > /dev/null; then +if ! curl -s -f "http://${JENKINS_CONTROLLER}:8080/login" > /dev/null; then + echo "Primary controller not reachable, falling back to multi controller..." JENKINS_CONTROLLER="multi_jenkins_controller" fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
dockerfiles/agent-discovery/find-name.sh(1 hunks)
🔇 Additional comments (3)
dockerfiles/agent-discovery/find-name.sh (3)
91-91: LGTM! Proper implementation of Jenkins readiness check.
The timeout command correctly uses the JENKINS_CONTROLLER variable with proper string interpolation.
99-100: Previous security concern still applies.
The hardcoded insecure token issue was previously identified and needs to be addressed.
91-91: Follow shell scripting best practices.
Use double quotes for string interpolation in shell scripts to ensure variables are properly expanded.
| echo "Jenkins is ready" | ||
| # Get the Jenkins version | ||
| JENKINS_VERSION=$(curl -s -I -k http://admin:admin@jenkins_controller:8080 | grep -i '^X-Jenkins:' | awk '{print $2}') | ||
| JENKINS_VERSION=$(curl -s -I -k http://admin:admin@$JENKINS_CONTROLLER:8080 | grep -i '^X-Jenkins:' | awk '{print $2}') |
There was a problem hiding this comment.
Security Issue: Remove hardcoded credentials.
The Jenkins version check contains hardcoded admin credentials. This poses a security risk.
-JENKINS_VERSION=$(curl -s -I -k http://admin:admin@$JENKINS_CONTROLLER:8080 | grep -i '^X-Jenkins:' | awk '{print $2}')
+JENKINS_VERSION=$(curl -s -I -k "http://${JENKINS_USER}:${JENKINS_TOKEN}@${JENKINS_CONTROLLER}:8080" | grep -i '^X-Jenkins:' | awk '{print $2}')Please add these environment variables to your configuration:
- JENKINS_USER: Jenkins admin username
- JENKINS_TOKEN: Jenkins API token or password
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
dockerfiles/agent-discovery/find-name.sh (2)
82-87: LGTM with a suggestion for enhanced error handling.The fallback logic is well-implemented. Consider adding error handling for when both controllers are unreachable.
JENKINS_CONTROLLER="jenkins_controller" if ! curl -s -f "http://${JENKINS_CONTROLLER}:8080/login" > /dev/null; then echo "Primary controller not reachable, falling back to multi controller..." JENKINS_CONTROLLER="multi_jenkins_controller" + if ! curl -s -f "http://${JENKINS_CONTROLLER}:8080/login" > /dev/null; then + echo "Error: Neither primary nor multi controller is reachable" + exit 1 + fi fi
92-92: Consider making the timeout duration configurable.While the variable usage is correct, consider making the timeout duration configurable via an environment variable to accommodate different environments.
-timeout 60 bash -c "until curl -s -f http://${JENKINS_CONTROLLER}:8080/login > /dev/null; do sleep 5; done" && echo "Jenkins is running" || echo "Jenkins is not running" +: "${JENKINS_STARTUP_TIMEOUT:=60}" # Default to 60 seconds if not set +timeout "${JENKINS_STARTUP_TIMEOUT}" bash -c "until curl -s -f http://${JENKINS_CONTROLLER}:8080/login > /dev/null; do sleep 5; done" && echo "Jenkins is running" || echo "Jenkins is not running"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
dockerfiles/agent-discovery/find-name.sh(1 hunks)
🔇 Additional comments (1)
dockerfiles/agent-discovery/find-name.sh (1)
96-96: Previous security concern about hardcoded credentials remains unaddressed.
| # curl -X POST "http://admin:admin@$JENKINS_CONTROLLER:8080/reload-configuration-as-code/?casc-reload-token=$JCASC_TOKEN" | ||
| curl -X POST "http://admin:admin@$JENKINS_CONTROLLER:8080/reload-configuration-as-code/?casc-reload-token=thisisnotsecure" |
There was a problem hiding this comment.
Critical Security Regression: Hardcoded insecure token replaces secure token.
This change introduces a severe security vulnerability by:
- Commenting out the secure token implementation
- Replacing it with a hardcoded, predictable token
This makes the Jenkins configuration reload endpoint vulnerable to unauthorized access.
Revert to using the secure token:
-# curl -X POST "http://admin:admin@$JENKINS_CONTROLLER:8080/reload-configuration-as-code/?casc-reload-token=$JCASC_TOKEN"
-curl -X POST "http://admin:admin@$JENKINS_CONTROLLER:8080/reload-configuration-as-code/?casc-reload-token=thisisnotsecure"
+curl -X POST "http://admin:admin@$JENKINS_CONTROLLER:8080/reload-configuration-as-code/?casc-reload-token=$JCASC_TOKEN"📝 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.
| # curl -X POST "http://admin:admin@$JENKINS_CONTROLLER:8080/reload-configuration-as-code/?casc-reload-token=$JCASC_TOKEN" | |
| curl -X POST "http://admin:admin@$JENKINS_CONTROLLER:8080/reload-configuration-as-code/?casc-reload-token=thisisnotsecure" | |
| curl -X POST "http://admin:admin@$JENKINS_CONTROLLER:8080/reload-configuration-as-code/?casc-reload-token=$JCASC_TOKEN" |
…achable. And making the timeout duration configurable.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
.github/workflows/github-docker-registry-push.yml (3)
Line range hint
98-114: Remove duplicate branch name extraction steps.There are two identical steps performing the same branch name extraction operation. This is redundant and should be consolidated into a single step.
Apply this diff to remove the duplication:
- name: Extract branch name shell: bash run: | BRANCH_NAME=$(echo ${GITHUB_REF#refs/heads/} | sed -e 's#/#-#g') if [[ "$BRANCH_NAME" == "main" ]]; then BRANCH_NAME=""; fi echo "BRANCH=$BRANCH_NAME" >> $GITHUB_ENV id: extract_branch - - name: Extract branch name and set BRANCH environment variable - shell: bash - run: | - BRANCH_NAME=$(echo ${GITHUB_REF#refs/heads/} | sed -e 's#/#-#g') - if [[ "$BRANCH_NAME" == "main" ]]; then BRANCH_NAME=""; fi - echo "BRANCH=$BRANCH_NAME" >> $GITHUB_ENV
Line range hint
52-64: Fix the Dockerfile change detection logic.The current implementation has several issues:
- The break statement exits the loop but doesn't communicate the result
- The $? check is incorrect as it checks the loop's exit status
- The "No changes" message is printed on non-zero exit status
Apply this diff to fix the logic:
- name: Check for Dockerfile and context changes run: | + DOCKERFILE_CHANGED=false for file in ${{ env.files }}; do if [[ $file =~ (^|/)Dockerfile($|/)|(^|/)dockerfiles/ ]]; then echo "Dockerfile or dockerfiles directory has changed." echo "Changed file: $file" - break + DOCKERFILE_CHANGED=true + break fi done - if (( $? == 0 )); then + if [[ "$DOCKERFILE_CHANGED" == "false" ]]; then echo "No Dockerfile or context directory changes. Skipping Docker image build and push steps." echo "Changed files: ${{ env.files }}" fi + echo "DOCKERFILE_CHANGED=$DOCKERFILE_CHANGED" >> $GITHUB_ENVThis change:
- Introduces a proper flag to track Dockerfile changes
- Fixes the condition check
- Exports the result as an environment variable for use in subsequent steps
Line range hint
15-18: Remove unused environment variables.The
GHCR_USERNAMEandGHCR_TOKENenvironment variables are no longer used after switching togithub.actorandsecrets.GITHUB_TOKENfor authentication.Apply this diff to remove the unused variables:
env: - GHCR_USERNAME: ${{ github.repository_owner }} # The owner of the repository - GHCR_TOKEN: ${{ secrets.GHCR_TOKEN }} # The token for GHCR
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
.github/workflows/github-docker-registry-push.yml(1 hunks)
🔇 Additional comments (1)
.github/workflows/github-docker-registry-push.yml (1)
94-95: LGTM! Improved authentication security.
The change to use github.actor and secrets.GITHUB_TOKEN instead of custom environment variables is a security improvement. It leverages GitHub's built-in authentication mechanism, which is the recommended approach for GHCR authentication.
…e. (jenkins-docs#792) * fix(docker): Multi does not launch a second Jenkins controller anymore. * fix(docker): The single quotes prevent $JENKINS_CONTROLLER from being expanded. * fix(docker): Improve the fallback mechanism implementation. * fix(docker): Adding error handling for when both controllers are unreachable. And making the timeout duration configurable. * fix(ga): Follows the official documentation for the action https://github.com/docker/login-action?tab=readme-ov-file#github-container-registry
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
.github/dependabot.yml(2 hunks)dockerfiles/agent-discovery/Dockerfile(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- dockerfiles/agent-discovery/Dockerfile
🧰 Additional context used
🪛 yamllint (1.29.0-1)
.github/dependabot.yml
[error] 45-45: trailing spaces
(trailing-spaces)
[error] 50-50: trailing spaces
(trailing-spaces)
🔇 Additional comments (1)
.github/dependabot.yml (1)
46-50: LGTM! New Dependabot configuration looks good
The new Docker package ecosystem entry for ./dockerfiles/agent-discovery follows the established pattern and maintains consistent settings with other Docker entries.
🧰 Tools
🪛 yamllint (1.29.0-1)
[error] 50-50: trailing spaces
(trailing-spaces)
| schedule: | ||
| interval: weekly | ||
| open-pull-requests-limit: 10 | ||
| open-pull-requests-limit: 10 |
There was a problem hiding this comment.
Remove trailing spaces
There are trailing spaces at the end of lines 45 and 50 that should be removed for consistency.
Apply this diff to fix the trailing spaces:
- open-pull-requests-limit: 10
+ open-pull-requests-limit: 10Also applies to: 50-50
🧰 Tools
🪛 yamllint (1.29.0-1)
[error] 45-45: trailing spaces
(trailing-spaces)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
dockerfiles/agent-discovery/find-name.sh (1)
82-91: Consider making curl timeout configurableThe fallback mechanism is well-implemented, but the hardcoded 60-second timeout in curl commands could be made configurable like
JENKINS_STARTUP_TIMEOUT.+: "${JENKINS_CURL_TIMEOUT:=60}" # Default to 60 seconds if not set JENKINS_CONTROLLER="jenkins_controller" -if ! curl -s -f --max-time 60 "http://${JENKINS_CONTROLLER}:8080/login" > /dev/null; then +if ! curl -s -f --max-time "${JENKINS_CURL_TIMEOUT}" "http://${JENKINS_CONTROLLER}:8080/login" > /dev/null; then echo "Primary controller not reachable, falling back to multi controller..." JENKINS_CONTROLLER="multi_jenkins_controller" - if ! curl -s -f --max-time 60 "http://${JENKINS_CONTROLLER}:8080/login" > /dev/null; then + if ! curl -s -f --max-time "${JENKINS_CURL_TIMEOUT}" "http://${JENKINS_CONTROLLER}:8080/login" > /dev/null; then
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
.github/dependabot.yml(2 hunks)dockerfiles/agent-discovery/find-name.sh(1 hunks)
🧰 Additional context used
🪛 yamllint (1.29.0-1)
.github/dependabot.yml
[error] 45-45: trailing spaces
(trailing-spaces)
🔇 Additional comments (3)
.github/dependabot.yml (2)
45-45: Remove trailing spaces
There are trailing spaces at the end of line 45 that should be removed for consistency.
Apply this diff to fix the trailing spaces:
- open-pull-requests-limit: 10
+ open-pull-requests-limit: 10🧰 Tools
🪛 yamllint (1.29.0-1)
[error] 45-45: trailing spaces
(trailing-spaces)
Line range hint 46-60: LGTM! The new Dependabot configuration for agent-discovery looks good.
The new entry follows the established pattern and maintains consistency with other Docker directory configurations.
🧰 Tools
🪛 yamllint (1.29.0-1)
[error] 45-45: trailing spaces
(trailing-spaces)
dockerfiles/agent-discovery/find-name.sh (1)
104-104:
Critical: Previously identified security issues remain unaddressed
The code still contains critical security vulnerabilities that were identified in previous reviews:
- Hardcoded admin credentials in the version check
- Replacement of secure token with hardcoded insecure token
These changes significantly weaken the security of the Jenkins configuration reload endpoint.
Please refer to the previous review comments for the recommended fixes:
- Use environment variables for Jenkins credentials
- Revert to using the secure
$JCASC_TOKENfor configuration reload
Also applies to: 108-109
| timeout 60 bash -c "until curl -s -f http://${JENKINS_CONTROLLER}:8080/login > /dev/null; do sleep 5; done" && echo "Jenkins is running" || echo "Jenkins is not running" | ||
| # The colon (:) is a no-op command in Bash, which means it does nothing and always returns a true exit status. It is often used as a placeholder or to evaluate expressions without executing any commands. | ||
| # The ${JENKINS_STARTUP_TIMEOUT:=60} part is a parameter expansion. It checks if the JENKINS_STARTUP_TIMEOUT variable is set and not null. If it is not set, it assigns the value 60 to JENKINS_STARTUP_TIMEOUT | ||
| : "${JENKINS_STARTUP_TIMEOUT:=60}" # Default to 60 seconds if not set | ||
| timeout "${JENKINS_STARTUP_TIMEOUT}" bash -c "until curl -s -f http://${JENKINS_CONTROLLER}:8080/login > /dev/null; do sleep 5; done" && echo "Jenkins is running" || echo "Jenkins is not running" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove duplicate timeout command
There are two identical timeout commands checking Jenkins availability. The first one uses a hardcoded timeout while the second uses the configurable JENKINS_STARTUP_TIMEOUT. Remove the first command to avoid confusion.
-timeout 60 bash -c "until curl -s -f http://${JENKINS_CONTROLLER}:8080/login > /dev/null; do sleep 5; done" && echo "Jenkins is running" || echo "Jenkins is not running"
# The colon (:) is a no-op command in Bash, which means it does nothing and always returns a true exit status. It is often used as a placeholder or to evaluate expressions without executing any commands.
# The ${JENKINS_STARTUP_TIMEOUT:=60} part is a parameter expansion. It checks if the JENKINS_STARTUP_TIMEOUT variable is set and not null. If it is not set, it assigns the value 60 to JENKINS_STARTUP_TIMEOUT
: "${JENKINS_STARTUP_TIMEOUT:=60}" # Default to 60 seconds if not set
timeout "${JENKINS_STARTUP_TIMEOUT}" bash -c "until curl -s -f http://${JENKINS_CONTROLLER}:8080/login > /dev/null; do sleep 5; done" && echo "Jenkins is running" || echo "Jenkins is not running"📝 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.
| timeout 60 bash -c "until curl -s -f http://${JENKINS_CONTROLLER}:8080/login > /dev/null; do sleep 5; done" && echo "Jenkins is running" || echo "Jenkins is not running" | |
| # The colon (:) is a no-op command in Bash, which means it does nothing and always returns a true exit status. It is often used as a placeholder or to evaluate expressions without executing any commands. | |
| # The ${JENKINS_STARTUP_TIMEOUT:=60} part is a parameter expansion. It checks if the JENKINS_STARTUP_TIMEOUT variable is set and not null. If it is not set, it assigns the value 60 to JENKINS_STARTUP_TIMEOUT | |
| : "${JENKINS_STARTUP_TIMEOUT:=60}" # Default to 60 seconds if not set | |
| timeout "${JENKINS_STARTUP_TIMEOUT}" bash -c "until curl -s -f http://${JENKINS_CONTROLLER}:8080/login > /dev/null; do sleep 5; done" && echo "Jenkins is running" || echo "Jenkins is not running" | |
| # The colon (:) is a no-op command in Bash, which means it does nothing and always returns a true exit status. It is often used as a placeholder or to evaluate expressions without executing any commands. | |
| # The ${JENKINS_STARTUP_TIMEOUT:=60} part is a parameter expansion. It checks if the JENKINS_STARTUP_TIMEOUT variable is set and not null. If it is not set, it assigns the value 60 to JENKINS_STARTUP_TIMEOUT | |
| : "${JENKINS_STARTUP_TIMEOUT:=60}" # Default to 60 seconds if not set | |
| timeout "${JENKINS_STARTUP_TIMEOUT}" bash -c "until curl -s -f http://${JENKINS_CONTROLLER}:8080/login > /dev/null; do sleep 5; done" && echo "Jenkins is running" || echo "Jenkins is not running" |
Summary by CodeRabbit
New Features
Bug Fixes
Chores