Fix :Envoy Go control plane adaptation issues #86

Eeshu-Yadav · 2025-09-05T08:45:18Z

Add support for host_rewrite_specifier in RouteAction conversion
Handle missing name field in FilterChain by providing default name
Fix compilation errors and update tests

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 aims to resolve several adaptation issues within the Envoy Go control plane integration. The changes enhance the system's ability to correctly handle host rewriting specifications in routing actions and improve the robustness of filter chain configurations by providing default values where names are absent. This leads to better compatibility and stability of the control plane.

Highlights

Envoy RouteAction: Implemented support for host_rewrite_specifier within RouteAction conversions, allowing for more flexible host rewriting configurations.
FilterChain Naming: Ensured robustness by providing a default name for FilterChain instances when the name field is missing or empty.
Code Stability: Addressed compilation errors and updated existing tests to reflect the new changes and ensure continued correctness.

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 addresses adaptation issues with the Envoy Go control plane by adding support for host_rewrite_specifier and handling missing name fields in FilterChain. The fix for the missing filter chain name is correct. However, I found a logic error in the implementation of auto_host_rewrite which could lead to incorrect behavior. My review includes a comment with a suggested fix for this issue.

gemini-code-assist · 2025-09-05T08:48:17Z

orion-configuration/src/config/network_filters/http_connection_manager/route.rs

+            let authority_rewrite = host_rewrite_specifier
+                .map(|spec| match spec {
+                    EnvoyHostRewriteSpecifier::HostRewriteLiteral(literal) => {
+                        Authority::from_str(&literal)
+                            .map(AuthorityRewriteSpecifier::Authority)
+                            .map_err(|e| GenericError::from_msg_with_cause(
+                                format!("failed to parse host rewrite literal '{literal}' as authority"), e))
+                    },
+                    EnvoyHostRewriteSpecifier::AutoHostRewrite(_) => Ok(AuthorityRewriteSpecifier::AutoHostRewrite),
+                    EnvoyHostRewriteSpecifier::HostRewriteHeader(header) => {
+                        match HeaderName::from_str(&header) {
+                            Ok(_) => Ok(AuthorityRewriteSpecifier::Header(header.into())),
+                            Err(e) => Err(GenericError::from_msg_with_cause(
+                                format!("failed to parse host rewrite header '{header}' as header name"), e)),
+                        }
+                    },
+                    EnvoyHostRewriteSpecifier::HostRewritePathRegex(regex) => {
+                        regex.try_into().map(AuthorityRewriteSpecifier::Regex)
+                    },
+                })
+                .transpose()
+                .with_node("host_rewrite_specifier")?;


The current implementation of AutoHostRewrite does not correctly handle the boolean value associated with it. According to Envoy's documentation, if auto_host_rewrite is set to false, no rewrite should occur. The current code treats any presence of auto_host_rewrite as a trigger for rewriting, which is incorrect.

I suggest filtering out the auto_host_rewrite: false case before mapping the host_rewrite_specifier to ensure the correct behavior.

let authority_rewrite = host_rewrite_specifier .filter(|spec| { if let EnvoyHostRewriteSpecifier::AutoHostRewrite(bv) = spec { bv.value } else { true } }) .map(|spec| match spec { EnvoyHostRewriteSpecifier::HostRewriteLiteral(literal) => { Authority::from_str(&literal) .map(AuthorityRewriteSpecifier::Authority) .map_err(|e| GenericError::from_msg_with_cause( format!("failed to parse host rewrite literal '{literal}' as authority"), e)) }, EnvoyHostRewriteSpecifier::AutoHostRewrite(_) => Ok(AuthorityRewriteSpecifier::AutoHostRewrite), EnvoyHostRewriteSpecifier::HostRewriteHeader(header) => { match HeaderName::from_str(&header) { Ok(_) => Ok(AuthorityRewriteSpecifier::Header(header.into())), Err(e) => Err(GenericError::from_msg_with_cause( format!("failed to parse host rewrite header '{header}' as header name"), e)), } }, EnvoyHostRewriteSpecifier::HostRewritePathRegex(regex) => { regex.try_into().map(AuthorityRewriteSpecifier::Regex) }, }) .transpose() .with_node("host_rewrite_specifier")?;

Eeshu-Yadav · 2025-09-05T08:58:02Z

@YaoZengzeng kindly review

- Add support for host_rewrite_specifier in RouteAction conversion - Handle missing name field in FilterChain by providing default name - Fix compilation errors and update tests Signed-off-by: Eeshu-Yadav <eeshuyadav123@gmail.com>

hzxuzhonghu · 2025-09-09T01:29:34Z

orion-configuration/src/config/listener.rs

                transport_socket_connect_timeout // name,
            )?;
-            let name: CompactString = required!(name)?.into();
+            let name: CompactString = if name.is_empty() { "default_filter_chain".into() } else { name.into() };


if empty can we use ""

hzxuzhonghu · 2025-09-09T12:34:31Z

orion-configuration/src/config/network_filters/http_connection_manager/route.rs

            let retry_policy = retry_policy.map(RetryPolicy::try_from).transpose().with_node("retry_policy")?;
            let upgrade_config = upgrade_configs.try_into().with_node("upgrade_configs").ok();
            let hash_policy = convert_vec!(hash_policy)?;
+            let authority_rewrite = host_rewrite_specifier


can you use match

Eeshu-Yadav · 2025-09-10T11:07:55Z

@hzxuzhonghu is it fine ?

…me and refactor authority_rewrite to use match Signed-off-by: Eeshu-Yadav <eeshuyadav123@gmail.com>

hzxuzhonghu · 2025-09-11T01:44:28Z

