feat(route53-targets): add `AppSync` route53 target #31976

ScottRobinson03 · 2024-11-01T12:02:35Z

Issue # (if applicable)

Closes #26109

Reason for this change

This PR adds support for creating alias records on AppSync's GraphqlApi.

Description of changes

Add appsync-target.ts file, with the AppSync class.
Add unit tests for this new AppSync target class.
Update README.md of aws-cdk-lib/aws-route53-targets

Description of how you validated changes

Add unit tests

Checklist

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

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

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

ScottRobinson03 · 2024-11-01T12:44:40Z

Clarification Request

I've written an integration test, but when trying to run it from aws-cdk/packages/@aws-cdk-testing/framework-integ via yarn integ --update-on-failed integ.appsync-target.js (as per the instructions) I'm getting the following error:

Trying to debug I thought maybe I have to build things first, so I ran yarn build, but that then spews out a load of the following:

Running yarn install says "Already up-to-date", so I'm not sure what's going on or how I'm meant to do run the added integration test. Assistance is appreciated.

paulhcsun · 2024-11-01T18:51:17Z

Hey Scott, I don't see anything incorrect in your integ test file. I would recommend just running a full clean rebuild with

$ git clean -fqdx .
$ yarn install
$ yarn build

and see if that resolves the import error.

ScottRobinson03 · 2024-11-05T19:06:59Z

Hey Scott, I don't see anything incorrect in your integ test file. I would recommend just running a full clean rebuild with
$ git clean -fqdx .
$ yarn install
$ yarn build
and see if that resolves the import error.

@paulhcsun Thanks, this did indeed fix the issue and I'm now able run the test. Although, as the comment within the file kinda says (copied from the route53-targets ApiGateway integ test), it's a somewhat redundant test since the stack can never deploy due to the hardcoded certificate & hosted zone arns.

When running (via yarn integ --update-on-failed aws-route53-targets/test/integ.appsync-target.js) I get the following output:

I think this means I need a "Exemption Request".

paulhcsun · 2024-11-06T19:29:29Z

Hey Scott, I don't see anything incorrect in your integ test file. I would recommend just running a full clean rebuild with
$ git clean -fqdx .
$ yarn install
$ yarn build
and see if that resolves the import error.
@paulhcsun Thanks, this did indeed fix the issue and I'm now able run the test. Although, as the comment within the file kinda says (copied from the route53-targets ApiGateway integ test), it's a somewhat redundant test since the stack can never deploy due to the hardcoded certificate & hosted zone arns.

When running (via yarn integ --update-on-failed aws-route53-targets/test/integ.appsync-target.js) I get the following output:

I think this means I need a "Exemption Request".

Awesome! And gotcha, let me just confirm with the team member who added that explanation and if there's no other way to test this then I can add the integ teste exempted label.

Edit:

Actually it seems like there are some integ tests for the specific targets, for example this one for interface-vpc-endpoint-target. Would you be able to reference these tests and write out for AppSync as a target as well?

ScottRobinson03 · 2024-11-07T09:13:20Z

Actually it seems like there are some integ tests for the specific targets, for example this one for interface-vpc-endpoint-target. Would you be able to reference these tests and write out for AppSync as a target as well?

@paulhcsun I believe the key difference with API Gateway and AppSync is that they both require a certificate to be created in order to have a custom domain.

The other route53-targets (such as interface-vpc-endpoint-target) don't require this certificate, which is why integ tests are possible.

It's not entirely clear to me why having a certificate makes it impossible to integ test, but that's how the API Gateway integ tests' comment reads to me.

If I'm misunderstanding then please advise accordingly.

mazyu36 · 2024-11-08T02:51:03Z

Hi.
Just for information, to run a test that requires a domain and a certificate, you'll need to set up a domain and configure it in the Environment Variables before running an integ test.

This requirement is noted in the following docs:
https://github.com/aws/aws-cdk/blob/main/packages/%40aws-cdk-testing/framework-integ/README.md

I've recently created an integ test that requires a domain:
https://github.com/aws/aws-cdk/pull/30784/files#diff-f22841cd9a989a8be48e18db20b731abb510c127c0c9f7dc07e953b702a7a385

