feat: move DNS checker slackbot from lambda #1367
Conversation
| @@ -41,5 +42,26 @@ const handleWhitelistEmails: RequestHandler< | |||
| } | |||
| } | |||
|
|
|||
| const handleDnsChecker: RequestHandler< | |||
There was a problem hiding this comment.
trouble you to remove the console.log in line 22 please
support/routes/v2/isobot/index.ts
Outdated
| message: `Sorry, \`${req.body.text}\` is not a valid domain name. Please try again with a valid one instead.`, | ||
| }) | ||
|
|
||
| // We need to return 200 OK to Slack before we process the request |
There was a problem hiding this comment.
is this comment misplaced? seems like we only return a 200 after we actually to the dns calls and get a proper response.
also why are we optimising for api response times here ah? shouldn't this just be a ping to the domain name, should be at most 2-3 network calls no?
There was a problem hiding this comment.
Oh yeah the comment was for a previous iteration where the response was done later, but opted to return immediately for simplicity.
Removed in 15d5512!
| Promise.race([ | ||
| dns.resolveCname(domain), | ||
| new Promise<null>((_, reject) => | ||
| setTimeout(() => reject(null), DNS_TIMEOUT) |
There was a problem hiding this comment.
rather than null can just put something like timeout exceeded
There was a problem hiding this comment.
arh unless this was a typing thing
There was a problem hiding this comment.
Ah yeah the logger.info would have captured this.
|
|
||
| getValidatedDomain(payload: SlackPayload) { | ||
| const { text: domain } = payload | ||
| const DOMAIN_NAME_REGEX = /^[a-zA-Z0-9][a-zA-Z0-9-]{0,61}[a-zA-Z0-9](?:\.[a-zA-Z]{2,})+$/ |
There was a problem hiding this comment.
damn where did 61 come from
There was a problem hiding this comment.
Domain names have a character limit of 63 characters per part, so the 61 comes from the 63 minus the first and last character.
| : `${domain.replaceAll(".", "-")}.${DNS_INDIRECTION_DOMAIN}` | ||
|
|
||
| return ResultAsync.combine([ | ||
| okAsync(null), |
There was a problem hiding this comment.
i believe this is misplaced
There was a problem hiding this comment.
Hmm no, this one is explicit to be null because the cnameDomain does not actually exist, so we know that it is not valid.
| ]) | ||
| }) | ||
| .andThen( | ||
| ([cnameDomain, cnameRecord, indirectionDomain, indirectionRecords]) => { |
There was a problem hiding this comment.
nit: prefer obj over passing in an array so we dont mix up the elements accidently
There was a problem hiding this comment.
No strong opinions here but was thinking of structuring it this way so that each step of the checking process is separate. Alternatively doing the object method means they would be in a single ResultAsync.combine(). Possible but would prefer keeping it the current way for better understanding of the process flow, let me know your thoughts.
There was a problem hiding this comment.
Adjusted in d33c2d0!
| const isIntermediateValid = | ||
| intermediateRecords && | ||
| ((cnameRecord && | ||
| cnameRecord.endsWith(".kxcdn.com") && |
There was a problem hiding this comment.
i think we declared the suffixes in a different constants.ts file
There was a problem hiding this comment.
Hmm couldn't find this definition :(
src/constants/constants.ts
Outdated
| @@ -77,6 +77,7 @@ export const REDIRECTION_SERVER_IPS = [ | |||
| "18.139.47.66", | |||
| ] | |||
| export const DNS_INDIRECTION_DOMAIN = "hostedon.isomer.gov.sg" | |||
| export const DNS_CNAME_SUFFIXES = ["cloudfront.net", "kxcdn.com"] | |||
There was a problem hiding this comment.
sir I meant that its declared in the constants file here haha, approving this pr with nits to use the constants rather than declaring the string in line
There was a problem hiding this comment.
Ah okay, updated in 3ea7d34!
harishv7
force-pushed
the
harish/isom-1037-create-automation-for-prod-ops-to-whitelist
branch
from
11af7df to
89e4bf5
Compare
seaerchin
force-pushed
the
harish/isom-1037-create-automation-for-prod-ops-to-whitelist
branch
from
3103275 to
4e961c3
Compare
seaerchin
force-pushed
the
feat/slackbot-dnschecker
branch
from
3ea7d34 to
a404285
Compare
Problem
The DNS checker Slackbot currently lives inside a lambda.
Solution
Breaking Changes
Features:
Tests
/stagingasjklasdinside our Slack (this is connected to our staging instance)Deploy Notes
None