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

Update filter chain creation for listeners #11540

Merged
merged 1 commit into from
Nov 9, 2021
Merged

Conversation

freddygv
Copy link
Contributor

@freddygv freddygv commented Nov 9, 2021

Backport of #11245


The duo of makeUpstreamFilterChainForDiscoveryChain and makeListenerForDiscoveryChain were really hard to reason about, and led to concealing a bug in their branching logic. There were several issues here:

  • They tried to accomplish too much: determining filter name, cluster name, and whether RDS should be used.
  • They embedded logic to handle significantly different kinds of upstream listeners (passthrough, prepared query, typical services, and catch-all)
  • They needed to coalesce different data sources (Upstream and CompiledDiscoveryChain)

Rather than handling all of those tasks inside of these functions, this PR pulls out the RDS/clusterName/filterName logic.

This refactor also fixed a bug with the handling of UpstreamDefaults. These defaults get stored as UpstreamConfig in the proxy snapshot with a DestinationName of "*", since they apply to all upstreams. However, this wildcard destination name must not be used when creating the name of the associated upstream cluster. The coalescing logic in the original functions here was in some situations creating clusters with a *. prefix, which is not a valid destination.

@github-actions github-actions bot added the theme/envoy/xds Related to Envoy support label Nov 9, 2021
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging November 9, 2021 22:06 Inactive
@vercel vercel bot temporarily deployed to Preview – consul November 9, 2021 22:07 Inactive
Copy link
Member

@rboyer rboyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Compared this patch vs the main patch and they were effectively the same minus the file relocations and partition removal.

@freddygv freddygv merged commit ec87f4f into release/1.10.x Nov 9, 2021
@freddygv freddygv deleted the backport-11245 branch November 9, 2021 23:25
@hc-github-team-consul-core
Copy link
Collaborator

🍒 If backport labels were added before merging, cherry-picking will start automatically.

To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/498678.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme/envoy/xds Related to Envoy support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants