sds: Ignore Delta Removals

[keithmattix]

Commit Message: Ignore Delta SDS removals
Additional Description: Fixes #24373 and #32832
Risk Level: Low
Testing: Manual testing with the Istio scenario described in #32832. Investigating how to add a unit test
Docs Changes:
Release Notes: Delta SDS removals will no longer result in a "Missing SDS resources" error message
Platform Specific Features:

[adisuissa]

Thanks for tackling this issue!
Overall I think this is ok, but I think more tests are needed.
Specifically I think we need to ensure that the owning cluster/listener are actually removing the resource prior to receiving a response with a removed_resources,
and tests that exercise the different validation paths.

[keithmattix]

Thanks for the review @adisuissa!

Specifically I think we need to ensure that the owning cluster/listener are actually removing the resource prior to receiving a response with a removed_resources,
and tests that exercise the different validation paths.

I'm still getting up to speed on Envoy's testing approaches; is there a section of the integration tests that allow testing the dependent resource flow (specifically with SDS)?

[adisuissa]

I'm still getting up to speed on Envoy's testing approaches; is there a section of the integration tests that allow testing the dependent resource flow (specifically with SDS)?

There are a bunch of tests in https://github.com/envoyproxy/envoy/blob/main/test/integration/sds_dynamic_integration_test.cc and in https://github.com/envoyproxy/envoy/blob/main/test/integration/sds_generic_secret_integration_test.cc.
There's an xDS upstream that sends updates to the Envoy.

If you want to look at a more comprehensive test that includes many xDS types that are delivered to the Envoy, then the ads_integration_test.cc would probably be where they are, but I cannot recall if there are tests covering SDS there.

[keithmattix]

Specifically I think we need to ensure that the owning cluster/listener are actually removing the resource prior to receiving a response with a removed_resources,

I added an integration test that I think covers this; I'd appreciate feedback! I also modified the implementation so that removing a secret won't complete cluster warming.

[adisuissa]

Overall LGTM, thanks!
Left on comment on the organization of the integration test, but otherwise LGTM.

[adisuissa]

Assigning Harvey as a senior-maintainer reviewer in the domain.
/assign @htuch

[adisuissa]

LGTM, thanks!
One minor comment.

[keithmattix]

/retest

[keithmattix]

/retest

[keithmattix]

/retest

[keithmattix]

/retest

[htuch]

I think this is right - if you look at what eds.cc is doing, it just ignores the removed resources for delta. LGTM modulo comments.

[htuch]

Please expand on the comment here. I think the key idea is this is a singleton resource subscription, so in normal operation, an xDS server should not be removing the resource until the referencing cluster / listener is no longer using it.

@adisuissa why don't we structurally forbid singleton resource subscriptions from having removals in the baseline xDS infra, wondering why something like SDS needs to worry about this?

[keithmattix]

+1 to enforcing this in the generic xDS machinery. I also think more documentation around "singleton" resources would be helpful as well; in any case, I've expanded the comment

[adisuissa]

I completely agree that it should be maintained by the xDS infra, although at the moment we have per-type handlers.

Note that singleton resource subscription should not be determined by the type, but by the subscription (unfortunately at the moment it is highly dependent). For SDS, there is a request to support a collection subscription in addition to singletons.

The proposal in #24773 should address this (and other things as well).

[htuch]

LGTM, thanks!