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

Upgrade to v1.6 fails during cdk synth #603

Closed
marora90 opened this issue Jul 27, 2023 · 3 comments
Closed

Upgrade to v1.6 fails during cdk synth #603

marora90 opened this issue Jul 27, 2023 · 3 comments
Assignees
Milestone

Comments

@marora90
Copy link
Contributor

Describe the bug

While upgrading to v1.6 with a custom domain setting, the build fails during cdk synth stage.

Complete logs from the build stage: https://gist.github.com/blitzmohit/7fb21ac8c3fdb01453a8254304211b3e

How to Reproduce

Upgrade from v1.5 to v1.6 with internet_facing: true along with custom_domain including certificate_arn

Expected behavior

No response

Your project

No response

Screenshots

No response

OS

Linux

Python version

3.9.17

AWS data.all version

1.6

Additional context

The issue seems related to the modifications made in https://github.com/awslabs/aws-dataall/pull/529/files#diff-c65de5ab1eebd2a930807381430fa602793cc9966ab2a064cb29603162377030

It seems it is related to the ViewerCert being passed and difference in configuration properties mentioned in https://docs.aws.amazon.com/cdk/api/v2/python/aws_cdk.aws_cloudfront/README.html#the-distribution

@dbalintx
Copy link
Contributor

Hello @blitzmohit ,
Thank you for opening the issue.
I've prepared a fix for this in #607 , and I'm starting to do a fresh deployment (-> v1.5 -> v1.6) to test if it works as expected.

Do you mind also trying out the fix in your deployment?

Thanks a lot!

@marora90
Copy link
Contributor Author

@dbalintx Thank you for looking into this. I was able to get the cloudstack to work with similar modifications late last night and was hoping to create the PR this morning.

Note that there were 2 other issues related to cloudfront that our deploy encountered after this :

  1. Resource handler returned message: "Invalid request provided: AWS::CloudFront::Distribution: One or more of the CNAMEs you provided are already associated with a different resource. (Service: CloudFront, Status Code: 409, Request ID: xxxxx-xxxx-xxxx-xxxx)" (RequestToken: xxxxx-xxxx-xxxx-xxxx, HandlerErrorCode: InvalidRequest)
    For this one I had to manually remove the CNAME from the current distribution and then rerun the deploy. Not sure if this something we can work around in the code and would be good to mention this in the release notes if that is the case.

  2. Resource handler returned message: "Invalid request provided: AWS::CloudFront::Distribution: The certificate that is attached to your distribution doesn't cover the alternate domain name (CNAME) that you're trying to add. For more details, see: https://docs.aws.amazon.com/AmazonCloudFront/latest/DeveloperGuide/CNAMEs.html#alternate-domain-names-requirements (Service: CloudFront, Status Code: 400, Request ID: xxxxx-xxxx-xxxx-xxxx)" (RequestToken: xxxxx-xxxx-xxxx-xxxx, HandlerErrorCode: InvalidRequest)
    The certificate we were using only contained the CNAME for the root domain however did not have the userguide subdomain which caused the userguide distribution to fail. I'm not certain when this check was enabled as our deploys for v1.5 were working fine with same setup. This was easy enough to fix with a new cert containing both the domains.

@dbalintx
Copy link
Contributor

dbalintx commented Aug 1, 2023

Hello Mohit,

Thank you for confirming.
I was able to confirm that the changes in the PR works.
I could also confirm both of your 2 issues that you mentioned.
Regarding 1: I added a note to the v1.6 release note
Regarding 2: I'm not sure why this wasn't an issue beforehand, but as the documentation describes, alternate domains should be added to the TLS certificate.

Thanks again for pointing out the issue!

dbalintx added a commit that referenced this issue Aug 2, 2023
### Feature or Bugfix
- Bugfix

### Detail
In case a custom domain and ACM certificate is configured for
CloudFront, these parameters have to be passed differently to the newly
introduced CloudFront distribution CDK class, introduced in
[v1.6](84c555e#diff-c65de5ab1eebd2a930807381430fa602793cc9966ab2a064cb29603162377030)

### Relates
#603 

Testing:
Tested by,

- creating a fresh deployment with v1.5 with a custom domain and SSL
certificate
- upgrading to v1.6, with the bugfix content from this PR

2 issues that are detailed in #603 arose (both related to the upgrade
process), mitigation method of them is detailed there in the issue.


By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license.
dbalintx added a commit to dbalintx/aws-dataall that referenced this issue Aug 3, 2023
### Feature or Bugfix
- Bugfix

### Detail
In case a custom domain and ACM certificate is configured for
CloudFront, these parameters have to be passed differently to the newly
introduced CloudFront distribution CDK class, introduced in
[v1.6](data-dot-all@84c555e#diff-c65de5ab1eebd2a930807381430fa602793cc9966ab2a064cb29603162377030)

### Relates
data-dot-all#603 

Testing:
Tested by,

- creating a fresh deployment with v1.5 with a custom domain and SSL
certificate
- upgrading to v1.6, with the bugfix content from this PR

2 issues that are detailed in data-dot-all#603 arose (both related to the upgrade
process), mitigation method of them is detailed there in the issue.


By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license.
@dlpzx dlpzx closed this as completed Aug 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants