Service config parse failures should be UNAVAILABLE #9346
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
INVALID_ARGUMENT is propagated to the data plane if no previous config is available. INVALID_ARGUMENT is reserved for application use; LBs should pretty much use UNAVAILABLE exclusively. While most of the changes are in xds, there do not appear to be likely xds code paths that would propagate a bad status to the data plane. Internal policies either don't use parseLoadBalancingPolicyConfig() and instead have their configuration objects constructed directly or are constructed transitively through the cluster manager which uses INTERNAL if there's a child failure. There was a worrisome hole before this commit for StatusRuntimeExceptions received by the cluster manager, but the audit didn't find any locations throwing such an exception. User-selected policies produce a NACK and are protected from the existing xds client watcher paths. The worst that appears could happen is the channel could panic (which uses INTERNAL) if a bug let a bad configuration through.
ejona86
added
the
TODO:backport
temawi
approved these changes
Jul 8, 2022
ejona86
removed
the
TODO:backport
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
INVALID_ARGUMENT is propagated to the data plane if no previous config
is available. INVALID_ARGUMENT is reserved for application use; LBs
should pretty much use UNAVAILABLE exclusively.
While most of the changes are in xds, there do not appear to be likely
xds code paths that would propagate a bad status to the data plane.
Internal policies either don't use parseLoadBalancingPolicyConfig() and
instead have their configuration objects constructed directly or are
constructed transitively through the cluster manager which uses INTERNAL
if there's a child failure. There was a worrisome hole before this
commit for StatusRuntimeExceptions received by the cluster manager, but
the audit didn't find any locations throwing such an exception.
User-selected policies produce a NACK and are protected from the
existing xds client watcher paths. The worst that appears could happen
is the channel could panic (which uses INTERNAL) if a bug let a bad
configuration through.