feat(pipes-targets): add API destination

[msambol]

Add EventBridge API destination as a Pipes target.

CloudFormation groups EventBridge API destination with API Gateway REST API
as PipeTargetHttpParameters, but I think separating them here similar to aws-event-targets
makes more sense, as API Gateway requires stage, path, and method (see here).

[nmussy]

Mostly integ coverage, LGTM overall

[nmussy]

I'm also curious to see what happens when you also set the same header in ApiDestination.headerParameters. It's either ignored or overrides the authorization, but I think we should probably warn the user either way

[msambol]

See comment below with screenshot.

[msambol]

@nmussy Here's the result from the Lambda logs. The x-api-key defined in Connection has precedence over the one defined in headerParameters.

[msambol]

@nmussy I added this note about precedence. From the logs we can see that x-api-key is abc123, not apiKeyFromHeaderParams.

[Leo10Gama]

I'm not sure whether this is enough to indicate to users which value will take precedence. Is there somewhere we can print a warning when there are two identical headers provided, which explicitly lets the user know that the value is being set twice, and which of those values it's using?

[msambol]

@redwheeler3 jfyi. I think it's worth mentioning in Docs and/or Console about precedence regarding conflicting headers.

[Leo10Gama]

Thanks for your contribution, and sorry for the delay! Overall seems good to me, just a few things to note before moving forward with these changes.

[Leo10Gama]

I'm not sure whether this is enough to indicate to users which value will take precedence. Is there somewhere we can print a warning when there are two identical headers provided, which explicitly lets the user know that the value is being set twice, and which of those values it's using?

[msambol]

I'm not sure whether this is enough to indicate to users which value will take precedence. Is there somewhere we can print a warning when there are two identical headers provided, which explicitly lets the user know that the value is being set twice, and which of those values it's using?

I'm not quite sure how to do this. There could be any number of custom headers, no?

[Leo10Gama]

I'm not sure whether this is enough to indicate to users which value will take precedence. Is there somewhere we can print a warning when there are two identical headers provided, which explicitly lets the user know that the value is being set twice, and which of those values it's using?

I'm not quite sure how to do this. There could be any number of custom headers, no?

I believe that's the case, yeah. I was thinking some check like iterating through the header keys to see which overlap, like turning them into a set and taking the intersection type of operation. Then if we know the Connection key has precedence, like your logs show, we print a warning along the lines of:

"Key <so-and-so> is defined in both Connection [connection] and ApiDestinationTarget [api destination target]. Value <my value> from Connection will be used."

Pull request has been modified.

[msambol]

I'm not sure whether this is enough to indicate to users which value will take precedence. Is there somewhere we can print a warning when there are two identical headers provided, which explicitly lets the user know that the value is being set twice, and which of those values it's using?

I'm not quite sure how to do this. There could be any number of custom headers, no?

I believe that's the case, yeah. I was thinking some check like iterating through the header keys to see which overlap, like turning them into a set and taking the intersection type of operation. Then if we know the Connection key has precedence, like your logs show, we print a warning along the lines of:

"Key <so-and-so> is defined in both Connection [connection] and ApiDestinationTarget [api destination target]. Value <my value> from Connection will be used."

I'd prefer to do this in a follow-up PR. What do you think? I think the logic could get messy/complicated. I think the documentation note is sufficient for now.

[Leo10Gama]

I'd prefer to do this in a follow-up PR. What do you think? I think the logic could get messy/complicated. I think the documentation note is sufficient for now.

I do think we should include these changes in this PR, unless there are any other extra changes you wanted to add that might make the logic more confusing to follow. But ideally, including them in this PR means the logic is bundled together from the get-go.

[Leo10Gama]

Thanks for the changes! Aside from my comment above regarding including the warning, just one more minor nit in the README.

[msambol]

I'd prefer to do this in a follow-up PR. What do you think? I think the logic could get messy/complicated. I think the documentation note is sufficient for now.

I do think we should include these changes in this PR, unless there are any other extra changes you wanted to add that might make the logic more confusing to follow. But ideally, including them in this PR means the logic is bundled together from the get-go.

I'm a little confused on what this would look like. An ApiDestination doesn't have headers associated with it. In my integration test example, the Connection has an API key (x-api-key). Happy to add the warning but I need some direction as there is nothing to iterate through on ApiDestination ?

Pull request has been modified.

[Leo10Gama]

I'd prefer to do this in a follow-up PR. What do you think? I think the logic could get messy/complicated. I think the documentation note is sufficient for now.

I do think we should include these changes in this PR, unless there are any other extra changes you wanted to add that might make the logic more confusing to follow. But ideally, including them in this PR means the logic is bundled together from the get-go.

I'm a little confused on what this would look like. An ApiDestination doesn't have headers associated with it. In my integration test example, the Connection has an API key (x-api-key). Happy to add the warning but I need some direction as there is nothing to iterate through on ApiDestination ?

If I'm understanding the code correctly, the area of conflict is between ApiDestination.headerParameters and the API key, so we'd iterate on the header parameters. We should be able to compare the headers since the target has access to the Connection via ApiDestinationTarget.destination.connection.

[msambol]

I'd prefer to do this in a follow-up PR. What do you think? I think the logic could get messy/complicated. I think the documentation note is sufficient for now.

I do think we should include these changes in this PR, unless there are any other extra changes you wanted to add that might make the logic more confusing to follow. But ideally, including them in this PR means the logic is bundled together from the get-go.

I'm a little confused on what this would look like. An ApiDestination doesn't have headers associated with it. In my integration test example, the Connection has an API key (x-api-key). Happy to add the warning but I need some direction as there is nothing to iterate through on ApiDestination ?

If I'm understanding the code correctly, the area of conflict is between ApiDestination.headerParameters and the API key, so we'd iterate on the header parameters. We should be able to compare the headers since the target has access to the Connection via ApiDestinationTarget.destination.connection.

I think I would need to change

private destination: IApiDestination;

to

private destination: ApiDestination;

Excuse my naïveté. Good with that?

[msambol]

@moelasmar updated with your feedback—thanks!

[Leo10Gama]

Seems all good to me, thanks for the quick responses on feedback! Just adding in one quick typehint that I missed before, and will wait to make sure the tests pass.

[msambol]

@mergify update

[mergify]

update

✅ Branch has been successfully updated

[aws-cdk-automation]

AWS CodeBuild CI Report

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

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

[mergify]

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

[github-actions]

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.