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

feat(stepfunctions-tasks): specify step functions alias or version ARN in StartExecution #26546

Closed
wants to merge 9 commits into from

Conversation

lpizzinidev
Copy link
Contributor

The stateMachineArn property allows specifying the ARN of a state machine alias or version into the StepFunctionsStartExecution construct.

This allows to start the execution of a specific state machine alias or version.

Closes #26532.


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@github-actions github-actions bot added the star-contributor [Pilot] contributed between 25-49 PRs to the CDK label Jul 28, 2023
@aws-cdk-automation aws-cdk-automation requested a review from a team July 28, 2023 09:45
@github-actions github-actions bot added effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2 labels Jul 28, 2023
@aws-cdk-automation aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jul 28, 2023
Copy link
Contributor

@wong-a wong-a 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 contributing this! A few small comments for permissions and docs. The approach looks good to me

@@ -119,7 +131,7 @@ export class StepFunctionsStartExecution extends sfn.TaskStateBase {
const policyStatements = [
new iam.PolicyStatement({
actions: ['states:StartExecution'],
resources: [this.props.stateMachine.stateMachineArn],
resources: [this.stateMachineArn],
Copy link
Contributor

Choose a reason for hiding this comment

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

StartExecution only has resource-level permissions on the unqualified state machine ARN. If this.stateMachineArn is a version or alias ARN, we need to strip the qualifier :<versionId> or <:aliasName> at the end


/**
* The Step Functions state machine ARN to start the execution on.
* This allows to specify the ARN of a specific state machine version or alias.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* This allows to specify the ARN of a specific state machine version or alias.
* This allows you to specify a state machine version or alias ARN.

* This allows to specify the ARN of a specific state machine version or alias.
* If specified, it overrides the ARN of the stateMachine.
*
* @default - The ARN of stateMachine is used
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure what the convention is for referencing other properties. Perhaps specify stateMachine is from StepFunctionsStartExecutionProps?

Suggested change
* @default - The ARN of stateMachine is used
* @default - The ARN of StepFunctionsStartExecutionProps.stateMachine is used

name: 'myExecutionTask',
});

new IntegTest(app, 'cdk-integ-sfn-start-execution-alias', {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: stack name says "alias` but there's only a version here

@@ -0,0 +1,86 @@
{
"Resources": {
"ChildRole1E3E0EF5": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this have some permissions?

@aws-cdk-automation aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jul 31, 2023
@kaizencc kaizencc changed the title fix(stepfunctions-tasks): allow to specify step functions alias or version ARN in StartExecution feat(stepfunctions-tasks): specify step functions alias or version ARN in StartExecution Aug 1, 2023
Copy link
Contributor

@kaizencc kaizencc left a comment

Choose a reason for hiding this comment

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

@lpizzinidev I chnaged this to a feat which means that there should be readme change introducing this option too :)

Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

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

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

@aws-cdk-automation aws-cdk-automation dismissed their stale review August 2, 2023 06:50

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

@aws-cdk-automation
Copy link
Collaborator

This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 2e50523
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

Comment on lines +51 to +53
* @default - The ARN of StepFunctionsStartExecutionProps.stateMachine is used
*/
readonly stateMachineArn?: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do this, instead writing:

new StartExecution({
  stateMachine: StateMachine.fromStateMachineArn(this, 'SM', 'arn:aws:...'),
});

Like we do for every other construct?

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess that works from looking at the implementation. However, StateMachineVersion and StateMachineAlias are different constructs. The ARN of an alias (e.g. arn:aws:....:stateMachine:MyStateMachineName:MyAlias) is not a state machine ARN

public static fromStateMachineArn(scope: Construct, id: string, stateMachineArn: string): IStateMachine {
class Import extends StateMachineBase {
public readonly stateMachineArn = stateMachineArn;

If that's the preferred approach, it needs to be documented.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure it will work because, as you can see in the integration test, I'm referring to the attrArn of the machine alias-version.
From my understanding stateMachineArn is not returning the ARN with the alias or version (might be wrong of course).
If that is the case, maybe changing this to:

this.stateMachineArn = props.stateMachine.attrArn;

will do the job?

Copy link
Contributor

Choose a reason for hiding this comment

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

I see that we don't have L2 support for StateMachineVersion and StateMachineAlias, so you are trying to pass the ARN of an alias into stateMachineArn ?

What we do in Lambda for a very similar situation, is that we have IFunction which is implemented by all of Function, Version and Alias:

interface IFunction {
  /**
   * Plain ARN, version ARN or Alias ARN, whatever the case may be
   */
  readonly functionArn: string;

  // More
}

Now -- that's not entirely great because the double-duty that functionArn does is sometimes a bit confusion. So if we were to repeat that pattern, I think I would go something like:

interface IStartableStateMachine /* or something */ { 
  /** Machine ARN, version ARN or alias ARN */
  readonly startableStateMachineArn: string;
}

interface IStateMachine extends IStartableStateMachineArn {
  /** Just the plain state machine ARN */
  readonly stateMachineArn: string;
}

And then when we were interested in just staring the state machine and nothing else, we can accept IStartableStateMachine, and perhaps that also has a grantStartExecution() method.

However

This PR wasn't about adding L2s for versions and aliases, it was just about ALLOWING aliases to be used where StateMachines were expected.

Since StateMachine.fromStacheMachineArn(..., 'my-arn') basically is just a fancy typed version of the string 'my-arn', and:

StateMachine.fromStateMachineArn(..., 'my-arn').stateMachineArn == 'my-arn'

I'm confused why just calling .fromStateMachineArn() on an alias ARN wouldn't work. Is it because it is doing validation and it's rejecting the additional ':Version' part of the ARN?

@aws-cdk-automation
Copy link
Collaborator

This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

@indrora
Copy link
Contributor

indrora commented Sep 18, 2023

@lpizzinidev There are outstanding comments that need to be addressed on this PR. Can you address them or should this PR be closed?

@lpizzinidev
Copy link
Contributor Author

lpizzinidev commented Sep 19, 2023

@indrora
I'll need to run some tests to verify that

StateMachine.fromStateMachineArn(..., 'my-arn').stateMachineArn == 'my-arn'

holds for ARNs of versions or aliases.
If that is the case, this PR will resolve to a doc update to specify this behavior.
Otherwise, a code change will be required to allow those ARNs to be passed in some way (probably not in the current way, as discussed).

@aws-cdk-automation
Copy link
Collaborator

This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error.

@aws-cdk-automation aws-cdk-automation added the closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. label Sep 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2 star-contributor [Pilot] contributed between 25-49 PRs to the CDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

aws-stepfunctions-tasks: support revision id in StepFunctionsStartExecution
6 participants