Make invocation URL dynamic and fix default function name #46

skwashd · 2021-09-04T06:20:31Z

Issue #, if available:

Hardcoded function name #43

Description of changes:

The API endpoint in Amazon's implementation of Lambda used the path /2015-03-31/functions/[func]/invocations to invoke the function. [func] the name of the function and is set as the value of the AWS_LAMBDA_FUNCTION_NAME environment variable when the function is executing on AWS.

The emulator uses a hard coded endpoint of /2015-03-31/functions/function/invocations. This is even the case
when the AWS_LAMBDA_FUNCTION_NAME environment variable is set to another value.

This patch changes the endpoint URL when the AWS_LAMBDA_FUNCTION_NAME environment variable is set. In this case the invocation URL will be /2015-03-31/functions/${AWS_LAMBDA_FUNCTION_NAME}/invocations. When the environment variable isn't set, the current behaviour persists and the function name is set to function.

The end result is the emulator behaviour is closer to the real environment the functions execute in.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

As noted in aws#46, the function name is derived from the URL used to invoke it. The emulator uses `function` in the API endpoint, but the `AWS_LAMBDA_FUNCTION_NAME` environment variable is set to `test_function`. This is an inconsistency between the emulator and the AWS environment.

Merge changes for v1.2 release

valerena

This change looks good, but there are conflicts in the test cases (we renamed the file and added testing capabilities for multiple architectures). If you bring those changes and fix the tests, we should be good to go.

skwashd · 2021-10-04T16:52:32Z

@valerena thanks. Fixed, but before merging this you should review my comment on #49 (comment).

As noted in aws#46, the function name is derived from the URL used to invoke it. The emulator uses `function` in the API endpoint, but the `AWS_LAMBDA_FUNCTION_NAME` environment variable is set to `test_function`. This is an inconsistency between the emulator and the AWS environment.

skwashd · 2021-11-02T03:45:12Z

@valerena bumping the thread as suggested in #49 (comment)

valerena · 2021-11-03T15:43:06Z

Hi @skwashd , we had a discussion with the team and there are concerns about backwards compatibility and breaking existing customers that had automated local calls for testing using RIE. The consensus is that this behavior could be optional, having to pass an extra parameter to have it like this.

To have this as the default behavior might come in the future, but it's something that we still have to confirm with more stakeholders and if it happens it could take several months, to make sure that we have all use cases covered and the right communication about that change.

I know that this has already taken more time than what I imagined you originally expected, but if you change this Pull Request to work as an optional behavior I would work to prioritize its revision and include it in a shorter amount of time that what has taken to review the rest of the changes so far.
Let me know if this sounds okay for you.

skwashd · 2021-11-03T16:05:28Z

@valerena it's always "fun" trying to fix broken behaviour when people rely on the (broken) behaviour 😢

Before I spend more time on this, I'd like to make sure we're aligned on the details.

Is the proposal to keep the default function name fix as implemented in ebc933d as is? I hope this is a less controversial change as the default function name should match the path in the url. I hope many users aren't depending on the AWS_LAMBDA_FUNCTION_NAME environment variable being set to test_function.

For the invoke URL change, I propose that if AWS_LAMBDA_FUNCTION_NAME is set, we have the server listen on both the hardcoded function path and the one generated from the environment variable. It will use the same handler function for both. If the variable isn't set, we simply listen on the function path.

This approach keeps backwards compatibility and makes it easier to remove the broken behaviour down the road. I will ensure I minimise any duplicated logic. Does this work for you and the team? I'd like to see the old hardcoded function path url when the variable is set marked as deprecated in the docs, or is that too soon?

valerena · 2021-11-03T20:59:07Z

@skwashd The team's stance on this is that RIE wasn't launched to be an exact copy of Lambda's Control Plane, so the current behavior is not "broken", and it's just "the way that RIE works". So, we shouldn't make any changes to the "default behavior", even if we think not many people use that. Every change that could break existing customers should be activated through a specific flag passed, and we won't change the recommended path url for now, even if the other url is also available.

