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(Site Launch): Mock calls to Amplify #704

Merged
merged 12 commits into from
Apr 19, 2023
Merged
Show file tree
Hide file tree
Changes from 9 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
1 change: 1 addition & 0 deletions .env-example
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ export SITE_CREATE_FORM_KEY=""
# generate your own access key and secret access key from AWS
export AWS_ACCESS_KEY_ID=""
export AWS_SECRET_ACCESS_KEY=""
export MOCK_AMPLIFY_CREATE_DOMAIN_CALLS="true"

# Required to run end-to-end tests
export E2E_TEST_REPO="e2e-test-repo"
Expand Down
6 changes: 6 additions & 0 deletions src/config/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,12 @@ const config = convict({
format: String,
default: "",
},
mockAmplifyCreateDomainAssociationCalls: {
doc: "Mock createDomainAssociation calls to Amplify ",
env: "MOCK_AMPLIFY_CREATE_DOMAIN_ASSOCIATION_CALLS",
format: "required-boolean",
default: true,
},
seaerchin marked this conversation as resolved.
Show resolved Hide resolved
},
sqs: {
incomingQueueUrl: {
Expand Down
54 changes: 30 additions & 24 deletions src/routes/formsgSiteLaunch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,7 @@ export class FormsgSiteLaunchRouter {
) || []

res.sendStatus(200) // we have received the form and obtained relevant field
await this.handleSiteLaunchResults(formResponses, submissionId)
this.handleSiteLaunchResults(formResponses, submissionId)
}

sendLaunchError = async (
Expand Down Expand Up @@ -301,33 +301,39 @@ export class FormsgSiteLaunchRouter {
formResponses: FormResponsesProps[],
submissionId: string
) {
const launchResults = await Promise.all(
formResponses.map(this.launchSiteFromForm)
)
const successResults: DnsRecordsEmailProps[] = []
launchResults.forEach((launchResult) => {
if (launchResult.isOk()) {
successResults.push(launchResult.value)
}
})
try {
const launchResults = await Promise.all(
formResponses.map(this.launchSiteFromForm)
)
seaerchin marked this conversation as resolved.
Show resolved Hide resolved
const successResults: DnsRecordsEmailProps[] = []
launchResults.forEach((launchResult) => {
if (launchResult.isOk()) {
successResults.push(launchResult.value)
}
})

await this.sendDNSRecords(submissionId, successResults)
await this.sendDNSRecords(submissionId, successResults)

const failureResults: LaunchFailureEmailProps[] = []
const failureResults: LaunchFailureEmailProps[] = []

launchResults.forEach((launchResult) => {
if (launchResult.isErr() && launchResult.error) {
failureResults.push(launchResult.error)
return
}
if (launchResult.isErr()) {
failureResults.push({
error: "Unknown error",
})
}
})
launchResults.forEach((launchResult) => {
if (launchResult.isErr() && launchResult.error) {
failureResults.push(launchResult.error)
return
}
if (launchResult.isErr()) {
failureResults.push({
error: "Unknown error",
})
}
})

await this.sendLaunchError(submissionId, failureResults)
await this.sendLaunchError(submissionId, failureResults)
} catch (e) {
logger.error(
`Something unexpected went wrong when launching sites from form submission ${submissionId}. Error: ${e}`
)
}
}

getRouter() {
Expand Down
137 changes: 129 additions & 8 deletions src/services/identity/LaunchClient.ts
Original file line number Diff line number Diff line change
@@ -1,26 +1,41 @@
import {
AmplifyClient,
CreateDomainAssociationCommand,
CreateDomainAssociationCommandInput,
CreateDomainAssociationCommandInput as AmplifySDKCreateDomainAssociationCommandInput,
CreateDomainAssociationCommandOutput,
GetDomainAssociationCommand,
GetDomainAssociationCommandInput,
GetDomainAssociationCommandInput as AmplifySDKGetDomainAssociationCommandInput,
GetDomainAssociationCommandOutput,
SubDomainSetting,
} from "@aws-sdk/client-amplify"
import { options } from "joi"
import { SubDomain } from "aws-sdk/clients/amplify"

import logger from "@root/logger/logger"
import { config } from "@config/config"

// stricter typing to interact with Amplify SDK
type CreateDomainAssociationCommandInput = {
seaerchin marked this conversation as resolved.
Show resolved Hide resolved
[K in keyof AmplifySDKCreateDomainAssociationCommandInput]: NonNullable<
AmplifySDKCreateDomainAssociationCommandInput[K]
>
}

type GetDomainAssociationCommandInput = {
[K in keyof AmplifySDKGetDomainAssociationCommandInput]: NonNullable<
AmplifySDKGetDomainAssociationCommandInput[K]
>
}

class LaunchClient {
private readonly amplifyClient: InstanceType<typeof AmplifyClient>

private readonly mockDomainAssociations: Map<string, SubDomainSetting[]>

constructor() {
const AWS_REGION = "ap-southeast-1"
this.amplifyClient = new AmplifyClient({
region: AWS_REGION,
})
this.mockDomainAssociations = new Map()
}

createDomainAssociationCommandInput = (
Expand All @@ -35,8 +50,15 @@ class LaunchClient {

sendCreateDomainAssociation = (
input: CreateDomainAssociationCommandInput
): Promise<CreateDomainAssociationCommandOutput> =>
this.amplifyClient.send(new CreateDomainAssociationCommand(input))
): Promise<CreateDomainAssociationCommandOutput> => {
if (this.shouldMockAmplifyDomainCalls()) {
return this.mockCreateDomainAssociationOutput(input)
}
const output = this.amplifyClient.send(
new CreateDomainAssociationCommand(input)
)
return output
}

createGetDomainAssociationCommandInput = (
appId: string,
Expand All @@ -48,8 +70,107 @@ class LaunchClient {

sendGetDomainAssociationCommand = (
input: GetDomainAssociationCommandInput
): Promise<GetDomainAssociationCommandOutput> =>
this.amplifyClient.send(new GetDomainAssociationCommand(input))
): Promise<GetDomainAssociationCommandOutput> => {
if (this.shouldMockAmplifyDomainCalls()) {
// handle mock input
return this.mockGetDomainAssociationOutput(input)
}
return this.amplifyClient.send(new GetDomainAssociationCommand(input))
}

/**
* The rate limit for Create Domain Association is 10 per hour.
* We want to limit interference with operations, as such we mock this call during development.
* @returns Mocked output for CreateDomainAssociationCommand
*/
mockCreateDomainAssociationOutput = (
input: CreateDomainAssociationCommandInput
): Promise<CreateDomainAssociationCommandOutput> => {
// We are mocking the response from the Amplify API, so we need to store the input
this.mockDomainAssociations.set(input.domainName, input.subDomainSettings)
const mockResponse: CreateDomainAssociationCommandOutput = {
$metadata: {
httpStatusCode: 200,
},
domainAssociation: {
autoSubDomainCreationPatterns: [],
autoSubDomainIAMRole: undefined,
certificateVerificationDNSRecord: undefined,
domainAssociationArn: `arn:aws:amplify:ap-southeast-1:11111:apps/${input.appId}/domains/${input.domainName}`,
domainName: input.domainName,
domainStatus: "CREATING",
enableAutoSubDomain: false,
statusReason: undefined,
subDomains: undefined,
},
}

const subDomainSettingsList = input.subDomainSettings
if (!subDomainSettingsList || subDomainSettingsList.length === 0) {
return Promise.resolve(mockResponse)
}

const subDomains: SubDomain[] = this.getSubDomains(subDomainSettingsList)

// We know that domainAssociation is not undefined, so we can use the non-null assertion operator
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
mockResponse.domainAssociation!.subDomains = subDomains
harishv7 marked this conversation as resolved.
Show resolved Hide resolved
return Promise.resolve(mockResponse)
}

private shouldMockAmplifyDomainCalls(): boolean {
return config.get("aws.amplify.mockAmplifyCreateDomainAssociationCalls")
}

private getSubDomains(
subDomainList: SubDomainSetting[],
isCreated = false
): SubDomain[] {
const subDomains = subDomainList
.map((subDomain) => subDomain.prefix)
harishv7 marked this conversation as resolved.
Show resolved Hide resolved
.filter((prefix) => prefix !== undefined && prefix !== null)
.map((subDomainPrefix) => ({
dnsRecord: `${subDomainPrefix} CNAME ${
isCreated ? "test.cloudfront.net" : "<pending>"
}`,
subDomainSetting: {
branchName: "master",
prefix: subDomainPrefix,
},
verified: false,
})) as SubDomain[]
seaerchin marked this conversation as resolved.
Show resolved Hide resolved
return subDomains
}

mockGetDomainAssociationOutput(
seaerchin marked this conversation as resolved.
Show resolved Hide resolved
input: GetDomainAssociationCommandInput
): Promise<GetDomainAssociationCommandOutput> {
const isSubDomainCreated = true // this is a `get` call, assume domain has already been created
const subDomainSettings = this.mockDomainAssociations.get(input.domainName)
if (!subDomainSettings) {
throw new Error(
`NotFoundException: Domain association ${input.domainName} not found.`
)
}
const subDomains = this.getSubDomains(subDomainSettings, isSubDomainCreated)

const mockResponse: GetDomainAssociationCommandOutput = {
$metadata: {
httpStatusCode: 200,
},
domainAssociation: {
certificateVerificationDNSRecord: `testcert.${input.domainName}. CNAME testcert.acm-validations.aws.`,
domainAssociationArn: `arn:aws:amplify:ap-southeast-1:11111:apps/${input.appId}/domains/${input.domainName}`,
domainName: input.domainName,
domainStatus: "PENDING_VERIFICATION",
kishore03109 marked this conversation as resolved.
Show resolved Hide resolved
enableAutoSubDomain: false,
subDomains,
statusReason: undefined,
},
}

return Promise.resolve(mockResponse)
harishv7 marked this conversation as resolved.
Show resolved Hide resolved
}
}

export default LaunchClient
37 changes: 22 additions & 15 deletions src/services/infra/InfraService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ import { SubDomainSettings } from "aws-sdk/clients/amplify"
import Joi from "joi"
import { Err, err, errAsync, Ok, ok, okAsync, Result } from "neverthrow"

import { config } from "@config/config"

import { Site } from "@database/models"
import { User } from "@database/models/User"
import {
Expand Down Expand Up @@ -226,23 +228,28 @@ export default class InfraService {
)

// Get DNS records from Amplify
/**
* note: we wait for ard 90 sec as there is a time taken
* for amplify to generate the certification manager in the first place
* This is a dirty workaround for now, and will cause issues when we integrate
* this directly within the Isomer CMS.
* todo: push this check into a queue-like system when integrating this with cms
*/
await new Promise((resolve) => setTimeout(resolve, 90000))

/**
* todo: add some level of retry logic if get domain association command
* does not contain the DNS redirections info.
*/
const isTestEnv =
config.get("env") === "test" || config.get("env") === "dev"
// since we mock values during development, we don't have to await for the dns records
if (!isTestEnv) {
/**
* note: we wait for ard 90 sec as there is a time taken
* for amplify to generate the certification manager in the first place
* This is a dirty workaround for now, and will cause issues when we integrate
* this directly within the Isomer CMS.
* todo: push this check into a queue-like system when integrating this with cms
*/
await new Promise((resolve) => setTimeout(resolve, 90000))
Copy link
Contributor

Choose a reason for hiding this comment

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

This is going to block the main thread for 90s right?

Seems like "todo: push this check into a queue-like system when integrating this with cms" is crucial for integration with CMS

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm correct me if I am wrong, but set setTimeout does not block the main thread right?

Copy link
Contributor

Choose a reason for hiding this comment

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

@kishore03109 hmm, you can do a simple test on console -

async function test() { if(true) { await new Promise((resolve) => setTimeout(resolve, 2000)) } console.log("DONE") }

"DONE" wont be printed until the promise is done. you might want to double check the actual behaviour on this

Copy link
Contributor Author

@kishore03109 kishore03109 Apr 14, 2023

Choose a reason for hiding this comment

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

Hmm wait isnt that the point? I want to only resolve this after 90 seconds to give Amplify enough time to generate the DNS results

eg for the code

async function test() {
  await new Promise((resolve) => setTimeout(resolve, 10000))
  console.log("PROMISE DONE")
}
console.log("STARTING")
test()
console.log("ENDED")

in the console log, we would see:
STARTING
ENDED
PROMISE DONE

thus the other functions in the thread will not be blocked?

Copy link
Contributor

Choose a reason for hiding this comment

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

think the confusion stems from the await - if you await in an async function:

const x = await work()
doOtherWork()
const y = x

the main thread is free to continue on until the result of the await-ed promise is used (ie, const y = x). however, because we await here, i think we'll pause execution for 90s

Copy link
Contributor

Choose a reason for hiding this comment

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

@kishore03109 if you added await on test:

console.log("STARTING")
await test()
console.log("ENDED")

it will wait for promise to be done first

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@harishv7, for ease of mind, I have made the main caller (this.handleSiteLaunchResults) function non-blocking (ie, removing the await), since we technically can run this async after returning the 200 response. However, this means that any error thrown by this will not be caught by handleSiteLaunchFormRequest. Therefore, I am wrapping a try catch around and logging the error to any unintended effects of some unexpected error. Ideally, during an error, this would return something to the user, but I think this can be revisited when we are integrating the features with the frontend!

Copy link
Contributor

Choose a reason for hiding this comment

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

i think this is non-blocking - top-most caller is handleSiteLaunchFormRequest in formsgSiteLaunch and it calls this.handleSiteLaunchResults(formResponses, submissionId) without await (which calls this method later on)

/**
* todo: add some level of retry logic if get domain association command
* does not contain the DNS redirections info.
*/
}

const dnsInfo = await this.launchesService.getDomainAssociationRecord(
primaryDomain,
appId
appId,
subDomainSettings
seaerchin marked this conversation as resolved.
Show resolved Hide resolved
)

const certificationRecord = this.parseDNSRecords(
Expand Down Expand Up @@ -368,7 +375,7 @@ export default class InfraService {
await Promise.all(
messages.map(async (message) => {
const site = await this.sitesService.getBySiteName(message.repoName)
if (!site || site.isErr()) {
if (site.isErr()) {
return
}
const isSuccess = message.status === SiteLaunchLambdaStatus.SUCCESS
Expand Down