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 linkerd inject url handling #3039

Closed
wants to merge 1 commit into from
Closed

Fix linkerd inject url handling #3039

wants to merge 1 commit into from

Conversation

siggy
Copy link
Member

@siggy siggy commented Jul 5, 2019

PR #2988 introduced support for linkerd inject with a URL parameter.
Some edge cases such as /foo/bar/baz.yml will return without error
from url.ParseRequestURI, even though we know this is a valid local
filename.

Modify inject's URL check to specifically look for a valid http[s]://
prefix. If that check succeeds, we will still rely on http.Get for URL
validation, as it calls url.Parse under the hood.

Fixes #3037

Signed-off-by: Andrew Seigner siggy@buoyant.io

PR #2988 introduced support for `linkerd inject` with a URL parameter.
Some edge cases such as `/foo/bar/baz.yml` will return without error
from `url.ParseRequestURI`, even though we know this is a valid local
filename.

Modify inject's URL check to specifically look for a valid `http[s]://`
prefix. If that check succeeds, we will still rely on `http.Get` for URL
validation, as it calls `url.Parse` under the hood.

Fixes #3037

Signed-off-by: Andrew Seigner <siggy@buoyant.io>
@siggy siggy requested a review from Pothulapati July 5, 2019 12:15
@siggy siggy self-assigned this Jul 5, 2019
@l5d-bot
Copy link
Collaborator

l5d-bot commented Jul 5, 2019

Integration test results for 736e1a7: success 🎉
Log output: https://gist.github.com/cfe50314b19cfc76e8ad588af2cd3f60

Copy link
Contributor

@Pothulapati Pothulapati left a comment

Choose a reason for hiding this comment

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

This should work and wouldn't need any tests too 😄

@siggy
Copy link
Member Author

siggy commented Jul 5, 2019

@Pothulapati I really did try to add a test file, but the read function depends on filesystem and network and did not want to abstract that away for such a small change. 😄

@siggy siggy closed this Jul 5, 2019
@siggy siggy deleted the siggy/url-inject branch July 5, 2019 14:21
@siggy
Copy link
Member Author

siggy commented Jul 5, 2019

closing in favor of #3038

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

Successfully merging this pull request may close these issues.

linkerd inject doesn't work when supplying a full path to a file outside the current dir
3 participants