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(cdk): add AppSync GraphQLSchema and pipeline resolvers as hot swappable #27197

Merged
merged 5 commits into from
Sep 28, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion packages/aws-cdk/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -421,7 +421,8 @@ Hotswapping is currently supported for the following changes
- Container asset changes of AWS ECS Services.
- Website asset changes of AWS S3 Bucket Deployments.
- Source and Environment changes of AWS CodeBuild Projects.
- VTL mapping template changes for AppSync Resolvers and Functions
- VTL mapping template changes for AppSync Resolvers and Functions.
- Schema changes for AppSync GraphQL Apis.

**⚠ Note #1**: This command deliberately introduces drift in CloudFormation stacks in order to speed up deployments.
For this reason, only use it for development purposes.
Expand Down
12 changes: 12 additions & 0 deletions packages/aws-cdk/lib/api/evaluate-cloudformation-template.ts
Original file line number Diff line number Diff line change
Expand Up @@ -413,6 +413,8 @@ const RESOURCE_TYPE_ATTRIBUTES_FORMATS: { [type: string]: { [attribute: string]:
},
'AWS::DynamoDB::Table': { Arn: stdSlashResourceArnFmt },
'AWS::AppSync::GraphQLApi': { ApiId: appsyncGraphQlApiApiIdFmt },
'AWS::AppSync::FunctionConfiguration': { FunctionId: appsyncGraphQlFunctionIDFmt },
'AWS::AppSync::DataSource': { Name: appsyncGraphQlDataSourceNameFmt },
};

