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

fix(events): additional plaintext header are not set on eventbridge connection #21857

Merged

Conversation

RaphaelManke
Copy link
Contributor

@RaphaelManke RaphaelManke commented Aug 31, 2022

Fixes: #21855

While creating a Eventbridge connection to make api calls to an external api one sometimes have to add additional header parameters like Content-Type = application/json
These additional headers can be either be a secret value or a plaintext value specified at deploy time.
The connection class provides a HttpParameter class that alows you to set a static/unsecure/plaintext value for a header key

const connection = new Connection(this, "connection", {
  authorization: Authorization.apiKey( "authorization", secret.secretValue),
  headerParameters: {
    "Content-Type": HttpParameter.fromString("application/json"),
  },
});

This should lead to api calls made with the connection have a Header present with key/value "Content-Type": "application/json",

The actual behavior was prior to this Fix that the header wasn't present in the api calls made with this connection.

While debugging the issue I used the following aws cli commands to check what has been deployed by cdk/cloudformation

aws events describe-connection --name <name-of-the-connection>

which result was similair to this

{
    "ConnectionArn": "arn:aws:events:eu-west-1:XXXXXXX:connection/SomeConnection/0848ec46-413a-4d40-8834-XXXXXX",
    "Name": "SomeConnection",
    "ConnectionState": "AUTHORIZED",
    "AuthorizationType": "API_KEY",
    "SecretArn": "arn:aws:secretsmanager:eu-west-1:XXXXXXX:secret:events!connection/SomeSecret/1e74cbb0-dfc6-4b77-a49f-b204e6b74a46-XXXXXX",
    "AuthParameters": {
        "ApiKeyAuthParameters": {
            "ApiKeyName": "authorization"
        },
        "InvocationHttpParameters": {
            "HeaderParameters": [
                {
                    "Key": "Content-Type",
                    "IsValueSecret": true
                }
            ]
        }
    },
    "CreationTime": "2022-08-29T16:57:35+02:00",
    "LastModifiedTime": "2022-08-29T16:57:35+02:00",
    "LastAuthorizedTime": "2022-08-29T16:57:35+02:00"
}

Which indicates that the header value is not set because it is treated as secret value and needs to be provided by the referenced secret.

Then i checked the Cloudformation spec
https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-events-connection-parameter.html
There it is indicated that there is the property isValueSecret which indicates if the value is a secret or not.

The next step was to check why cdk generates a template that doesn't work and thereby checked the HttpParameter class.
This class is responsible for generating the AWS::Events::Connection Parameter properties.
I noticed that only the HttpParameter.fromSecret() sets the isValueSecret flag.
But it seems to be the case that for this property the default value is true by cloudformation, so omiting this attribute in the _render function results to isValueSecret: true at deploy time.

After that i explicity set the value to false for the case the user specifies a plaintext value throught the HttpParameter.fromString() method.

To make sure the correct values are deployed by cloudformation I added a integration test including an assertion that the deployed connection has the correct isValueSecret flag set and the value for the header is set.


All Submissions:

Adding new Unconventional Dependencies:

  • This PR adds new unconventional dependencies following the process described here

New Features

  • Have you added the new feature to an integration test?
    • Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

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

@gitpod-io
Copy link

gitpod-io bot commented Aug 31, 2022

@github-actions github-actions bot added the p2 label Aug 31, 2022
@aws-cdk-automation aws-cdk-automation requested a review from a team August 31, 2022 17:08
@RaphaelManke RaphaelManke changed the title fix(aws-events): Add isValueSecret to HttpParameter fix(aws-events): Add missing attribute isValueSecret to HttpParameter Aug 31, 2022
Copy link
Contributor

@TheRealAmazonKendra TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

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

Please make sure that your PR title confirms to the conventional commit standard (fix, feat, chore) and that it is written in a style that will reflect correctly in the change log (See Contributing Guide, Pull Requests).

Additionally, please make sure that your PR body describes the problem the PR is solving, and the design approach and alternatives considered. Explain why the PR solves the problem. A link to an issue is helpful, but does not replace an explanation of your thought process.

Lastly, we need an integration test for this change.

@@ -199,6 +199,7 @@ export abstract class HttpParameter {
return {
key: name,
value,
isValueSecret: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why will this always be false? This PR needs context.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes this value needs to be set to false to indicate the provided value is not secret. Since this is the render method from the HttpParameter.fromString() the value is always treated as not secret. If you like to provide a secret value you have to use the HttpParameter.fromSecret() method.
The default value for isValueSecret seams to be true if not provided. See the updated PR description or linked issue.

Copy link
Contributor

@TheRealAmazonKendra TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

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

Hit the wrong button and didn't put this into request changes.

@mergify mergify bot dismissed TheRealAmazonKendra’s stale review September 1, 2022 06:52

Pull request has been modified.

@RaphaelManke RaphaelManke changed the title fix(aws-events): Add missing attribute isValueSecret to HttpParameter fix(aws-events): Additional plaintext header are not set on eventbridge connection Sep 1, 2022
@TheRealAmazonKendra TheRealAmazonKendra changed the title fix(aws-events): Additional plaintext header are not set on eventbridge connection fix(events): additional plaintext header are not set on eventbridge connection Sep 2, 2022
Copy link
Contributor

@TheRealAmazonKendra TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

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

Putting this back in request changes since there are still some things to be addressed.

@mergify mergify bot dismissed TheRealAmazonKendra’s stale review September 2, 2022 08:29

Pull request has been modified.

@RaphaelManke RaphaelManke force-pushed the raphaelmanke/fix-HttpParameter-fromString branch from bd41798 to cefb64d Compare September 2, 2022 09:27
Copy link
Contributor

@TheRealAmazonKendra TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

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

Needs merge conflicts resolved.

@RaphaelManke RaphaelManke force-pushed the raphaelmanke/fix-HttpParameter-fromString branch from 6352580 to af59ed6 Compare September 3, 2022 09:15
@mergify mergify bot dismissed TheRealAmazonKendra’s stale review September 3, 2022 09:15

Pull request has been modified.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

@mergify
Copy link
Contributor

mergify bot commented Sep 3, 2022

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).

@mergify mergify bot merged commit f3f4814 into aws:main Sep 3, 2022
@RaphaelManke RaphaelManke deleted the raphaelmanke/fix-HttpParameter-fromString branch September 3, 2022 15:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(aws-events): HttpParameter.fromString misses isValueSecret attribute
3 participants