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

Perform ETL events via Step Functions #757

Merged
merged 4 commits into from
Oct 18, 2024
Merged

Perform ETL events via Step Functions #757

merged 4 commits into from
Oct 18, 2024

Conversation

coilysiren
Copy link
Contributor

@coilysiren coilysiren commented Sep 27, 2024

Ticket

Resolves #744

Changes

  • Changes the ETL job from invoking ECS directly, to instead invoking Step Functions which then invokes ECS
  • Makes some changes to the scheduled changes to increase their distinction from the events jobs, and the readability of their IAM roles

Context for reviewers

This is a similar PR to #745. In-fact they use many of the same resources. To goal of this PR is to run ETL events via Step Functions. Step Functions create a tracking layer that's not available when invoking ECS directly. This tracking layer allows you to see the success and failure status of your jobs.

Testing

navapbc/platform-test#136

@coilysiren coilysiren marked this pull request as ready for review September 27, 2024 23:17
@coilysiren coilysiren requested a review from lorenyu September 27, 2024 23:18
Copy link
Contributor

@lorenyu lorenyu left a comment

Choose a reason for hiding this comment

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

Amazing looking really clean. just fyi template deploys to platform-test-nextjs and platform-test-flask are temporarily broken tanner is gonna fix them on monday see 🔒 https://nava.slack.com/archives/C03G1SWD9H7/p1727471310172749?thread_ts=1727463533.550949&cid=C03G1SWD9H7

@coilysiren
Copy link
Contributor Author

Wow I forgot to merge this!

@coilysiren
Copy link
Contributor Author

terratest error:

image
          	Error:      	Received unexpected error:
          	            	error while running command: exit status 2; ╷
          	            	│ Error: creating IAM Policy (app-dev-run-access): operation error IAM: CreatePolicy, https response error StatusCode: 400, RequestID: 7f8059b2-faf0-4408-ad4d-066694979b72, MalformedPolicyDocument: Policy statement must contain resources.
          	            	│ 
          	            	│   with module.service.aws_iam_policy.run_task,
          	            	│   on ../../modules/service/events_role.tf line 26, in resource "aws_iam_policy" "run_task":
          	            	│   26: resource "aws_iam_policy" "run_task" {
          	            	│ 
          	            	╵
          	            	╷
          	            	│ Error: creating IAM Policy (app-dev-scheduler): operation error IAM: CreatePolicy, https response error StatusCode: 400, RequestID: eb3c6499-a7ae-4f67-8c3f-9c30b21a3cc7, MalformedPolicyDocument: Policy statement must contain resources.
          	            	│ 
          	            	│   with module.service.aws_iam_policy.scheduler,
          	            	│   on ../../modules/service/scheduler_role.tf line 22, in resource "aws_iam_policy" "scheduler":
          	            	│   22: resource "aws_iam_policy" "scheduler" {
          	            	│ 
          	            	╵

@coilysiren
Copy link
Contributor Author

each statement contains resources... that this worked on platform test... :/

@lorenyu
Copy link
Contributor

lorenyu commented Oct 17, 2024

@coilysiren interesting, looks like it fails when there are no scheduled jobs or event-based jobs defined, since the resources looks like resources = ["arn:aws:events:${data.aws_region.current.name}:${data.aws_caller_identity.current.account_id}:rule/StepFunctionsGetEventsForStepFunctionsExecutionRule"], so if the for loop is empty then resources would be an empty array

@lorenyu
Copy link
Contributor

lorenyu commented Oct 17, 2024

@coilysiren maybe go back to using the dynamic statement that you had previously rather than the pattern I recommended e.g.

dynamic "statement" {
    for_each = aws_sfn_state_machine.scheduled_jobs

    content {
      actions = [
        "states:StartExecution",
      ]
      resources = [statement.value.arn]
    }
  }

and add a comment along the lines of

# A dynamic statement is necessary since if we instead use a static statement
# with a dynamic resources block like resources = [for job in aws_sfn_state_machine.scheduled_jobs : "${job.arn}"]
# then if there are no jobs defined the resources block would be empty which IAM does not allow for AWS Step Functions
# policies, which would result in an error: "MalformedPolicyDocument: Policy statement must contain resources"

@coilysiren
Copy link
Contributor Author

@lorenyu that makes sense! I'm on it

@lorenyu
Copy link
Contributor

lorenyu commented Oct 18, 2024

btw once you've made the changes and tested them and the tests all pass feel free to proceed to merge, no need to get another review from me unless there's anything you think will be suprising

@coilysiren coilysiren merged commit 077a980 into main Oct 18, 2024
9 checks passed
@coilysiren coilysiren deleted the kai/events-via-sfn branch October 18, 2024 18:04
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.

Use step functions to run event jobs
2 participants