function iamArnFmt(parts: ArnParts): string {
Expand Down Expand Up @@ -440,6 +442,16 @@ function appsyncGraphQlApiApiIdFmt(parts: ArnParts): string {
return parts.resourceName.split('/')[1];
}

function appsyncGraphQlFunctionIDFmt(parts: ArnParts): string {
// arn:aws:appsync:us-east-1:111111111111:apis/<apiId>/functions/<functionId>
return parts.resourceName.split('/')[3];
}

function appsyncGraphQlDataSourceNameFmt(parts: ArnParts): string {
// arn:aws:appsync:us-east-1:111111111111:apis/<apiId>/datasources/<name>
return parts.resourceName.split('/')[3];
}

interface Intrinsic {
readonly name: string;
readonly args: any;
Expand Down
1 change: 1 addition & 0 deletions packages/aws-cdk/lib/api/hotswap-deployments.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ const RESOURCE_DETECTORS: { [key:string]: HotswapDetector } = {
// AppSync
'AWS::AppSync::Resolver': isHotswappableAppSyncChange,
'AWS::AppSync::FunctionConfiguration': isHotswappableAppSyncChange,
'AWS::AppSync::GraphQLSchema': isHotswappableAppSyncChange,

'AWS::ECS::TaskDefinition': isHotswappableEcsServiceChange,
'AWS::CodeBuild::Project': isHotswappableCodeBuildProjectChange,
Expand Down
98 changes: 81 additions & 17 deletions packages/aws-cdk/lib/api/hotswap/appsync-mapping-templates.ts
Original file line number Diff line number Diff line change
@@ -1,29 +1,32 @@
import { ChangeHotswapResult, classifyChanges, HotswappableChangeCandidate, lowerCaseFirstCharacter, reportNonHotswappableChange, transformObjectKeys } from './common';
import { GetSchemaCreationStatusRequest, GetSchemaCreationStatusResponse } from 'aws-sdk/clients/appsync';
import { ChangeHotswapResult, classifyChanges, HotswappableChangeCandidate, lowerCaseFirstCharacter, transformObjectKeys } from './common';
import { ISDK } from '../aws-auth';

import { EvaluateCloudFormationTemplate } from '../evaluate-cloudformation-template';

export async function isHotswappableAppSyncChange(
logicalId: string, change: HotswappableChangeCandidate, evaluateCfnTemplate: EvaluateCloudFormationTemplate,
): Promise<ChangeHotswapResult> {
const isResolver = change.newValue.Type === 'AWS::AppSync::Resolver';
const isFunction = change.newValue.Type === 'AWS::AppSync::FunctionConfiguration';
const isGraphQLSchema = change.newValue.Type === 'AWS::AppSync::GraphQLSchema';

if (!isResolver && !isFunction) {
if (!isResolver && !isFunction && !isGraphQLSchema) {
return [];
}

const ret: ChangeHotswapResult = [];
if (isResolver && change.newValue.Properties?.Kind === 'PIPELINE') {
reportNonHotswappableChange(
ret,
change,
undefined,
'Pipeline resolvers cannot be hotswapped since they reference the FunctionId of the underlying functions, which cannot be resolved',
);
return ret;
}

comcalvi marked this conversation as resolved.
Show resolved Hide resolved
const classifiedChanges = classifyChanges(change, ['RequestMappingTemplate', 'ResponseMappingTemplate']);
const classifiedChanges = classifyChanges(change, [
'RequestMappingTemplate',
'RequestMappingTemplateS3Location',
'ResponseMappingTemplate',
'ResponseMappingTemplateS3Location',
'Code',
'CodeS3Location',
'Definition',
'DefinitionS3Location',
]);
classifiedChanges.reportNonHotswappablePropertyChanges(ret);

const namesOfHotswappableChanges = Object.keys(classifiedChanges.hotswappableProps);
Expand All @@ -49,25 +52,86 @@ export async function isHotswappableAppSyncChange(

const sdkProperties: { [name: string]: any } = {
...change.oldValue.Properties,
Definition: change.newValue.Properties?.Definition,
DefinitionS3Location: change.newValue.Properties?.DefinitionS3Location,
requestMappingTemplate: change.newValue.Properties?.RequestMappingTemplate,
requestMappingTemplateS3Location: change.newValue.Properties?.RequestMappingTemplateS3Location,
responseMappingTemplate: change.newValue.Properties?.ResponseMappingTemplate,
responseMappingTemplateS3Location: change.newValue.Properties?.ResponseMappingTemplateS3Location,
code: change.newValue.Properties?.Code,
codeS3Location: change.newValue.Properties?.CodeS3Location,
};
const evaluatedResourceProperties = await evaluateCfnTemplate.evaluateCfnExpression(sdkProperties);
const sdkRequestObject = transformObjectKeys(evaluatedResourceProperties, lowerCaseFirstCharacter);

// resolve s3 location files as SDK doesn't take in s3 location but inline code
if (sdkRequestObject.requestMappingTemplateS3Location) {
sdkRequestObject.requestMappingTemplate = (await fetchFileFromS3(sdkRequestObject.requestMappingTemplateS3Location, sdk))?.toString('utf8');
delete sdkRequestObject.requestMappingTemplateS3Location;
Copy link
Contributor

Choose a reason for hiding this comment

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

What does the user flow look like with any of the *S3Location properties? The MappingTemplate doesn't support an S3 location, and it also doesn't use it under the hood.

Even if it was supported, the user would have to change the s3 location for hotswap to be able to see it, not the object itself; diff will only detect a change in the object's location, not its contents. Users won't want to rename the object every time they make a change to it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tagging @alharris-at for more details. I believe we are using L1 CFN resource here and manually managing the requestMappingTemplateS3Location properties from our L3 data construct. (Don't know if it's the right reference https://github.com/aws-amplify/amplify-category-api/blob/c6772cfb9c0c68d6de4c04aa8190489fba0557a9/packages/graphql-transformer-core/src/util/SchemaResourceUtil.ts#L37)

In my testing, when updating high level amplify schema that results in a resolver change, I found only *S3Location properties to have changed. @alharris-at can you provide more details here?

Copy link

@alharris-at alharris-at Sep 27, 2023

Choose a reason for hiding this comment

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

Because Synth produces assets with a hash in the filename, I think the S3Location will change if a resolver or function's code is updated, and yeah we use L1 constructs here.

@comcalvi not urgent, but we might actually want to open the discussion to using S3Location instead of the inline approach for the L2 (when a file is provided), since that means customers will far more quickly hit Cfn template size limits (we see this happen for Amplify customers w/o inlining much of these assets, since with AppSync it's easy to produce a large number of resources quickly, and if we're inlining all code then it's going to expand very quickly).

Copy link
Contributor

Choose a reason for hiding this comment

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

ahh okay, so the s3 locations are managed through the CDK here. That makes sense, thanks for clarifying.

}
if (sdkRequestObject.responseMappingTemplateS3Location) {
sdkRequestObject.responseMappingTemplate = (await fetchFileFromS3(sdkRequestObject.responseMappingTemplateS3Location, sdk))?.toString('utf8');
delete sdkRequestObject.responseMappingTemplateS3Location;
}
if (sdkRequestObject.definitionS3Location) {
sdkRequestObject.definition = await fetchFileFromS3(sdkRequestObject.definitionS3Location, sdk);
delete sdkRequestObject.definitionS3Location;
}
if (sdkRequestObject.codeS3Location) {
sdkRequestObject.code = await fetchFileFromS3(sdkRequestObject.codeS3Location, sdk);
delete sdkRequestObject.codeS3Location;
}

if (isResolver) {
await sdk.appsync().updateResolver(sdkRequestObject).promise();
} else {
} else if (isFunction) {

const { functions } = await sdk.appsync().listFunctions({ apiId: sdkRequestObject.apiId }).promise();
const { functionId } = functions?.find(fn => fn.name === physicalName) ?? {};
await sdk.appsync().updateFunction({
...sdkRequestObject,
functionId: functionId!,
}).promise();
await simpleRetry(
() => sdk.appsync().updateFunction({ ...sdkRequestObject, functionId: functionId! }).promise(),
3,
'ConcurrentModificationException');
} else {
Comment on lines +91 to +94
Copy link
Contributor

@comcalvi comcalvi Sep 20, 2023

Choose a reason for hiding this comment

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

This is a good workaround for the orchestration / resource dependency problem, nice work

let schemaCreationResponse: GetSchemaCreationStatusResponse = await sdk.appsync().startSchemaCreation(sdkRequestObject).promise();
while (schemaCreationResponse.status && ['PROCESSING', 'DELETING'].some(status => status === schemaCreationResponse.status)) {
await sleep(1000); // poll every second
const getSchemaCreationStatusRequest: GetSchemaCreationStatusRequest = {
apiId: sdkRequestObject.apiId,
};
schemaCreationResponse = await sdk.appsync().getSchemaCreationStatus(getSchemaCreationStatusRequest).promise();
}
if (schemaCreationResponse.status === 'FAILED') {
throw new Error(schemaCreationResponse.details);
}
}
},
});
}

return ret;
}

async function fetchFileFromS3(s3Url: string, sdk: ISDK) {
const s3PathParts = s3Url.split('/');
const s3Bucket = s3PathParts[2]; // first two are "s3:" and "" due to s3://
const s3Key = s3PathParts.splice(3).join('/'); // after removing first three we reconstruct the key
return (await sdk.s3().getObject({ Bucket: s3Bucket, Key: s3Key }).promise()).Body;
}

async function simpleRetry(fn: () => Promise<any>, numOfRetries: number, errorCodeToRetry: string) {
try {
await fn();
} catch (error: any) {
if (error && error.code === errorCodeToRetry && numOfRetries > 0) {
await sleep(500); // wait half a second
await simpleRetry(fn, numOfRetries - 1, errorCodeToRetry);
} else {
throw error;
}
}
}

async function sleep(ms: number) {
return new Promise(ok => setTimeout(ok, ms));
}
Loading
Loading