feat(api, worker, application-generic): Add exhaustive error handling for bridge requests #6715
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,8 +4,19 @@ import { | |
| NotFoundException, | ||
| BadRequestException, | ||
| HttpException, | ||
| GatewayTimeoutException, | ||
| } from '@nestjs/common'; | ||
| import got, { | ||
|
There was a problem hiding this comment. In the era of fetch, do we need to rely on There was a problem hiding this comment. We will tackle this offline. There was a problem hiding this comment. Elaborating a little, is the thinking that we should handle retries elsewhere? At the queue layer, for example? I tend to agree, the value we're getting from retries in practice is minimal, especially considering the total retry duration after incorporating backoff is less than 1 minute. It's unlikely an external bridge endpoint would recover from a total network fault within that timespan. |
||
| CacheError, | ||
| HTTPError, | ||
| MaxRedirectsError, | ||
| OptionsOfTextResponseBody, | ||
| ReadError, | ||
| RequestError, | ||
| TimeoutError, | ||
| UnsupportedProtocolError, | ||
| UploadError, | ||
| } from 'got'; | ||
| import { createHmac } from 'node:crypto'; | ||
|
|
||
| import { | ||
|
|
@@ -33,14 +44,15 @@ export const RETRYABLE_HTTP_CODES: number[] = [ | |
| 408, 413, 429, 500, 502, 503, 504, 521, 522, 524, | ||
| ]; | ||
| const RETRYABLE_ERROR_CODES: string[] = [ | ||
| 'EAI_AGAIN', // DNS resolution failed, retry | ||
| 'ECONNREFUSED', // Connection refused by the server | ||
| 'ECONNRESET', // Connection was forcibly closed by a peer | ||
| 'EADDRINUSE', // Address already in use | ||
| 'EPIPE', // Broken pipe | ||
| 'ETIMEDOUT', // Operation timed out | ||
| 'ENOTFOUND', // DNS lookup failed | ||
| 'EHOSTUNREACH', // No route to host | ||
| 'ENETUNREACH', // Network is unreachable | ||
| ]; | ||
|
|
||
| const LOG_CONTEXT = 'ExecuteBridgeRequest'; | ||
|
|
@@ -76,11 +88,6 @@ export class ExecuteBridgeRequest { | |
| ); | ||
| } | ||
|
|
||
|
There was a problem hiding this comment. Refactoring many of these to discrete methods to make the |
||
| const bridgeUrl = this.getBridgeUrl( | ||
| environment.bridge?.url || environment.echo?.url, | ||
| command.environmentId, | ||
|
|
@@ -125,25 +132,13 @@ export class ExecuteBridgeRequest { | |
| }, | ||
| }; | ||
|
|
||
| const request = [PostActionEnum.EXECUTE, PostActionEnum.PREVIEW].includes( | ||
| command.action as PostActionEnum, | ||
| ) | ||
| ? got.post | ||
| : got.get; | ||
|
|
||
| const headers = await this.buildRequestHeaders(command); | ||
|
|
||
| Logger.log(`Making bridge request to \`${url}\``, LOG_CONTEXT); | ||
| try { | ||
|
|
@@ -152,75 +147,38 @@ export class ExecuteBridgeRequest { | |
| headers, | ||
| }).json(); | ||
| } catch (error) { | ||
|
There was a problem hiding this comment. Moved to a discrete handler for errors. |
||
| this.handleResponseError(error, url); | ||
| } | ||
| } | ||
|
|
||
| private async buildRequestHeaders(command: ExecuteBridgeRequestCommand) { | ||
| const novuSignatureHeader = await this.buildRequestSignature(command); | ||
|
|
||
| return { | ||
| [HttpRequestHeaderKeysEnum.BYPASS_TUNNEL_REMINDER]: 'true', | ||
| [HttpRequestHeaderKeysEnum.CONTENT_TYPE]: 'application/json', | ||
| [HttpHeaderKeysEnum.NOVU_SIGNATURE_DEPRECATED]: novuSignatureHeader, | ||
| [HttpHeaderKeysEnum.NOVU_SIGNATURE]: novuSignatureHeader, | ||
| }; | ||
| } | ||
|
|
||
| private async buildRequestSignature(command: ExecuteBridgeRequestCommand) { | ||
| const secretKey = await this.getDecryptedSecretKey.execute( | ||
| GetDecryptedSecretKeyCommand.create({ | ||
| environmentId: command.environmentId, | ||
| }), | ||
| ); | ||
|
|
||
| const timestamp = Date.now(); | ||
| const novuSignatureHeader = `t=${timestamp},v1=${this.createHmacBySecretKey( | ||
| secretKey, | ||
| timestamp, | ||
| command.event || {}, | ||
| )}`; | ||
|
|
||
| return novuSignatureHeader; | ||
| } | ||
|
|
||
| private createHmacBySecretKey( | ||
| secretKey: string, | ||
| timestamp: number, | ||
|
|
@@ -256,8 +214,19 @@ export class ExecuteBridgeRequest { | |
| switch (workflowOrigin) { | ||
| case WorkflowOriginEnum.NOVU_CLOUD: | ||
| return `${this.getApiUrl()}/v1/environments/${environmentId}/bridge`; | ||
| case WorkflowOriginEnum.EXTERNAL: { | ||
| if (!environmentBridgeUrl) { | ||
| throw new BadRequestException({ | ||
| code: BRIDGE_EXECUTION_ERROR.INVALID_BRIDGE_URL.code, | ||
| message: | ||
| BRIDGE_EXECUTION_ERROR.INVALID_BRIDGE_URL.message( | ||
| environmentBridgeUrl, | ||
| ), | ||
| }); | ||
| } | ||
|
There was a problem hiding this comment. Throw an exception here explicitly if trying to request to an external Bridge URL when it doesn't exist on the environment. |
||
|
|
||
| return environmentBridgeUrl; | ||
| } | ||
| default: | ||
| throw new Error(`Unsupported workflow origin: ${workflowOrigin}`); | ||
| } | ||
|
|
@@ -272,4 +241,135 @@ export class ExecuteBridgeRequest { | |
|
|
||
| return apiUrl; | ||
| } | ||
|
|
||
| private handleResponseError(error: unknown, url: string) { | ||
|
There was a problem hiding this comment. Exhaustive error handling added for all of:
|
||
| if (error instanceof RequestError) { | ||
| let body: Record<string, unknown>; | ||
| try { | ||
| body = JSON.parse(error.response.body as string); | ||
| } catch (e) { | ||
| // If the body is not valid JSON, we'll just use an empty object. | ||
| body = {}; | ||
| } | ||
|
|
||
| if ( | ||
| error instanceof HTTPError && | ||
| Object.values(ErrorCodeEnum).includes(body.code as ErrorCodeEnum) | ||
| ) { | ||
| // Handle known Bridge errors. Propagate the error code and message. | ||
| throw new HttpException(body, error.response.statusCode); | ||
| } else if (body.code === TUNNEL_ERROR_CODE) { | ||
rifont marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| // Handle known tunnel errors | ||
| const tunnelBody = body as TunnelResponseError; | ||
| Logger.error( | ||
| `Could not establish tunnel connection for \`${url}\`. Error: \`${tunnelBody.message}\``, | ||
| LOG_CONTEXT, | ||
| ); | ||
| throw new NotFoundException({ | ||
| message: BRIDGE_EXECUTION_ERROR.TUNNEL_NOT_FOUND.message(url), | ||
| code: BRIDGE_EXECUTION_ERROR.TUNNEL_NOT_FOUND.code, | ||
| }); | ||
| } else if (error instanceof TimeoutError) { | ||
| Logger.error(`Bridge request timeout for \`${url}\``, LOG_CONTEXT); | ||
| throw new GatewayTimeoutException({ | ||
| message: BRIDGE_EXECUTION_ERROR.BRIDGE_REQUEST_TIMEOUT.message(url), | ||
| code: BRIDGE_EXECUTION_ERROR.BRIDGE_REQUEST_TIMEOUT.code, | ||
| }); | ||
| } else if (error instanceof UnsupportedProtocolError) { | ||
| Logger.error(`Unsupported protocol for \`${url}\``, LOG_CONTEXT); | ||
| throw new BadRequestException({ | ||
| message: BRIDGE_EXECUTION_ERROR.UNSUPPORTED_PROTOCOL.message(url), | ||
| code: BRIDGE_EXECUTION_ERROR.UNSUPPORTED_PROTOCOL.code, | ||
| }); | ||
| } else if (error instanceof ReadError) { | ||
| Logger.error( | ||
| `Response body could not be read for \`${url}\``, | ||
| LOG_CONTEXT, | ||
| ); | ||
| throw new BadRequestException({ | ||
| message: BRIDGE_EXECUTION_ERROR.RESPONSE_READ_ERROR.message(url), | ||
| code: BRIDGE_EXECUTION_ERROR.RESPONSE_READ_ERROR.code, | ||
| }); | ||
| } else if (error instanceof UploadError) { | ||
| Logger.error( | ||
| `Error uploading request body for \`${url}\``, | ||
| LOG_CONTEXT, | ||
| ); | ||
| throw new BadRequestException({ | ||
| message: BRIDGE_EXECUTION_ERROR.REQUEST_UPLOAD_ERROR.message(url), | ||
| code: BRIDGE_EXECUTION_ERROR.REQUEST_UPLOAD_ERROR.code, | ||
| }); | ||
| } else if (error instanceof CacheError) { | ||
| Logger.error(`Error caching request for \`${url}\``, LOG_CONTEXT); | ||
| throw new BadRequestException({ | ||
| message: BRIDGE_EXECUTION_ERROR.REQUEST_CACHE_ERROR.message(url), | ||
| code: BRIDGE_EXECUTION_ERROR.REQUEST_CACHE_ERROR.code, | ||
| }); | ||
| } else if (error instanceof MaxRedirectsError) { | ||
| Logger.error(`Maximum redirects exceeded for \`${url}\``, LOG_CONTEXT); | ||
| throw new BadRequestException({ | ||
| message: | ||
| BRIDGE_EXECUTION_ERROR.MAXIMUM_REDIRECTS_EXCEEDED.message(url), | ||
| code: BRIDGE_EXECUTION_ERROR.MAXIMUM_REDIRECTS_EXCEEDED.code, | ||
| }); | ||
| } else if (error.response?.statusCode === 502) { | ||
| /* | ||
| * Tunnel was live, but the Bridge endpoint was down. | ||
| * 502 is thrown by the tunnel service when the Bridge endpoint is not reachable. | ||
| */ | ||
| Logger.error( | ||
| `Local Bridge endpoint not found for \`${url}\``, | ||
| LOG_CONTEXT, | ||
| ); | ||
| throw new BadRequestException({ | ||
| message: | ||
| BRIDGE_EXECUTION_ERROR.BRIDGE_ENDPOINT_NOT_FOUND.message(url), | ||
| code: BRIDGE_EXECUTION_ERROR.BRIDGE_ENDPOINT_NOT_FOUND.code, | ||
| }); | ||
| } else if ( | ||
| error.response?.statusCode === 404 || | ||
| RETRYABLE_ERROR_CODES.includes(error.code) | ||
| ) { | ||
| Logger.error(`Bridge endpoint unavailable for \`${url}\``, LOG_CONTEXT); | ||
|
|
||
| let codeToThrow: string; | ||
| if (RETRYABLE_ERROR_CODES.includes(error.code)) { | ||
| codeToThrow = error.code; | ||
| } else { | ||
| codeToThrow = BRIDGE_EXECUTION_ERROR.BRIDGE_ENDPOINT_UNAVAILABLE.code; | ||
| } | ||
| throw new NotFoundException({ | ||
| message: | ||
| BRIDGE_EXECUTION_ERROR.BRIDGE_ENDPOINT_UNAVAILABLE.message(url), | ||
| code: codeToThrow, | ||
| }); | ||
| } else if (error.response?.statusCode === 405) { | ||
| Logger.error( | ||
| `Bridge endpoint method not configured for \`${url}\``, | ||
| LOG_CONTEXT, | ||
| ); | ||
| throw new BadRequestException({ | ||
| message: | ||
| BRIDGE_EXECUTION_ERROR.BRIDGE_METHOD_NOT_CONFIGURED.message(url), | ||
| code: BRIDGE_EXECUTION_ERROR.BRIDGE_METHOD_NOT_CONFIGURED.code, | ||
| }); | ||
| } else { | ||
| Logger.error( | ||
| `Unknown bridge request error calling \`${url}\`: \`${JSON.stringify( | ||
| body, | ||
| )}\``, | ||
| error, | ||
| LOG_CONTEXT, | ||
| ); | ||
| throw error; | ||
| } | ||
| } else { | ||
| Logger.error( | ||
| `Unknown bridge non-request error calling \`${url}\``, | ||
| error, | ||
| LOG_CONTEXT, | ||
| ); | ||
| throw error; | ||
| } | ||
| } | ||
| } | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simplifying the error thrown here.
Let non-HTTP exceptions bubble up to the API exception handler.