It's possible than during next year we will have a longer discussion about the future here and potentially add your recommendations as the default behavior, but at this moment we don't have the resources to start that conversation.

Merge changes for v1.3 release

As noted in aws#46, the function name is derived from the URL used to invoke it. The emulator uses `function` in the API endpoint, but the `AWS_LAMBDA_FUNCTION_NAME` environment variable is set to `test_function`. This is an inconsistency between the emulator and the AWS environment.

skwashd · 2021-12-28T17:18:13Z

@valerena lets agree to disagree on inconsistent behaviour is a bug or a feature. I've incorporated the discussions we've discussed here. All the existing tests pass and I added tests to exercise the new functionality.

Use alpine as a base image, and install clamav properly so that it's just available on PATH. This led to a few changes to how we call `clamscan` and `freshclam`. Without lambci, we now need `aws-lambda-rie` to provide the Lambda runtime environment (i.e. the HTTP API). Because this is just needed for local dev, mount it rather than installing on the image directly. `aws-lambda-rie` causes two restrictions: - The function name must be "function". There's [a PR to make this configurable](aws/aws-lambda-runtime-interface-emulator#46), but AWS don't seem keen - It can only handle one invokation at a time, so we can't upload the second test file (and therefore invoke the Lamdba) until the first is finished #minor

skwashd · 2022-03-29T06:28:53Z

@valerena any update on this one?

swantzter · 2022-05-06T10:43:32Z

Hi, @valerena any updates on this? We're running into issues due to not being able to change the function name when trying to invoke the function from AWS step functions local this is making it hard for us to develop and run test locally

valerena · 2022-05-24T15:38:17Z

Hi. I'm really sorry this is taking so long. Our team is very very short on resources, and at the moment we only have this package in maintenance mode, only upgrading the version of Go because of security related findings.

The change overall looks good, but we don't have the bandwidth to review it thoroughly, merge it, release it, and monitor in the future if this causes any unintended consequences that would cause us to have to revert it back.

I can't promise any dates, but I can tell that we won't be able to take this during June either, but we'll see during July how are things by then and potentially have bandwidth to take a deeper look.

I'm sorry this is affecting your flows. Maybe you can compile the binary with these changes and use it manually instead of using the official version for now as a workaround (if that works for your use case), and I hope we can get enough resourcing in July to dedicate more time to this repository.

Merge changes for v1.8 release

Merge changes for v1.9 release

swantzter · 2022-11-09T18:38:48Z

Hi @valerena, just wanted to check in on this and see if there's any potential for progress, but given the commit history I assume the situation for your team hasn't gotten to a better state yet?

skwashd · 2022-12-31T14:38:22Z

@valerena Can I please get an update on this?

cppntn · 2023-03-02T14:36:02Z

Can you guys finally merge this please?

timini · 2023-03-23T08:54:47Z

hit the button, merge it!

Merge changes for v1.11 release

Merge changes for v1.15 release

Merge changes for v1.17 release

- Update to Go 1.22 - Allow user-defined client context - Refactor test cases - Add workflow for automated releases

calh · 2024-06-05T16:29:53Z

I'm getting into developing step functions, developing and testing locally along with executing local lambda functions using this. This feature really should have been in the RIE from the first release. I don't see how a hard coded function name is usable when doing test driven development.

Please merge this @valerena! I really want to give AWS my money, but my wallet is stuck because it only responds to the name Wallet, not function.

Merge the check for vulnerabilities

skwashd · 2024-06-06T05:02:45Z

Given the recent activity in the repo is appears the team has resources again. @valerena can I please get an update? It's been over 2 years since you last responded.

calh · 2024-06-06T11:15:36Z

Hey @skwashd, would you be willing to pull AWS's commits into your dynamic-url branch and publish a release of your code? You're the captain now!

skwashd · 2024-07-06T16:07:38Z

@calh I'm not going to expend any more energy on this until there is a clear commitment from AWS to accept it. Given the radio silence from @valerena I don't think it is going to be accepted.

valerena · 2024-07-08T21:10:52Z

Hi everyone. I apologize for not replying earlier and for the silence for so long. The last few months we've been able to work on this again, mostly on security-related improvements, but I'd say we are in a position today where we can actually review and try to bring this in.

I think last time I looked at this, I got stuck trying to suggest a better naming than AWS_LAMBDA_RIE_INCONSISTENT_BEHAVIOUR, because it's confusing and it's unclear what the inconsistency is. Also, I think a positive env var is more appropriate instead of having to set something to "false". I suggest changing it to AWS_LAMBDA_RIE_CONSISTENT_FUNCTION_NAME, that has to be set to TRUE (or to any value, really)

There are currently some conflicts (the tests have changed a lot), so if you can update the PR with those changes and the variable name, I can take a look and see how we can include this in an upcoming release (better late than never?).

Do you think you can do that, @skwashd ? And apologies again.

As noted in aws#46, the function name is derived from the URL used to invoke it. The emulator uses `function` in the API endpoint, but the `AWS_LAMBDA_FUNCTION_NAME` environment variable is set to `test_function`. This is an inconsistency between the emulator and the AWS environment.

skwashd · 2024-07-09T17:03:58Z

I went with AWS_LAMBDA_RIE_DYNAMIC_FUNCTION_URL as it is more descriptive and is a positive name. Calling it AWS_LAMBDA_RIE_CONSISTENT_FUNCTION_NAME is less clear. I look forward to it being merged soon.

I'd love to see this fixed properly in v2. It shouldn't require two environment variables to make RIE handle URLs the same way as Lambda.

As noted in aws#46, the function name is derived from the URL used to invoke it. The emulator uses `function` in the API endpoint, but the `AWS_LAMBDA_FUNCTION_NAME` environment variable is set to `test_function`. This is an inconsistency between the emulator and the AWS environment.

The API endpoint in Amazon's implementation of Lambda used the path `/2015-03-31/functions/[func]/invocations` to invoke the function. `[func]` the name of the function and is set as the value of the `AWS_LAMBDA_FUNCTION_NAME` environment variable when the function is executing on AWS. The emulator uses a hard coded endpoint of `/2015-03-31/functions/function/invocations`. This is even the case when the `AWS_LAMBDA_FUNCTION_NAME` environment variable is set to another value. This patch changes the endpoint URL when the `AWS_LAMBDA_FUNCTION_NAME` environment variable is set. In this case the invocation URL will be `/2015-03-31/functions/${AWS_LAMBDA_FUNCTION_NAME}/invocations`. When the environment variable isn't set, the current behaviour persists and the function name is set to `function`. The end result is the emulator behaviour is closer to the real environment the functions execute in. This PR fixes aws#43 I raised a few weeks ago.

valerena · 2024-07-10T23:32:45Z

test/integration/testdata/main.py

        raise("Function name was not overwritten")
    else:
        return "My lambda ran succesfully"

 def assert_lambda_arn_in_context(event, context):
-    if context.invoked_function_arn == f"arn:aws:lambda:us-east-1:012345678912:function:{os.environ.get('AWS_LAMBDA_FUNCTION_NAME', 'test_function')}":
+    if context.invoked_function_arn == f"arn:aws:lambda:us-east-1:012345678912:function:{os.environ.get('AWS_LAMBDA_FUNCTION_NAME', 'function')}":


I think I found an issue with our tests thanks to this change.
Because the integration tests are passing but here they should fail.

Your new test works correctly with this code, but there was already a previous test test_lambda_function_arn_exists() -> arnexists-x86_64 that doesn't pass the new env var, so this ARN should have test_function instead (the old value).

Looking at the output of the test run arnexists-x86_64 doesn't appear on the list of tests that were run, so it looks it might be ignoring the failing tests instead of fail overall.

I'll have to figure out this issue, but in the meantime you should change this to have different cases depending if the new env var was passed or not (probably just create a new handler here to be called form the new integ test).

The previous handler in this file also has the same issue, but because it's the branch that shouldn't happen, then it's not relevant and the test still succeed, but you should change that too.

Everything else looks good

Digging a little deeper, it looks like this shouldn't really be a problem, because when running this we will always have a value for AWS_LAMBDA_FUNCTION_NAME, so in this code it will never go to the second parameter of the os.environ.get(),

It's still weird that the previous test that uses this code doesn't show up on the output for the tests, so I'm still investigating what's going on there.

Did you make this change because the tests failed if you didn't? Or did you change it before actually running the tests? (My point is that: either this change is making the old test fail, or this change shouldn't be needed).

