From a0baf7c99ab37ed651a576921776a98b91284656 Mon Sep 17 00:00:00 2001 From: Michael Lumish Date: Wed, 4 Aug 2021 11:54:21 -0700 Subject: [PATCH] Fix bugs and add tracing --- packages/grpc-js-xds/scripts/xds.sh | 2 +- packages/grpc-js-xds/src/http-filter.ts | 30 ++++++++++++++++--- .../src/http-filter/fault-injection-filter.ts | 28 ++++++++++++----- .../src/xds-stream-state/lds-state.ts | 11 +++++-- 4 files changed, 56 insertions(+), 15 deletions(-) diff --git a/packages/grpc-js-xds/scripts/xds.sh b/packages/grpc-js-xds/scripts/xds.sh index f845abfb..8d94ae19 100755 --- a/packages/grpc-js-xds/scripts/xds.sh +++ b/packages/grpc-js-xds/scripts/xds.sh @@ -48,7 +48,7 @@ git clone -b master --single-branch --depth=1 https://github.com/grpc/grpc.git grpc/tools/run_tests/helper_scripts/prep_xds.sh -GRPC_NODE_TRACE=xds_client,xds_resolver,cds_balancer,eds_balancer,priority,weighted_target,round_robin,resolving_load_balancer,subchannel,keepalive,dns_resolver \ +GRPC_NODE_TRACE=xds_client,xds_resolver,cds_balancer,eds_balancer,priority,weighted_target,round_robin,resolving_load_balancer,subchannel,keepalive,dns_resolver,fault_injection,http_filter \ GRPC_NODE_VERBOSITY=DEBUG \ NODE_XDS_INTEROP_VERBOSITY=1 \ GRPC_XDS_EXPERIMENTAL_FAULT_INJECTION=1 \ diff --git a/packages/grpc-js-xds/src/http-filter.ts b/packages/grpc-js-xds/src/http-filter.ts index e7ea3295..a00cde5d 100644 --- a/packages/grpc-js-xds/src/http-filter.ts +++ b/packages/grpc-js-xds/src/http-filter.ts @@ -16,7 +16,7 @@ // This is a non-public, unstable API, but it's very convenient import { loadProtosWithOptionsSync } from '@grpc/proto-loader/build/src/util'; -import { experimental } from '@grpc/grpc-js'; +import { experimental, logVerbosity } from '@grpc/grpc-js'; import { Any__Output } from './generated/google/protobuf/Any'; import Filter = experimental.Filter; import FilterFactory = experimental.FilterFactory; @@ -24,6 +24,12 @@ import { TypedStruct__Output } from './generated/udpa/type/v1/TypedStruct'; import { FilterConfig__Output } from './generated/envoy/config/route/v3/FilterConfig'; import { HttpFilter__Output } from './generated/envoy/extensions/filters/network/http_connection_manager/v3/HttpFilter'; +const TRACER_NAME = 'http_filter'; + +function trace(text: string): void { + experimental.trace(logVerbosity.DEBUG, TRACER_NAME, text); +} + const TYPED_STRUCT_URL = 'type.googleapis.com/udpa.type.v1.TypedStruct'; const TYPED_STRUCT_NAME = 'udpa.type.v1.TypedStruct'; @@ -37,7 +43,8 @@ const resourceRoot = loadProtosWithOptionsSync([ includeDirs: [ // Paths are relative to src/build __dirname + '/../../deps/udpa/', - __dirname + '/../../deps/envoy-api/' + __dirname + '/../../deps/envoy-api/', + __dirname + '/../../deps/protoc-gen-validate/' ], } ); @@ -60,6 +67,7 @@ export interface HttpFilterRegistryEntry { const FILTER_REGISTRY = new Map(); export function registerHttpFilter(typeName: string, entry: HttpFilterRegistryEntry) { + trace('Registered filter with type URL ' + typeName); FILTER_REGISTRY.set(typeName, entry); } @@ -71,7 +79,8 @@ const toObjectOptions = { } function parseAnyMessage(message: Any__Output): MessageType | null { - const messageType = resourceRoot.lookup(message.type_url); + const typeName = message.type_url.substring(message.type_url.lastIndexOf('/') + 1); + const messageType = resourceRoot.lookup(typeName); if (messageType) { const decodedMessage = (messageType as any).decode(message.value); return decodedMessage.$type.toObject(decodedMessage, toObjectOptions) as MessageType; @@ -80,7 +89,7 @@ function parseAnyMessage(message: Any__Output): MessageType | null } } -function getTopLevelFilterUrl(encodedConfig: Any__Output): string { +export function getTopLevelFilterUrl(encodedConfig: Any__Output): string { let typeUrl: string; if (encodedConfig.type_url === TYPED_STRUCT_URL) { const typedStruct = parseAnyMessage(encodedConfig) @@ -96,6 +105,7 @@ function getTopLevelFilterUrl(encodedConfig: Any__Output): string { export function validateTopLevelFilter(httpFilter: HttpFilter__Output): boolean { if (!httpFilter.typed_config) { + trace(httpFilter.name + ' validation failed: typed_config unset'); return false; } const encodedConfig = httpFilter.typed_config; @@ -103,16 +113,21 @@ export function validateTopLevelFilter(httpFilter: HttpFilter__Output): boolean try { typeUrl = getTopLevelFilterUrl(encodedConfig); } catch (e) { + trace(httpFilter.name + ' validation failed with error ' + e.message); return false; } const registryEntry = FILTER_REGISTRY.get(typeUrl); if (registryEntry) { const parsedConfig = registryEntry.parseTopLevelFilterConfig(encodedConfig); + if (parsedConfig === null) { + trace(httpFilter.name + ' validation failed: config parsing failed'); + } return parsedConfig !== null; } else { if (httpFilter.is_optional) { return true; } else { + trace(httpFilter.name + ' validation failed: filter is not optional and registry does not contain type URL ' + typeUrl); return false; } } @@ -129,9 +144,11 @@ export function validateOverrideFilter(encodedConfig: Any__Output): boolean { if (filterConfig.config) { realConfig = filterConfig.config; } else { + trace('Override filter validation failed: FilterConfig config field is empty'); return false; } } else { + trace('Override filter validation failed: failed to parse FilterConfig message'); return false; } } else { @@ -142,6 +159,7 @@ export function validateOverrideFilter(encodedConfig: Any__Output): boolean { if (typedStruct) { typeUrl = typedStruct.type_url; } else { + trace('Override filter validation failed: failed to parse TypedStruct message'); return false; } } else { @@ -150,11 +168,15 @@ export function validateOverrideFilter(encodedConfig: Any__Output): boolean { const registryEntry = FILTER_REGISTRY.get(typeUrl); if (registryEntry) { const parsedConfig = registryEntry.parseOverrideFilterConfig(encodedConfig); + if (parsedConfig === null) { + trace('Override filter validation failed: config parsing failed. Type URL: ' + typeUrl); + } return parsedConfig !== null; } else { if (isOptional) { return true; } else { + trace('Override filter validation failed: filter is not optional and registry does not contain type URL ' + typeUrl); return false; } } diff --git a/packages/grpc-js-xds/src/http-filter/fault-injection-filter.ts b/packages/grpc-js-xds/src/http-filter/fault-injection-filter.ts index 825b7c05..e0ee658c 100644 --- a/packages/grpc-js-xds/src/http-filter/fault-injection-filter.ts +++ b/packages/grpc-js-xds/src/http-filter/fault-injection-filter.ts @@ -16,7 +16,7 @@ // This is a non-public, unstable API, but it's very convenient import { loadProtosWithOptionsSync } from '@grpc/proto-loader/build/src/util'; -import { experimental, Metadata, status } from '@grpc/grpc-js'; +import { experimental, logVerbosity, Metadata, status } from '@grpc/grpc-js'; import { Any__Output } from '../generated/google/protobuf/Any'; import Filter = experimental.Filter; import FilterFactory = experimental.FilterFactory; @@ -27,14 +27,20 @@ import { HTTPFault__Output } from '../generated/envoy/extensions/filters/http/fa import { envoyFractionToFraction, Fraction } from '../fraction'; import { Duration__Output } from '../generated/google/protobuf/Duration'; +const TRACER_NAME = 'fault_injection'; + +function trace(text: string): void { + experimental.trace(logVerbosity.DEBUG, TRACER_NAME, text); +} + const resourceRoot = loadProtosWithOptionsSync([ - 'envoy/extendsion/filtesr/http/fault/v3/fault.proto'], { + 'envoy/extensions/filters/http/fault/v3/fault.proto'], { keepCase: true, includeDirs: [ - // Paths are relative to src/build - __dirname + '/../../deps/udpa/', - __dirname + '/../../deps/envoy-api/', - __dirname + '/../../deps/protoc-gen-validate/' + // Paths are relative to src/build/http-filter + __dirname + '/../../../deps/udpa/', + __dirname + '/../../../deps/envoy-api/', + __dirname + '/../../../deps/protoc-gen-validate/' ], } ); @@ -82,7 +88,8 @@ const toObjectOptions = { } function parseAnyMessage(message: Any__Output): MessageType | null { - const messageType = resourceRoot.lookup(message.type_url); + const typeName = message.type_url.substring(message.type_url.lastIndexOf('/') + 1); + const messageType = resourceRoot.lookup(typeName); if (messageType) { const decodedMessage = (messageType as any).decode(message.value); return decodedMessage.$type.toObject(decodedMessage, toObjectOptions) as MessageType; @@ -111,12 +118,15 @@ function httpCodeToGrpcStatus(code: number): status { function parseHTTPFaultConfig(encodedConfig: Any__Output): FaultInjectionFilterConfig | null { if (encodedConfig.type_url !== FAULT_INJECTION_FILTER_URL) { + trace('Config parsing failed: unexpected type URL: ' + encodedConfig.type_url); return null; } const parsedMessage = parseAnyMessage(encodedConfig); if (parsedMessage === null) { + trace('Config parsing failed: failed to parse HTTPFault message'); return null; } + trace('Parsing HTTPFault message ' + JSON.stringify(parsedMessage, undefined, 2)); const result: FaultInjectionConfig = { delay: null, abort: null, @@ -125,6 +135,7 @@ function parseHTTPFaultConfig(encodedConfig: Any__Output): FaultInjectionFilterC // Parse delay field if (parsedMessage.delay !== null) { if (parsedMessage.delay.percentage === null) { + trace('Config parsing failed: delay.percentage unset'); return null; } const percentage = envoyFractionToFraction(parsedMessage.delay.percentage); @@ -143,6 +154,7 @@ function parseHTTPFaultConfig(encodedConfig: Any__Output): FaultInjectionFilterC }; break; default: + trace('Config parsing failed: delay.fault_delay_secifier has unexpected value ' + parsedMessage.delay.fault_delay_secifier); // Should not be possible return null; } @@ -150,6 +162,7 @@ function parseHTTPFaultConfig(encodedConfig: Any__Output): FaultInjectionFilterC // Parse abort field if (parsedMessage.abort !== null) { if (parsedMessage.abort.percentage === null) { + trace('Config parsing failed: abort.percentage unset'); return null; } const percentage = envoyFractionToFraction(parsedMessage.abort.percentage); @@ -175,6 +188,7 @@ function parseHTTPFaultConfig(encodedConfig: Any__Output): FaultInjectionFilterC }; break; default: + trace('Config parsing failed: abort.error_type has unexpected value ' + parsedMessage.abort.error_type); // Should not be possible return null; } diff --git a/packages/grpc-js-xds/src/xds-stream-state/lds-state.ts b/packages/grpc-js-xds/src/xds-stream-state/lds-state.ts index 3efc64b9..8ab6ed9d 100644 --- a/packages/grpc-js-xds/src/xds-stream-state/lds-state.ts +++ b/packages/grpc-js-xds/src/xds-stream-state/lds-state.ts @@ -22,7 +22,7 @@ import { RdsState } from "./rds-state"; import { Watcher, XdsStreamState } from "./xds-stream-state"; import { HttpConnectionManager__Output } from '../generated/envoy/extensions/filters/network/http_connection_manager/v3/HttpConnectionManager'; import { decodeSingleResource, HTTP_CONNECTION_MANGER_TYPE_URL_V2, HTTP_CONNECTION_MANGER_TYPE_URL_V3 } from '../resources'; -import { validateTopLevelFilter } from '../http-filter'; +import { getTopLevelFilterUrl, validateTopLevelFilter } from '../http-filter'; import { EXPERIMENTAL_FAULT_INJECTION } from '../environment'; const TRACER_NAME = 'xds_client'; @@ -108,20 +108,25 @@ export class LdsState implements XdsStreamState { const filterNames = new Set(); for (const [index, httpFilter] of httpConnectionManager.http_filters.entries()) { if (filterNames.has(httpFilter.name)) { + trace('LDS response validation failed: duplicate HTTP filter name ' + httpFilter.name); return false; } filterNames.add(httpFilter.name); if (!validateTopLevelFilter(httpFilter)) { + trace('LDS response validation failed: ' + httpFilter.name + ' filter validation failed'); return false; } /* Validate that the last filter, and only the last filter, is the * router filter. */ + const filterUrl = getTopLevelFilterUrl(httpFilter.typed_config!) if (index < httpConnectionManager.http_filters.length - 1) { - if (httpFilter.name === ROUTER_FILTER_URL) { + if (filterUrl === ROUTER_FILTER_URL) { + trace('LDS response validation failed: router filter is before end of list'); return false; } } else { - if (httpFilter.name !== ROUTER_FILTER_URL) { + if (filterUrl !== ROUTER_FILTER_URL) { + trace('LDS response validation failed: final filter is ' + filterUrl); return false; } }