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

Properly support adding/removing resources and getting updates from them #6

Closed
wants to merge 1 commit into from

Conversation

valerian-roche
Copy link
Owner

This PR is handling envoyproxy#583
This issue is impacting both sotw and delta with linear cache

Multiple aspects here:

  • delta with linear cache does not currently implement ack/nack. It should be feasible reusing the new streamState attributes here quite easily
  • sotw with LinearCache does handle two cases very differently
    • wildcard: will always send all resources as long as the version doesn't match. This is anyway mandatory given how envoy implements cds/lds. I don't have a clear use case for this (the wildcard feature of cds makes it hardly usable with a linear cache). This might change if Simple cache is reimplemented as LinearCaches per id
    • specified resources: only send changed resources. This is mainly used for EDS and sending all endpoints at once each time can put quite a lot of pressure on envoy. This PR keeps this behavior

In the end, this PR includes:

  • for delta + linear, ensures an unsubscribed resource is removed from knownResources, so when subscribed again it will be sent in the response, fixing the issue
  • for simple cache (delta and sotw): use the common GetResourceVersions instead of GetKnownResourceNames to be compatible with both sotw and delta. Those two distinct attributes representing the same thing is a pain to maintain. Also returns the resourceNames
  • for sotw + linear:
    • fixes a bug when the request passed with the response was not the actual request. It was actually used in the server code, and was fully wrong
    • fixes a potentially critical bug when a watch refers to multiple resources and updates are sent for only single resources one by one. The watch was only deleted from the updated resource, leaking it in other resources. When those end up updated later on they will queue in the channel which is no longer listened to. As it is buffered by 1 it would accept the first update then deadlock further updates
    • updated to return resourceNames in the response to properly keep track of what was returned to the user (which may not be what was returned)
    • properly consider the resources added even if the version isn't changed (which is how envoy seems to process it), and whether it was subscribed to before or not
  • sotw in general:
    • replaces the parallel lastResponse part with staged resources within the streamState. This will also be applicable for delta to properly support ACK/NACK mechanism

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant