Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix returning value for empty header and make possible to differentiate between missing header and empty header #305

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

prembhaskal
Copy link

@prembhaskal prembhaskal commented Feb 28, 2025

The method get_http_request_headers_bytes return empty vector for empty header value but get_http_request_header_bytes returns None for empty header value.

Similarly get_http_request_headers returns empty String but get_http_request_header returns None.

This commit intends to fix this inconsistency.

With this, it will be possible to differentiate between two cases, a missing header and a header with empty value.

Copy link

google-cla bot commented Feb 28, 2025

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@prembhaskal prembhaskal force-pushed the fix-missing-vs-empty-header-value branch from 079fa02 to e74213e Compare March 4, 2025 17:36
@PiotrSikora
Copy link
Member

Which host returns NULL value here?

This code is a legacy code path for "header not found" from Proxy-Wasm ABI v0.1.0, and I'd expect that hosts would return a valid pointer to a zero-sized string, although I guess returning NULL here is also fine.

Also, you need to make the same change in get_map_value_bytes.

(Please ignore the CI failures, they'll get fixed once #306 is merged)

@prembhaskal
Copy link
Author

prembhaskal commented Mar 17, 2025

@PiotrSikora I was working with envoy 1.31. I think i got same result with latest envoy(1.33), but I will confirm that.

Also, you need to make the same change in get_map_value_bytes

Actually I made change in that method itself.

@prembhaskal
Copy link
Author

@PiotrSikora I think pub fn get_map_value is the legacy one, which returns utf8 string value. I have not changed that, but please let me know if I should.

@PiotrSikora
Copy link
Member

@PiotrSikora I think pub fn get_map_value is the legacy one, which returns utf8 string value. I have not changed that, but please let me know if I should.

Oops, sorry! Yes, I've meant that the same change should be made in both get_map_value and get_map_value_bytes, otherwise we're adding pretty weird inconsistency within the SDK.

Also, get_map_value isn't really legacy, since get_map_value_bytes only moves the UTF-8 / HTTP encoding issue out of the SDK and into each plugin. The proper solution is in #287, once that's reviewed and merged.

The method get_http_request_headers_bytes return empty vec for empty header value but get_http_request_header_bytes returns None for empty header value.
Also added same fix in get_map_value for get_http_request_header method.

This commit intends to fix this inconsistency.

Signed-off-by: prembhaskal <[email protected]>
@prembhaskal prembhaskal force-pushed the fix-missing-vs-empty-header-value branch from e74213e to 6061be8 Compare March 17, 2025 07:12
@prembhaskal
Copy link
Author

@PiotrSikora I have updated the method get_map_value now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants