-
Notifications
You must be signed in to change notification settings - Fork 183
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: Handle empty or invalid Sumo base URL #2240
fix: Handle empty or invalid Sumo base URL #2240
Conversation
Tested with the following script: #!/bin/bash
function fix_sumo_base_url() {
local BASE_URL=$SUMOLOGIC_BASE_URL
if [[ ! "$BASE_URL" =~ ^https:\/\/.*sumologic\.com\/api\/.*$ ]]; then
BASE_URL="https://api.sumologic.com/api/"
fi
OPTIONAL_REDIRECTION="$(curl -XGET -s -o /dev/null -D - \
-u "${SUMOLOGIC_ACCESSID}:${SUMOLOGIC_ACCESSKEY}" \
"${BASE_URL}"v1/collectors \
| grep -Fi location )"
if [[ ! $OPTIONAL_REDIRECTION =~ ^\s*$ ]]; then
BASE_URL=$( echo $OPTIONAL_REDIRECTION | sed -E 's/.*: (https:\/\/.*(au|ca|de|eu|fed|in|jp|us2)?\.sumologic\.com\/api\/).*/\1/' )
fi
BASE_URL=${BASE_URL%v1*}
echo $BASE_URL
}
TESTS=( \
"" \
"https://blabla.com/api/" \
"http://api.sumologic.com/api/" \
"https://sumologic.com/" \
"https://api.jp.sumologic.com/api/" \
"https://api.sumologic.com/api/" \
"https://api.de.sumologic.com/api/" \
"https://api.de.sumologic.com/api/v1/" \
"https://api.de.sumologic.com/api/v1/collectors" \
)
for TEST in ${TESTS[@]}; do
SUMOLOGIC_BASE_URL=$TEST
echo $SUMOLOGIC_BASE_URL
echo $(fix_sumo_base_url)
echo "----------------------"
done
unset SUMOLOGIC_BASE_URL
echo "unset"
echo $(fix_sumo_base_url) Output with keys from
|
function fix_sumo_base_url() { | ||
local BASE_URL=$SUMOLOGIC_BASE_URL | ||
|
||
if [[ ! "$BASE_URL" =~ ^https:\/\/.*sumologic\.com\/api\/.*$ ]]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should just check if this is set instead of making sure if it's a sumologic.com subdomain. It's valid to put other urls here for testing - this is why integration tests are currently failing.
| grep -Fi location )" | ||
|
||
if [[ ! $OPTIONAL_REDIRECTION =~ ^\s*$ ]]; then | ||
BASE_URL=$( echo $OPTIONAL_REDIRECTION | sed -E 's/.*: (https:\/\/.*(au|ca|de|eu|fed|in|jp|us2)?\.sumologic\.com\/api\/).*/\1/' ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BASE_URL=$( echo $OPTIONAL_REDIRECTION | sed -E 's/.*: (https:\/\/.*(au|ca|de|eu|fed|in|jp|us2)?\.sumologic\.com\/api\/).*/\1/' ) | |
BASE_URL=$( echo "$OPTIONAL_REDIRECTION" | sed -E 's/.*: (https:\/\/.*(au|ca|de|eu|fed|in|jp|us2)?\.sumologic\.com\/api\/).*/\1/' ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"${OPTIONAL_REDIRECTION}"
|
||
BASE_URL=${BASE_URL%v1*} | ||
|
||
echo $BASE_URL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
echo $BASE_URL | |
echo "$BASE_URL" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"${BASE_URL}"
@starzu-sumo overall LGTM, but please run shellcheck against generated |
From shellcheck:
|
Please fix it 😅 I believe first is obvious, the second is explained here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
# Fix URL to remove "v1" or "v1/" | ||
export SUMOLOGIC_BASE_URL=${SUMOLOGIC_BASE_URL%v1*} | ||
function fix_sumo_base_url() { | ||
local BASE_URL=$SUMOLOGIC_BASE_URL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@starzu-sumo Sorry for another comment, I was just checking before merge
Didn't lint complain about this line? 😨
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, do you want to change it to the following anyway?
BASE_URL=$SUMOLOGIC_BASE_URL
local BASE_URL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking rather about "${SUMOLOGIC_BASE_URL}"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but I believe both changes should be added
@starzu-sumo thank you for the change and patience, I created an issue which will help to resolve the bash syntax. Ideally I would ask you to do it manually for now, but as we have an issue we will resolve it sooner or later. So, it's up to you :) |
Description
It's allowed to left
sumologic.endpoint
empty. In such caseSUMOLOGIC_BASE_URL
is empty and breaks logic ofget_remaining_fields
and other API calls.Checklist
Testing performed