basic sandwich #789

stevenctl · 2024-01-24T20:41:20Z

When the Waypoint is captured, zTunnel sees the connection address as the Waypoint's address and the HBONE address as the actual destination.

This impl currently uses that to detect whether the current connection is part of a sandwich, and verifies that the HBONE target actually references the connection address as a Waypoint. Instead of sending traffic to the HBONE target, we send it to the connection address.

Depends on #832 so the relevant commits are: https://github.com/istio/ztunnel/pull/789/files/b5af5ac0fb7e71805ee6f5b9fe56e8010511dead..a17e43e3488a4567c4c84eaae0638bf09d045c4d

istio-policy-bot · 2024-01-24T20:41:23Z

😊 Welcome @stevenctl! This is either your first contribution to the Istio ztunnel repo, or it's been
a while since you've been here.

You can learn more about the Istio working groups, Code of Conduct, and contribution guidelines
by referring to Contributing to Istio.

Thanks for contributing!

Courtesy of your friendly welcome wagon.

src/proxy/inbound.rs

ilrudie

direction seems good

ilrudie · 2024-01-30T17:34:37Z

src/proxy/inbound.rs

+                                    match Builder::with_addresses(
+                                        Version::Two | Command::Proxy,
+                                        Protocol::Stream, proxy_protocol).
+                                        write_tlv(0xD0, id.as_bytes()).unwrap().


Is write_tlv infallible in this context?

ilrudie · 2024-01-30T17:40:21Z

src/proxy/inbound.rs

+                                    if let Err(e) = super::copy_hbone(
+                                        &mut upgraded,
+                                        &mut stream,
+                                        &metrics,
+                                        transferred_bytes,
+                                    )


So even if we fail to write proxy proto headers for some reason we're going to try to press forward and I suppose it's up to the gateway to decide if that's ok or not. Would it be better to fail fast in zt or do we think most of the time this would be recoverable at the gateway somehow?

ilrudie · 2024-01-30T18:00:10Z

src/proxy/inbound.rs

+        if addr.is_err() {
+            info!("Sending 400, {:?}", addr.err());
+            return Ok(Response::builder()
+                .status(StatusCode::BAD_REQUEST)
+                .body(Empty::new())
+                .unwrap());
+        }

-                let addr: SocketAddr = addr.unwrap();
-                if addr.ip() != conn.dst.ip() {
-                    info!("Sending 400, ip mismatch {addr} != {}", conn.dst);
-                    return Ok(Response::builder()
-                        .status(StatusCode::BAD_REQUEST)
-                        .body(Empty::new())
-                        .unwrap());
-                }
-                // Orig has 15008, swap with the real port
-                let conn = Connection { dst: addr, ..conn };
-                let dst_network_addr = NetworkAddress {
-                    network: conn.dst_network.to_string(), // dst must be on our network
-                    address: addr.ip(),
-                };
-                let Some((upstream, upstream_service)) =
-                    state.fetch_workload_services(&dst_network_addr).await
-                else {
-                    info!(%conn, "unknown destination");
-                    return Ok(Response::builder()
-                        .status(StatusCode::NOT_FOUND)
-                        .body(Empty::new())
-                        .unwrap());
-                };
-                let has_waypoint = upstream.waypoint.is_some();
-                let from_waypoint = Self::check_waypoint(state.clone(), &upstream, &conn).await;
-                let from_gateway = Self::check_gateway(state.clone(), &upstream, &conn).await;
+        let addr: SocketAddr = addr.unwrap();


nitty: this looks a bit odd. I think you can let addr = match addr{...} and it works Ok so long as arms that won't result in the desired type return.

src/proxy/inbound.rs

src/state/service.rs

hzxuzhonghu · 2024-02-01T11:30:01Z

@stevenctl Is there a proposal, i missed the context

stevenctl · 2024-02-05T23:10:39Z

@hzxuzhonghu

See this doc (primarily the propagating L4 info section) and istio/istio#48362

The goal is to provide alternative Waypoint implementations with an option other than supporting CONNECT.

tests/namespaced.rs

stevenctl · 2024-03-11T22:28:02Z

src/proxy/outbound.rs

+        ) {
+            (_, Ok(None)) => {} // workload doesn't have a waypoint; this is fine
+            (true, _) => {}     // we already traversed the waypoint
+            (false, Ok(Some(waypoint_us))) => {


In this branch (and the one for to Service) we're using the port WDS GatewayAddress sends us.

ztunnel/src/proxy/outbound.rs

Line 423 in a17e43e

let waypoint_socket_address = SocketAddr::new(waypoint_ip, waypoint_us.port);

Right now, the CP happens to send us 15008 which happens to be what the ztunnel listens on.

Is the CP going to be smart and send the zTunnel address, or do we need to be smart and use pi.hbone_port?

Right now as per the protocol 15008 is "the" HBONE port and we assume that in other spots, I suspect.

For this I would suggest maybe splitting the difference and taking what CP sends us, but explicitly checking here that waypoint_us.port == pi.hbone_port and throwing an error if not. If we wish to intentionally change that behavior in all of ambient later, we can actually remove the check.

stevenctl · 2024-03-11T22:28:49Z

cc @bleggett

bleggett · 2024-03-12T21:20:36Z

Makefile.core.mk

@@ -11,6 +11,9 @@ endif
 test:
 	RUST_BACKTRACE=1 cargo test --benches --tests --bins $(FEATURES)

+test.root:


nit: +1 on adding RUST_BACKTRACE here too, but without #821 this will just cause make test.root to hang for people (unless they explicitly set BUILD_WITH_CONTAINER=0 - which is not the default)

src/proxy.rs

ilrudie

Looking good. couple comments/questions

ilrudie · 2024-03-14T14:30:31Z

src/proxy/inbound.rs

+                let from_waypoint = proxy::check_from_waypoint(
+                    pi.state.clone(),
+                    &upstream,
+                    conn.src_identity.as_ref(),
+                    &conn.src_ip,
+                )
+                .await;


Don't change anything just yet but not sure we need the check anymore per discussions around svc-addressed vs wl-addressed traffic. A user may have a waypoint for their service but still allow workload addressed without a waypoint and this would be considered valid so either inbound must take this type of configuration into account or elide checks that outbound ztunnel routing was correct.

Yep, I just don't want to take an opinion on that in this PR.

#851

created this so I/we don't forget to circle back around on it until a user files a report that things don't work like we said they do

src/proxy/inbound.rs

ilrudie

LGTM, thanks!

howardjohn

LGTM, all comments are theoretical future improvements so good to go ahead

howardjohn · 2024-03-14T22:17:40Z

src/proxy/inbound.rs

+                return Some(None);
+            };
+
+            // Validate that the HBONE target references the Waypoint we're connecting to


I am not 100% sure we will want to be this strict forever... but I cannot think of any concrete reasons why not to, and its easier to loosen later. So LGTm

howardjohn · 2024-03-14T23:31:28Z

src/proxy.rs

+    src_ip: &IpAddr,
+) -> bool {
+    let is_waypoint = |wl: &Workload| {
+        Some(wl.identity()).as_ref() == src_identity && wl.workload_ips.contains(src_ip)


Long term it would be nice to have a method more reliable than source IP. That being said, since its node-local it would be quite strange for this check to not work somehow.

stevenctl requested a review from a team as a code owner January 24, 2024 20:41

istio-testing added do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 24, 2024

stevenctl commented Jan 24, 2024

View reviewed changes

src/proxy/inbound.rs Outdated Show resolved Hide resolved

stevenctl commented Jan 24, 2024

View reviewed changes

src/proxy/inbound.rs Show resolved Hide resolved

stevenctl mentioned this pull request Jan 29, 2024

Waypoint Sandwich istio/istio#48362

Closed

9 tasks

ilrudie reviewed Jan 30, 2024

View reviewed changes

stevenctl force-pushed the sandwich branch from 958b4bd to 11cdd9d Compare January 30, 2024 23:38

istio-testing added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 31, 2024

stevenctl commented Jan 31, 2024

View reviewed changes

src/state/service.rs Outdated Show resolved Hide resolved

istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Feb 5, 2024

stevenctl force-pushed the sandwich branch from 7f2ea1a to c7eddbc Compare February 5, 2024 21:24

istio-testing removed the needs-rebase Indicates a PR needs to be rebased before being merged label Feb 5, 2024

stevenctl force-pushed the sandwich branch 2 times, most recently from 7c64261 to 28b50e7 Compare February 5, 2024 22:56

stevenctl force-pushed the sandwich branch from 28b50e7 to a37c2da Compare February 5, 2024 23:12

istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Feb 25, 2024

stevenctl mentioned this pull request Feb 26, 2024

Ambient waypoint 2.0 xDS design istio/istio#49562

Closed

stevenctl mentioned this pull request Mar 5, 2024

Prototype sandwiching Envoy and zTunnel istio/istio#42033

Closed

stevenctl force-pushed the sandwich branch from 424f302 to a17e43e Compare March 11, 2024 22:15

stevenctl requested a review from a team as a code owner March 11, 2024 22:15

istio-policy-bot added the lifecycle/stale Indicates a PR or issue hasn't been manipulated by an Istio team member for a while label Mar 11, 2024

istio-testing added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 11, 2024

stevenctl mentioned this pull request Mar 11, 2024

proxy protocol for sandwich #850

Merged

stevenctl changed the title ~~wip: sandwich: send proxy protocol~~ basic sandwich implementation Mar 11, 2024

istio-testing removed the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Mar 11, 2024

stevenctl commented Mar 11, 2024

View reviewed changes

tests/namespaced.rs Outdated Show resolved Hide resolved

istio-policy-bot removed the lifecycle/stale Indicates a PR or issue hasn't been manipulated by an Istio team member for a while label Mar 11, 2024

stevenctl commented Mar 11, 2024

View reviewed changes

stevenctl added 2 commits March 12, 2024 11:32

basic sandwich

2b2d3ff

unit test inbound

7aaa6b1

stevenctl force-pushed the sandwich branch from a17e43e to 7aaa6b1 Compare March 12, 2024 20:32

istio-testing added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed needs-rebase Indicates a PR needs to be rebased before being merged size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Mar 12, 2024

bleggett reviewed Mar 12, 2024

View reviewed changes

stevenctl changed the title ~~basic sandwich implementation~~ basic sandwich Mar 13, 2024

ilrudie reviewed Mar 14, 2024

View reviewed changes

validate from_waypoint with IPs

1b53bef

stevenctl force-pushed the sandwich branch from 272a82c to 1b53bef Compare March 14, 2024 17:14

ilrudie approved these changes Mar 14, 2024

View reviewed changes

stevenctl requested a review from howardjohn March 14, 2024 19:12

bleggett approved these changes Mar 14, 2024

View reviewed changes

howardjohn approved these changes Mar 14, 2024

View reviewed changes

istio-testing merged commit 9b7bf7a into istio:master Mar 14, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

basic sandwich #789

basic sandwich #789

stevenctl commented Jan 24, 2024 •

edited

Loading

istio-policy-bot commented Jan 24, 2024

ilrudie left a comment

ilrudie Jan 30, 2024

ilrudie Jan 30, 2024

ilrudie Jan 30, 2024

hzxuzhonghu commented Feb 1, 2024

stevenctl commented Feb 5, 2024

stevenctl Mar 11, 2024

stevenctl Mar 11, 2024

bleggett Mar 14, 2024 •

edited

Loading

stevenctl commented Mar 11, 2024

bleggett Mar 12, 2024

ilrudie left a comment

ilrudie Mar 14, 2024

stevenctl Mar 14, 2024

ilrudie Mar 14, 2024

ilrudie left a comment

howardjohn left a comment

howardjohn Mar 14, 2024

howardjohn Mar 14, 2024

basic sandwich #789

basic sandwich #789

Conversation

stevenctl commented Jan 24, 2024 • edited Loading

istio-policy-bot commented Jan 24, 2024

ilrudie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hzxuzhonghu commented Feb 1, 2024

stevenctl commented Feb 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bleggett Mar 14, 2024 • edited Loading

Choose a reason for hiding this comment

stevenctl commented Mar 11, 2024

Choose a reason for hiding this comment

ilrudie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilrudie left a comment

Choose a reason for hiding this comment

howardjohn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevenctl commented Jan 24, 2024 •

edited

Loading

bleggett Mar 14, 2024 •

edited

Loading