feat: Implement TLV listener filter support #83

Eeshu-Yadav · 2025-09-03T04:12:42Z

Add simplified TLV listener filter protobuf definition matching kmesh approach
Implement TlvListenerFilter struct with async stream processing for IPv4/IPv6
Support TLV_TYPE_SERVICE_ADDRESS (0x1) and TLV_TYPE_ENDING (0xfe)
Use empty KmeshTlv message wrapped in UDPA TypedStruct (no configuration parameters)

fixes : #60

gemini-code-assist

Summary of Changes

Hello @Eeshu-Yadav, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request introduces a new listener filter designed to process TLV (Type-Length-Value) encoded data. This feature is crucial for extracting original destination information from network connections, particularly for use cases like Kmesh waypoint connections. The changes involve defining the filter's configuration via protobuf, implementing the core logic for parsing TLV streams, and integrating it into the existing listener framework to enable dynamic application based on listener settings.

Highlights

TLV Listener Filter Definition: Added a new protobuf definition for the TLV listener filter, allowing configuration of a maximum TLV length.
TlvListenerFilter Implementation: Implemented the TlvListenerFilter struct, which includes asynchronous stream processing capabilities for both IPv4 and IPv6 traffic.
TLV Type Support: The filter now supports TLV_TYPE_SERVICE_ADDRESS (0x1) for extracting original destination addresses and TLV_TYPE_ENDING (0xfe) to signify the end of TLV data.
Asynchronous Stream Processing: The filter incorporates asynchronous stream processing with rewind capability to efficiently handle incoming data.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in issue comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

gemini-code-assist

Code Review

This pull request introduces a TLV listener filter to extract original destination information from connections. The implementation is comprehensive, covering protobuf definitions, configuration, and the filter logic itself. However, I've identified a few critical issues that need to be addressed. These include using an incorrect enum variant for metadata, a bug in the destination extraction logic that will fail with multiple TLVs, an incorrect protobuf type URL, and ignoring the filter's configuration. I've also included a suggestion for a minor performance improvement in the stream processing loop. Addressing these points will ensure the filter is robust and correctly configurable.

gemini-code-assist · 2025-09-03T04:15:48Z

orion-configuration/src/config/listener_filters.rs

+                "type.googleapis.com/envoy.extensions.filters.listener.tlv_listener_filter.v3.TlvListenerFilter" => {
+                    let _config = crate::orion::extensions::filters::listener::tlv_listener_filter::v3::TlvListenerFilter::decode(typed_config.value.as_slice())
+                        .map_err(|e| GenericError::from_msg_with_cause("failed to decode TlvListenerFilter protobuf", e))?;
+                    Ok(Self::TlvListenerFilter)
+                },


There are a couple of issues in this block:

Critical: The type URL for the TLV listener filter is incorrect. The protobuf package is orion.extensions.filters.listener.tlv_listener_filter.v3, so the type URL should be type.googleapis.com/orion.extensions.filters.listener.tlv_listener_filter.v3.TlvListenerFilter, not one prefixed with envoy.. This will cause deserialization of the filter from an Any protobuf to fail.

High: The TlvListenerFilter configuration is decoded but then its value (_config) is discarded. The max_tlv_length field from the configuration is ignored, and a default value will be used in the filter implementation. The configuration should be stored and passed to the filter.

This will require some refactoring to pass the configuration down to orion-lib.

"type.googleapis.com/orion.extensions.filters.listener.tlv_listener_filter.v3.TlvListenerFilter" => { let _config = crate::orion::extensions::filters::listener::tlv_listener_filter::v3::TlvListenerFilter::decode(typed_config.value.as_slice()) .map_err(|e| GenericError::from_msg_with_cause("failed to decode TlvListenerFilter protobuf", e))?; // TODO: The `_config` should be passed down to the filter implementation. Ok(Self::TlvListenerFilter) },

gemini-code-assist · 2025-09-03T04:15:49Z

orion-lib/src/transport/tlv_listener_filter.rs

+                    let metadata = DownstreamConnectionMetadata::FromProxyProtocol {
+                        original_peer_address: peer_address,
+                        original_destination_address: original_dest,
+                        protocol: v2::Protocol::Stream,
+                        tlv_data: std::collections::HashMap::new(),
+                        proxy_peer_address: peer_address,
+                        proxy_local_address: local_address,
+                    };


The code is using DownstreamConnectionMetadata::FromProxyProtocol to store metadata from the TLV filter. However, a new DownstreamConnectionMetadata::FromTlv variant was introduced for this purpose. Using the wrong enum variant is incorrect and can lead to logic errors, especially as FromProxyProtocol is intended for PROXY protocol data and has different fields. You should use DownstreamConnectionMetadata::FromTlv here.

let metadata = DownstreamConnectionMetadata::FromTlv { original_destination_address: original_dest, tlv_data: std::collections::HashMap::new(), proxy_peer_address: peer_address, proxy_local_address: local_address, };

gemini-code-assist · 2025-09-03T04:15:49Z

orion-lib/src/transport/tlv_listener_filter.rs

+    fn extract_original_destination(&self) -> Option<SocketAddr> {
+        let content_start = TLV_TYPE_LEN + TLV_LENGTH_LEN;
+
+        if self.content_length == 0 || self.buffer.len() < content_start + self.content_length as usize {
+            return None;
+        }
+
+        if self.content_length == TLV_TYPE_SERVICE_ADDRESS_IPV4_LEN {
+            let ip_bytes = [
+                self.buffer[content_start],
+                self.buffer[content_start + 1],
+                self.buffer[content_start + 2],
+                self.buffer[content_start + 3],
+            ];
+            let port_bytes = [
+                self.buffer[content_start + 4],
+                self.buffer[content_start + 5],
+            ];
+
+            let ip = IpAddr::V4(Ipv4Addr::from(ip_bytes));
+            let port = u16::from_be_bytes(port_bytes);
+
+            Some(SocketAddr::new(ip, port))
+
+        } else if self.content_length == TLV_TYPE_SERVICE_ADDRESS_IPV6_LEN {
+            let mut ip_bytes = [0u8; 16];
+            ip_bytes.copy_from_slice(&self.buffer[content_start..content_start + 16]);
+            let port_bytes = [
+                self.buffer[content_start + 16],
+                self.buffer[content_start + 17],
+            ];
+
+            let ip = IpAddr::V6(Ipv6Addr::from(ip_bytes));
+            let port = u16::from_be_bytes(port_bytes);
+
+            Some(SocketAddr::new(ip, port))
+        } else {
+            None
+        }
+    }


extract_original_destination incorrectly assumes the service address TLV is always the first TLV in the buffer. It uses a fixed content_start which is only correct for the very first TLV. If other TLVs precede the service address, or if there are multiple service address TLVs, this function will read from the wrong offset or use an incorrect content length, leading to incorrect parsing or panics. The function should iterate through the buffer, parsing each TLV until it finds the first TLV_TYPE_SERVICE_ADDRESS.

fn extract_original_destination(&self) -> Option<SocketAddr> { let mut idx = 0; while idx + TLV_TYPE_LEN + TLV_LENGTH_LEN <= self.buffer.len() { let tlv_type = self.buffer[idx]; if tlv_type == TLV_TYPE_ENDING { break; } let content_len_bytes: [u8; 4] = self.buffer[idx + TLV_TYPE_LEN..idx + TLV_TYPE_LEN + TLV_LENGTH_LEN].try_into().ok()?; let content_len = u32::from_be_bytes(content_len_bytes); let content_start = idx + TLV_TYPE_LEN + TLV_LENGTH_LEN; if content_start + content_len as usize > self.buffer.len() { // Not enough data for content return None; } if tlv_type == TLV_TYPE_SERVICE_ADDRESS { if content_len == TLV_TYPE_SERVICE_ADDRESS_IPV4_LEN { let ip_bytes: [u8; 4] = self.buffer[content_start..content_start + 4].try_into().ok()?; let port_bytes: [u8; 2] = self.buffer[content_start + 4..content_start + 6].try_into().ok()?; let ip = IpAddr::V4(Ipv4Addr::from(ip_bytes)); let port = u16::from_be_bytes(port_bytes); return Some(SocketAddr::new(ip, port)); } else if content_len == TLV_TYPE_SERVICE_ADDRESS_IPV6_LEN { let mut ip_bytes = [0u8; 16]; ip_bytes.copy_from_slice(&self.buffer[content_start..content_start + 16]); let port_bytes: [u8; 2] = self.buffer[content_start + 16..content_start + 18].try_into().ok()?; let ip = IpAddr::V6(Ipv6Addr::from(ip_bytes)); let port = u16::from_be_bytes(port_bytes); return Some(SocketAddr::new(ip, port)); } } idx += TLV_TYPE_LEN + TLV_LENGTH_LEN + content_len as usize; } None }

gemini-code-assist · 2025-09-03T04:15:49Z

orion-lib/src/transport/tlv_listener_filter.rs

+        match metadata {
+            DownstreamConnectionMetadata::FromProxyProtocol { original_destination_address, .. } => {
+                assert_eq!(original_destination_address, "192.168.1.100:8080".parse::<SocketAddr>().unwrap());
+            },
+            _ => panic!("Expected FromProxyProtocol metadata"),


This test asserts that the metadata is of type DownstreamConnectionMetadata::FromProxyProtocol. This is incorrect as the TLV filter should produce DownstreamConnectionMetadata::FromTlv. The test should be updated to assert the correct enum variant to match the fix in process_stream.

match metadata { DownstreamConnectionMetadata::FromTlv { original_destination_address, .. } => { assert_eq!(original_destination_address, "192.168.1.100:8080".parse::<SocketAddr>().unwrap()); }, _ => panic!("Expected FromTlv metadata"), }

gemini-code-assist · 2025-09-03T04:15:49Z

orion-lib/src/transport/tlv_listener_filter.rs

+        loop {
+            while self.buffer.len() < self.expected_length {
+                let bytes_needed = self.expected_length - self.buffer.len();
+                let mut temp_buf = vec![0u8; bytes_needed];


A new Vec is allocated in each iteration of the while loop. This can be inefficient if data arrives in small chunks, causing multiple reads and allocations. Consider allocating a temporary buffer once before the loop and reusing it.

Eeshu-Yadav · 2025-09-03T09:25:15Z

@YaoZengzeng kindly review

YaoZengzeng · 2025-09-04T03:31:53Z

orion-lib/src/transport/tlv_listener_filter.rs

@@ -0,0 +1,372 @@
+// SPDX-FileCopyrightText: © 2025 kmesh authors


As we are a little far from integrating orion with istio and kmesh.

In order to make sure this filter work properly, it would be good if you could provide a config yaml to run orion with tlv filter and a simple client to access orion.

Through the log of orion, we could verify that all work fine.

As we are a little far from integrating orion with istio and kmesh.

In order to make sure this filter work properly, it would be good if you could provide a config yaml to run orion with tlv filter and a simple client to access orion.

Through the log of orion, we could verify that all work fine.

okkk

As we are a little far from integrating orion with istio and kmesh.

In order to make sure this filter work properly, it would be good if you could provide a config yaml to run orion with tlv filter and a simple client to access orion.

Through the log of orion, we could verify that all work fine.

kindly review the examples folder

hzxuzhonghu · 2025-09-04T08:00:51Z

Would also like @dawid-nowak @awgn @fciaccia to have a look from the framework view

hzxuzhonghu · 2025-09-04T08:26:43Z

orion-configuration/build.rs

@@ -0,0 +1,27 @@
+fn main() -> std::io::Result<()> {
+    let protos = vec!["proto/extensions/filters/listener/tlv_listener_filter/v3/tlv_listener_filter.proto"];


we donot need to have two build script, can add it into the existing one

yaa updated that

...iguration/proto/extensions/filters/listener/tlv_listener_filter/v3/tlv_listener_filter.proto

hzxuzhonghu · 2025-09-05T01:25:47Z

examples/tlv-filter-demo/test_tlv_config.sh

@@ -0,0 +1,48 @@
+#!/bin/bash


we can construct a tlv proto packet, and send it to orion to test the result

updated that

envoy-data-plane-api/build.rs

orion-lib/src/transport/tlv_listener_filter.rs

- Add simplified TLV listener filter protobuf definition matching kmesh approach - Implement TlvListenerFilter struct with async stream processing for IPv4/IPv6 - Support TLV_TYPE_SERVICE_ADDRESS (0x1) and TLV_TYPE_ENDING (0xfe) - Use empty KmeshTlv message wrapped in UDPA TypedStruct (no configuration parameters) Signed-off-by: Eeshu-Yadav <eeshuyadav123@gmail.com>

hzxuzhonghu · 2025-09-09T12:42:08Z

@Eeshu-Yadav Please donot squash commits before it is ok to merge, keep the commits can simplify reviewing procedure.

hzxuzhonghu · 2025-09-09T13:47:31Z

LGTM

hzxuzhonghu

the parsing logic canbe more readable

hzxuzhonghu · 2025-09-09T12:44:13Z

.gitignore

 *.log.*
+*.log
+examples/*/send_tlv
+examples/*/*.log


we donot want to add such stuff

will remove them

Eeshu-Yadav · 2025-09-10T08:45:14Z

@Eeshu-Yadav Please donot squash commits before it is ok to merge, keep the commits can simplify reviewing procedure.

ok

- Remove unnecessary .gitignore entries - Refactor TLV parsing into smaller methods - Add helper methods for common operations Signed-off-by: Eeshu-Yadav <eeshuyadav123@gmail.com>

Eeshu-Yadav · 2025-09-10T11:50:22Z

@hzxuzhonghu all the changes done , as suggested

hzxuzhonghu · 2025-09-11T01:49:20Z

proto/extensions/filters/listener/kmesh_tlv/v3/kmesh_tlv.proto

@@ -0,0 +1,21 @@
+syntax = "proto3";


please add copyright

hzxuzhonghu

@Eeshu-Yadav can you add a CI job to check copyright following up this pr

hzxuzhonghu · 2025-09-11T06:45:23Z

I mean the copyright checker can be in a separate pr. Our concention is to keep one pr focused and make it as little as possible

Eeshu-Yadav · 2025-09-11T06:49:37Z

I mean the copyright checker can be in a separate pr. Our concention is to keep one pr focused and make it as little as possible

ok will remove that

- Add copyright to kmesh_tlv.proto and send_tlv.rs Signed-off-by: Eeshu-Yadav <eeshuyadav123@gmail.com>

Eeshu-Yadav · 2025-09-11T21:59:13Z

@hzxuzhonghu added the copyright to the .proto file.

hzxuzhonghu · 2025-09-12T01:22:07Z

/lgtm
/approve

kmesh-bot · 2025-09-12T01:22:15Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hzxuzhonghu

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [hzxuzhonghu]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

kmesh-bot requested review from dawid-nowak and hzxuzhonghu September 3, 2025 04:12

kmesh-bot added the size/XL label Sep 3, 2025

gemini-code-assist bot reviewed Sep 3, 2025

View reviewed changes

Eeshu-Yadav force-pushed the feat/tlv-listener-filter-60_new branch from f603e89 to b042a57 Compare September 3, 2025 05:09

Eeshu-Yadav changed the title ~~feat: Implement TLV listener filter support (#60)~~ feat: Implement TLV listener filter support Sep 3, 2025

Eeshu-Yadav force-pushed the feat/tlv-listener-filter-60_new branch 4 times, most recently from 67584f1 to e6f1e66 Compare September 3, 2025 07:05

YaoZengzeng reviewed Sep 4, 2025

View reviewed changes

hzxuzhonghu reviewed Sep 4, 2025

View reviewed changes

Eeshu-Yadav force-pushed the feat/tlv-listener-filter-60_new branch 15 times, most recently from 858442e to 216fa4f Compare September 4, 2025 14:21

hzxuzhonghu reviewed Sep 5, 2025

View reviewed changes

envoy-data-plane-api/build.rs Outdated Show resolved Hide resolved

hzxuzhonghu reviewed Sep 5, 2025

View reviewed changes

orion-lib/src/transport/tlv_listener_filter.rs Show resolved Hide resolved

Eeshu-Yadav force-pushed the feat/tlv-listener-filter-60_new branch from 216fa4f to 681449d Compare September 5, 2025 07:22

YaoZengzeng mentioned this pull request Sep 9, 2025

feat: add internal listener and upstream transport support #87

Merged

hzxuzhonghu reviewed Sep 9, 2025

View reviewed changes

Improve TLV listener filter readability and clean up .gitignore

5a72903

- Remove unnecessary .gitignore entries - Refactor TLV parsing into smaller methods - Add helper methods for common operations Signed-off-by: Eeshu-Yadav <eeshuyadav123@gmail.com>

kmesh-bot added size/XXL and removed size/XL labels Sep 10, 2025

hzxuzhonghu reviewed Sep 11, 2025

View reviewed changes

proto/extensions/filters/listener/kmesh_tlv/v3/kmesh_tlv.proto

@@ -0,0 +1,21 @@

syntax = "proto3";

Copy link

Member

hzxuzhonghu Sep 11, 2025

Choose a reason for hiding this comment

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

please add copyright

hzxuzhonghu approved these changes Sep 11, 2025

View reviewed changes

kmesh-bot added the approved label Sep 11, 2025

Eeshu-Yadav force-pushed the feat/tlv-listener-filter-60_new branch from 1a79695 to 89ef906 Compare September 11, 2025 06:42

Eeshu-Yadav force-pushed the feat/tlv-listener-filter-60_new branch from 611923d to 5a72903 Compare September 11, 2025 06:56

Add copyright to TLV files

9f932bd

- Add copyright to kmesh_tlv.proto and send_tlv.rs Signed-off-by: Eeshu-Yadav <eeshuyadav123@gmail.com>

Eeshu-Yadav mentioned this pull request Sep 11, 2025

Add CI Job to Check Copyright Headers #95

Closed

kmesh-bot assigned hzxuzhonghu Sep 12, 2025

kmesh-bot added the lgtm label Sep 12, 2025

kmesh-bot merged commit 1618cbe into kmesh-net:main Sep 12, 2025
4 checks passed

Eeshu-Yadav deleted the feat/tlv-listener-filter-60_new branch September 19, 2025 16:08

		@@ -0,0 +1,372 @@
		// SPDX-FileCopyrightText: © 2025 kmesh authors

		@@ -0,0 +1,27 @@
		fn main() -> std::io::Result<()> {
		let protos = vec!["proto/extensions/filters/listener/tlv_listener_filter/v3/tlv_listener_filter.proto"];

feat: Implement TLV listener filter support #83

feat: Implement TLV listener filter support #83

Uh oh!

Conversation

Eeshu-Yadav commented Sep 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Summary of Changes

Highlights

Footnotes

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Sep 3, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Sep 3, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Sep 3, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Sep 3, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Sep 3, 2025

Choose a reason for hiding this comment

Uh oh!

Eeshu-Yadav commented Sep 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hzxuzhonghu commented Sep 4, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hzxuzhonghu commented Sep 9, 2025

Uh oh!

hzxuzhonghu commented Sep 9, 2025

Uh oh!

hzxuzhonghu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Eeshu-Yadav commented Sep 10, 2025

Uh oh!

Eeshu-Yadav commented Sep 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hzxuzhonghu left a comment

Choose a reason for hiding this comment

Uh oh!

hzxuzhonghu commented Sep 11, 2025

Uh oh!

Eeshu-Yadav commented Sep 11, 2025

Uh oh!

Eeshu-Yadav commented Sep 11, 2025

Uh oh!

Eeshu-Yadav commented Sep 3, 2025 •

edited

Loading