rds: use http filters as part of identifier of rds config provider

[wbpcode]

Commit Message: rds: use http filters as part of identifier of rds subcription
Additional Description:

When we are creating a rds subcription, the rds configration will be used as an unique identifier of the subscription or route config provider.
If there is an exist subcription that has same identifier, we will re-use it directly. By this way, different listeners/HCMs could share same route config provider/subcription.

However, it should be noticed that the http_filters (the optional filter set) of HCM is necessary to create the ConfigImpl instance from the proto route config.

It means the route config providers/subcriptions will depend on the HCM's http_filters.

However, in the previous implementation, only the rds configration is used as identifier. It means that two HCMs may share same subscription and route config instance even they have different http_filters. It should be wrong.

This patch make the http_filters of HCM as part of the identifier of route config provider/subcription.

Risk Level: Low.
Testing: Added.
Docs Changes: n/a.
Release Notes: n/a.

[wbpcode]

/wait fix test and ci

[wbpcode]

cc @markdroth @adisuissa I think this PR is related to #15025. By this patch, only listeners that have same http filters and rds config will share same subscription.

But I have no much background knowledge about caching xDS client layer, so i am not sure whether this patch will break something?

[adisuissa]

When we are creating a rds subcription, the rds configration will be used as an unique identifier of the subscription or route config provider. If there is an exist subcription that has same identifier, we will re-use it directly. By this way, different listeners/HCMs could share same route config provider/subcription.

The rds configuration should be the only identifier to be used to identify a route.
There may be a problem in the usage pattern of the provider. The provider should return a const shared pointer, and all worker threads (and multiple HCMs) can use the same object.

However, in the previous implementation, only the rds configration is used as identifier. It means that two HCMs may share same subscription and route config instance even they have different http_filters. It should be wrong.

Can you explain what do you mean here?

[wbpcode]

Can you explain what do you mean here?

When we creating the ConfigImpl instance from proto config, the optional_http_filters is necessary and it's come from the http_filters of HCMs.

In the current implementation, the Listener/HCM A and Listener/HCM B may have same rds configuration and complete different http_filters configurations.

In this scene and current impl, the ConfigImpl instance may still be shared by Listener/HCM A and Listener/HCM B because the same rds configuration. Random one optional_http_filters of A/B will be used to create the instance from proto config.

However, I think the correct way should be creating a ConfigImpl instance for listener/HCM A based on the proto config and A's optional_http_filters, creating another ConfigImpl instance for istener/HCM B based on the proto config and B's optional_http_filters.

[markdroth]

For discussion of the intended semantics of the xDS transport protocol caching layer, see #13009. @adisuissa is working on implementing that.

I think that once we have that caching layer, this PR will not be necessary. The RDS subscription will be uniquely identified by the RDS resource name alone (not including any state from the Listener such as the list of HTTP filters), and any Listener that uses that RDS resource name will share the same subscription.

This is why I reopened #15025: The solution of plumbing the set of filters from the Listener into the RDS subscription is inherently broken, because the client will have only one subscription to that RDS resource, and it needs to validate that resource without knowing or caring which Listener is going to use it.

[wbpcode]

@markdroth in the current implementation, the rds subscriptions have been identified by the rds configuration and could be shared by multiple listeners/hcms. Will the cache layer change something? Maybe I need to check the background context for more information. 🙂

---

One of the reasons I choosed this way to solve the problem of current implementation is I want make the http_filters available to the route table. Then we can do more optimizations to the http filter chain at configuration loading time. For example, configurations merging in loading time that proposed by @ravenblackx

🤔

[markdroth]

The main requirement of the caching layer is that the RDS subscription be uniquely identified only by the RDS resource name. No other information can be part of that key, because if it is, that means we wind up needing multiple subscriptions to the same resource, which we can't do without creating a duplicate stream to the management server.

If the current code uniquely identifies the RDS subscription by the RDS resource name, then the caching layer will not change anything. But if this PR is trying to change the code to identify the RDS subscription by both the RDS resource name and the list of HTTP filters, then that change is incompatible with the direction we're heading in for the caching layer. In other words, I don't think we want to proceed with this PR.

Fundamentally, I don't think it's possible to make the HTTP filters available to the route table subscription, because with the caching layer, there will be a single RDS subscription no matter how many Listeners are using that RDS resource, and each of those Listeners may have a different set of HTTP filters. To say that another way, this change is basically incompatible with the xDS transport protocol semantics.

[kyessenov]

@wbpcode I had a similar issue with ECDS because it is per-server but used per-listener. We need a distinction between the subscription and a route provider. The provider uses a subscription but it is contextualized by the point-of-use in the HCM, while the subscription is shared and global. I think it implies RDS needs to be kept verbatim until after the context is available.

@markdroth Is this an implicit requirement for the caching layer? I can't think of another way to keep a resource in Envoy without a context apart from the verbatim proto stored. That would add some memory overhead because the runtime "processed" xDS is generally smaller and faster.

[wbpcode]

Fundamentally, I don't think it's possible to make the HTTP filters available to the route table subscription, because with the caching layer, there will be a single RDS subscription no matter how many Listeners are using that RDS resource, and each of those Listeners may have a different set of HTTP filters. To say that another way, this change is basically incompatible with the xDS transport protocol semantics.

I think we can split the subcription (which will output the proto config) and config provider (which will output the parsed config, ConfigImpl instance for RDS). The subcription could shared globally and used the resource name as the identifier.
And the config provider will contains more context about the users of the config (for example, http_filters of HCMs) and could identified by resource name or some other flag. Multiple config providers may shared same subcription if they have same resource name.

By this way, I think we will not violate the xDS transport protocol semantics becase the API is the core of xDS but the configuration loading is not?

cc @markdroth

[wbpcode]

I had a similar issue with ECDS because it is per-server but used per-listener. We need a distinction between the subscription and a route provider. The provider uses a subscription but it is contextualized by the point-of-use in the HCM, while the subscription is shared and global. I think it implies RDS needs to be kept verbatim until after the context is available.

I think split the subscription and config provider is a reasonable solution to this issue. The subcription could be shared by anyone the subcribe the resource. And the config provider should container more context about the config users then we can do more optimization here.

In the the implementation of rds, the subcription and config provider are bound together.

[wbpcode]

I readed the code of ECDS, seems it's a good example. If you agree that split subcription and config provider is acceptable, I can create a PR to do this for RDS. And after that PR is done, we can go back to proceed with this PR. cc @markdroth

[markdroth]

I'm not actually that familiar with how Envoy's xDS code is currently laid out, but I suspect that most of this will wind up changing as part of the work that @adisuissa is doing to implement a caching transport protocol interface. For details, see the design he's posted on #24773. So keep in mind that whatever changes you make here may be purely short-term ones.

From the transport protocol perspective, I think the key requirements are:

• Any given xDS resource should be subscribed to only once on the wire.
• Once the server sends a resource that the client is subscribed to, the server is not required to resend that resource unless the resource changes.
• This means that if another data plane component subscribes to a resource that the client is already subscribed to on the wire (e.g., if Envoy creates a new cluster instance due to a CDS update and the new cluster instance wants to subscribe to the existing EDS resource), the client must provide that resource from its cache instead of expecting the server to resend it (which is the problem discussed in xds: Consistency while handling CDS updates and EDS #13009).
• Because there can be multiple data plane components subscribing to a single resource (e.g., multiple Listeners using the same RDS resource), we cannot use the context from the data plane component when determining whether to ACK or NACK a given resource; the ACK/NACK must be driven purely from validating the individual resource in isolation, without any knowledge of the context in which it will be used. (There can still be cases where there are problems that are visible only when the resources are combined, but we don't currently have a good general-purpose mechanism for reporting such problems to the control plane. I've been thinking about that problem in the background for a while now; see Support xDS server subscription to client resources #11396 for some related discussion.)

I'm not sure what the exact interface is in the current code between the subscription and the config provider, so I don't think I'm the right person to answer whether that will be sufficient to solve the problem here. But ultimately, I think as long as the behavior meets the above requirements, it will probably be fine.

[wbpcode]

cc @markdroth Thanks for you patient and detailed explanation. I think most requirements will be adhered. However, the validating the individual resource in isolation may be more difficult to achieve term.

Take the ECDS as an example, the shared resource is validated by all subscribers (in other words, the resource is validated by every config users). And if there a new subscriber, the new subscriber will validate the last config again. An accepted resource by other subscribers my be not valid for new subscriber in some extreme cases. But this will not effect the ACK/NACK, so I am not sure if it violet the requirement.

Take the CDS as another example, if one cluster is failed to pass the validation, a NACK will be returned. But in fact, only one cluster is rejected and all other clusters in the discovery response have been accepted. If I treat the entire discovery response as a resource, it is partially accepted. If we treat each cluster as a resource, we don't have a way to respond to each cluster with an ACK/NACK.

[wbpcode]

If #24773 cannot land in the short term, I will refer the current implementation of ECDS to proceed this work and require the review of @adisuissa to ensure any change will not be incompatible with the proposal of unified xds manager.

[wbpcode]

Changed my goal priorities. I will try to land the feature of FilterConfig first. 🤠

[wbpcode]

/wait

[markdroth]

cc @markdroth Thanks for you patient and detailed explanation. I think most requirements will be adhered. However, the validating the individual resource in isolation may be more difficult to achieve term.

Take the ECDS as an example, the shared resource is validated by all subscribers (in other words, the resource is validated by every config users). And if there a new subscriber, the new subscriber will validate the last config again. An accepted resource by other subscribers my be not valid for new subscriber in some extreme cases. But this will not effect the ACK/NACK, so I am not sure if it violet the requirement.

As long as that per-subscriber validation does not impact the ACK/NACK response, I think this is fine. This is exactly the kind of case I was referring to in my last comment when I said "cases where there are problems that are visible only when the resources are combined".

The "validating the individual resource in isolation" requirement refers only to validation that impacts the ACK/NACK response and the updating of the cache.

Also, it's worth noting that once #24773 lands, ECDS may not be necessary as a separate layer of indirection, because it will be easy for any Envoy component to subscribe to any resource of an arbitrary type, which means that each filter can directly subscribe to a resource of its own config type if it wants.

Take the CDS as another example, if one cluster is failed to pass the validation, a NACK will be returned. But in fact, only one cluster is rejected and all other clusters in the discovery response have been accepted. If I treat the entire discovery response as a resource, it is partially accepted. If we treat each cluster as a resource, we don't have a way to respond to each cluster with an ACK/NACK.

I think this is actually a separate problem: this isn't about what validation we do to determine whether a resource should be ACKed or NACKed, it's about a limitation in the transport protocol in that ACK/NACK responses are per-response instead of per-resource.

I think there's general agreement that this is a problem, and there's been discussion about adding a mechanism to do a per-resource NACK (see #5739). However, it may make sense to wait on fixing that until we can come up with a more general solution for #11396, since that would likely address both this problem and the "cases where there are problems that are visible only when the resources are combined" problem.

Note that in the current transport protocol, the proper behavior is to validate each resource independently and NACK the response if any one resource failed validation. However, even when NACKing a response, the client may still use any resource from the response that passed validation. See discussion in #5739 and https://github.com/grpc/proposal/blob/master/A46-xds-nack-semantics-improvement.md.

[github-actions]

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

[github-actions]

This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!