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

(stepfunctions-tasks): support bring your own role for Lambda created by EvaluteExpression #29350

Open
2 tasks done
lagroujl opened this issue Mar 4, 2024 · 5 comments
Open
2 tasks done
Labels
@aws-cdk/aws-stepfunctions-tasks effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2

Comments

@lagroujl
Copy link

lagroujl commented Mar 4, 2024

Describe the feature

EvaluateExpression creates a singleton Lambda function. For customers that requiring strict monitoring of the IAM roles in their AWS accounts, the auto-generated roles may not fit their requirements. This can be alleviated by providing the ability to bring their own role

Use Case

Customers with strict controls over IAM roles that need more precise control of the IAM roles created in their AWS accounts

Proposed Solution

  • Add a property Role to EvaluateExpressionProps that takes an IRole to be used by the SingletonFunction
  • Add the role to the uuid of the SingletonFunction, so that using a role or different roles creates a new function for each role

Other Information

No response

Acknowledgements

  • I may be able to implement this feature request
  • This feature might incur a breaking change

CDK version used

2.130.0

Environment details (OS name and version, etc.)

n/a

@lagroujl lagroujl added feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels Mar 4, 2024
@lagroujl
Copy link
Author

lagroujl commented Mar 4, 2024

I had already created a pull request for this. But I jumped in when I thought this was a little simpler than it turned out to be. So I thought a better approach was to create an issue/socialize the change & implementation.

the main issue is getting around the fact that EvaluateExpression uses SingletonFunction. Which is fine for the existing implementation, but adding the role property can cause some confusing behavior if its not implemented correctly. The first thing I noticed was the UUID is based only on the nodejs version, so you need to also take into account the role passed in, or the first instance of SingletonFunction evaluated will prevent other roles from being used.

@pahud
Copy link
Contributor

pahud commented Mar 4, 2024

Makes sense.

Before we have the fix I guess you probably can find out the IAM role of the lambda SingletonFunction and override it with your existing one. You probably can modify my sample to fit your needs:

@pahud pahud added p2 effort/medium Medium work item – several days of effort and removed needs-triage This issue or PR still needs to be triaged. labels Mar 4, 2024
@pahud
Copy link
Contributor

pahud commented Mar 4, 2024

While workaround is possible, we still welcome and appreciate any pull requests to address this.

@lagroujl
Copy link
Author

lagroujl commented Mar 4, 2024

#29212 is also a good example of what can go wrong. With my implementation, if you were to change the role between deployments, any currently running workflows could fail.

  • I also considered just accepting the first occurrence of EvaluateExpression wins scenario. Then as a user, you have to make sure the role property is set on every occurrence of EvaluateExpression in the stack.

  • Another approach could be to just add the uuid to the API of EvaluateExpression. If omitted, EvaluteExpression uses the current behavior, otherwise users can force a uuid without messing with private APIs. It can still land in some non-intuitive behavior but at least the users have a public API & visibility into what the issue is + solves the issue of keeping the same lambda function through configuration updates.

  • my implemented approach of adding the role's node addr into the uuid, so each role used will create different lambda functions, but reusing a role should also reuse the Lambda function. that only solves this issue and not 29212

@lagroujl
Copy link
Author

lagroujl commented Mar 12, 2024

@pahud I created this PR: #29288. I'm just curious if I could get some feedback to see if this is the right approach or the CDK even wants to add this feature.

I don't think that your sample is really sufficient. One of the things my customer needs is the ability to use roles that are imported into the stack. Some modification to your sample would let them rename the role that is created, but I don't think it would be possible or that easy to use an imported role.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-stepfunctions-tasks effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants