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

Add filter for cognito identity fields #6005

Merged
merged 4 commits into from May 5, 2022
Merged

Conversation

dfangl
Copy link
Member

@dfangl dfangl commented May 3, 2022

This PR adds a hacky filter, to the cognito_identity field of the lambda context. This will only allow correct values to be passed into it.

Once a real fix is available (by rethinking and redoing the localstack.services.apigateway.apigateway_listener.invoke_rest_api_integration_backend and localstack.services.apigateway.helpers.get_event_request_context methods @whummer @calvernaz ) so that this part:

        "identity": {
            "accountId": account_id,
            "sourceIp": source_ip,
            "userAgent": headers.get("User-Agent"),
        },

does not show up in the cognito_identity part of the lambda context, this PR should be reverted.

Until then, it will only allow "cognitoIdentityId" and "cognitoIdentityPoolId" as directory keys for cognito_identity.

This should help fixing #5783 .

Test for apigateway - rust lambda integration added to prevent further regressions.

@dfangl dfangl temporarily deployed to localstack-ext-tests May 3, 2022 18:05 Inactive
@dfangl dfangl requested review from whummer and calvernaz May 3, 2022 18:05
@@ -108,6 +108,9 @@
# CWD folder of handler code in Lambda containers
DOCKER_TASK_FOLDER = "/var/task"

# TODO remove once clarification of apigateway contexts is complete. Really bad hack!!
Copy link
Member

Choose a reason for hiding this comment

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

It's a good bad hack in the meantime though 😄

Copy link
Member

@dominikschubert dominikschubert left a comment

Choose a reason for hiding this comment

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

Will leave the approval to the others, but from my side I don't see an issue with having it in there as a temporary fix 👍

@github-actions
Copy link

github-actions bot commented May 3, 2022

LocalStack integration with Pro

       3 files  ±  0         3 suites  ±0   1h 1m 58s ⏱️ + 1m 15s
1 004 tests +  1     963 ✔️ +  1  41 💤 ±0  0 ±0 
1 298 runs  +71  1 228 ✔️ +70  70 💤 +1  0 ±0 

Results for commit 2b4e9c2. ± Comparison against base commit 0b038d1.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@calvernaz calvernaz left a comment

Choose a reason for hiding this comment

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

I think it's an ok fix, the request context that is being to the event is being wrongly passed to the context of the lambda. I'll create an issue to complete the fix, by correctly passing the necessary information to build the content object.

Copy link
Member

@whummer whummer left a comment

Choose a reason for hiding this comment

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

Great catch and nice fix @dfangl ! 👍

localstack/services/awslambda/lambda_executors.py Outdated Show resolved Hide resolved
@dfangl dfangl force-pushed the fix-cognito-identity-header branch from 5dec774 to a1daa62 Compare May 4, 2022 12:59
@dfangl dfangl temporarily deployed to localstack-ext-tests May 4, 2022 12:59 Inactive
@dfangl dfangl temporarily deployed to localstack-ext-tests May 4, 2022 13:06 Inactive
@dfangl dfangl temporarily deployed to localstack-ext-tests May 4, 2022 13:13 Inactive
@dfangl
Copy link
Member Author

dfangl commented May 4, 2022

Due to a lot of lambda integrations tested in there, I added test_apigateway.py to the tests executed for different lambda executors (new test is also in there). At some point we have to find a solution for this which scales better (pytest marks, decide on used fixtures etc).

Copy link
Member

@thrau thrau left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Copy link
Member

@alexrashed alexrashed left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@dfangl dfangl force-pushed the fix-cognito-identity-header branch from 9fc7702 to 2b4e9c2 Compare May 5, 2022 09:45
@dfangl dfangl temporarily deployed to localstack-ext-tests May 5, 2022 09:45 Inactive
@dfangl dfangl merged commit 33bb976 into master May 5, 2022
@dfangl dfangl deleted the fix-cognito-identity-header branch May 5, 2022 12:22
@github-actions github-actions bot locked and limited conversation to collaborators May 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants