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

Avoid rewriting URLs in the proxy, if the application is proxy-aware. #14697

Merged
merged 1 commit into from
Oct 1, 2015
Merged

Avoid rewriting URLs in the proxy, if the application is proxy-aware. #14697

merged 1 commit into from
Oct 1, 2015
+12 −1

Conversation

vishh
Copy link
Contributor

@vishh vishh commented Sep 29, 2015

Related to #14624

cc @lavalamp

lavalamp
lavalamp reviewed Sep 29, 2015
View reviewed changes
pkg/util/proxy/transport.go
// Add back the trailing slash, which was stripped by path.Join().
url.Path += "/"
// Do not rewrite URL if the sourceURL already contains the necessary prefix.
if !strings.HasPrefix(url.Path, t.PathPrepend) {
Copy link
Member

Choose a reason for hiding this comment

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

nit: Phrase in positive:

if strings.HasPrefix(url.Path, t.PathPrepend) {
  return url.String()
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@lavalamp
Copy link
Member

LGTM

@lavalamp lavalamp added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 29, 2015
@lavalamp
Copy link
Member

From our chat this morning, I'm guessing you'll probably want to cherry-pick this.

@vishh
Copy link
Contributor Author

vishh commented Sep 29, 2015

Yeah. This one and another patch that updated Grafana configs.

On Mon, Sep 28, 2015 at 5:22 PM, Daniel Smith notifications@github.com
wrote:

From our chat this morning, I'm guessing you'll probably want to
cherry-pick this.

—
Reply to this email directly or view it on GitHub
#14697 (comment)
.

@k8s-bot
Copy link

k8s-bot commented Sep 29, 2015

Unit, integration and GCE e2e test build/test passed for commit ee20ca07d1293203d08771256e3ddd8a73b98c2d.

@k8s-bot
Copy link

k8s-bot commented Sep 29, 2015

Unit, integration and GCE e2e build/test failed for commit e048cd445da3104a667c5af9616be09b7d406ec7.

@k8s-bot
Copy link

k8s-bot commented Sep 29, 2015

Unit, integration and GCE e2e test build/test passed for commit 2788b47.

@k8s-github-robot k8s-github-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Sep 29, 2015
@k8s-github-robot
Copy link

Labelling this PR as size/S

@bgrant0607
Copy link
Member

See #5897.
cc @enisoc

@bgrant0607 bgrant0607 assigned lavalamp and unassigned bgrant0607 Sep 29, 2015
@vishh
Copy link
Contributor Author

vishh commented Oct 1, 2015

@davidopp: This PR needs to go in soon because it needs to be cherry-picked into v1.1

@davidopp
Copy link
Member

davidopp commented Oct 1, 2015

Tests finally passed, merging manually.

davidopp added a commit that referenced this pull request Oct 1, 2015
@davidopp
Merge pull request #14697 from vishh/api-proxy
a448b5e 
Avoid rewriting URLs in the proxy, if the application is proxy-aware.
@davidopp davidopp merged commit a448b5e into kubernetes:master Oct 1, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@lavalamp lavalamp

Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@vishh @lavalamp @k8s-bot @k8s-github-robot @bgrant0607 @davidopp @googlebot