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

feat(appsync): support custom domain mappings #19368

Merged
merged 11 commits into from Mar 18, 2022
35 changes: 35 additions & 0 deletions packages/@aws-cdk/aws-appsync/README.md
Expand Up @@ -285,6 +285,41 @@ ds.createResolver({
});
```

## Custom Domain Names

For many use cases you may want to associate a custom domain name with your
GraphQL API. This can be done during the API creation.

```ts
import { Certificate } from '@aws-cdk/aws-certificatemanager';

declare const certificate: Certificate;
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this work? You're not initializing it to anything.

Copy link
Contributor

Choose a reason for hiding this comment

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

Will this work? no. Can it compile this way? yes. This is something we are using in our examples for necessary properties that are not essential to the overall example. We don't want to clutter our examples with a new lambda.Function(this, 'myfn', {...}); each time we need one, so it is simpler to write declare const fn: lambda.Function and leave it to the user to provide the exact function they need if they want to use the example.

That being said, I feel like providing a real certificate is necessary to this example, since it is part of the domainName property we are demonstrating. So I do think in this situation we should provide a real certificate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I did not invent this pattern I modeled most of the work from the rest api pattern here

const api = new appsync.GraphqlApi(this, 'api', {
cgarvis marked this conversation as resolved.
Show resolved Hide resolved
name: 'myApi',
domainName: {
certificate,
domainName: 'api.mycompany.com',
},
});

// hosted zone and route53 features
declare const hostedZoneId: string;
declare const zoneName = 'mycompany.com'
cgarvis marked this conversation as resolved.
Show resolved Hide resolved

// hosten zone for adding appsync domain
const zone = HostedZone.fromHostedZoneAttributes(this, `HostedZone`, {
hostedZoneId,
zoneName,
});

