Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ECDS deletion with Delta XDS behaves differently than other dependent resources, leading to 5xx errors #32823

Closed
howardjohn opened this issue Mar 11, 2024 · 25 comments
Labels
area/ecds area/xds bug stale stalebot believes this issue/PR has not been touched recently

Comments

@howardjohn
Copy link
Contributor

Title: ECDS deletion with Delta XDS behaves differently than other dependent resources, leading to 5xx errors

Description:
For most dependant resource types, like SDS, RDS, EDS, etc, resources cannot actually be deleted. If they are referenced by a parent resource they are retained, and if no parents reference them anymore they are removed.

ECDS behaves differently. If a removal is sent, the filter is immediately removed and NFCF errors will be returned. Sending the LDS update (without the filter referenced) before ECDS removal is not sufficient; the new listener will warm for a while, so we will get 500 errors until its done warming.

@howardjohn howardjohn added bug triage Issue requires triage labels Mar 11, 2024
@howardjohn
Copy link
Contributor Author

cc @zirain

@kyessenov
Copy link
Contributor

kyessenov commented Mar 11, 2024

ECDS is a top level resource like CDS or LDS, so it seems appropriate for the removal to be effective immediately. It is also rather heavy in Istio since it contains Wasm engines inside AFAIR, each ~50MB, far bigger than any other xDS. Third, ECDS is used for authorization, and removing "permissive Authz filter or config" should be immediate.

Can you explain at a high-level why Istio removes ECDS resources and expects the change to be not effective?

@howardjohn
Copy link
Contributor Author

Consider removal of a filter

Starting state: ECDS=[foo], LDS=[listener referencing foo]
Desired state: ECDS=[], LDS=[listener without any ECDS references]

In the control plane, we send an LDS push updating the resource, and an ECDS push with remove=foo.

Desired behavior (and behavior with SotW): the new listener has no filter, old one has filter while draining. Once drained, ECDS is removed
Behavior now: the old filter 500s while draining, new listener has no filter

@kyessenov
Copy link
Contributor

Isn't this intended with ECDS? Filter lifecycle is decoupled from the listener lifecycle. You can offer a default no-op filter which will make a removal appear as no-op?

@howardjohn
Copy link
Contributor Author

then how do I know when to eventually remove it?

One thing I am confused about is how ecds is a top level resource. it's not requested unless it's referenced afaik?

@kyessenov
Copy link
Contributor

kyessenov commented Mar 11, 2024

It's on-demand, like on-demand CDS. SotW vs Delta has nothing to do with on-demand vs on-demand, a top-level resource is about referential integrity. With ECDS, a dangling reference is effective immediately, just like CDS (cluster removal). As a safety pre-caution, you can specify a "fallback" config in ECDS which will use another config instead of the removed one, as opposed to NFCF. The deletion is effective immediately for new requests regardless of whether the fallback is specified.

@sschepens
Copy link
Contributor

Isn't it kind of weird that if LDS receives an update deleting a filter and then immediately after gets an ECDS update it applies it to the filters in the old listener? 🤔 I'm assuming that's what you're saying happens @howardjohn

Maybe a hacky solution could be to never send filter deletions to ECDS, that means, never send a DeltaDiscoveryResponse withe removed_resources for ECDS. If a filter is deleted then it should be deleted in LDS and we should eventually receive a resource_names_unsubscribe for its config.

@kyessenov
Copy link
Contributor

@sschepens that's not acceptable for authorization purposes. If a filter is added to "allow-list" that must be revertible.

@sschepens
Copy link
Contributor

sschepens commented Mar 11, 2024

@kyessenov i'm not sure I understand what you mean with "authorization purposes".

I'm thinking this:

  • create an AuthorizationPolicy
  • LDS is updated adding ext_authz filter
  • ECDS is configured with the filter config
  • delete AuthorizationPolicy
  • LDS is updated without ext_authz filter
  • ECDS response is really not necessary, what we want is to delete the filter, not change it's config

@howardjohn
Copy link
Contributor Author

Why is the semantics different from every other resource? SDS can apply authorization, for instance, via validation_context. RDS, arguably, is providing authorization in some form via what routes are specificied

@kyessenov
Copy link
Contributor

kyessenov commented Mar 11, 2024

The semantics are different because of the scalability problem with each listener owning its own independent filter chains. RDS, etc, are lingering in memory independently for each listener during drain, so you're in fact seeing copies of the routing tables at various historical snapshots. This does not scale with large filters, such as Wasm where a single instance of it is O(50MB).

ECDS is at a level above or next to LDS. This was always the case for Wasm, but done in an indirect way for Wasm, ECDS codified the principles and made it safer with the defaults. You don't have to use ECDS if it does not fit your constraints, there's definitely value in the simplicity of the independent ownership of config by listeners.

It's better to think of ECDS as an independent service, like ext_authz where a filter invocation is externalized to a different entity with its own lifecycle.

@howardjohn
Copy link
Contributor Author