orion-configuration/src/config/network_filters/http_connection_manager/route.rs

            let upgrade_config = upgrade_configs.try_into().with_node("upgrade_configs").ok();
            let hash_policy = convert_vec!(hash_policy)?;
+            let authority_rewrite = match host_rewrite_specifier {
+                Some(EnvoyHostRewriteSpecifier::AutoHostRewrite(bv)) if !bv.value => Ok(None),


I have never seem Ok(None), the semantic should be when bv.value = true, we need to autorewrite, and vice verse, Then how can this express branch true?

hzxuzhonghu · 2025-09-11T01:45:16Z

orion-configuration/src/config/network_filters/http_connection_manager/route.rs

+                    },
+                }
+                .map(Some),
+                None => Ok(None),


None instead of Ok(None)

Signed-off-by: Eeshu-Yadav <eeshuyadav123@gmail.com>

Eeshu-Yadav · 2025-09-11T06:01:05Z

@hzxuzhonghu Thanks for the review! Fixed both issues:

AutoHostRewrite logic: Made it explicit - when bv.value is true→ enable auto-rewrite, false → disable (return None)
Ok(None) vs None: The None => Ok(None) is correct because the match returns Result<Option<...>, ...> and gets unwrapped with .with_node(...)

hzxuzhonghu · 2025-09-11T06:39:27Z

/lgtm
/approve

i guess the autorewrite does not really work during request handling, So can you file a issue to track

kmesh-bot · 2025-09-11T06:39:34Z

[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

This commit implements the missing AutoHostRewrite functionality that was configured but not actually used during request processing. The implementation adds support for all AuthorityRewriteSpecifier variants: - AutoHostRewrite: Uses upstream cluster's authority - Authority: Uses a specific authority - Header: Uses value from a request header - Regex: Applies regex transformation to current authority Both the request URI authority and Host header are updated when authority rewriting is applied, ensuring consistent behavior across the request. Fixes the issue mentioned in PR kmesh-net#86 review where autorewrite configuration was parsed but not executed during request handling. Signed-off-by: Eeshu-Yadav <eeshuyadav123@gmail.com>

kmesh-bot requested review from dawid-nowak and hzxuzhonghu September 5, 2025 08:45

gemini-code-assist bot reviewed Sep 5, 2025

View reviewed changes

kmesh-bot added the size/M label Sep 5, 2025

gemini-code-assist bot reviewed Sep 5, 2025

View reviewed changes

Eeshu-Yadav force-pushed the fix-envoy-go-control-plane branch 2 times, most recently from 7edb1dd to 319e34e Compare September 5, 2025 08:57

Eeshu-Yadav force-pushed the fix-envoy-go-control-plane branch from 319e34e to 35dbe51 Compare September 5, 2025 09:02

Fix Envoy Go control plane adaptation issues

9564ecf

- Add support for host_rewrite_specifier in RouteAction conversion - Handle missing name field in FilterChain by providing default name - Fix compilation errors and update tests Signed-off-by: Eeshu-Yadav <eeshuyadav123@gmail.com>

Eeshu-Yadav force-pushed the fix-envoy-go-control-plane branch from 35dbe51 to 9564ecf Compare September 5, 2025 09:13

Eeshu-Yadav changed the title ~~Fix Envoy Go control plane adaptation issues~~ Fix :Envoy Go control plane adaptation issues Sep 5, 2025

hzxuzhonghu reviewed Sep 9, 2025

View reviewed changes

Address review comments: use empty string for default filter chain na…

bd20ca1

…me and refactor authority_rewrite to use match Signed-off-by: Eeshu-Yadav <eeshuyadav123@gmail.com>

Eeshu-Yadav force-pushed the fix-envoy-go-control-plane branch from 463c39e to bd20ca1 Compare September 10, 2025 11:09

hzxuzhonghu reviewed Sep 11, 2025

View reviewed changes

Fix AutoHostRewrite boolean logic in route configuration

780b3b8

Signed-off-by: Eeshu-Yadav <eeshuyadav123@gmail.com>

kmesh-bot assigned hzxuzhonghu Sep 11, 2025

kmesh-bot added the lgtm label Sep 11, 2025

kmesh-bot added the approved label Sep 11, 2025

kmesh-bot merged commit 0b5bdab into kmesh-net:main Sep 11, 2025
4 checks passed

Eeshu-Yadav mentioned this pull request Sep 11, 2025

Implement AutoHostRewrite functionality in request handling #93

Closed

Eeshu-Yadav deleted the fix-envoy-go-control-plane branch September 19, 2025 16:01

Fix :Envoy Go control plane adaptation issues #86

Fix :Envoy Go control plane adaptation issues #86

Conversation

Eeshu-Yadav commented Sep 5, 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 5, 2025

Choose a reason for hiding this comment

Uh oh!

Eeshu-Yadav commented Sep 5, 2025

Uh oh!

hzxuzhonghu Sep 9, 2025

Choose a reason for hiding this comment

Uh oh!

hzxuzhonghu Sep 9, 2025

Choose a reason for hiding this comment

Uh oh!

Eeshu-Yadav commented Sep 10, 2025

Uh oh!

hzxuzhonghu Sep 11, 2025

Choose a reason for hiding this comment

Uh oh!

hzxuzhonghu Sep 11, 2025

Choose a reason for hiding this comment

Uh oh!

Eeshu-Yadav commented Sep 11, 2025

Uh oh!

hzxuzhonghu commented Sep 11, 2025

Uh oh!

kmesh-bot commented Sep 11, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Eeshu-Yadav commented Sep 5, 2025 •

edited

Loading