DataDog / datadog-ci

Use Datadog from your CI.
https://datadoghq.com
Apache License 2.0
129 stars 55 forks source link

Skip sfn->Lambda context injection if custom Payload is used without $ #1450

Closed lym953 closed 2 months ago

lym953 commented 2 months ago

Problem

If a Lambda function in a State Machine has a custom Payload defined using "Payload": ..., e.g.,

    "Lambda Invoke": {
      "Type": "Task",
      "Resource": "arn:aws:states:::lambda:invoke",
      "OutputPath": "$.Payload",
      "Parameters": {
        "FunctionName": "...",
        "Payload": {
          "action": "service/delete_customer"
        }
      },
      ...
    }
  }

then context injection for the Step Function will fail with the error:

[Error] Invalid State Machine Definition: 'SCHEMA_VALIDATION_FAILED: Cannot have both the field 'Payload.$' and the field 'Payload' at the same time at /States/Lambda Invoke/Parameters'. Failed to inject context into lambda functions' payload of arn:aws:states:sa-east-1:425362996713:stateMachine:YimingTestExpressStateMachine2

This is because our instrumentation code will add a field "Payload.$": ..., which duplicates with "Payload": .... Right now we only check for custom "Payload.$": ... (with $, i.e. using JSONPath) but don't check for custom "Payload": ....

What

Also skip context injection when a custom "Payload": ... field is used (without $)

Testing

Automated testing

Pass the added test, which would fail without the changes on helpers.ts

Manual testing

Steps:

  1. change the Payload field of a Lambda function to what's shown in "Problem" section
  2. run datadog-ci stepfunctions instrument

Result: a warning is printed as expected image

Notes

  1. Thanks @agocs for bringing this up in https://github.com/DataDog/datadog-ci/pull/1443#pullrequestreview-2293937693
  2. The behavior will be changed soon: we will soon allow custom Payload field as long as it doesn't contain Execution or State field inside. But I want to first merge this PR to make the existing behavior more clear, so the changes in future PRs will be more clear so the PRs will be easier to review.

Review checklist

datadog-datadog-prod-us1[bot] commented 2 months ago

Datadog Report

Branch report: yiming.luo/fix-step-func-3 Commit report: e9c812d Test service: datadog-ci-tests

:white_check_mark: 0 Failed, 372 Passed, 0 Skipped, 1m 29.08s Total duration (1m 58.6s time saved)

nine5two7 commented 2 months ago

Also skip context injection when a custom "Payload": ... field is used (without $) For customized payload, we can still explicitly inject

"Execution.$": "$$.Execution",
"State.$": "$$.State"

Will there be another PR for such handling?

lym953 commented 2 months ago

Will there be another PR for such handling?

@nine5two7 You mean the injection thing? Yes I'll do it in separate PRs. All the PRs so far are small fixes, either printing warnings or making error messages more clear.