Ok, given that is there a way we can achieve the basic use case of removing a filter safely (#32823 (comment) we do with other dependant resources?

@kyessenov
Copy link
Contributor

@howardjohn , yes, specify a "default filter config" that is no-op. This has nothing to do with Delta IMHO, it was always the case for ECDS.

@howardjohn
Copy link
Contributor Author

Also this seems to not happen with sotw. Is that expected?

@howardjohn
Copy link
Contributor Author

We do not want to nop though, we want to use the old config on the old listener and new config on the new listener.

Imagine if we replaced an ecds deny-all with an inline deny-all.
Replacing the old one with a NOP would bypass

@kyessenov
Copy link
Contributor

@howardjohn There should be no difference between ECDS for SotW and Delta, bar some bugs in the transport implementation. The semantics is incompatible with inline filter config so you cannot safely move between ECDS and non-ECDS.

@howardjohn
Copy link
Contributor Author

The semantics is incompatible with inline filter config so you cannot safely move between ECDS and non-ECDS.

That was not really the point I was trying to make. My assertion is that I should be able to safely transition from any [config set initial] -> [config set new]. There are a few violations of this in Envoy (such as OpenCensus being immutable, etc), but for the most part this works and is critical to how control planes operate today (and is largely the value add of Envoy! dynamic configuration).

This is broken with this ECDS behavior.

For example, imagine I want to have LDS=[filter1 ecds] then LDS=[filter2 ecds]. This is all ECDS, same problem as ecds->non-ecds.

Maybe a hacky solution could be to never send filter deletions to ECDS, that means, never send a DeltaDiscoveryResponse withe removed_resources for ECDS. If a filter is deleted then it should be deleted in LDS and we should eventually receive a resource_names_unsubscribe for its config.

We do not get a resource_names_unsubscribe in this case

@kyessenov
Copy link
Contributor

@howardjohn there are many violations to that assertion, e.g. rename of clusters is definitely not graceful in general, and cannot be done without an intermediate "union" config. I don't think it's the principle of xDS to always permit that.

The point of ECDS is to apply config in a fine-grained way that does not require a full listener drain. By definition, it requires affecting the existing listeners. You can have disjoint ECDS resources for listeners if you put listener name and versions into ECDS identifier, but then you don't have to use ECDS to begin with.

@howardjohn
Copy link
Contributor Author

You can have disjoint ECDS resources for listeners if you put listener name and versions into ECDS identifier, but then you don't have to use ECDS to begin with.

I don't think this will be viable for the WASM case since, as you mentioned, it can be O(50mb) per listener?

@nezdolik nezdolik added area/ecds and removed triage Issue requires triage labels Mar 11, 2024
@howardjohn
Copy link
Contributor Author

Behavior of other types:
SDS just ignores removes from delta. This is somewhat broken, see #32832
RDS ignores removes
On-demand CDS doesn't support SotW. Removals are only treated as a "notify this resource is not present" (

notifier_.notifyMissingCluster(resource_name);
), they cannot remove a cluster that has been recieved
LDS explicitly diffs in SotW to find removals, and uses removals from Delta directly

In ECDS, there is no deletion logic for SotW direcly in the onConfigUpdate. Even if a config is no longer pushed in the ECDS response, and its no longer referenced, it will stick around until the listeners drain.For delta, it actually deletes it, which comes with its own set of problems as discussed here.

The listeners drain aspect seems handled by DynamicFilterConfigProviderImp which actually unsubscribes from the resource at the XDS layer.

So we can actually get the SotW code in Delta, we just need to have our control plane never send removes at all, and have different behavior for how we handle removes for different types

@adisuissa
Copy link
Contributor

Thanks for reporting this issue. It is another reason why Envoy should strive towards using a unified xDS resource management layer (#24773).

My opinion here is that the type (ECDS/CDS/EDS/etc.) should not dictate what the lifetime of the resource in the resources-cache should be, but whether the subscription is for a (glob) collection (a.k.a. wildcard) or for a singleton-resource.
Resource collections can be updated in delta-xDS by populating the resources and removed_resources fields.
For singleton resource subscriptions, I think the description in the xDS-protocol implies that the removed_resources response are not necessarily required. That is because the Envoy already sends that it unsubscribes from that resource, and so it can remove the resource from the resources-cache at that moment.

Note that specifically for Envoy, there should be a difference between the the resources-cache that is updated only by the main thread following xDS-related events, and what the worker threads observe. If a listener is draining and still needs a dependent resource, it should be able to use it. IMO it is not different than having an explicit large in-line filter config and still keeping it in memory because it is referenced. The operator of that Envoy should take this into consideration when configuring draining limitations.

@kyessenov
Copy link
Contributor

@adisuissa

The semantics for ECDS is different. The config is applied per-request, not per-connection, which is why the draining listener sees the config update. So I don't think it's not about the resource model. This is about when the workers dereference the resource. We can do something simple, like snapshotting the filter config in ECDS once the listener is draining, so that the draining listener no longer sees the updates.

Speaking of draining listeners, what about SDS? I think TLS and oauth2 filter might be getting SDS updates during draining as well.

Copy link

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or "no stalebot" or other activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Apr 14, 2024
Copy link

This issue has been automatically closed because it has not had activity in the last 37 days. If this issue is still valid, please ping a maintainer and ask them to label it as "help wanted" or "no stalebot". Thank you for your contributions.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Apr 21, 2024
@howardjohn
Copy link
Contributor Author

Can we put a 'help wanted'?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ecds area/xds bug stale stalebot believes this issue/PR has not been touched recently
Projects
None yet
Development

No branches or pull requests

5 participants