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

fix(dig): dig not working #1246

Merged
merged 3 commits into from
Apr 3, 2024
Merged

Conversation

kishore03109
Copy link
Contributor

@kishore03109 kishore03109 commented Mar 27, 2024

Problem

We do have some checks in the backend to check prior to any site launch that there are AAAA records/CAA records present. In this case, the dig commands to check the AAAA records failed.

The reason for above is that we were using a library called node-dig-dns. This called the dig command directly at a system level. However, our docker container does not have the dig command out of the box. This resulted in the existence of AAAA records not being caught.

We are also codifying a check for CAA records and ensuring that if there exist at least one caa record and it uses our redirection service, it should have letsencrypt as one of the caa record.

To prevent accidental commits to live indirection repo during dev, also adding a check to only commit to the indirection repository iff it is in prod environment.

Solution

just use node's dns resolver directly. this way we dont have to depend on an external library's implementation of node and dont have to install unnecessary deps in the docker.

remove dep introduced in #1244

Breaking Changes

  • Yes - this PR contains breaking changes
    • Details ...
  • No - this PR is backwards compatible with ALL of the following feature flags in this doc

Features:
Screenshot 2024-03-27 at 11 21 44 PM

Manual test (not to be copied over to deployment notes)

  • add these lines of code at the end of server.js
const formResponses = [
  {
    submissionId: "",
    requesterEmail: "[email protected]",
    repoName: "kishore-test-dev-emil",
    primaryDomain: "google.com",
    redirectionDomain: "www.google.com",
    agencyEmail: "[email protected]",
  },
]

formsgSiteLaunchRouter.handleSiteLaunchResults(formResponses, "test")
  • Assert that the email comes out to

Copy link
Contributor Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @kishore03109 and the rest of your teammates on Graphite Graphite

@kishore03109 kishore03109 force-pushed the 03-27-fix_dig_dig_not_working branch 5 times, most recently from 2281bcd to f00c2db Compare April 1, 2024 05:35
@kishore03109 kishore03109 marked this pull request as ready for review April 1, 2024 05:35
@kishore03109 kishore03109 requested a review from a team April 1, 2024 05:35
@kishore03109 kishore03109 force-pushed the 03-27-fix_dig_dig_not_working branch from f00c2db to 23f90dc Compare April 1, 2024 05:39
Copy link
Contributor

@seaerchin seaerchin left a comment

Choose a reason for hiding this comment

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

since we're removing the external dependency here, should we also update the dockerfile to remove the installation of bind-tools?

Comment on lines 247 to 253
} catch (e: any) {
if (e.code && e.code === "ENODATA") {
logger.info(
`Unable to get dig response for domain: ${launchResult.primaryDomainSource}. Skipping check for AAAA records`
)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

could we change the any? separately, why are we not logging the error here? should it be info or should we set to warn?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added type guard 5e7d5e9

Comment on lines 306 to 321
} catch (e: any) {
if (e.code && e.code === "ENODATA") {
logger.info(
`Unable to get dig response for domain: ${launchResult.primaryDomainSource}. Skipping check for CAA records`
)
return null
})
}
throw e
}
Copy link
Contributor

Choose a reason for hiding this comment

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

ssee above comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

src/types/siteLaunch.ts Show resolved Hide resolved
src/routes/formsg/formsgSiteLaunch.ts Outdated Show resolved Hide resolved
@kishore03109 kishore03109 force-pushed the 03-27-fix_dig_dig_not_working branch from 61f077a to 5e7d5e9 Compare April 2, 2024 10:55
@kishore03109 kishore03109 requested a review from seaerchin April 2, 2024 10:57
Copy link
Contributor

@seaerchin seaerchin left a comment

Choose a reason for hiding this comment

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

lgtm, just accept suggestion to remove any

