Use URL from response to support redirects

[jpyams]

Pull Request Check List

• Added tests for changed code.

I don't know if this change needs additional tests or documentation, since it is a small bugfix from an issue I've encountered. Also don't know if I should have created an issue first to report the bug. Any guidance here is appreciated.

On my team, we host our custom packages on a pypiserver that redirects to an Artifactory PyPI mirror if the package isn't a custom one. So we add a source section to our pyproject.toml.

[[tool.poetry.source]]
name = "ourserver"
url = "https://ourserver.example.com/pypiserver"
default = true

However, when we have default=True, the installation fails.

$> ~/.local/bin/poetry install -vvv
Creating virtualenv tool-G6amg8OA-py3.8 in /home/jyamauchi/.cache/pypoetry/virtualenvs
Using virtualenv: /home/jyamauchi/.cache/pypoetry/virtualenvs/tool-G6amg8OA-py3.8
Updating dependencies
Resolving dependencies...
   1: fact: tool is 0.7.4
   1: derived: tool
   1: fact: tool depends on click (^7.1.1)
   1: fact: tool depends on openpyxl (^3.0.3)
   1: fact: tool depends on pytest (^5.2)
   1: fact: tool depends on pytest (^5.2)
   1: selecting tool (0.7.4)
   1: derived: pytest (^5.2)
   1: derived: openpyxl (^3.0.3)
   1: derived: click (^7.1.1)
   1: selecting click (7.1.2)
   1: fact: openpyxl (3.0.5) depends on jdcal (*)
   1: fact: openpyxl (3.0.5) depends on et-xmlfile (*)
   1: selecting openpyxl (3.0.5)
   1: derived: et-xmlfile (*)
   1: derived: jdcal (*)
   1: selecting et-xmlfile (1.0.1)
   1: selecting jdcal (1.4.1)
   1: fact: pytest (5.4.3) depends on py (>=1.5.0)
   1: fact: pytest (5.4.3) depends on packaging (*)
   1: fact: pytest (5.4.3) depends on attrs (>=17.4.0)
   1: fact: pytest (5.4.3) depends on more-itertools (>=4.0.0)
   1: fact: pytest (5.4.3) depends on pluggy (>=0.12,<1.0)
   1: fact: pytest (5.4.3) depends on wcwidth (*)
   1: fact: pytest (5.4.3) depends on importlib-metadata (>=0.12)
   1: fact: pytest (5.4.3) depends on atomicwrites (>=1.0)
   1: fact: pytest (5.4.3) depends on colorama (*)
   1: selecting pytest (5.4.3)
   1: derived: colorama (*)
   1: derived: atomicwrites (>=1.0)
   1: derived: importlib-metadata (>=0.12)
   1: derived: wcwidth (*)
   1: derived: pluggy (>=0.12,<1.0)
   1: derived: more-itertools (>=4.0.0)
   1: derived: attrs (>=17.4.0)
   1: derived: packaging (*)
   1: derived: py (>=1.5.0)
   1: fact: pluggy (0.13.1) depends on importlib-metadata (>=0.12)
   1: selecting pluggy (0.13.1)
   1: selecting atomicwrites (1.4.0)
   1: selecting attrs (20.2.0)
   1: selecting py (1.9.0)
   1: selecting wcwidth (0.2.5)
   1: selecting more-itertools (8.5.0)
   1: fact: importlib-metadata (2.0.0) depends on zipp (>=0.5)
   1: selecting importlib-metadata (2.0.0)
   1: derived: zipp (>=0.5)
   1: selecting zipp (3.3.0)
   1: fact: packaging (20.4) depends on pyparsing (>=2.0.2)
   1: fact: packaging (20.4) depends on six (*)
   1: selecting packaging (20.4)
   1: derived: six (*)
   1: derived: pyparsing (>=2.0.2)
   1: selecting six (1.15.0)
   1: selecting pyparsing (2.4.7)
   1: selecting colorama (0.4.4)
   1: Version solving took 4.621 seconds.
   1: Tried 1 solutions.

Writing lock file

Finding the necessary packages for the current system

Package operations: 13 installs, 0 updates, 0 removals

  • Installing pyparsing (2.4.7): Pending...
  • Installing pyparsing (2.4.7): Failed

  HTTPError

  404 Client Error: Not Found for url: https://mtecsvr4.micron.com/packages/packages/8a/bb/488841f56197b13700afd5658fc279a2025a39e22449b7cf29864669b15d/pyparsing-2.4.7-py2.py3-none-any.whl#sha256=ef9d7589ef3c200abe66653d3f1ab1033c3c419ae9b9bdb1240a85b024efc88b

  at ~/.local/pipx/venvs/poetry/lib/python3.8/site-packages/requests/models.py:941 in raise_for_status
      937│         elif 500 <= self.status_code < 600:
      938│             http_error_msg = u'%s Server Error: %s for url: %s' % (self.status_code, reason, self.url)
      939│ 
      940│         if http_error_msg:
    → 941│             raise HTTPError(http_error_msg, response=self)
      942│ 
      943│     def close(self):
      944│         
      945│         called the underlying ``raw`` object must not be accessed again.

  • Installing six (1.15.0): Pending...
  • Installing six (1.15.0): Failed

  HTTPError

  404 Client Error: Not Found for url: https://ourserver.example.com/packages/packages/ee/ff/48bde5c0f013094d729fe4b0316ba2a24774b3ff1c52d924a8a4cb04078a/six-1.15.0-py2.py3-none-any.whl#sha256=8b74bedcbbbaca38ff6d7491d76f2b06b3592611af620f8426e82dddb04a5ced

  at ~/.local/pipx/venvs/poetry/lib/python3.8/site-packages/requests/models.py:941 in raise_for_status
      937│         elif 500 <= self.status_code < 600:
      938│             http_error_msg = u'%s Server Error: %s for url: %s' % (self.status_code, reason, self.url)
      939│ 
      940│         if http_error_msg:
    → 941│             raise HTTPError(http_error_msg, response=self)
      942│ 
      943│     def close(self):
      944│         
      945│         called the underlying ``raw`` object must not be accessed again.