Of course it had to be a simple thing. The old test is not running because the new test you added has the same name! (def test_lambda_function_arn_exists) so it's actually overwriting the old test, but it succeeds.

What I would say is: don't change this file. This change doesn't actually affect the test because the env var is always set. We could probably just remove that second parameter in the test, but for now, it's better if you don't modify the file, so it doesn't get mixed up as a needed change.

valerena · 2024-07-11T00:08:27Z

test/integration/testdata/main.py

        raise("Function name was not overwritten")
    else:
        return "My lambda ran succesfully"

 def assert_lambda_arn_in_context(event, context):
-    if context.invoked_function_arn == f"arn:aws:lambda:us-east-1:012345678912:function:{os.environ.get('AWS_LAMBDA_FUNCTION_NAME', 'test_function')}":
+    if context.invoked_function_arn == f"arn:aws:lambda:us-east-1:012345678912:function:{os.environ.get('AWS_LAMBDA_FUNCTION_NAME', 'function')}":


Of course it had to be a simple thing. The old test is not running because the new test you added has the same name! (def test_lambda_function_arn_exists) so it's actually overwriting the old test, but it succeeds.

What I would say is: don't change this file. This change doesn't actually affect the test because the env var is always set. We could probably just remove that second parameter in the test, but for now, it's better if you don't modify the file, so it doesn't get mixed up as a needed change.

