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

Make URLLibFetcher aware of basic auth info in scheduler URL. #2791

Merged
merged 3 commits into from
Nov 26, 2019

Conversation

riga
Copy link
Contributor

@riga riga commented Sep 24, 2019

Description

This PR makes luigi.rpc.URLLibFetcher aware of basic auth infos encoded in the scheduler URL so that a valid Authorization header for basic auth is added to the request.

Motivation and Context

We are running luigid behind an nginx proxy with basic auth (docker image). User and password are defined right in the [core] default-scheduler-host config value, e.g., "http://user:pass@localhost".

When the requests package is available, luigi.rpc.RequestsFetcher is used which automatically extracts this information and creates the Authorization header for basic auth when session.post() is called.

The fallback, URLLibFetcher, does not do that. urllib2.urlopen responds with

<urlopen error [Errno -2] Name or service not known>

This PR adds that feature so that the behavior is as expected and in particular independent of the presence of requests.

Have you tested this? If so, how?

I added unit tests to check if the created request object contains the correct auth header for multiple use cases. Also I tested again with the docker image above and everything works as expected.

Copy link
Contributor

@Tarrasch Tarrasch left a comment

Choose a reason for hiding this comment

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

Wouldn't say I fully understand this but seems legit!

@honnix honnix merged commit ff9342d into spotify:master Nov 26, 2019
drowoseque pushed a commit to drowoseque/luigi that referenced this pull request Nov 26, 2019
…y#2791)

* Make URLLibFetcher aware of basic auth.

* Fix setting request body in URLLibFetcher.

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

Successfully merging this pull request may close these issues.

3 participants