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

Serve correct openapi spec basepath when path is altered by reverse-proxy #823

Merged
merged 11 commits into from
Dec 17, 2019

Conversation

dtkav
Copy link
Collaborator

@dtkav dtkav commented Dec 16, 2018

Fixes #820
Fixes #392
Fixes #527

Changes proposed in this pull request:

  • change swagger-ui, and openapi spec location to build the path using url_for
  • modify the openapi spec basePath or servers block to include the reversed api path
  • modify the swagger-ui page to load the reversed openapi spec using url_for
  • add a openapi3/reverseproxy example
  • add a openapi3/reverseproxy_aiohttp example

Thanks to @Jyhess for the collaboration, tests, and review!

@bhechinger
Copy link

That fixes the Explore bar nicely so the spec is loaded into the UI Console.

That also sets the link to the spec file correctly.

The Try it out section still has the wrong URL, however.

@dtkav
Copy link
Collaborator Author

dtkav commented Dec 17, 2018

Ah ok - great thanks for the feedback. I need to set up a proper test environment :)
@Jyhess also has a similar PR at #826 if you want to try that out and provide feedback.

@dtkav dtkav mentioned this pull request Dec 17, 2018
@dtkav
Copy link
Collaborator Author

dtkav commented Dec 17, 2018

note: this doesn't yet work with aiohttp

@dtkav
Copy link
Collaborator Author

dtkav commented Jan 9, 2019

hey @bhechinger - can you try out the latest example, and let me know if it is working for you?

@dtkav
Copy link
Collaborator Author

dtkav commented Jan 9, 2019

The build is broken because of #844

@dtkav dtkav changed the title WIP: use url_for to reverse the route for openapi.json Support path-altering reverse proxies, dyamicly rewrite spec and swagger-ui (h/t @Jyhess) Jan 13, 2019
@dtkav
Copy link
Collaborator Author

dtkav commented Jan 13, 2019

I think I have this pretty much where I want it. It could be a bit more DRY.

Potential future work could be to actually add the reverse proxy code to connexion/apps/*.
This would need to be done carefully, since it's very easy to mess up and easy to cause security problems.

I think for now it's probably best to leave it as an example for the user to learn about and adapt to their use case.

@dtkav
Copy link
Collaborator Author

dtkav commented Jan 22, 2019

I'm stuck on a single aiohttp test in python 3.4, where it seems to think that aiohttp is installed after calling sys.modules.pop("aiohttp").
Does anyone know what's going on there? I'm not able to reproduce it locally.

@Jyhess
Copy link
Contributor

Jyhess commented Jan 22, 2019

I'm stuck on a single aiohttp test in python 3.4, where it seems to think that aiohttp is installed after calling sys.modules.pop("aiohttp").
Does anyone know what's going on there? I'm not able to reproduce it locally.

Not reproduced on my side either, but you can try another strategy with mocker:

def test_run_with_aiohttp_not_installed(mock_app_run, spec_file, mocker):
    import sys
    mocker.patch.dict(sys.modules, {'aiohttp': None})
    runner = CliRunner()

    # missing aiohttp
    result = runner.invoke(main,
                           ['run', spec_file, '-f', 'aiohttp'],
                           catch_exceptions=False)

    assert 'aiohttp library is not installed' in result.output
    assert result.exit_code == 1

@dtkav dtkav force-pushed the dynamic_ui_path branch 4 times, most recently from 988009f to 75438f2 Compare January 24, 2019 08:35
@dtkav
Copy link
Collaborator Author

dtkav commented Jan 24, 2019

@Jyhess thanks for your help. I was able to sort it out by using mock.

@dtkav
Copy link
Collaborator Author

dtkav commented Jan 24, 2019

@jmcs can you take a look at this one please?


.. code-block:: bash

$ sudo pip3 install --upgrade connexion[swagger-ui] # install Connexion from PyPI
Copy link
Contributor

Choose a reason for hiding this comment

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

Better to specify extra dependency:
sudo pip3 install --upgrade connexion[swagger-ui] aiohttp-remotes

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good catch! oops

@@ -1,4 +1,5 @@
aiohttp>=2.2.5,<3.5.2
aiohttp-remotes>=0.1.2
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it should not be part of project dependencies when it is used only in an example

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah, but it is needed for the unit tests. I'll add it to setup.py under test-requres for > python 3.5.2

@dtkav
Copy link
Collaborator Author

dtkav commented Nov 5, 2019

Hey @hjacobs if you find some time - these changes will enable people to use connexion behind a path-altering reverse proxy (like AWS API Gateway).

@dtkav
Copy link
Collaborator Author

dtkav commented Nov 18, 2019

@hjacobs can you please take a look at this fix for path-altering reverse proxies?

@dtkav
Copy link
Collaborator Author

dtkav commented Dec 3, 2019

@rafaelcaricio are you happy with the changes to address your comments?

Copy link
Contributor

@cognifloyd cognifloyd left a comment

Choose a reason for hiding this comment

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

This looks great and needs to be merged. 👍 I have a few minor nitpicks, but it can be merged without addressing them.

examples/openapi3/reverseproxy/app.py Outdated Show resolved Hide resolved
examples/openapi3/reverseproxy_aiohttp/app.py Outdated Show resolved Hide resolved
connexion/__init__.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
@dtkav dtkav changed the title Support path-altering reverse proxies, dyamicly rewrite spec and swagger-ui (h/t @Jyhess) Serve correct openapi spec basepath when path is altered by reverse-proxy Dec 17, 2019
@dtkav
Copy link
Collaborator Author

dtkav commented Dec 17, 2019

@hjacobs can you please have a look?

@hjacobs
Copy link
Contributor

hjacobs commented Dec 17, 2019

👍

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