valerena · 2024-07-11T00:09:23Z

test/integration/local_lambda/test_end_to_end.py

+            self.assertEqual(b'"My lambda ran succesfully"', r.content)
+
+
+    def test_lambda_function_arn_exists(self):


This one should be test_lambda_function_arn_exists_consistent

skwashd · 2024-07-18T04:24:34Z

I'll try to look at this again in the coming days

retrry · 2025-02-08T08:59:00Z

What is the status of this? This would make local development easier in some cases, where you use separate orchestration engine for starting lambdas. Now I have to override function name there when developing.

skwashd mentioned this pull request Sep 4, 2021

Fix default function name #49

Closed

Merge pull request aws#53 from aws/develop

b57d8f4

Merge changes for v1.2 release

valerena requested changes Oct 4, 2021

View reviewed changes

skwashd force-pushed the dynamic-url branch from 434d1a6 to 8f229fd Compare October 4, 2021 16:48

skwashd force-pushed the dynamic-url branch from 8f229fd to 8b307b7 Compare October 5, 2021 04:00

valerena changed the title ~~Make invocation URL dynamic~~ Make invocation URL dynamic and fix default function name Oct 19, 2021

valerena self-requested a review October 19, 2021 16:04

v1.3 release

49c35f6

Merge changes for v1.3 release

skwashd force-pushed the dynamic-url branch from 8b307b7 to a451e7e Compare December 28, 2021 17:16

gregtyler mentioned this pull request Mar 3, 2022

Move to smaller base image ministryofjustice/opg-s3-antivirus#20

Merged

valerena added 2 commits September 22, 2022 13:34

Merge changes for v1.8 release

43a7a09

Merge changes for v1.8 release

Merge changes for v1.9 release

5a79e1e

Merge changes for v1.9 release

valerena and others added 5 commits May 3, 2023 10:02

Merge changes for v1.11 release

9de974f

Merge changes for v1.11 release

Merge changes for v1.12 release

23f8171

Merge changes for v1.15 release

c6779f7

Merge changes for v1.15 release

Merge changes for v1.17 release

25a2eac

Merge changes for v1.17 release

Merge changes for v1.19 release

c99378b

- Update to Go 1.22 - Allow user-defined client context - Refactor test cases - Add workflow for automated releases

Merge the check for vulnerabilities

394ab66

Merge the check for vulnerabilities

skwashd force-pushed the dynamic-url branch from a451e7e to 961449d Compare July 9, 2024 17:00

skwashd added 3 commits July 10, 2024 03:04

Fix tests

cb1fbb7

skwashd force-pushed the dynamic-url branch from 961449d to cb1fbb7 Compare July 9, 2024 17:04

skwashd temporarily deployed to integ-tests July 9, 2024 19:11 — with GitHub Actions Inactive

skwashd temporarily deployed to integ-tests July 9, 2024 19:12 — with GitHub Actions Inactive

valerena requested changes Jul 10, 2024

View reviewed changes

valerena requested changes Jul 11, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make invocation URL dynamic and fix default function name #46

Make invocation URL dynamic and fix default function name #46

skwashd commented Sep 4, 2021

valerena left a comment

skwashd commented Oct 4, 2021

skwashd commented Nov 2, 2021

valerena commented Nov 3, 2021

skwashd commented Nov 3, 2021 •

edited

Loading

valerena commented Nov 3, 2021

skwashd commented Dec 28, 2021

skwashd commented Mar 29, 2022

swantzter commented May 6, 2022

valerena commented May 24, 2022

swantzter commented Nov 9, 2022

skwashd commented Dec 31, 2022

cppntn commented Mar 2, 2023

timini commented Mar 23, 2023

calh commented Jun 5, 2024

skwashd commented Jun 6, 2024

calh commented Jun 6, 2024

skwashd commented Jul 6, 2024

valerena commented Jul 8, 2024

skwashd commented Jul 9, 2024

valerena Jul 10, 2024

valerena Jul 10, 2024

valerena Jul 11, 2024

valerena Jul 11, 2024

valerena Jul 11, 2024

skwashd commented Jul 18, 2024

retrry commented Feb 8, 2025

		self.assertEqual(b'"My lambda ran succesfully"', r.content)


		def test_lambda_function_arn_exists(self):

Make invocation URL dynamic and fix default function name #46

Are you sure you want to change the base?

Make invocation URL dynamic and fix default function name #46

Conversation

skwashd commented Sep 4, 2021

valerena left a comment

Choose a reason for hiding this comment

skwashd commented Oct 4, 2021

skwashd commented Nov 2, 2021

valerena commented Nov 3, 2021

skwashd commented Nov 3, 2021 • edited Loading

valerena commented Nov 3, 2021

skwashd commented Dec 28, 2021

skwashd commented Mar 29, 2022

swantzter commented May 6, 2022

valerena commented May 24, 2022

swantzter commented Nov 9, 2022

skwashd commented Dec 31, 2022

cppntn commented Mar 2, 2023

timini commented Mar 23, 2023

calh commented Jun 5, 2024

skwashd commented Jun 6, 2024

calh commented Jun 6, 2024

skwashd commented Jul 6, 2024

valerena commented Jul 8, 2024

skwashd commented Jul 9, 2024

valerena Jul 10, 2024

Choose a reason for hiding this comment

valerena Jul 10, 2024

Choose a reason for hiding this comment

valerena Jul 11, 2024

Choose a reason for hiding this comment

valerena Jul 11, 2024

Choose a reason for hiding this comment

valerena Jul 11, 2024

Choose a reason for hiding this comment

skwashd commented Jul 18, 2024

retrry commented Feb 8, 2025

skwashd commented Nov 3, 2021 •

edited

Loading