Skip to content

Conversation

@largetimmo
Copy link
Collaborator

@largetimmo largetimmo commented Jul 31, 2025

✨ PR Description

Purpose: Add DMR cluster status and synchronization verification to message broker readiness check script for improved reliability monitoring.

Main changes:

  • Added broker version detection to conditionally execute checks based on feature availability
  • Implemented DMR cluster operational status verification for broker versions 8 and above
  • Added DMR cluster synchronization status check for broker versions 11 and above

Generated by LinearB AI and added by gitStream.
AI-generated content may contain inaccuracies. Please verify before using. We'd love your feedback! 🚀

@largetimmo largetimmo changed the base branch from main to dev1.4.0 July 31, 2025 18:32
Copy link

@gitstream-cm gitstream-cm bot left a comment

Choose a reason for hiding this comment

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

✨ PR Review

The PR adds DMR cluster status and sync checks with version-based conditional logic. The implementation introduces potential security and reliability concerns that should be addressed.

3 issues detected:

🔒 Security - Unescaped variable expansion in shell commands creates security vulnerabilities.

Details: The password variable is directly interpolated into shell commands without proper escaping or quoting. If the password contains special shell characters (backticks, dollar signs, semicolons), it could lead to command injection vulnerabilities.
File: controllers/brokerscripts/readiness_check.sh (179-179)

🐞 Bug - Version parsing assumes fixed format and lacks error handling for unexpected version strings.

Details: The version parsing logic assumes a specific format (X.Y.Z) and uses cut -d'.' -f2 to extract the major version. This approach is brittle and could fail if the version format changes or contains unexpected characters.
File: controllers/brokerscripts/readiness_check.sh

🐞 Bug - No validation of query results before processing could lead to incorrect readiness status determination.

Details: The SEMP queries lack proper error handling. If the queries fail or return unexpected responses, the xmllint parsing could fail silently or produce unexpected results, potentially causing false positives or negatives in the readiness check.
File: controllers/brokerscripts/readiness_check.sh

Generated by LinearB AI and added by gitStream.
AI-generated content may contain inaccuracies. Please verify before using. We'd love your feedback! 🚀

broker_version=$(/mnt/disks/solace/semp_query.sh -n admin -p ${password} -u http://localhost:8080 \
-q "<rpc><show><version/></show></rpc>" \
-v "/rpc-reply/rpc/show/version/current-load[text()]")
broker_version=`echo ${broker_version} | xmllint -xpath "string(returnInfo/valueSearchResult)" - | cut -d'.' -f2`
Copy link

Choose a reason for hiding this comment

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

🔒 Security - Command Injection Risk: Properly quote the password variable or use a more secure method to pass credentials, such as reading from a secure file or using environment variables with proper escaping.

Suggested change
broker_version=`echo ${broker_version} | xmllint -xpath "string(returnInfo/valueSearchResult)" - | cut -d'.' -f2`
broker_version=`echo ${broker_version} | xmllint -xpath "string(returnInfo/valueSearchResult)" -`
# Extract major version with robust parsing and validation
if [[ "$broker_version" =~ ^[0-9]+\.[0-9]+\.[0-9]+ ]]; then
broker_version=$(echo "$broker_version" | cut -d'.' -f1)
else
echo "$(date) WARNING: Unable to parse broker version format: $broker_version"
broker_version=0
fi

@gitstream-cm
Copy link

gitstream-cm bot commented Jul 31, 2025

Please mark whether you used Copilot to assist coding in this PR

  • Copilot Assisted

@LewisKSaint LewisKSaint merged commit 4c20bbc into dev1.4.0 Jul 31, 2025
13 of 22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants