A36 update: edit for clarity, fix some inconsistencies #427

murgatroid99 · 2024-04-18T22:34:54Z

From my perspective working on implementing this design, there are three major parts:

How the server interacts with the xDS API.
How the server behaves as a result of the configurations from xDS.
The changes to xDS resource validation rules.

I rearranged some of the information to structure the design that way. I also made a few other changes that improve clarity, in my opinion:

Removed a couple of statements that contradict other gRFCs
Decreased focus on some potential use cases that have not yet materialized
Added a couple of callouts using GitHub markdown's recent alerts feature.
Added more subheadings to break up longer sections.

markdroth

Thanks for doing this! Overall, this looks like a good improvement to the spec; my comments are mostly minor.

Please also update the "last modified date" near the top of the file.

Please let me know if you have any questions. Thanks!

markdroth · 2024-07-18T23:03:44Z

A36-xds-for-servers.md

 XdsServer will use the normal `XdsClient` to communicate with the xDS server.
-xDS v3 support is required. xDS v2 support is optional and may have lower


It seems like a bit of revisionist history to remove discussion of xDS v2 vs. v3, since that was very much a thing when this design was written. I agree that it is no longer a thing today, but gRFCs are really documenting individual design changes relative to their start state, not trying to document the current state.

I think it's more confusing than useful to keep this in. There's no discussion in this document of how the relevant APIs differ in xDS v2 vs v3, and in what way an implementation would "have lower quality standards" as a result.

markdroth · 2024-07-18T23:51:53Z

A36-xds-for-servers.md

+> [!NOTE]
+> If an XdsServer implementation does not use RouteConfiguration or support
+> any HTTP filters other than the hard-coded Router, then `RouteConfiguration`
+> handling can be skipped.


We might want to say something here about how implementations may choose to implement support for this design in two parts, the first part being required for A29 and the second part being required for A41.

I'm not sure what would be a good way to say that.

markdroth · 2024-07-18T23:53:20Z

A36-xds-for-servers.md

+> Once other `Fitler` types are supported, the validation logic will need to
+> be expanded to accept other `Filter` types. `Filter` types are the type contained in the
+> [`typed_config`][Filter.typed_config] Any. If the type is
+> `udpa.type.v1.TypedStruct`, then its [`type_url`][TypedStruct.type_url] is used


We should also support xds.type.v3.TypedStruct.

markdroth · 2024-07-18T23:55:53Z

A36-xds-for-servers.md

-implementation, the network filters' configuration is valid, and the filter
-names are unique within the `filters` list. Additionally, the `FilterChain` is
-only valid if `filters` contains exactly one entry for the
+Resource validation rules guarantee that each entry in


This paragraph seems like it duplicates information in the "resource validation" section below. I suggest just removing it.

I think the right change here is to expand this paragraph with more information about how to process the HttpConnectionManager message. I think I meant to include that originally.

markdroth · 2024-07-18T23:59:28Z

A36-xds-for-servers.md

+
+#### Configuration Error Logging
+
+There are situations when an XdsServer can clearly tell the configuration will


@yashykt, do we actually do this logging in C-core? I don't see it from a quick glance of our code.

