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

[NET-5772] Make tcp external service registered on terminating gw reachable from peered cluster #19881

Merged
merged 5 commits into from
Apr 3, 2024

Conversation

nathancoleman
Copy link
Member

@nathancoleman nathancoleman commented Dec 8, 2023

Note

This is a followup to #18959 which fixed the same problem specifically when using http protocol.

Description

The terminating gateways needs to be able to handle TCP connections from peers which are not TLS-terminated at the local mesh gateway. This amounts to including the target SNI that downstreams from peers would use when building the TLS context for the terminating gateway.

Filter chain match before this change
filter_chain_match: {
  server_names: [
    "destination.default.dc2.internal.b81d6ee2-8454-768e-0a06-38a62620f76a.consul"
  ]
}
Filter chain match after this change
filter_chain_match: {
  server_names: [
    "destination.default.dc2.internal.b81d6ee2-8454-768e-0a06-38a62620f76a.consul",
    "destination.default.default.dc1.external.b81d6ee2-8454-768e-0a06-38a62620f76a.consul"
  ]
}

Testing & Reproduction steps

You can test this fix using the setup here, changing the <ServiceDefaults>.spec.protocol as necessary in resources/dc2/external-service.yaml if you want to see the behavior for http services as well.

You will need to pin a build of this branch as global.image in values-dc2.yaml. I would recommend doing an install without this build to witness things in their current broken state, and then doing a helm upgrade with global.image set.

Links

PR Checklist

  • updated test coverage
  • external facing docs updated
  • appropriate backport labels added
  • not a security concern

@nathancoleman nathancoleman added theme/terminating-gw Track terminating gateway work theme/cluster-peering Related to Consul's cluster peering feature labels Dec 8, 2023
@github-actions github-actions bot added the theme/envoy/xds Related to Envoy support label Dec 8, 2023
@nathancoleman
Copy link
Member Author

nathancoleman commented Dec 12, 2023

@erichaberkorn @jm96441n this still needs tests, a changelog entry, etc.; however, I'd appreciate it if you guys could sanity check my change here. I've verified that it works for tcp services and will verify that everything is what we expect with http services shortly.

@@ -1188,29 +1190,9 @@ func createDownstreamTransportSocketForConnectTLS(cfgSnap *proxycfg.ConfigSnapsh
}

// Inject peering trust bundles if this service is exported to peered clusters.
if len(peerBundles) > 0 {
Copy link
Member Author

Choose a reason for hiding this comment

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

This logic gets moved out to a re-usable function below

@jm96441n
Copy link
Member

approach overall LGTM, do you have an existing setup to run/test it out locally that I could steal to do some testing as well?

@nathancoleman nathancoleman added backport-inactive/1.16 This release series is no longer active backport-inactive/1.17 This release series is no longer active. Use backport/ent/1.17. backport/1.18 labels Feb 8, 2024
@nathancoleman nathancoleman marked this pull request as ready for review February 8, 2024 16:55
…chain

This allows an external service registered on a terminating gateway to be exported to and reachable from a peered cluster
@nathancoleman nathancoleman force-pushed the export-terminating-gw-service branch 2 times, most recently from 9d0edc5 to 1105345 Compare April 1, 2024 18:57
@nathancoleman nathancoleman added backport-inactive/1.15 This release series is longer active. Use backport/ent/1.15. pr/no-backport backport/1.18 and removed backport-inactive/1.15 This release series is longer active. Use backport/ent/1.15. backport-inactive/1.16 This release series is no longer active backport-inactive/1.17 This release series is no longer active. Use backport/ent/1.17. backport/1.18 pr/no-backport labels Apr 2, 2024
Copy link
Member

@jm96441n jm96441n left a comment

Choose a reason for hiding this comment

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

LGTM! nice work

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme/cluster-peering Related to Consul's cluster peering feature theme/envoy/xds Related to Envoy support theme/terminating-gw Track terminating gateway work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants