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.

Sign up for GitHub

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

Jump to bottom

added --uri_base_prefix option (#2018) #2117

Merged
merged 1 commit into from
Dec 4, 2018
Merged

added --uri_base_prefix option (#2018) #2117

merged 1 commit into from
Dec 4, 2018

Conversation

tghosgor
Copy link
Contributor

@tghosgor tghosgor commented Dec 3, 2018 •
edited
Loading

Some reverse proxies do not support overriding 'Location' header
in 3xx responses. This commit adds a --uri_base_prefix option
that adds a prefix to all URLs of cAdvisor so that
redirects go to the correct places.

@k8s-ci-robot
Copy link
Collaborator

Hi @tghosgor. Thanks for your PR.

I'm waiting for a google or kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@dashpole dashpole self-assigned this Dec 3, 2018
@dashpole
Copy link
Collaborator

dashpole commented Dec 3, 2018

/ok-to-test

dashpole
dashpole reviewed Dec 3, 2018
View reviewed changes
Copy link
Collaborator

@dashpole dashpole left a comment

Choose a reason for hiding this comment

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

I'm not super-familiar with this space. Can you go into a little more detail on why you need to add a prefix to all endpoints to work with some proxies?

cadvisor.go Outdated
@@ -62,6 +62,8 @@ var collectorKey = flag.String("collector_key", "", "Key for the collector's cer

var storeContainerLabels = flag.Bool("store_container_labels", true, "convert container labels and environment variables into labels on prometheus metrics for each container. If flag set to false, then only metrics exported are container name, first alias, and image name")

var basePrefix = flag.String("base_prefix", "", "prefix path that will be prepended to all paths to support some reverse proxies")
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we make this less generic-sounding? Maybe url_base_prefix?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea.

@tghosgor tghosgor changed the title added --base_prefix option (#2018) added --uri_base_prefix option (#2018) Dec 4, 2018
@tghosgor
Copy link
Contributor Author

tghosgor commented Dec 4, 2018 •
edited
Loading

I'm not super-familiar with this space. Can you go into a little more detail on why you need to add a prefix to all endpoints to work with some proxies?

Sure.

We need to serve cAdvisor behind a reverse proxy on a sub-path, e.g. /cadvisor. I have seen you have done some work to support reverse proxies (#667, #680). It would normally be enough for reverse proxies such as nginx with it's proxy_redirect as it supports rewriting Location: header in 3xx redirects. Unfortunately, this is not an option for all reverse proxies.

What is happening is that ServeMux does the redirects with absolute paths to add a trailing slash. This happens quite a lot while navigating around (i.e. <a href="../docker">Docker Containers</a>).

I would expect nesting ServeMux would be enough instead of all this but it is not aware of its parents when doing redirects and it provides no way to override/extend its redirect behavior. The link /cadvisor/containers/../docker resolves to a 3xx to /docker/ which is out of /cadvisor sub-path. The official way seems to be adding a mux handler to /cadvisor/containers/../docker to do a correct redirect to /cadvisor/docker/. These eventually resulted in the url_base_prefix, serving everything on a sub-path.

@tghosgor
added --url_base_prefix option (#2018)
8018ee1 
Some reverse proxies do not support overriding 'Location' header
in 3xx responses. This commit adds a --url_base_prefix option
that adds a prefix to all URLs of cAdvisor so that
redirects go to the correct places.
dashpole
dashpole approved these changes Dec 4, 2018
View reviewed changes
Copy link
Collaborator

@dashpole dashpole left a comment

Choose a reason for hiding this comment

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

Thanks for the explanation. This change lgtm

@dashpole dashpole merged commit 27baaca into google:master Dec 4, 2018
@davidjsanders
Copy link

Hi All, is there any timeline when this will make it into a Docker build? We're waiting for this feature to run cAdvisor behind Traefik.

Thanks, David.

@dashpole
Copy link
Collaborator

You are welcome to run the canary build. We will have a release within the next month that corresponds with the kubernetes 1.14 release

@davidjsanders
Copy link

@dashpole Thanks for the guidance - much appreciated :)

@dashpole dashpole mentioned this pull request Jan 16, 2020
Closed
karl-tpio added a commit to karl-tpio/cadvisor that referenced this pull request Jan 16, 2020
@karl-tpio
Update runtime_options.md
08d0a0b 
Added a quick descriptor for the `url_base_prefix` option.

Closes: google#2377
Documents: google#2117
@karl-tpio karl-tpio mentioned this pull request Jan 16, 2020
Merged
@aparedero
Copy link

Hi. I'm using last version pushed in dockerhub but option --uri_base_prefix is not current available

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dashpole dashpole dashpole approved these changes

Assignees

@dashpole dashpole

Labels
ok-to-test
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@tghosgor @k8s-ci-robot @dashpole @davidjsanders @aparedero