xds: parse HttpFault filter from LDS/RDS response by dapengzhang0 · Pull Request #7841 · grpc/grpc-java

dapengzhang0 · 2021-01-25T17:37:15Z

Parse HTTPFault from 4 possible places:

HttpFilters in HttpConnectionManager in LDS response
- EnvoyProtoData.HttpFault will be a field of LdsUpdate
TypedPerFilterConfigMap in VirtualHost
- EnvoyProtoData.HttpFault will be a field of EnvoyProtoData.VirtualHost
TypedPerFilterConfigMap in Route
- EnvoyProtoData.HttpFault will be a field of EnvoyProtoData.Route
TypedPerFilterConfigMap in ClusterWeight
- EnvoyProtoData.HttpFault will be a field of EnvoyProtoData.ClusterWeight

voidzcy · 2021-01-25T20:01:25Z

  static final String TRANSPORT_SOCKET_NAME_TLS = "envoy.transport_sockets.tls";
+  static final String HTTP_FAULT_FILTER_NAME = "envoy.fault";
+  @VisibleForTesting
+  static boolean enableFaultInjection =


Why define the env var here? You can alway parse configurations regardless the feature is turned on or not. It should be used to gate the feature implementation (in XdsNameResolver).

I have thought about this and felt EnvoyProtoData is a better location than XdsNameResolver. Because it's also awkward to define the flag in XdsNameResolver:

either EnvoyProtoData will be depending on XdsNameResolver

or XdsNameResolver can not gate the feature because if the env var is false but EnvoyProtoData parsing the fault proto returns an error, xds will fail because of the error.

or pass a flag in the args of StructOrError<Route> EnvoyProtoData.fromEnvoyProtoRoute()

I'm also considering to group all env variables to a class XdsEnv, WDYT?

I don't think we should couple response parsing with specific features. We should just parse whatever data we want from response messages based on our development stage, but not based on what features we are releasing. A invalid configuration is always invalid. I believe most importantly we are using env var to gate our implementation codepath, because we have not tested that and it could be buggy/wrong. We do not want code to execute branches implementing the feature. Ideally, it should gate both name resolver's code and response parsing code. But, the parsing code is more mechanical and less critical, plus we aren't applying any of our own feature-specific logic to the parsing. The fault inject configurations are either absent or being valid.

group all env variables to a class XdsEnv

That sounds a horrible idea, at least for Java (like our existing XdsLbPolicies, it's horrible Java). Those env vars aren't related to other, we shouldn't just dump everything together just because we want easy references.

Parse proto regardless of feature flag now.

voidzcy · 2021-01-25T20:19:04Z


+  static StructOrError<HttpFault> decodeFaultFilterConfig(Any rawFaultFilterConfig) {
+    if (rawFaultFilterConfig.getTypeUrl().equals(
+        "type.googleapis.com/envoy.config.filter.http.fault.v2.HTTPFault")) {


nit: define constants for these type urls.

Well, this is style preference 😛. If the constant is used only one place in the main code then I don't favor static constant as this is more straightforward.

There is little to do with being used once or multiple times.

This string is long. Also more importantly, this is something coming from the xDS API definition, instead of something that the implementer comes up with. So declaring it as a constant on the top is much readable and understandable for readers.

I disagree. Declaring it as a constant adds an extra indirection and it's less readable than put it inline. An inline constant is still a constant. There is little to do with the string being long or short, unless it has too many lines.

It makes your code more maintainable since it's easier to see what can be changed at a later time (if necessary).

If you read posts like this and this. Most people would vote for declaring a String constant on the top.

Still, I am not in favor of having these two String literals used here. It looks so messy. We've been doing the conventional way in XdsClient. This is really bad.

voidzcy · 2021-01-25T20:38:15Z

+            hasFaultInjection = true;
+            if (httpFilter.hasTypedConfig()) {
+              StructOrError<HttpFault> httpFaultOrError =
+                  decodeFaultFilterConfig(httpFilter.getTypedConfig());


nit: do not statically import methods. Also, shouldn't this be EnvoyProtoData.HttpFault. fromEnvoyProtoHttpFault()? EnvoyProtoData should not expose any static method for other external classes to use, as in the future we may want to move each individual data type into its own file.

Refactored to EnvoyProtoData.HttpFault. decodeFaultFilterConfig().

voidzcy

Ah, this is indeed very cumbersome... But as always, the more feature added, the more cumbersome...

Still reserve my comment on the typeUrl String literals.

dapengzhang0 added 2 commits January 25, 2021 09:27

xds: parse HttpFault filter from LDS/RDS response

a192aa4

add tests

5e8c3e1

dapengzhang0 requested a review from voidzcy January 25, 2021 17:39

voidzcy reviewed Jan 25, 2021

View reviewed changes

dapengzhang0 added 5 commits January 27, 2021 11:03

add missing fields maxActiveFaults etc

502e0a3

use Status.fromCodeValue

d91cb37

refactor decodeFaultFilterConfig

35121ca

use explicit boolean for headerDelay and headerAbort

f8a26e9

parse regardless of feature flag

537477d

voidzcy approved these changes Jan 28, 2021

View reviewed changes

dapengzhang0 merged commit a6df2b2 into grpc:master Jan 28, 2021

dapengzhang0 deleted the fault-inject branch January 28, 2021 04:45

github-actions Bot locked as resolved and limited conversation to collaborators Jun 3, 2021

Conversation

dapengzhang0 commented Jan 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dapengzhang0 Jan 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

voidzcy Jan 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

voidzcy left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dapengzhang0 commented Jan 25, 2021 •

edited

Loading

dapengzhang0 Jan 27, 2021 •

edited

Loading

voidzcy Jan 25, 2021 •

edited

Loading

voidzcy left a comment •

edited

Loading