Tracking through the code, it appears the code uses the initially requested URL, but doesn't realize the request was redirected. So when requesting click, for example, it requests the URL https://ourserver.example.com/pypiserver/click, but that got redirected to https://artifactory.example.com/long/url/thing. Further down the line, poetry tries to grab the resource using a relative URL and combining it with the first URL, when in fact it should be combining it with the second URL.

[abn]

@jpyams appreciate your detailed description. That is definitely okay in lieu of an issue. Although, would be great if you can look for any existing issues that might be fixed with this.

Couple of initial questions pop into mind.

1. Are there any security considerations here? Do we really want to have this behaviour by default? (ie. implicit redirect)
2. If there are should this somehow support an explicit option of "allow-redirects"?

In nexus deployments, the endpoint usually proxies mirrored packages instead of issueing a redirect. While this is a small change, it would be good to make sure that this beahviour is either allowed or is currently unspecified for legacy repsitories as per PyPA, warehouse or similar. Any references would be helpful.

As for tests, it would definitely be great to have a test that atleast verifies the redirected url is used. This is to ensure that any we do not miss this scenario if (most likelt when) we rewrite parts of this code.

A note regarding best practice considerations, would it be better to be more explicit about where your private packages come from and add artifactory mirror explicitly as a secondary or even primary source? I do not want to indicate that "this is the way to do it" as there are various reasons why your setup is required and might even be recommended. There are also conversations happening elsewhere to support PyPI mirrors as first class citizens in poetry, which might fit into this model.

[jpyams]

Thanks. I can't think of any security considerations that allowing redirects would introduce. pip has no problem here with installing packages through our system.

Sure, I'll add a test for this.

Regarding multiple sources (indexes/indices), that will not fit our use model. For security considerations, if there is a name conflict between one of our packages and something in PyPI, we want our package to win. pypiserver lets us do this with the redirects. pip with multiple indices treats all packages with the same name as identical, regardless of their source.

[jpyams]

@abn I added a test. Is there anything else I need to do before this pull request can be merged?

[jpyams]

Is there anything else I need to do on this pull request?

[jpyams]

@abn or somebody, can you please tell me what's holding up this PR or if there's something I need to do before it can be merged?

[sinoroc]

It's just a matter of patience. One of the maintainers needs to find the time to get to it.

[sinoroc]

@jpyams It would probably give the maintainers more confidence if you somehow managed to test this PR in a larger variety of environments (for example against other server softwares).

@qiuwei You apparently encountered a similar issue in #3293, are you able to test this PR in your own environment and see if it fixes things for your workflow?

[sinoroc]

As far as I can tell, this is the exact same fix (but different tests?) as #3133.

Maybe the authors of both PRs (@neilvana @jpyams) can agree on something here to make the life of the maintainers a bit easier.

[neilvana]

I'm okay with either PR being the one which gets merged, let's plan on getting this one from @jpyams ready. I'll comment on this one with one small piece of feedback which I do think should be updated if this does get merged. The test implementation in this PR is simpler and therefore might be better. The implementation in my PR took a more system level approach relying on requests ability to redirect and is probably overkill.

[jpyams]

@neilvana good observation on the logging message! I have updated the pull request.

Ditto with regard to being OK for either pull request. I just want to see this issue fixed!

[jpyams]

Made the change. One test is failing now, but it looks like something went wrong with the test runner at that moment in time. I don't know how to rerun that one test.

Anyway, all the required tests are passing.

[sinoroc]

Made the change. One test is failing now, but it looks like something went wrong with the test runner at that moment in time. I don't know how to rerun that one test.

Either there is a button in the UI to rerun the test (as author of the PR, you might be allowed to see it). Otherwise I guess maybe I would just amend the commit and force-push again.

[jpyams]

Thanks @sinoroc, that helped the tests pass!

How do we get this issue to the attention of approvers? @abn sorry for pinging you repeatedly, but I don't know how to get anyone else who could get this approved.

We're in the same boat as @neilvana: we want to use Poetry, but this issue is throwing a serious wrench in our ability to use it.

[abn]

@jpyams a few minor suggestions otherwise looks good to me.

[jpyams]

Thanks for the feedback, @abn! Changes are implemented.

[abn]

One more minor suggestion :)

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.