// create a cname to the appsync domain. will map to something like xxxx.cloudfront.net
new CnameRecord(this, `CnameApiRecord`, {
recordName: 'api',
zone,
domainName: appsyncDomain.attrAppSyncDomainName
});
```

## Schema

Every GraphQL Api needs a schema to define the Api. CDK offers `appsync.Schema`
Expand Down
41 changes: 38 additions & 3 deletions packages/@aws-cdk/aws-appsync/lib/graphqlapi.ts
@@ -1,9 +1,10 @@
import { ICertificate } from '@aws-cdk/aws-certificatemanager';
import { IUserPool } from '@aws-cdk/aws-cognito';
import { ManagedPolicy, Role, IRole, ServicePrincipal, Grant, IGrantable } from '@aws-cdk/aws-iam';
import { IFunction } from '@aws-cdk/aws-lambda';
import { ArnFormat, CfnResource, Duration, Expiration, IResolvable, Stack } from '@aws-cdk/core';
import { Construct } from 'constructs';
import { CfnApiKey, CfnGraphQLApi, CfnGraphQLSchema } from './appsync.generated';
import { CfnApiKey, CfnGraphQLApi, CfnGraphQLSchema, CfnDomainName, CfnDomainNameApiAssociation } from './appsync.generated';
import { IGraphqlApi, GraphqlApiBase } from './graphqlapi-base';
import { Schema } from './schema';
import { IIntermediateType } from './schema-base';
Expand Down Expand Up @@ -254,6 +255,18 @@ export interface LogConfig {
readonly role?: IRole;
}

export interface DomainOptions {
/**
* The certificate to use with the domain name
*/
readonly certificate: ICertificate;

/**
* The actual domain name e.g. api.example.com
cgarvis marked this conversation as resolved.
Show resolved Hide resolved
*/
readonly domainName: string;
}

/**
* Properties for an AppSync GraphQL API
*/
Expand Down Expand Up @@ -292,6 +305,16 @@ export interface GraphqlApiProps {
* @default - false
*/
readonly xrayEnabled?: boolean;

/**
* The domain name configuration for the GraphQL API
*
* The hosted zone and CName must be configured in addition to this setting to
* enable custom domain URL
*
* @default - none a unique name is generated
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a comma or parenthesis after "none" ?

This comment was marked as outdated.

*/
readonly domainName?: DomainOptions;
cgarvis marked this conversation as resolved.
Show resolved Hide resolved
}

/**
Expand Down Expand Up @@ -391,7 +414,7 @@ export class GraphqlApi extends GraphqlApiBase {
class Import extends GraphqlApiBase {
public readonly apiId = attrs.graphqlApiId;
public readonly arn = arn;
constructor (s: Construct, i: string) {
constructor(s: Construct, i: string) {
super(s, i);
}
}
Expand Down Expand Up @@ -450,7 +473,7 @@ export class GraphqlApi extends GraphqlApiBase {
const additionalModes = props.authorizationConfig?.additionalAuthorizationModes ?? [];
const modes = [defaultMode, ...additionalModes];

this.modes = modes.map((mode) => mode.authorizationType );
this.modes = modes.map((mode) => mode.authorizationType);

this.validateAuthorizationProps(modes);

Expand All @@ -472,6 +495,18 @@ export class GraphqlApi extends GraphqlApiBase {
this.schema = props.schema ?? new Schema();
this.schemaResource = this.schema.bind(this);

if (props.domainName) {
Copy link
Contributor

Choose a reason for hiding this comment

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

In the docs for domainName you write that "The hosted zone and CName must be configured in addition to this setting to enable custom domain URL" but we do not check for that here.

Are we leaving that requirement as a deploy-time check right now? If so, we probably need to think about how we can make it a synth-time check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Again based on how the rest api works today this is a separate action. I felt like we should tell the user to do this, but if we nested this all in this construct I think it would be doing too much. I assume the rest api developers felt the same way, but of course I do not know.

new CfnDomainName(this, 'DomainName', {
domainName: props.domainName.domainName,
certificateArn: props.domainName.certificate.certificateArn,
description: `domain for ${this.name} at ${this.graphqlUrl}`,
});
new CfnDomainNameApiAssociation(this, 'DomainAssociation', {
domainName: props.domainName.domainName,
apiId: this.apiId,
});
}

if (modes.some((mode) => mode.authorizationType === AuthorizationType.API_KEY)) {
const config = modes.find((mode: AuthorizationMode) => {
return mode.authorizationType === AuthorizationType.API_KEY && mode.apiKeyConfig;
Expand Down
2 changes: 2 additions & 0 deletions packages/@aws-cdk/aws-appsync/package.json
Expand Up @@ -89,6 +89,7 @@
"jest": "^27.5.1"
},
"dependencies": {
"@aws-cdk/aws-certificatemanager": "0.0.0",
"@aws-cdk/aws-cognito": "0.0.0",
"@aws-cdk/aws-dynamodb": "0.0.0",
"@aws-cdk/aws-ec2": "0.0.0",
Expand All @@ -104,6 +105,7 @@
},
"homepage": "https://github.com/aws/aws-cdk",
"peerDependencies": {
"@aws-cdk/aws-certificatemanager": "0.0.0",
"@aws-cdk/aws-cognito": "0.0.0",
"@aws-cdk/aws-dynamodb": "0.0.0",
"@aws-cdk/aws-ec2": "0.0.0",
Expand Down
36 changes: 36 additions & 0 deletions packages/@aws-cdk/aws-appsync/test/appsync.test.ts
Expand Up @@ -3,6 +3,7 @@ import { Template } from '@aws-cdk/assertions';
import * as iam from '@aws-cdk/aws-iam';
import * as cdk from '@aws-cdk/core';
import * as appsync from '../lib';
import { Certificate } from '@aws-cdk/aws-certificatemanager';

let stack: cdk.Stack;
let api: appsync.GraphqlApi;
Expand Down Expand Up @@ -155,3 +156,38 @@ test('appsync GraphqlApi should not use custom role for CW Logs when not specifi
},
});
});

test('appsync GraphqlApi should be configured with custom domain when specified', () => {
const domainName = 'api.example.com';
// GIVEN
const certificate = new Certificate(stack, 'AcmCertificate', {
domainName,
});

// WHEN
new appsync.GraphqlApi(stack, 'api-custom-cw-logs-role', {
authorizationConfig: {},
name: 'apiWithCustomRole',
schema: appsync.Schema.fromAsset(path.join(__dirname, 'appsync.test.graphql')),
domainName: {
domainName,
certificate,
},
});

// THEN
Template.fromStack(stack).hasResourceProperties('AWS::AppSync::DomainNameApiAssociation', {
ApiId: {
"Fn::GetAtt": [
"apicustomcwlogsrole508EAC74",
"ApiId"
]
cgarvis marked this conversation as resolved.
Show resolved Hide resolved
},
DomainName: domainName,
});

Template.fromStack(stack).hasResourceProperties('AWS::AppSync::DomainName', {
CertificateArn: { "Ref": "AcmCertificate49D3B5AF" },
DomainName: domainName,
});
});