Skip to content
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 infinite redirect loop with httpd collector, support HTTPS #567

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

alexmv
Copy link

@alexmv alexmv commented Dec 21, 2016

Without these, a status page which redirects HTTP to HTTPS causes an infinite loop and wedges the local collector.

Alex Vandiver added 2 commits December 21, 2016 13:48
Without an upper bound on the number of redirects, redirects loops
(caused either by client- or server-side bugs) will lead to infinite
loops in the collector, potentially preventing any statistics from
being reported at all.

Set a default of 5 redirects, maximum, with a configurable setting to
adjust this.
Previously, HttpdCollector assumed all URLs on their default ports
were requests to port 80. In the case of a `http://localhost/` ->
`https://localhost/` redirect, the latter URL is parsed by `urlparse`
as "Host localhost on the default port" (`parts.netloc` aka `parts[1]`
did not contain a `:`), and a request is made to `http://localhost/`,
which promptly re-redirects to HTTPS again.

Transition to using the attribute names of `urlparse` for readability,
and support both HTTP and HTTPS URLs, by examining the scheme and
constructing the correct httplib class accordingly.
@coveralls
Copy link

coveralls commented Dec 21, 2016

Coverage Status

Coverage remained the same at 59.729% when pulling 8bd0585 on alexmv:httpd-https into f4a523f on python-diamond:master.

Copy link
Member

@shortdudey123 shortdudey123 left a comment

Choose a reason for hiding this comment

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

docs/collectors/HttpdCollector.md needs updating and possible to add tests for the new behavior?

@alexmv
Copy link
Author

alexmv commented Feb 18, 2017

Will do!

@shortdudey123
Copy link
Member

shortdudey123 commented Jul 26, 2022

Will merge once previous comment is handled and merge conflicts are fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants