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 function to invoke a lambda using an invocation type of "DryRun" #817

Merged
merged 6 commits into from
Apr 6, 2021

Conversation

kbalk
Copy link
Contributor

@kbalk kbalk commented Mar 23, 2021

Provides the functionality to invoke a Lambda with an InvocationType of "DryRun". This functionality provides a simple verification of a Lambda's installation.

This functionality was implemented through the new function InvokeDryRunE(), which returns a status code and an error. I debated about the return values, but a status code and error seemed consistent with the return values from the AWS SDK. When the the AWS SDK Invoke() is called with a invocation type of DryRun, StatusCode is the only field in the returned InvokeOutput struct that has a value, even when there is an error. A StatusCode value of 204 indicates the DryRun was successful and further details are then found in the returned error:

// The HTTP status code is in the 200 range for a successful request. For the
// RequestResponse invocation type, this status code is 200. For the Event invocation
// type, this status code is 202. For the DryRun invocation type, the status
// code is 204.
StatusCode *int64 location:"statusCode" type:"integer"

I also debated whether there was a value to creating a InvokeDryRun() (no "E" at the end) version of the function to simply return the status code and no error. I assumed if the dry run failed, the error was needed, so InvokeDryRun() wouldn't have much utility.

And I debated adding a test of an error condition. The dry run can determine whether the user or role has permission to execute the Lambda, which would be more involved to set up. I could specify a bad function name, but that's not really the point of a dry run invocation.

This pull request is a bit of a trial balloon to determine just what you'd like to see for this added functionality.

Issue: #802
Test results: https://gist.github.com/kbalk/fbb68f156d391eacf4d330b7870e1401

Copy link
Member

@brikis98 brikis98 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I'm a little worried about a sprawling collection of InvokeXXX functions for every permutation: e.g., one for dry run, one not dry run, one async, one not, etc. Perhaps the new thing to add is an InvokeFunctionWithParams function that lets you pass in the settings, such as InvocationType, in a struct?

@kbalk
Copy link
Contributor Author

kbalk commented Mar 23, 2021

Thanks for the PR! I'm a little worried about a sprawling collection of InvokeXXX functions for every permutation: e.g., one for dry run, one not dry run, one async, one not, etc. Perhaps the new thing to add is an InvokeFunctionWithParams function that lets you pass in the settings, such as InvocationType, in a struct?

I would have taken that approach, but your comment to my issue said: "PR to add this, perhaps in a new function, is welcome!" The "new function" suggested you didn't want what you are now suggesting. I can do it the other way ... I just want to make sure which way you truly want it done.

@kbalk
Copy link
Contributor Author

kbalk commented Mar 23, 2021

I should also point out that there are only three invocation types. Adding InvokeDryRun() means there is no impact on the current InvokeFunction(), i.e., no optional InvocationType struct parameter. It also means you have two of the three types defined.

@brikis98
Copy link
Member

Thanks for the PR! I'm a little worried about a sprawling collection of InvokeXXX functions for every permutation: e.g., one for dry run, one not dry run, one async, one not, etc. Perhaps the new thing to add is an InvokeFunctionWithParams function that lets you pass in the settings, such as InvocationType, in a struct?

I would have taken that approach, but your comment to my issue said: "PR to add this, perhaps in a new function, is welcome!" The "new function" suggested you didn't want what you are now suggesting. I can do it the other way ... I just want to make sure which way you truly want it done.

InvokeFunctionWithParams is a new function.

@kbalk
Copy link
Contributor Author

kbalk commented Mar 24, 2021

My apologies for the misunderstanding. I am confused so I want to make sure we're on the same page before I modify any more code. And more apologies if I repeat what you already know ...

You're suggesting a new function InvokeFunctionWithParams that includes a parameter for the InvocationType. The AWS SDK for Golang defines InvocationType enum values as shown below and the SDK's InvokeInput struct contains a field for the InvocationType. In Terratest, InvokeFunction initializes an InvokeInput struct, but the InvocationType field is not initialized and the default value of "RequestResponse" is assumed.

const (
    // InvocationTypeEvent is a InvocationType enum value
    InvocationTypeEvent = "Event"

    // InvocationTypeRequestResponse is a InvocationType enum value
    InvocationTypeRequestResponse = "RequestResponse"

    // InvocationTypeDryRun is a InvocationType enum value
    InvocationTypeDryRun = "DryRun"
)

I initially assumed the enhancement request would involve a boolean argument to Terratest's InvokeFunction that would indicate whether or not a "DryRun" invocation was requested. Silly me -- I've been working with Python where it'd be easy to add an optional argument without affecting existing callers of the API.

So to be clear, when you suggest the creation of an InvokeFunctionWithParams function, are you suggesting a duplicate of InvokeFunction with an argument that can be one of the InvocationType enum values? InvokeFunction uses the default InvocationType of "RequestResponse" and I can add the support for "DryRun", but are you suggesting I also add support for "Event"? If so, I would prefer that the addition of the "Event" logic be handled in a different PR as it's not something that I'm volunteering to do.

Regarding your concern about multiple flavors of InvokeFunction, I'd like to point out that the AWS SDK for Golang has 6 types of invocation functions: Invoke, InvokeAsync, InvokeAsyncRequest, InvokeAsyncWithContext, InvokeRequest and InvokeWithContext.

Also, I see the "DryRun" invocation as a pass/fail sort of function. It doesn't actually invoke the function; it just tests that it has permissions to do so. So, it kind of does it's own thing. When support for an an "Event" invocation is handled, that's a bit more complicated and you might end up splitting that out as well. Just my two cents.

@brikis98
Copy link
Member

I initially assumed the enhancement request would involve a boolean argument to Terratest's InvokeFunction that would indicate whether or not a "DryRun" invocation was requested. Silly me -- I've been working with Python where it'd be easy to add an optional argument without affecting existing callers of the API.

Yup, this is an annoying limitation of Go.

So to be clear, when you suggest the creation of an InvokeFunctionWithParams function, are you suggesting a duplicate of InvokeFunction with an argument that can be one of the InvocationType enum values?

Not quite. It's a new function that accepts a new struct called something like LambdaOptions. The struct allows us to support various other params we may want to pass in while invoking the Lambda function. One of those can be InvocationType, as you're asking for here, but having the struct allows us to add support for other options in the future. Under the hood, the original InvokeFunction would then be updated to call InvokeFunctionWithParams, passing the proper options struct.

@kbalk
Copy link
Contributor Author

kbalk commented Mar 26, 2021

I've added the function you requested and updated the gist with the test results. When handling different invocation types, you have to be prepared for different types of output, so I needed a struct for the output (e.g., "DryRun" has no payload and the status is in the returned structure). Here's the signature for the new function:

// InvokeFunctionWithParams invokes a lambda function using parameters
// supplied in the LambdaOptions struct and returns values in a LambdaOutput
// struct.
func InvokeFunctionWithParams(t testing.TestingT, region string, input *LambdaOptions) (*LambdaOutput, error) {

LambdaOptions and LambdaOutput are subsets of lambda.InvokeInput and lambda.InvokeOutput, respectively.

Note: this actually is an "E" function; I didn't create a non-E and E function, but I'm not sure it makes sense to have a non-E version.

modules/aws/lambda.go Outdated Show resolved Hide resolved
modules/aws/lambda.go Outdated Show resolved Hide resolved
modules/aws/lambda.go Outdated Show resolved Hide resolved
modules/aws/lambda.go Outdated Show resolved Hide resolved
modules/aws/lambda.go Outdated Show resolved Hide resolved
@kbalk kbalk changed the title Add function that invokes a lambda with an invocation type of "DryRun" Add function to invoke a lambda using an invocation type of "DryRun" Mar 30, 2021
kbalk added 5 commits April 1, 2021 11:33
Provides a more general solution for other types of Lambda
invocations.
- Restore InvokeFunction so it once again calls InvokeFunctionE
  version InvokeFunctionWithParams
- Use functionName as an argument and not a field of the
  LambdaOptions struct.
Restore original InvokeFunctionE().
Copy link
Member

@brikis98 brikis98 left a comment

Choose a reason for hiding this comment

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

Thanks! Just a couple tweaks and then I can run tests.

test/terraform_aws_lambda_example_test.go Outdated Show resolved Hide resolved
test/terraform_aws_lambda_example_test.go Outdated Show resolved Hide resolved
Copy link
Member

@brikis98 brikis98 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! I'll kick off tests now.

@brikis98
Copy link
Member

brikis98 commented Apr 6, 2021

Tests passed. Merging now.

@brikis98 brikis98 merged commit a87d10f into gruntwork-io:master Apr 6, 2021
@brikis98
Copy link
Member

brikis98 commented Apr 6, 2021

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

Successfully merging this pull request may close these issues.

2 participants