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: handle and retry DNS errors #5326

Merged
merged 4 commits into from
Nov 1, 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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions core/src/analytics/analytics.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import { Profile } from "../util/profiling"
import { ModuleConfig } from "../config/module"
import { UserResult } from "@garden-io/platform-api-types"
import { uuidv4 } from "../util/random"
import { GardenError, NodeJSErrnoErrorCodes, StackTraceMetadata } from "../exceptions"
import { GardenError, NodeJSErrnoException, StackTraceMetadata } from "../exceptions"
import { ActionConfigMap } from "../actions/types"
import { actionKinds } from "../actions/types"
import { getResultErrorProperties } from "./helpers"
Expand Down Expand Up @@ -163,7 +163,7 @@ export type AnalyticsGardenErrorDetail = {
/**
* If this error was caused by an underlying NodeJSErrnoException, this will be the code.
*/
code?: NodeJSErrnoErrorCodes
code?: NodeJSErrnoException["code"]

stackTrace?: StackTraceMetadata
}
Expand Down
47 changes: 38 additions & 9 deletions core/src/exceptions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,19 +14,48 @@ import stripAnsi from "strip-ansi"
import { Cycle } from "./graph/common"
import indentString from "indent-string"
import { constants } from "os"
import dns from "node:dns"

// Unfortunately, NodeJS does not provide a list of all error codes, so we have to maintain this list manually.
// See https://nodejs.org/docs/latest-v18.x/api/dns.html#error-codes
const dnsErrorCodes = [
dns.NODATA,
dns.FORMERR,
dns.SERVFAIL,
dns.NOTFOUND,
dns.NOTIMP,
dns.REFUSED,
dns.BADQUERY,
dns.BADNAME,
dns.BADFAMILY,
dns.BADRESP,
dns.CONNREFUSED,
dns.TIMEOUT,
dns.EOF,
dns.FILE,
dns.NOMEM,
dns.DESTRUCTION,
dns.BADSTR,
dns.BADFLAGS,
dns.NONAME,
dns.BADHINTS,
dns.NOTINITIALIZED,
dns.LOADIPHLPAPI,
dns.ADDRGETNETWORKPARAMS,
dns.CANCELLED,
]

// See https://nodejs.org/api/os.html#error-constants
type NodeJSErrnoErrors = typeof constants.errno
export type NodeJSErrnoErrorCodes = keyof NodeJSErrnoErrors
const errnoErrors = Object.keys(constants.errno)

const errnoErrorCodeSet = new Set(Object.keys(constants.errno))
const errnoErrorCodeSet = new Set([errnoErrors, dnsErrorCodes].flat())

/**
* NodeJS native errors with a code property.
*/
export type NodeJSErrnoException = NodeJS.ErrnoException & {
code: NodeJSErrnoErrorCodes
errno: number
// Unfortunately we can't make this type more concrete, as DNS error codes are not known at typescript compile time.
code: string
}

export type EAddrInUseException = NodeJSErrnoException & {
Expand All @@ -37,7 +66,7 @@ export type EAddrInUseException = NodeJSErrnoException & {
}

export function isErrnoException(err: any): err is NodeJSErrnoException {
return typeof err.code === "string" && typeof err.errno === "number" && errnoErrorCodeSet.has(err.code)
return typeof err.code === "string" && errnoErrorCodeSet.has(err.code)
}

export function isEAddrInUseException(err: any): err is EAddrInUseException {
Expand All @@ -64,7 +93,7 @@ export interface GardenErrorParams {
*/
readonly taskType?: string

readonly code?: NodeJSErrnoErrorCodes
readonly code?: NodeJSErrnoException["code"]
}

export abstract class GardenError extends Error {
Expand All @@ -81,7 +110,7 @@ export abstract class GardenError extends Error {
/**
* If there was an underlying NodeJSErrnoException, the error code
*/
public code?: NodeJSErrnoErrorCodes
public code?: NodeJSErrnoException["code"]

public wrappedErrors?: GardenError[]

Expand Down Expand Up @@ -354,7 +383,7 @@ export class InternalError extends GardenError {
static wrapError(error: Error | string | any, prefix?: string): InternalError {
let message: string | undefined
let stack: string | undefined
let code: NodeJSErrnoErrorCodes | undefined
let code: NodeJSErrnoException["code"] | undefined

if (isErrnoException(error)) {
message = error.message
Expand Down
9 changes: 6 additions & 3 deletions core/src/plugins/kubernetes/retry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,9 @@ import { dedent, deline } from "../../util/string"
import { LogLevel } from "../../logger/logger"
import { KubernetesError } from "./api"
import requestErrors = require("request-promise/errors")
import { InternalError, NodeJSErrnoErrorCodes, isErrnoException } from "../../exceptions"
import { InternalError, NodeJSErrnoException, isErrnoException } from "../../exceptions"
import { ErrorEvent } from "ws"
import dns from "node:dns"

/**
* The flag {@code forceRetry} can be used to avoid {@link shouldRetry} helper call in case if the error code
Expand Down Expand Up @@ -93,7 +94,7 @@ export function toKubernetesError(err: unknown, context: string): KubernetesErro
let response: KubernetesClientHttpError["response"] | undefined
let body: any | undefined
let responseStatusCode: number | undefined
let code: NodeJSErrnoErrorCodes | undefined
let code: NodeJSErrnoException["code"] | undefined

if (err instanceof KubernetesClientHttpError) {
errorType = "HttpError"
Expand Down Expand Up @@ -159,7 +160,7 @@ export function toKubernetesError(err: unknown, context: string): KubernetesErro
export function shouldRetry(error: unknown, context: string): boolean {
const err = toKubernetesError(error, context)

if (err.code === "ECONNRESET") {
if (err.code && errorCodesForRetry.includes(err.code)) {
return true
}

Expand All @@ -185,6 +186,8 @@ export const statusCodesForRetry: number[] = [
524, // A Timeout Occurred
]

const errorCodesForRetry: NodeJSErrnoException["code"][] = ["ECONNRESET", dns.NOTFOUND]

const errorMessageRegexesForRetry = [
/ETIMEDOUT/,
/ENOTFOUND/,
Expand Down
45 changes: 45 additions & 0 deletions core/test/unit/src/exceptions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,54 @@ import {
RuntimeError,
StackTraceMetadata,
getStackTraceMetadata,
isErrnoException,
} from "../../../src/exceptions"
import dedent from "dedent"
import { testFlags } from "../../../src/util/util"
import { readFile } from "fs-extra"
import { resolve4 } from "dns/promises"
import dns from "node:dns"

describe("isErrnoException", async () => {
it("should return true for file not found errors", async () => {
let err: unknown

try {
await readFile("non-existent-file")
expect.fail("should have thrown")
} catch (e) {
err = e
}

if (isErrnoException(err)) {
expect(err.code).to.equal("ENOENT")
} else {
expect.fail("should have been an NodeJSErrnoException")
}
})

it("should return true for DNS ENOTFOUND errors", async () => {
let err: unknown

try {
await resolve4("non-existent-hostname")
expect.fail("should have thrown")
} catch (e) {
err = e
}

if (isErrnoException(err)) {
expect(err.code).to.equal(dns.NOTFOUND)
} else {
expect.fail("should have been an NodeJSErrnoException")
}
})

it("should return false for other errors", () => {
const err = new Error("test exception")
expect(isErrnoException(err)).to.be.false
})
})

describe("GardenError", () => {
// helper to avoid dealing with changing line numbers
Expand Down