Do you have a domain available for an integ test?
If not, it won't be possible to run the integ test.

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

ScottRobinson03 · 2024-11-08T14:29:56Z

Thanks for this @mazyu36, I was able to get a passing integ test running: fd783f7

Note that the submitted snapshot has dummy values, since I don't want to leak my domain/certificate/hostedzone.

I think this means I no longer require an exemption, but not sure how to cancel the request.

ScottRobinson03 · 2024-11-08T15:09:04Z

The CodeBuild is failing because it's not providing a certificate arn env var:

What's the correct way to fix this? I suppose I need to somehow tell CodeBuild to not run this specific test? Not sure...

mazyu36

I’m not entirely confident, but I commented based on my experience from when I previously ran the integ test that uses a domain.

mazyu36 · 2024-11-08T15:30:22Z

packages/@aws-cdk-testing/framework-integ/test/aws-route53-targets/test/integ.appsync-target.ts

+  }
+}
+
+const certificateArn = process.env.CDK_INTEG_CERTIFICATE_ARN ?? process.env.CERTIFICATE_ARN;


Suggested change

const certificateArn = process.env.CDK_INTEG_CERTIFICATE_ARN ?? process.env.CERTIFICATE_ARN;

const certificateArn = process.env.CDK_INTEG_CERT_ARN ?? process.env.CERTIFICATE_ARN;

It seems that the environment variable name used by integ-runner is incorrect.

https://github.com/aws/aws-cdk/blob/main/packages/%40aws-cdk/integ-runner/lib/runner/runner-base.ts#L431

Good catch. I've adjusted within 73a4a46. This could be why the PR CodeBuild was failing too.

mazyu36 · 2024-11-08T15:31:20Z

packages/@aws-cdk-testing/framework-integ/test/aws-route53-targets/test/integ.appsync-target.ts

+const testCase = new TestStack(app, 'aws-cdk-appsync-alias-integ', { certificateArn, domainName, hostedZoneId });
+new IntegTest(app, 'appsync-domain-name', {
+  testCases: [testCase],
+});


Suggested change

});

enableLookups: true,

stackUpdateWorkflow: false,

});

When I previously conducted a test using a domain, configuring this setting registered dummy values for the environment variables used by integ-runner in the template.

The current dummy values in the template seem different from the ones used by integ-runner. How were these values set?

https://github.com/aws/aws-cdk/blob/main/packages/%40aws-cdk/integ-runner/lib/runner/runner-base.ts#L429

I manually set the current dummy values -- I didn't realise there was an option to have the test runner set dummy values!

However, if I unset the environment variables and try to rerun the test, I get an error saying the env vars must be set. Is there an extra flag or something I'm meant to use?

Did you set your certificate arn to CERTIFICATE_ARN?

In the following docs, env name is CERT_ARN, but in your code env name is CERTIFICATE_ARN.

https://github.com/aws/aws-cdk/blob/main/packages/%40aws-cdk-testing/framework-integ/README.md

export CERTFICATE_ARN=your_certificate_arn or fix env name in your code is needed, I think.

I'm able to get the tests running and passing locally if I use the real certificate etc.:

My point is that I thought you were saying the committed snapshot should use the integ-test-defined dummy values.

How do I make the snapshot have the "correct" dummy values? Is there some command I'm meant to use, or do I just manually edit them?

If you run the integ test with the following configuration, I believe the dummy values will be automatically set in the template.

enableLookups: true, stackUpdateWorkflow: false, });

…ICATE_ARN'

aws-cdk-automation · 2024-11-08T17:07:18Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 40d5718
Result: FAILED
Build Logs (available for 30 days)

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

feat(route53-targets): Add AppSync target

a3d6535

github-actions bot added the beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK label Nov 1, 2024

aws-cdk-automation requested a review from a team November 1, 2024 12:02

github-actions bot added effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2 labels Nov 1, 2024

aws-cdk-automation previously requested changes Nov 1, 2024

View reviewed changes

ScottRobinson03 changed the title ~~feat(route53-targets): Add AppSync route53 target~~ feat(route53-targets): add AppSync route53 target Nov 1, 2024

ScottRobinson03 added 2 commits November 1, 2024 12:33

Add integration test

6a57f85

Fix import file path

2af31e5

aws-cdk-automation added the pr/reviewer-clarification-requested The contributor has requested clarification on feedback, a failing build, or a failing PR Linter run label Nov 1, 2024

paulhcsun removed the pr/reviewer-clarification-requested The contributor has requested clarification on feedback, a failing build, or a failing PR Linter run label Nov 1, 2024

github-actions bot mentioned this pull request Nov 4, 2024

Weekly PR metrics report #32003

Closed

aws-cdk-automation added the pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. label Nov 5, 2024

Fix integration test for new appsync route53-target

fd783f7

mazyu36 reviewed Nov 8, 2024

View reviewed changes

ScottRobinson03 added 2 commits November 8, 2024 15:46

Use correct 'CDK_INTEG_CERT_ARN' env var instead of 'CDK_INTEG_CERTIF…

73a4a46

…ICATE_ARN'

Only run appsync route53-target integration test in us-east-1

40d5718

paulhcsun removed the pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. label Nov 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(route53-targets): add `AppSync` route53 target #31976

feat(route53-targets): add `AppSync` route53 target #31976

ScottRobinson03 commented Nov 1, 2024

aws-cdk-automation left a comment

ScottRobinson03 commented Nov 1, 2024

paulhcsun commented Nov 1, 2024

ScottRobinson03 commented Nov 5, 2024

paulhcsun commented Nov 6, 2024 •

edited

Loading

ScottRobinson03 commented Nov 7, 2024

mazyu36 commented Nov 8, 2024

ScottRobinson03 commented Nov 8, 2024 •

edited

Loading

ScottRobinson03 commented Nov 8, 2024

mazyu36 left a comment

mazyu36 Nov 8, 2024

ScottRobinson03 Nov 8, 2024

mazyu36 Nov 8, 2024

ScottRobinson03 Nov 8, 2024

mazyu36 Nov 8, 2024 •

edited

Loading

ScottRobinson03 Nov 8, 2024 •

edited

Loading

mazyu36 Nov 8, 2024 •

edited

Loading

aws-cdk-automation commented Nov 8, 2024

	const certificateArn = process.env.CDK_INTEG_CERTIFICATE_ARN ?? process.env.CERTIFICATE_ARN;
	const certificateArn = process.env.CDK_INTEG_CERT_ARN ?? process.env.CERTIFICATE_ARN;

-});
+  enableLookups: true,
+  stackUpdateWorkflow: false,
+});

feat(route53-targets): add AppSync route53 target #31976

Are you sure you want to change the base?

feat(route53-targets): add AppSync route53 target #31976

Conversation

ScottRobinson03 commented Nov 1, 2024

Issue # (if applicable)

Reason for this change

Description of changes

Description of how you validated changes

Checklist

aws-cdk-automation left a comment

Choose a reason for hiding this comment

ScottRobinson03 commented Nov 1, 2024

paulhcsun commented Nov 1, 2024

ScottRobinson03 commented Nov 5, 2024

paulhcsun commented Nov 6, 2024 • edited Loading

ScottRobinson03 commented Nov 7, 2024

mazyu36 commented Nov 8, 2024

ScottRobinson03 commented Nov 8, 2024 • edited Loading

ScottRobinson03 commented Nov 8, 2024

mazyu36 left a comment

Choose a reason for hiding this comment

mazyu36 Nov 8, 2024

Choose a reason for hiding this comment

ScottRobinson03 Nov 8, 2024

Choose a reason for hiding this comment

mazyu36 Nov 8, 2024

Choose a reason for hiding this comment

ScottRobinson03 Nov 8, 2024

Choose a reason for hiding this comment

mazyu36 Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

ScottRobinson03 Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

mazyu36 Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

aws-cdk-automation commented Nov 8, 2024

AWS CodeBuild CI Report

feat(route53-targets): add `AppSync` route53 target #31976

feat(route53-targets): add `AppSync` route53 target #31976

paulhcsun commented Nov 6, 2024 •

edited

Loading

ScottRobinson03 commented Nov 8, 2024 •

edited

Loading

mazyu36 Nov 8, 2024 •

edited

Loading

ScottRobinson03 Nov 8, 2024 •

edited

Loading

mazyu36 Nov 8, 2024 •

edited

Loading