-
Notifications
You must be signed in to change notification settings - Fork 313
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
Properly authenticate at proxy server #652
Conversation
With this commit Rally will set the proper authentication header for proxies that require it. Closes elastic#636
I am documenting how I tested this, in case we want to add an integration test later on for similar stuff:
Created a
and run:
and to see access logs:
On the Rally side I first set:
Using current Rally master commit:
shows on the squid log:
plus of course Rally itself reports:
Using this PR with the same Rally command, squid log shows:
and the warning is gone from the Rally output. |
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.
LGTM also based on #652 (comment)
integration-test.sh
Outdated
if docker ps > /dev/null; then | ||
info "Docker is available. Proxy-related tests will be run" | ||
# Portably create a temporary config directory for Squid on Linux or MacOS | ||
local config_dir=$(mktemp -d 2>/dev/null || mktemp -d -t 'tmp_squid_cfg') |
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.
For Docker on MacOS this requires to (manually) add /var/folders
to the list of allowed paths (otherwise Docker will complain that it can't bind-mount the directory). /var/folders
is actually symlinked to /private/var/folders
and /private
is already configured (recursively) for Docker but not /var/folders
is not. I think we should add this to our developer docs. Wdyt?
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.
As discussed via another channel we'll instead use an already existing directory tree that we use for integration tests anyway. I'll push a new commit addressing this.
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.
LGTM, thanks for adding the integration test!
With this commit Rally will set the proper authentication header for
proxies that require it.
Closes #636