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

Transparent proxy support for cross-partition #11738

Merged
merged 5 commits into from
Dec 4, 2021
Merged

Transparent proxy support for cross-partition #11738

merged 5 commits into from
Dec 4, 2021

Conversation

freddygv
Copy link
Contributor

@freddygv freddygv commented Dec 4, 2021

There are a few changes here:

  • Transparent proxy listener configuration needs to account for the new tagged address with virtual IPs generated by Consul. These should be considered both for cross-partition and partition-local traffic. The plain "virtual" tagged address should only be used for partition-local traffic, since the address is not guaranteed to be unique across partitions.
  • The existing way of computing upstreams/downstreams from intentions was based on watching the "services" table and then evaluating a service's intentions against the unique service names. This won't work in 1.11 because the services table does not have a wildcard partition index, and cross-partition upstreams from intentions are supported. This watch was replaced with a watch of a table that stores service names by kind across all partitions.

This table purposefully does not index by partition/namespace. It's a
global view into all service names.

This table is intended to replace the current serviceListTxn watch in
intentionTopologyTxn. For cross-partition transparent proxying we need
to be able to calculate upstreams from intentions in any partition. This
means that the existing serviceListTxn function is insufficient since
it's scoped to a partition.

Moving away from that function is also beneficial because it watches the
main "services" table, so watchers will wake up when any instance is
registered or deregistered.
Given that we do not allow wildcard partitions in intentions, no one ixn
can override the DefaultAllow setting. Only the default ACL policy
applies across all partitions.
@freddygv freddygv requested a review from a team December 4, 2021 00:30
@github-actions github-actions bot added the theme/envoy/xds Related to Envoy support label Dec 4, 2021
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging December 4, 2021 00:31 Inactive
@vercel vercel bot temporarily deployed to Preview – consul December 4, 2021 00:31 Inactive
Copy link
Contributor

@markan markan left a comment

Choose a reason for hiding this comment

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

This looks pretty reasonable to me. One question, but shouldn't block merge.

if len(uniqueAddrs) > 1 {
s.Logger.Warn("detected multiple virtual IPs for an upstream, all will be used to match traffic",
"upstream", id)
if len(uniqueAddrs) > 2 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this now > 2 ? Is this because there's a virtual IP and a tagged virtual IP?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep one is populated by consul-k8s and one by Consul, so if we detect more than two it's unexpected

@freddygv freddygv merged commit f24a206 into main Dec 4, 2021
@freddygv freddygv deleted the ap/tproxy branch December 4, 2021 16:50
@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/517827.

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.

4 participants