Add inbound support for svc hostname in authority #1420

jaellio · 2025-01-10T00:18:28Z

Hoping to get feedback on the overall approach for this change. I would appreciate any rust specific feedback as well.

PR still needs some minor refactoring.

Main changes:

hbone_address supports hostname or ip when reading from the authority header
when a hostname is included in the authority pseudo header in is included as header in the proxy protocol. The hbone address included in the address header is set to the destination service vip
hostname is validated by looking up the respective service and verifying the destination pod ip is a member of that service.

Todo:

refactor hbone address validation to improve testability
update error types
confirm if port should also be included with hostname in custom protocol type
intelligently select service vip for proxy protocol address header
strictly validate hostname fqdn

Signed-off-by: Jackie Elliott <[email protected]>

istio-testing · 2025-01-10T00:18:31Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

istio-policy-bot · 2025-01-10T00:18:32Z

😊 Welcome @jaellio! 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.

jaellio · 2025-01-10T00:18:53Z

/test all

jaellio · 2025-01-10T00:20:09Z

src/proxy/inbound.rs

+            // TODO(jaellio): Should we also validate the hbone_addr port?
+            HboneAddress::SvcHostname(hbone_host, hbone_port) => {
+                // Validate a service or workload exists for the hostname
+                let parts: Vec<&str> = hbone_host.split('.').collect();


TODO - figure out if there is a better way to parse a hostname

I don't think we can. There is not really a mapping from hostname -> namespace. For example, I could have a hostname foo.com. We could have a guess or heuristic, but this doesn't seem like a time when doing the check only some of the time provides much value?

Since its to look up the service, perhaps we do something like find all the services with the hostname and pick the first preferring one with the same namespace (as the workload) if available

Signed-off-by: Jackie Elliott <[email protected]>

jaellio · 2025-01-10T00:42:04Z

/test test

Signed-off-by: Jackie Elliott <[email protected]>

jaellio · 2025-01-10T00:56:26Z

/test test

Signed-off-by: Jackie Elliott <[email protected]>

src/proxy/inbound.rs

keithmattix · 2025-01-10T21:40:23Z

src/proxy/inbound.rs


        // Determine the next hop.
+        let hbone_addr_clone = hbone_addr.clone();


Making hbone_addr an Arc may help with the clones

I think it could probably be changed so that find_inbound_upstream takes a borrow. Not sure why it accepts so many borrows and one owned TBH. All we really care about here is the port I think which is cheap to copy on the stack vs a clone of something more complex

keithmattix · 2025-01-10T21:42:11Z

src/proxy/inbound.rs

+            HboneAddress::SvcHostname(hbone_host, hbone_port) => {
+                // Validate a service or workload exists for the hostname
+                let parts: Vec<&str> = hbone_host.split('.').collect();
+                if parts.len() <= 1 {


We can probably be a little stronger here and require an FQDN (i.e. error if we detect a short name with less than 5 parts). I think Istio always requires FQDNs for multi-cluster addressing (/cc @howardjohn)

Agree. Kept it simple for the draft but this isn't sufficient atm.

Yes to require FQDN. However I am not convinced we can/should do the mapping of service --> namespace at all (see comment 2 lines up)

src/proxy/inbound.rs

keithmattix · 2025-01-10T21:46:05Z

src/proxy/inbound.rs

-        hbone_addr: SocketAddr,
-    ) -> Result<(), Error> {
+        hbone_addr: HboneAddress,
+    ) -> Result<Option<SocketAddr>, Error> {


Don't love the idea of returning a value from this function. I know you need it for the proxy protocol stuff so maybe the initial hbone_addr matching can be a part of a separate function that's called only if the :authority is a hostname

Agree, just a temporary hack. Will fix

Signed-off-by: Jackie Elliott <[email protected]>

jaellio · 2025-01-15T05:10:16Z

src/proxy/inbound.rs

+                                StatusCode::BAD_REQUEST,
+                            ));
+                        }
+                        // TODO(jaellio): Intelligently select the VIP


@keithmattix with regards to #1409 and intelligently selecting vip for a Address::Service, is it sufficient to rank vips by network comparison (higher rank if vip.network matches conn.dst_network) or should we also be comparing IpAddr ranges? Do we also want to implement ranking of Address::Services when finding hostnames (similar to how there would be duplicate workloads on lookup by IP)?