(This text isn't actually new in this PR; it just got moved around, which made me notice it.)

markdroth · 2024-07-19T00:01:58Z

A36-xds-for-servers.md

+ 3. If TLS is supported (see [gRFC A29: xDS TLS Security][A29]), apply the
+    corresponding TLS configuration from the `FilterChain` to the connection.
+
+When the server receives a request on an active connection, it should do the


The first two bullets below say "If RouteConfiguration is supported", and the last bullet says "If any server-side HTTP filters are supported". However, I think those basically all describe the same case, because if HTTP filters are supported, then RouteConfiguration also needs to be supported.

Given that, I suggest checking this sentence to start with "If the implementation supports HTTP filters", and then removing the conditionals from the individual bullets below.

markdroth · 2024-07-19T00:03:05Z

A36-xds-for-servers.md

+    references a NACKed resource without a previous good version, an
+    unavailable resource because of communication failures with control plane
+    or a triggered loading timeout, or a non-existent resource, fail the RPC
+    with the UNAVAILABLE status.


Do we want to say something about what information should be in the status message here? IIRC, we discussed not making this too specific for security reasons.

Same thing for the next bullet.

I added something.

markdroth · 2024-07-19T00:14:46Z

A36-xds-for-servers.md

+     - The `type_url` field must have the value
+       `envoy.extensions.filters.network.http_connection_manager.v3.HttpConnectionManager`
+     - Inside the `value` field, decoded as a `HttpConnectionManager` message:
+       - The `http_filters` field must be validated as specified in


As per my comment elsewhere, I think that these two bullets have the same conditions: if the implementation supports HTTP filters, then it must do both; if it does not, then it can do neither. So I suggest saying something like this:

The value field must contain a valid HttpConnectionManager message. If the implementation supports HTTP filters, then the following must be validated:

The http_filters field must be validated as per A39.

Either the route_config field or the rds field must be set:

If route_config is set, it must be validated according to the RouteConfiguration validation rules below.

If rds is set, an RDS watch must be started for the specified resource.

I don't think the part about starting an RDS watch belongs here, because it is not a validation rule. I think that belongs in the Listener resource handling section, and I'll make sure it's there.

Done other than that.

markdroth · 2024-07-19T00:19:07Z

A36-xds-for-servers.md

+ - Every entry of `virtual_hosts` is validated, not just the most specifically matching one.
+ - For each entry of `virtual_hosts`:
+   - For each entry of `routes`:
+     - The `action` field can now have the value `non_forwarding_action` in addition to other accepted values.


I think we should explicitly call out that this means that a parsed route can now have multiple types of actions: RouteAction (usable for clients), NonForwardingAction (usable for servers), or unknown (an action type not currently supported by gRPC). On clients, any action type other than RouteAction will cause RPCs to fail with UNAVAILABLE; on servers, any action type other than NonForwardingAction will cause RPCs to fail with UNAVAILABLE. And as per my comment above, we should also say something about what information should be included in the RPC failure status message.

I don't think the validation section is the right place for information about intended behaviors. The request handling section does cover the part about servers failing requests in that way. I'm not exactly sure where the information about the client behaviors should belong.

A36 update: edit for clarity, fix some inconsistencies

e56bf3d

murgatroid99 requested review from ejona86 and markdroth April 18, 2024 22:34

murgatroid99 added 2 commits April 23, 2024 11:10

Add a note about waiting for RDS

bce5ab6

Mention VirtualHost selection in RPC handling section

d235969

markdroth reviewed Jul 19, 2024

View reviewed changes

Address review comments

5b2f0af

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

A36 update: edit for clarity, fix some inconsistencies #427

A36 update: edit for clarity, fix some inconsistencies #427

murgatroid99 commented Apr 18, 2024

markdroth left a comment

markdroth Jul 18, 2024

murgatroid99 Jul 19, 2024

markdroth Jul 18, 2024

murgatroid99 Jul 19, 2024

markdroth Jul 18, 2024

murgatroid99 Jul 19, 2024

markdroth Jul 18, 2024

murgatroid99 Jul 19, 2024

markdroth Jul 18, 2024

markdroth Jul 19, 2024

murgatroid99 Jul 19, 2024

markdroth Jul 19, 2024

murgatroid99 Jul 19, 2024

markdroth Jul 19, 2024

murgatroid99 Jul 19, 2024

markdroth Jul 19, 2024

murgatroid99 Jul 19, 2024

		XdsServer will use the normal `XdsClient` to communicate with the xDS server.
		xDS v3 support is required. xDS v2 support is optional and may have lower


		#### Configuration Error Logging

		There are situations when an XdsServer can clearly tell the configuration will

A36 update: edit for clarity, fix some inconsistencies #427

Are you sure you want to change the base?

A36 update: edit for clarity, fix some inconsistencies #427

Conversation

murgatroid99 commented Apr 18, 2024

markdroth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment