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

Issue with multi stage APIG #194

Open
AntoineMontane opened this issue Nov 14, 2020 · 9 comments
Open

Issue with multi stage APIG #194

AntoineMontane opened this issue Nov 14, 2020 · 9 comments

Comments

@AntoineMontane
Copy link

AntoineMontane commented Nov 14, 2020

Hi !

Working with multi-stage and custom domain name in api gateway is hard on the path side of thing !

1/ First, i discovered that default mapping works only when there is one mapping .... otherwise all mapping requires an explicit key mapping set, even the default. Ok, fair enough : routing has to be done consistently.

But the next i discover, is on an another level.

2/ api gateway insists on mangling the path, prefixing it with the stage name.
/status become /{stage}/status

Not really good. Shall i really remap all my application routes to handle stages now ? Hooray for separation of concern ...
Still, could be logical, after all this is what the incoming request was made up.

3/ Right ? No. This mangling doesn't happen on $default stage. Incredible. We've just setup a key mapping for default because we had to, see /1, but now the key mapping is dropped of the path for default !

Now i need to handle 2 routes for each resource : one with prefix, one without. Clever ? Not at all.

In fact the prefix given as a key mapping has no impact on the path : it's always overwritten by the stage name or nothing for the default stage. So easy to debug, if you've not planned to use the stage name as the key mapping ...

4/ I'm already crying down the floor. But i won't stop trying. Forget key mapping, let's deploy each stage as default of it's own domain ! It might work ! Maybe ? No. Same sad result.

Really, don't leave me 30s in the same room as the guy who developed this API .... i will not refrain from anything.

Enough ranting, let's fix this. So let's add another middleware.
Or as we have standardized on apig-wsgi, could we add a de-bullshit-stage-path option to it ?

Not much needed on the dev side : just drop /{stage} from the beginning of the path if stage is not $default and drop_stage_prefix parameter is set.

@adamchainz, would you take a patch for this ?

CU
Antoine

@AntoineMontane
Copy link
Author

@jefforulez
Copy link

jefforulez commented Dec 30, 2020

i second this request.

i believe the fix is to set PATH_INFO to be a template merge of event['resource'] and event['pathParameters']['proxy'].

for example, with this event:

{
  "path" : "/stage/path/to/resource",
  "resource" : "/{proxy+}",
  "pathParameters":{
    "proxy" : "path/to/resource"
   },
  ...
}

PATH_INFO should "/path/to/resource", where "path/to/resource" replaces "{proxy+}".

if this makes sense, i'm happy to submit a PR.

@jefforulez
Copy link

jefforulez commented Dec 30, 2020

fyi, i just added this wrapper around the generated handler in my code and it seems to work well:

from apig_wsgi import make_lambda_handler
apig_wsgi_handler = make_lambda_handler(app)

def lambda_handler( event, context ):
    resource = event.get( 'resource' )
    proxy = event.get( 'pathParameters', {} ).get( 'proxy' )

    if proxy != None and re.search( '{proxy\+}', resource ):
        event['path'] = re.sub( '{proxy\+}', proxy, resource )

    return apig_wsgi_handler( event, context )

@jefforulez
Copy link

jefforulez commented Dec 30, 2020

@AntoineMonnet also, another nice solution that might work for you using flask's DispatcherMiddleware:

import os

# set this in the lambda environment
app_prefix = os.getenv( 'APP_PREFIX', '/foo' )

from myapp import app as myapp

from flask import Flask
from werkzeug.middleware.dispatcher import DispatcherMiddleware

app = Flask( __name__ )

app.wsgi_app = DispatcherMiddleware(
  myapp,
  { app_prefix: myapp.wsgi_app }
)

from apig_wsgi import make_lambda_handler
lambda_handler = make_lambda_handler(app)

@squeaky-pl
Copy link

squeaky-pl commented Feb 9, 2021

I was struggling with Flask's url_for not generating urls with /{stage-name} prefix and I solved it like this:

from apig_wsgi import make_lambda_handler
from flask import Flask


app = Flask(__name__)


def set_stage_prefix_middleware(app):
    def inner(environ, start_response):
        environ["SCRIPT_NAME"] = f"/{environ['apig_wsgi.request_context']['stage']}"

        return app(environ, start_response)

    return inner


app.wsgi_app = set_stage_prefix_middleware(app.wsgi_app)

handler = make_lambda_handler(app)

Note I only deploy on Lambda and locally I will be using Flask's builtin server.

@AntoineMontane
Copy link
Author

AntoineMontane commented Feb 18, 2021

Hi there,
Thank's @squeaky-pl, @jefforulez
i've settled on this middleware, which strips stage from PATH_INFO and add it to SCRIPT_NAME if PATH_INFO starts with the stage name.

def strip_stage_prefix_middleware(app):
    def inner(environ, start_response):
        prefix = f"/{environ['apig_wsgi.request_context']['stage']}"
        if environ['PATH_INFO'].startswith(prefix):
            environ['SCRIPT_NAME'] = prefix
            environ['PATH_INFO'] = environ['PATH_INFO'][len(prefix):]
        return app(environ, start_response)
    return inner

What do you think of this ?

@adamchainz
Copy link
Owner

I'd be happy to look at a PR with such changes to the environ. It would be nice to have the example app modified to reproduce the issue as well, alongside the fix.

@AntoineMontane
Copy link
Author

Hi @adamchainz
I've written the above PR.
As a conservative measure, i've left the default parameter value to disabled, as there could be an edge case where the path could begin by the stage name just by naming collision.

@scottwilkin
Copy link

I'd be happy to look at a PR with such changes to the environ. It would be nice to have the example app modified to reproduce the issue as well, alongside the fix.

Has there been any further consideration to the above PR? This is still an issue, especially with version 2.0 of the request payload.

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

No branches or pull requests

5 participants