howardjohn

A

howardjohn · 2025-01-15T22:45:06Z

src/proxy.rs

 const PROXY_PROTOCOL_AUTHORITY_TLV: u8 = 0xD0;
+// Custom TLV for including the svc hostname in the proxy protocol header
+const PROXY_PROTOCOL_SVC_HOSTNAME_TLV: u8 = 0xD1;


Hmm this is an interesting one. I don't see any strong reason not to do this other than I don't know of any use cases to do so, and if we do we probably can never remove it since its part of our runtime API contract.

Did you have a specific use in mind for this?

howardjohn · 2025-01-15T22:56:30Z

src/proxy/inbound.rs

@@ -262,38 +346,90 @@ impl Inbound {
        }

        let start = Instant::now();
+
+        // Extract the host or IP from the authority pseudo-header of the URI
        let hbone_addr = req


nit: you could make this a

impl TryFrom<&http::Uri> for HboneAddress { type Error = Error; fn try_from(value: &http::Uri) -> Result<Self, Self::Error> { ... }

Then the calling code is just req.uri().try_into()

howardjohn · 2025-01-15T22:57:59Z

src/proxy/inbound.rs

+                let hbone_host = req.uri().host().unwrap();
+                let hbone_port = req.uri().port_u16().unwrap();


I don't think these unwraps are safe. I suspect :authority: foo (no port) would panic here for example. Which we want to reject (probably with a BAD_REQUEST), but shouldn't unwrap.

howardjohn · 2025-01-15T23:03:03Z

src/proxy/inbound.rs

@@ -52,6 +53,56 @@ pub(super) struct Inbound {
    enable_orig_src: bool,
 }

+#[derive(Debug, Clone)]
+pub enum HboneAddress {


nit: not a big deal but given this is used outside inbound might make sense to put it up in proxy module instead

howardjohn · 2025-01-15T23:06:02Z

src/proxy/inbound.rs

+                        // TODO(jaellio): Intelligently select the VIP
+                        let svc_address = svc.vips[0].address;
+                        upstream_protocol_addr = format!("{}:{}", svc_address, hbone_addr.port())
+                            .parse::<SocketAddr>()


nit: No need to send to string and parse back, SocketAddr::new(svc_address, hbone_addr.port()) should work

howardjohn · 2025-01-15T23:10:21Z

src/proxy/inbound.rs

+            // TODO(jaellio): Should we also validate the hbone_addr port?
+            HboneAddress::SvcHostname(hbone_host, hbone_port) => {
+                // Validate a service or workload exists for the hostname
+                let parts: Vec<&str> = hbone_host.split('.').collect();


I don't think we can. There is not really a mapping from hostname -> namespace. For example, I could have a hostname foo.com. We could have a guess or heuristic, but this doesn't seem like a time when doing the check only some of the time provides much value?

howardjohn · 2025-01-15T23:11:09Z

src/proxy/inbound.rs

+            HboneAddress::SvcHostname(hbone_host, hbone_port) => {
+                // Validate a service or workload exists for the hostname
+                let parts: Vec<&str> = hbone_host.split('.').collect();
+                if parts.len() <= 1 {


Yes to require FQDN. However I am not convinced we can/should do the mapping of service --> namespace at all (see comment 2 lines up)

howardjohn · 2025-01-15T23:12:06Z

src/proxy/inbound.rs

+            // TODO(jaellio): Should we also validate the hbone_addr port?
+            HboneAddress::SvcHostname(hbone_host, hbone_port) => {
+                // Validate a service or workload exists for the hostname
+                let parts: Vec<&str> = hbone_host.split('.').collect();


Since its to look up the service, perhaps we do something like find all the services with the hostname and pick the first preferring one with the same namespace (as the workload) if available

howardjohn · 2025-01-15T23:13:55Z

src/proxy/inbound.rs

+        local_workload: &Workload,
+        hbone_addr: HboneAddress,
+    ) -> Result<Address, Error> {
+        match hbone_addr {


Its a bit of a codesmell to have the function take in an enum but then require a specific variant is passed.

In this specific case its pretty easy to resolve though, instead of taking in HboneAddress we can just take in a Strng and have them pass in the hostname.

howardjohn · 2025-01-15T23:17:13Z

src/proxy/inbound.rs

-            // Normal case: both are aligned. This is allowed (we really only need the HBONE address for the port.)
+        let hbone_ip = match hbone_addr.ip() {
+            Some(ip) => ip,
+            None => return Err(Error::ConnectAddress(hbone_addr.to_string())),


Its a bit odd to have validate_destination only called for IP, then reject non-IP. Maybe we can just call it in all cases. if we have nothing to validate for hostname cases for now (which seems reasonable and fine) then we can just return Ok(()) immediately if its a hostname

jaellio added 6 commits December 31, 2024 23:42

Support hostname in authority header

1e6f26e

Signed-off-by: Jackie Elliott <[email protected]>

remove todos

3f82f32

Signed-off-by: Jackie Elliott <[email protected]>

Use enum for host addr

c35a3c3

Signed-off-by: Jackie Elliott <[email protected]>

Add HboneAddress enum and fix tests

3d63493

Signed-off-by: Jackie Elliott <[email protected]>

Update String usage to Strng and add comments

d9a652c

Signed-off-by: Jackie Elliott <[email protected]>

Update svc_hostname to return strng

3d18762

Signed-off-by: Jackie Elliott <[email protected]>

istio-testing added the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Jan 10, 2025

istio-testing added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 10, 2025

jaellio commented Jan 10, 2025

View reviewed changes

Fix build errors

088ee04

Signed-off-by: Jackie Elliott <[email protected]>

Replace assert with panic for test configuration failure

f667288

Signed-off-by: Jackie Elliott <[email protected]>

Fix fmt errors

72c87e0

Signed-off-by: Jackie Elliott <[email protected]>

Stevenjin8 reviewed Jan 10, 2025

View reviewed changes

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

keithmattix reviewed Jan 10, 2025

View reviewed changes

keithmattix linked an issue Jan 14, 2025 that may be closed by this pull request

Add service hostname HBONE :authority support to ztunnel servers #1422

Open

Respond to PR feedback

cb07ea8

Signed-off-by: Jackie Elliott <[email protected]>

jaellio marked this pull request as ready for review January 14, 2025 23:28

jaellio requested a review from a team as a code owner January 14, 2025 23:28

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

jaellio commented Jan 15, 2025

View reviewed changes

jaellio added the do-not-merge/hold Block automatic merging of a PR. label Jan 15, 2025

howardjohn reviewed Jan 16, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add inbound support for svc hostname in authority #1420

Add inbound support for svc hostname in authority #1420

jaellio commented Jan 10, 2025 •

edited

Loading

istio-testing commented Jan 10, 2025

istio-policy-bot commented Jan 10, 2025

jaellio commented Jan 10, 2025

jaellio Jan 10, 2025

howardjohn Jan 15, 2025

howardjohn Jan 15, 2025

jaellio commented Jan 10, 2025

jaellio commented Jan 10, 2025

keithmattix Jan 10, 2025

ilrudie Jan 15, 2025 •

edited

Loading

keithmattix Jan 10, 2025

jaellio Jan 11, 2025

howardjohn Jan 15, 2025

keithmattix Jan 10, 2025

jaellio Jan 11, 2025

jaellio Jan 15, 2025

howardjohn left a comment

howardjohn Jan 15, 2025

howardjohn Jan 15, 2025

howardjohn Jan 15, 2025

howardjohn Jan 15, 2025

howardjohn Jan 15, 2025

howardjohn Jan 15, 2025

howardjohn Jan 15, 2025

howardjohn Jan 15, 2025

howardjohn Jan 15, 2025

howardjohn Jan 15, 2025


		// Determine the next hop.
		let hbone_addr_clone = hbone_addr.clone();

		let hbone_host = req.uri().host().unwrap();
		let hbone_port = req.uri().port_u16().unwrap();

Add inbound support for svc hostname in authority #1420

Are you sure you want to change the base?

Add inbound support for svc hostname in authority #1420

Conversation

jaellio commented Jan 10, 2025 • edited Loading

istio-testing commented Jan 10, 2025

istio-policy-bot commented Jan 10, 2025

jaellio commented Jan 10, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaellio commented Jan 10, 2025

jaellio commented Jan 10, 2025

Choose a reason for hiding this comment

ilrudie Jan 15, 2025 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this 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

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

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

jaellio commented Jan 10, 2025 •

edited

Loading

ilrudie Jan 15, 2025 •

edited

Loading