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.

Sign up for GitHub

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

Jump to bottom

(appsync): 2.1.0 breaks deployment #17925

Closed
monholm opened this issue Dec 9, 2021 · 10 comments · Fixed by #17981
Closed

(appsync): 2.1.0 breaks deployment #17925

monholm opened this issue Dec 9, 2021 · 10 comments · Fixed by #17981
Assignees
@otaviomacedo
Labels
@aws-cdk/aws-appsync Related to AWS AppSync bug This issue is a bug. p1

Comments

@monholm
Copy link
Contributor

monholm commented Dec 9, 2021 •
edited
Loading

What is the problem?

After upgrading from 2.0.0 to 2.1.0 we are no longer able to deploy, receiving:
TTL value cannot be smaller than 1 second and larger than 3600 seconds. (Service: AWSAppSync; Status Code: 400; Error Code: BadRequestException

This seems to be caused by #17815, that introduces cachingConfig to appsync resolvers.

Despite the Cloudformation documentation specifying both the CachingConfig and its two Properties CachingKeys & Ttl as optional, our tests show that when CachingConfig is set, Ttl must be as well.

Current state of v2.1.0:
Not specifying any cachingConfig fails.
Specifying a cachingConfig including only cachingKeys fails.
Specifying a cachingConfig including only ttl succeeds.
Specifying a cachingConfig with both cachingKeys and ttl succeeds.

So it seems like ttl cannot be optional, and we need to check if cachingConfig is passed before adding it to the cfnResolver at line 117 in resolver.ts

@kylevillegas93 Are you able to look into this? I'm happy to help, but also quite busy until next year, and it would be nice to have a fix before then.

Reproduction Steps

Deploy a stack with any appsync resolver, without specifying cachingConfig.

What did you expect to happen?

Deployment to succeed without any errors.

What actually happened?

Deployment fails with: TTL value cannot be smaller than 1 second and larger than 3600 seconds. (Service: AWSAppSync; Status Code: 400; Error Code: BadRequestException

CDK CLI Version

2.1.0

Framework Version

2.1.0

Node.js Version

14.18.2

OS

macOS 11.6.1

Language

Typescript

Language Version

No response

Other information

No response
@monholm monholm added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Dec 9, 2021
@github-actions github-actions bot added the @aws-cdk/aws-appsync Related to AWS AppSync label Dec 9, 2021
@ryparker ryparker added the p1 label Dec 9, 2021
@seanWLawrence
Copy link

seanWLawrence commented Dec 10, 2021 •
edited
Loading

Can confirm the same experience. I removed the CachingConfig: {} from my template and it deployed just fine. Def related to: 52b535b

@otaviomacedo otaviomacedo mentioned this issue Dec 10, 2021
Merged
@otaviomacedo
Copy link
Contributor

The PR linked above makes cachingConfig actually optional.

The CloudFormation team has been notified about the mismatch in the specification.

@otaviomacedo otaviomacedo removed the needs-triage This issue or PR still needs to be triaged. label Dec 10, 2021
mergify bot pushed a commit that referenced this issue Dec 10, 2021
@otaviomacedo
fix(appsync): empty caching config is created when not provided (#17947)
3a9f206 
If the `cachingConfig` property is not provided, the library is generating an empty config.

Change this to not add any config to the template.

Related to #17925.

----

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

The PR linked above makes cachingConfig actually optional.

The CloudFormation team has been notified about the mismatch in the specification.

Ah, so the CachingConfig: {} that's generated now by the construct is valid Cloudformation and the bug is with Cloudformation?

@otaviomacedo
Copy link
Contributor

otaviomacedo commented Dec 11, 2021 •
edited
Loading

@seanWLawrence CachingConfig: {} is valid according to the current CloudFormation spec. But the spec is incorrect and does not match the validation done by the AppSync control plane. Once CloudFormation fixes this bug, CachingConfig: {} will no longer be valid. Also, the ttl property will have to be made required in the CDK.

@otaviomacedo otaviomacedo mentioned this issue Dec 13, 2021
Merged
@bodokaiser
Copy link

What is the workaround for this?

@kylevillegas93
Copy link
Contributor

kylevillegas93 commented Dec 13, 2021 •
edited
Loading

My bad - happy to help if there are any other changes that need to be made but looks like they are all covered - just lemme know. @bodokaiser - you many want to use an older CDK version < 2.1.0 until the next release has both of the above changes.

@mergify mergify bot closed this as completed in #17981 Dec 14, 2021
mergify bot pushed a commit that referenced this issue Dec 14, 2021
@otaviomacedo
fix(appsync): ttl property of CachingConfig is not required (#17981)
73e5fec 
According to the current CloudFormaton spec for [AWS::AppSync::Resolver CachingConfig][1], the `Ttl` property is not required. But if it's not provided, the deploy will fail with the message:

>  TTL value cannot be smaller than 1 second and larger than 3600 seconds. (Service: AWSAppSync; Status Code: 400; Error Code: BadRequestException

The AppSync team has confirmed that the property is indeed required and they will push a change to the spec. This change is proactively making the property required on the CDK.

Fixes #17925.

BREAKING CHANGE: The `CachingConfig#ttl` property is now required. 

[1]: https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-appsync-resolver-cachingconfig.html#cfn-appsync-resolver-cachingconfig-ttl
----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

@manumaki
Copy link

2.2.0 has same issue

@monholm
Copy link
Contributor Author

monholm commented Dec 16, 2021

2.2.0 has same issue

We just deployed successfully after upgrading from 2.0.0 to 2.2.0.

@manumaki
Copy link

OK our stack doesn't actually use Appsync. I'll try to figure out what's the problem and create separate issue :) We get the same error though.

TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this issue Feb 21, 2022
@otaviomacedo @TikiTDO
fix(appsync): empty caching config is created when not provided (aws#…
83fc704 
…17947)

If the `cachingConfig` property is not provided, the library is generating an empty config.

Change this to not add any config to the template.

Related to aws#17925.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this issue Feb 21, 2022
@otaviomacedo @TikiTDO
fix(appsync): ttl property of CachingConfig is not required (aws#…
e44d527 
…17981)

According to the current CloudFormaton spec for [AWS::AppSync::Resolver CachingConfig][1], the `Ttl` property is not required. But if it's not provided, the deploy will fail with the message:

>  TTL value cannot be smaller than 1 second and larger than 3600 seconds. (Service: AWSAppSync; Status Code: 400; Error Code: BadRequestException

The AppSync team has confirmed that the property is indeed required and they will push a change to the spec. This change is proactively making the property required on the CDK.

Fixes aws#17925.

BREAKING CHANGE: The `CachingConfig#ttl` property is now required. 

[1]: https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-appsync-resolver-cachingconfig.html#cfn-appsync-resolver-cachingconfig-ttl
----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@otaviomacedo otaviomacedo

Labels
@aws-cdk/aws-appsync Related to AWS AppSync bug This issue is a bug. p1
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix(appsync): ttl property of CachingConfig is not required aws/aws-cdk
7 participants
@otaviomacedo @bodokaiser @ryparker @seanWLawrence @manumaki @monholm @kylevillegas93