Comment on lines 1 to 11
export function isErrnoException(obj: any): obj is NodeJS.ErrnoException {
return (
obj &&
(typeof obj.errno === "number" || obj.errno === undefined) &&
(typeof obj.code === "string" || obj.code === undefined) &&
(typeof obj.path === "string" || obj.path === undefined) &&
(typeof obj.syscall === "string" || obj.syscall === undefined)
)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
export function isErrnoException(obj: any): obj is NodeJS.ErrnoException {
return (
obj &&
(typeof obj.errno === "number" || obj.errno === undefined) &&
(typeof obj.code === "string" || obj.code === undefined) &&
(typeof obj.path === "string" || obj.path === undefined) &&
(typeof obj.syscall === "string" || obj.syscall === undefined)
)
}
export function isErrnoException(obj: unknown): obj is NodeJS.ErrnoException {
const err = obj as NodeJS.ErrnoException
return (
err &&
(typeof err.errno === "number" || err.errno === undefined) &&
(typeof err.code === "string" || err.code === undefined) &&
(typeof err.path === "string" || err.path === undefined) &&
(typeof err.syscall === "string" || err.syscall === undefined)
)
}

@kishore03109 kishore03109 force-pushed the 03-27-fix_dig_dig_not_working branch from d4920d2 to 59db516 Compare April 3, 2024 02:05
@kishore03109 kishore03109 merged commit 3fe0734 into develop Apr 3, 2024
10 checks passed
@mergify mergify bot deleted the 03-27-fix_dig_dig_not_working branch April 3, 2024 02:08
This was referenced Apr 4, 2024
alexanderleegs added a commit that referenced this pull request Apr 4, 2024
* fix(server): server should die if unable to connect to db (#1265)

## Problem

something in me wanted to check if we indeed exit if we fail to connect to db, and the answer is... no 


[node.js](https://nodejs.org/docs/latest-v18.x/api/process.html#processexitcode_1) states that 
> A number which will be the process exit code, when the process either exits gracefully, or is exited via process.exit() without specifying a code.

so it does not actually do the exiting, which leads to silent failures

* fix(dig): dig not working (#1246)

## Problem

We do have some checks in the backend to check prior to any site launch that there are AAAA records/CAA records present. In this case, the dig commands to check the AAAA records [failed](https://ogp.datadoghq.com/logs?query=service%3Aisomer%20env%3Aproduction%20%22An%20error%20occurred%20while%20performing%20dig%20for%20domain%3A%22%20&cols=host%2Cservice&fromUser=true&index=%2A&messageDisplay=inline&refresh_mode=sliding&storage=hot&stream_sort=desc&viz=stream&from_ts=1711457521321&to_ts=1711543921321&live=true).

The reason for above is that we were using a library called `node-dig-dns`. This called the dig command [directly](https://github.com/StephanGeorg/node-dig-dns/blob/master/src/index.js#L78) at a system level. However, our docker container does not have the dig command out of the box. This resulted in the existence of AAAA records not being caught.


We are also codifying a check for CAA records and ensuring that if there exist at least one  caa record and it uses our redirection service, it should have letsencrypt as one of the caa record. 

To prevent accidental commits to live indirection repo during dev, also adding a check to only commit to the indirection repository iff it is in prod environment. 
 
## Solution

just use node's dns resolver directly. this way we dont have to depend on an external library's implementation of node and dont have to install unnecessary deps in the docker.

remove dep introduced in #1244 

**Breaking Changes**

<!-- Does this PR contain any backward incompatible changes? If so, what are they and should there be special considerations for release? -->

- [ ] Yes - this PR contains breaking changes
  - Details ...
- [x] No - this PR is backwards compatible with ALL of the following feature flags in this [doc](https://www.notion.so/opengov/Existing-feature-flags-518ad2cdc325420893a105e88c432be5)

**Features**:
<img width="1493" alt="Screenshot 2024-03-27 at 11 21 44 PM" src="https://github.com/isomerpages/isomercms-backend/assets/42832651/73edd437-492b-4bff-86c9-3392cb40fe49">


### Manual test (not to be copied over to deployment notes)

- [ ] add these lines of code at the end of server.js 
```
const formResponses = [
  {
    submissionId: "",
    requesterEmail: "[email protected]",
    repoName: "kishore-test-dev-emil",
    primaryDomain: "google.com",
    redirectionDomain: "www.google.com",
    agencyEmail: "[email protected]",
  },
]

formsgSiteLaunchRouter.handleSiteLaunchResults(formResponses, "test")
```  

- [ ] Assert that the email comes out to

* fix: remove unecessary join and site retrieval (#1268)

* Improve APM spans (no more <anonymous>) (#1267)

* refactor: rename wrapper based on original function, only when original.name exists

* feat: utility to name all methods of an object

* feat: ensure all route handlers are named

* feat: drop the bound prefix in span names

* chore: remove unused eslint rule disabling

---------

Co-authored-by: Alexander Lee <[email protected]>

* fix: external links in top level nav (#1272)

* chore: bump version to v0.76.0

---------

Co-authored-by: Timothee Groleau <[email protected]>
Co-authored-by: Kishore <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants