Feat/kmesh waypoint integration #139
Conversation
- Add SetFilterState config structures with Envoy conversions - Support format strings (text/structured) and upstream sharing modes - Integrate with HTTP filter chain (runtime support pending) - Add comprehensive implementation documentation Signed-off-by: Eeshu-Yadav <eeshuyadav123@gmail.com>
Add FilterState module with thread-safe RwLock-based storage for dynamic per-request metadata. Supports read-only enforcement, upstream sharing modes (NONE/ONCE/TRANSITIVE), and concurrent access. Signed-off-by: Eeshu-Yadav <eeshuyadav123@gmail.com>
- Add FormatStringEvaluator with regex-based command operator parser - Support 9 core operators: REQ, DOWNSTREAM_REMOTE_ADDRESS, UPSTREAM_HOST, etc. - Implement RequestContext for evaluation with headers and metadata - Add max length truncation and missing value handling (default '-') - Support case-insensitive header lookup with :authority fallback Signed-off-by: Eeshu-Yadav <eeshuyadav123@gmail.com>
- Add SetFilterState variant to HttpFilterValue enum - Implement apply_set_filter_state_filters() to evaluate format strings - Build RequestContext from HTTP request (headers, method, path, addresses) - Integrate FilterState and FormatStringEvaluator into HttpConnectionManager - Hook into request pipeline before routing decisions - Store evaluated values with proper metadata (read_only, shared_with_upstream) Signed-off-by: Eeshu-Yadav <eeshuyadav123@gmail.com>
- Add 3 integration tests for xDS config parsing (Istio waypoint) - Add 5 HTTP pipeline tests for filter application - Fix skip_if_empty to handle '-' default values - Add :authority and :method pseudo-headers to RequestContext Signed-off-by: Eeshu-Yadav <eeshuyadav123@gmail.com>
- Update apply_request() to accept mutable request and context - Apply SetFilterState in apply_request() like other filters - Extract SetFilterState logic to dedicated submodule - Remove redundant filter iteration loop Signed-off-by: Eeshu-Yadav <eeshuyadav123@gmail.com>
Signed-off-by: Eeshu-Yadav <eeshuyadav123@gmail.com>
- Add Kmesh integration folder with deployment files - Include setup and test scripts for kind cluster Signed-off-by: Eeshu-Yadav <eeshuyadav123@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Summary of ChangesHello @Eeshu-Yadav, I'm Gemini Code Assist1! 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 significantly advances Orion's capabilities by integrating it as a Kmesh waypoint within an Istio ambient mesh. It provides a complete set of tools and configurations for easy deployment and verification in a Kind cluster. Crucially, it also introduces robust, Envoy-compatible mechanisms for dynamic filter state management and format string evaluation, laying the groundwork for more sophisticated HTTP filter functionalities. Highlights
Using Gemini Code AssistThe 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 by creating a comment using either
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 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. Footnotes
|
|
@YaoZengzeng last commit is the one where all kmesh related changes performed , above changes related to set filter state . |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant new feature: kmesh waypoint integration with Orion. This includes a large amount of new code for the integration tests, deployment artifacts, and the core implementation of set_filter_state and format_string evaluation in Orion, which is a big step towards Envoy compatibility.
The implementation of the new features in orion-lib and orion-configuration is well-structured. However, there are several critical issues in the integration scripts and deployment YAMLs that need to be addressed. There are also some changes in error handling that reduce strictness, which could hide configuration problems. I've left specific comments on these points.
| echo -e "${YELLOW}🔨 Step 9: Building Orion waypoint...${NC}" | ||
|
|
||
| # Navigate to kmesh-integration directory | ||
| KMESH_DIR="$(dirname "$SCRIPT_DIR")" |
There was a problem hiding this comment.
The variable SCRIPT_DIR is used here but it is not defined until line 167. This will cause the script to fail. The definition of SCRIPT_DIR on line 167 should be moved to the top of the script, before its first use.
| - name: NODE_ID | ||
| value: "waypoint~$(POD_IP)~$(POD_NAME).$(NAMESPACE)~$(NAMESPACE).svc.cluster.local" |
There was a problem hiding this comment.
The NODE_ID environment variable is set to a string that looks like it expects shell expansion ($(POD_IP), etc.), but Kubernetes will treat this as a literal string. The entrypoint.sh script already contains the logic to correctly construct this NODE_ID if it's not set. To fix this, you should remove the NODE_ID environment variable from this deployment YAML and let the entrypoint script handle its creation.
# - name: NODE_ID
# value: "waypoint~$(POD_IP)~$(POD_NAME).$(NAMESPACE)~$(NAMESPACE).svc.cluster.local"| data: | ||
| config.yaml: | | ||
| runtime: | ||
| num_cpus: 2 | ||
| num_runtimes: 2 | ||
| logging: | ||
| log_level: "info" | ||
| envoy_bootstrap: | ||
| node: | ||
| id: "${NODE_ID}" | ||
| cluster: "waypoint.${NAMESPACE}" | ||
| metadata: | ||
| NAMESPACE: "${NAMESPACE}" | ||
| NODE_NAME: "${NODE_NAME}" | ||
| ENABLE_HBONE: "true" | ||
| LABELS: | ||
| gateway.networking.k8s.io/gateway-name: "orion-waypoint" | ||
| istio.io/dataplane-mode: "none" | ||
| service.istio.io/canonical-name: "orion-waypoint" | ||
| sidecar.istio.io/inject: "false" | ||
| INSTANCE_IPS: "${POD_IP}" | ||
| METADATA_DISCOVERY: "true" | ||
| SERVICE_ACCOUNT: "${SERVICE_ACCOUNT}" | ||
| CLUSTER_ID: "Kubernetes" | ||
| NAME: "${POD_NAME}" | ||
| MESH_ID: "cluster.local" | ||
| WORKLOAD_NAME: "orion-waypoint" | ||
| ISTIO_VERSION: "1.27.3" | ||
| dynamic_resources: | ||
| ads_config: | ||
| api_type: GRPC | ||
| transport_api_version: V3 | ||
| grpc_services: | ||
| - envoy_grpc: | ||
| cluster_name: xds_cluster | ||
| static_resources: | ||
| clusters: | ||
| - name: xds_cluster | ||
| connect_timeout: 0.25s | ||
| type: STRICT_DNS | ||
| lb_policy: ROUND_ROBIN | ||
| typed_extension_protocol_options: | ||
| envoy.extensions.upstreams.http.v3.HttpProtocolOptions: | ||
| "@type": type.googleapis.com/envoy.extensions.upstreams.http.v3.HttpProtocolOptions | ||
| explicit_http_config: | ||
| http2_protocol_options: {} | ||
| load_assignment: | ||
| cluster_name: xds_cluster | ||
| endpoints: | ||
| - lb_endpoints: | ||
| - endpoint: | ||
| address: | ||
| socket_address: | ||
| address: istiod.istio-system.svc.cluster.local | ||
| port_value: 15010 |
There was a problem hiding this comment.
The config.yaml within this ConfigMap is incomplete compared to kmesh-integration/config/orion-waypoint.yaml. Specifically, it is missing the static_resources.listeners section which defines the health check endpoint. This will likely cause the pod's health checks to fail, preventing it from becoming ready. The configuration from orion-waypoint.yaml should be fully included here.
| echo "" | ||
|
|
||
| # Switch to minikube context | ||
| kubectl config use-context minikube &>/dev/null |
There was a problem hiding this comment.
This script switches the kubectl context to minikube, but the setup-kmesh-kind.sh script and the README.md are focused on a kind cluster. This inconsistency can be confusing for users. If this test is meant for minikube, it should be documented. If it's for kind, it should use kind-kind context.
| ($field:ident) => {{ | ||
| if $field.is_used() { | ||
| #[allow(dropping_copy_types, clippy::drop_non_drop)] | ||
| drop($field); | ||
| Err(GenericError::UnsupportedField(stringify!($field))) | ||
| } else { | ||
| #[allow(dropping_copy_types, clippy::drop_non_drop)] | ||
| drop($field); | ||
| Ok(()) | ||
| tracing::warn!( | ||
| "unsupported field '{}' used in configuration. This field will be ignored.", | ||
| stringify!($field) | ||
| ); | ||
| } | ||
| }; | ||
| ($field:ident, $($tail:ident),+) => { | ||
| if $field.is_used() { | ||
| #[allow(dropping_copy_types, clippy::drop_non_drop)] | ||
| drop($field); | ||
| Err(GenericError::UnsupportedField(stringify!($field))) | ||
| } else { | ||
| unsupported_field! ($($tail),+) | ||
| #[allow(dropping_copy_types, clippy::drop_non_drop)] | ||
| drop($field); | ||
| Ok::<(), GenericError>(()) | ||
| }}; | ||
| ($field:ident, $($tail:ident),+) => {{ | ||
| if $field.is_used() { | ||
| tracing::warn!( | ||
| "unsupported field '{}' used in configuration. This field will be ignored.", | ||
| stringify!($field) | ||
| ); | ||
| } | ||
| }; | ||
| #[allow(dropping_copy_types, clippy::drop_non_drop)] | ||
| drop($field); | ||
| unsupported_field!($($tail),+) | ||
| }}; |
There was a problem hiding this comment.
Changing the behavior of unsupported_field! from returning an error to logging a warning can hide configuration issues. If a user makes a typo in a configuration field, it will now be silently ignored at runtime (with just a log warning) instead of causing a startup failure. This can make debugging misconfigurations very difficult. It's generally safer to fail fast on unknown or unsupported configuration fields. I recommend reverting to the previous behavior of returning an Err.
| if filter_chains.len() != n_filter_chains { | ||
| warn!("Duplicate filter chains {:?}", envoy_filter_chains); | ||
| return Err(GenericError::from_msg("filter chain contains duplicate filter_chain_match entries") | ||
| .with_node("filter_chains")); | ||
| warn!( | ||
| "Duplicate filter chains detected ({} unique out of {}), using last one for each match. \ | ||
| Filter chain names: {:?}", | ||
| filter_chains.len(), | ||
| n_filter_chains, | ||
| envoy_filter_chains.iter().map(|fc| &fc.name).collect::<Vec<_>>() | ||
| ); | ||
| } |
There was a problem hiding this comment.
The logic for handling duplicate filter_chains has been changed from returning an error to logging a warning and proceeding. This means if there are duplicate filter chain matches, the last one will silently overwrite previous ones. This can lead to unexpected behavior and make debugging difficult. It's safer to treat duplicate filter chain matches as a configuration error and fail.
| RUN apt-get update && apt-get install -y \ | ||
| ca-certificates \ | ||
| libssl3 \ | ||
| gettext-base \ | ||
| && rm -rf /var/lib/apt/lists/* |
There was a problem hiding this comment.
To reduce the Docker image size, it's a good practice to use --no-install-recommends with apt-get install. This prevents the installation of packages that are only recommended, not strictly required, by the packages you are installing.
RUN apt-get update && apt-get install -y --no-install-recommends \
ca-certificates \
libssl3 \
gettext-base \
&& rm -rf /var/lib/apt/lists/*
| NAME: "${POD_NAME}" | ||
| MESH_ID: "cluster.local" | ||
| WORKLOAD_NAME: "orion-waypoint" | ||
| ISTIO_VERSION: "1.27.3" |
There was a problem hiding this comment.
The Istio version is hardcoded. This can become outdated and lead to inconsistencies if the actual Istio version in the cluster is different. It would be more robust to pass this value via an environment variable from the Kubernetes deployment, where it can be managed more centrally.
ISTIO_VERSION: "${ISTIO_VERSION:-1.27.3}"| #!/bin/bash | ||
| # Orion startup script that substitutes environment variables in config | ||
|
|
||
| set -e |
There was a problem hiding this comment.
For more robust shell scripting, it's recommended to use set -euo pipefail instead of just set -e.
e: exit immediately if a command exits with a non-zero status.u: treat unset variables as an error when substituting.o pipefail: the return value of a pipeline is the status of the last command to exit with a non-zero status, or zero if no command exited with a non-zero status.
| set -e | |
| set -euo pipefail |
There was a problem hiding this comment.
Pull request overview
This PR implements kmesh waypoint integration for Orion, enabling it to function as an Istio/Kmesh waypoint proxy. The implementation adds support for dynamic filter state management and Envoy-style format string evaluation, which are essential for waypoint functionality in service mesh architectures.
Key Changes:
- Introduces filter state extension mechanism for per-request metadata storage
- Implements Envoy-compatible format string evaluation for dynamic value generation
- Adds SetFilterState HTTP filter for setting filter state from request properties
- Provides comprehensive kmesh integration including Docker images, Kubernetes manifests, and setup scripts
Reviewed changes
Copilot reviewed 22 out of 23 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| orion-lib/src/filter_state.rs | New module implementing per-request filter state storage with read-only protection and upstream sharing semantics |
| orion-lib/src/format_string.rs | New module implementing Envoy-style format string evaluation supporting request headers, addresses, and metadata |
| orion-lib/src/listeners/http_connection_manager/set_filter_state.rs | New HTTP filter applying filter state configuration based on evaluated format strings |
| orion-lib/src/listeners/http_connection_manager.rs | Integrates SetFilterState filter, updates filter signatures to accept mutable requests and additional context |
| orion-lib/src/listeners/rate_limiter/mod.rs | Updates function signature to accept mutable request and additional context parameters |
| orion-lib/src/lib.rs | Exports new filter_state and format_string modules |
| orion-configuration/src/config/network_filters/http_connection_manager/http_filters/set_filter_state.rs | Complete rewrite to properly parse Envoy SetFilterState configuration with typed format strings |
| orion-configuration/src/config/network_filters/http_connection_manager/http_filters.rs | Updates SetFilterState type references throughout |
| orion-configuration/src/config/network_filters/http_connection_manager/http_filters/filter_registry.rs | Updates registry to use new SetFilterState type |
| orion-configuration/src/config/network_filters/http_connection_manager/http_filters/peer_metadata.rs | Adds try_from_raw_protobuf method for consistency |
| orion-configuration/src/config/common.rs | Changes unsupported_field macro from error to warning behavior |
| orion-configuration/src/config/listener.rs | Changes duplicate filter chain handling from error to warning, adds internal listener support |
| orion-configuration/tests/set_filter_state_integration.rs | Updates tests to work with new SetFilterState structure |
| kmesh-integration/* | Adds complete kmesh integration package with Dockerfile, scripts, manifests, and documentation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
| KMESH_DIR="$(dirname "$SCRIPT_DIR")" | ||
| cd "$KMESH_DIR" |
There was a problem hiding this comment.
Variable $SCRIPT_DIR is used before it's defined. The variable is defined later on line 167, but it's referenced on line 138. This will cause the script to fail or use an incorrect path when trying to navigate to $KMESH_DIR. Move the SCRIPT_DIR definition (lines 167-168) to before line 138, or define it at the top of the script.
| let name_lower = name.to_lowercase(); | ||
| self.headers.get(name_lower.as_str()) |
There was a problem hiding this comment.
The get_header method creates a new String allocation on every call by calling name.to_lowercase(). For frequently accessed headers in hot paths, consider using a Cow<str> or checking if the name is already lowercase before allocating. Alternatively, store headers in a case-insensitive HashMap wrapper to avoid repeated allocations during lookups.
| let name_lower = name.to_lowercase(); | |
| self.headers.get(name_lower.as_str()) | |
| // Avoid allocation if name is already lowercase | |
| if name.chars().all(|c| !c.is_ascii_uppercase()) { | |
| self.headers.get(name) | |
| } else { | |
| let name_lower = name.to_lowercase(); | |
| self.headers.get(name_lower.as_str()) | |
| } |
| for (name, value) in request.headers() { | ||
| match value.to_str() { | ||
| Ok(value_str) => ctx = ctx.with_header(name.as_str(), value_str), | ||
| Err(_) => tracing::warn!( | ||
| header_name = %name.as_str(), | ||
| "Skipping non-UTF8 header value in format string context" | ||
| ), | ||
| } | ||
| } |
There was a problem hiding this comment.
Building the RequestContext iterates over all headers and converts them to lowercase strings (via with_header), which allocates memory for each header. For requests with many headers, this could be expensive. Consider lazy evaluation or only processing headers that are actually used in the format strings.
| } | ||
|
|
||
| #[test] | ||
| fn test_apply_set_filter_state_filters_disabled_filter() { |
There was a problem hiding this comment.
The test name test_apply_set_filter_state_filters_disabled_filter is misleading. The test doesn't actually test a disabled filter - it tests a filter with a static text value "value" (not even a format string substitution). Consider renaming to test_apply_set_filter_state_static_value or similar to accurately reflect what's being tested.
| fn test_apply_set_filter_state_filters_disabled_filter() { | |
| fn test_apply_set_filter_state_static_value() { |
| warn!( | ||
| "Duplicate filter chains detected ({} unique out of {}), using last one for each match. \ | ||
| Filter chain names: {:?}", | ||
| filter_chains.len(), | ||
| n_filter_chains, | ||
| envoy_filter_chains.iter().map(|fc| &fc.name).collect::<Vec<_>>() | ||
| ); |
There was a problem hiding this comment.
The logic changed from returning an error when duplicate filter chains are detected to just logging a warning and using the last one. This is a significant behavioral change that could silently accept configurations that were previously rejected. While the warning message is helpful, users who had duplicate filter chains would now see different behavior (accepting instead of rejecting). This should be clearly documented as a breaking change or behavior change in the PR description.
| warn!( | |
| "Duplicate filter chains detected ({} unique out of {}), using last one for each match. \ | |
| Filter chain names: {:?}", | |
| filter_chains.len(), | |
| n_filter_chains, | |
| envoy_filter_chains.iter().map(|fc| &fc.name).collect::<Vec<_>>() | |
| ); | |
| return Err(GenericError::from_msg(format!( | |
| "Duplicate filter chains detected ({} unique out of {}). Filter chain names: {:?}", | |
| filter_chains.len(), | |
| n_filter_chains, | |
| envoy_filter_chains.iter().map(|fc| &fc.name).collect::<Vec<_>>() | |
| ))) | |
| .with_node("filter_chains"); |
| return Ok(FormatString::Text(CompactString::from(s))); | ||
| } | ||
| } | ||
| // Inline bytes or other specifiers not supported here |
There was a problem hiding this comment.
The comment says "Inline bytes or other specifiers not supported here" but there's no fallthrough error handling for these unsupported cases. If a user provides inline bytes, the code will fall through to checking other fields and might give a confusing "unsupported FormatString representation" error. Consider adding explicit error handling for unsupported specifiers to provide clearer error messages.
| // Inline bytes or other specifiers not supported here | |
| // Inline bytes or other specifiers not supported here | |
| if tf_source.get("inline_bytes").is_some() { | |
| return Err(serde::de::Error::custom("inline_bytes in text_format_source is not supported for FormatString")); | |
| } | |
| // Optionally, check for other unsupported specifiers here |
| ├── config/ | ||
| │ └── orion-waypoint.yaml # Orion configuration | ||
| ├── docs/ | ||
| │ └── KMESH-FULL-INTEGRATION.md # Complete verification & architecture |
There was a problem hiding this comment.
The file docs/KMESH-FULL-INTEGRATION.md is referenced in the directory structure but this file doesn't appear in the PR diff. Either this file is missing from the PR, or the README should reference the actual file that exists (docs/commands.md).
| │ └── KMESH-FULL-INTEGRATION.md # Complete verification & architecture | |
| │ └── commands.md # Complete verification & architecture |
| ($field:ident) => {{ | ||
| if $field.is_used() { | ||
| #[allow(dropping_copy_types, clippy::drop_non_drop)] | ||
| drop($field); | ||
| Err(GenericError::UnsupportedField(stringify!($field))) | ||
| } else { | ||
| #[allow(dropping_copy_types, clippy::drop_non_drop)] | ||
| drop($field); | ||
| Ok(()) | ||
| tracing::warn!( | ||
| "unsupported field '{}' used in configuration. This field will be ignored.", | ||
| stringify!($field) | ||
| ); | ||
| } | ||
| }; | ||
| ($field:ident, $($tail:ident),+) => { | ||
| if $field.is_used() { | ||
| #[allow(dropping_copy_types, clippy::drop_non_drop)] | ||
| drop($field); | ||
| Err(GenericError::UnsupportedField(stringify!($field))) | ||
| } else { | ||
| unsupported_field! ($($tail),+) | ||
| #[allow(dropping_copy_types, clippy::drop_non_drop)] | ||
| drop($field); | ||
| Ok::<(), GenericError>(()) | ||
| }}; | ||
| ($field:ident, $($tail:ident),+) => {{ | ||
| if $field.is_used() { | ||
| tracing::warn!( | ||
| "unsupported field '{}' used in configuration. This field will be ignored.", | ||
| stringify!($field) | ||
| ); | ||
| } | ||
| }; | ||
| #[allow(dropping_copy_types, clippy::drop_non_drop)] | ||
| drop($field); | ||
| unsupported_field!($($tail),+) | ||
| }}; |
There was a problem hiding this comment.
The unsupported_field! macro's behavior has been changed from returning an error to just logging a warning. This is a significant behavioral change that silently ignores unsupported fields instead of failing. This could hide configuration errors from users who might expect their fields to be processed. Consider whether this change is intentional and if it aligns with the system's error handling philosophy. At minimum, this behavioral change should be documented in the PR description.
| if ctx.get_header(":authority").is_none() { | ||
| if let Some(host) = ctx.get_header("host") { | ||
| let host = host.clone(); | ||
| ctx = ctx.with_header(":authority", host); | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic for setting :authority from host (lines 70-75) is duplicated with similar logic in lines 77-79 where :method is set. However, line 70 checks if :authority is missing before setting it from host, but there's no explanation why this fallback is needed or why :method doesn't need similar handling. Consider adding a comment explaining this logic, as it appears to be handling HTTP/1.1 vs HTTP/2 header differences.
| @@ -0,0 +1,160 @@ | |||
| #!/bin/bash | |||
There was a problem hiding this comment.
Missing set -e or proper error handling. The script uses set -e in setup-kmesh-kind.sh but not here. If any command fails (like kubectl calls), the script will continue executing, potentially giving misleading test results. Add set -e at the beginning or add explicit error checking after critical commands.
| #!/bin/bash | |
| #!/bin/bash | |
| set -e |
|
/assign @YaoZengzeng |
4 participants
#137