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

feat(proxy-wasm): Add new request property request.is_subrequest #663

Conversation

dev-null-undefined
Copy link
Contributor

Introduce a new request property request.is_subrequest to indicate whether the current request is a subrequest of an existing request (e.g., in slicing scenarios).

Note: Although the property uses the request.* prefix, this is not an Envoy attribute.

Fixes #658

Copy link

codecov bot commented Jan 15, 2025

Codecov Report

Attention: Patch coverage is 0% with 7 lines in your changes missing coverage. Please review.

Project coverage is 64.00078%. Comparing base (63f6784) to head (ba0d121).
Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
src/common/proxy_wasm/ngx_proxy_wasm_properties.c 0.00000% 7 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (63f6784) and HEAD (ba0d121). Click for more details.

HEAD has 13 uploads less than BASE
Flag BASE (63f6784) HEAD (ba0d121)
valgrind 5 3
unit 11 0
Additional details and impacted files

Impacted file tree graph

@@                 Coverage Diff                  @@
##                main        #663          +/-   ##
====================================================
- Coverage   90.82650%   64.00078%   -26.82572%     
====================================================
  Files             53          52           -1     
  Lines          11337       10253        -1084     
  Branches        1692           0        -1692     
====================================================
- Hits           10297        6562        -3735     
- Misses          1034        3691        +2657     
+ Partials           6           0           -6     
Files with missing lines Coverage Δ
src/common/proxy_wasm/ngx_proxy_wasm_properties.c 36.66667% <0.00000%> (-52.59507%) ⬇️

... and 50 files with indirect coverage changes

Flag Coverage Δ
unit ?
valgrind 64.00078% <0.00000%> (-18.53875%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Introduce a new request property `request.is_subrequest` to indicate
whether the current request is a subrequest of an existing request
(e.g., in slicing scenarios).

Note: Although the property uses the `request.*` prefix, this is not an
Envoy attribute.
@dev-null-undefined dev-null-undefined force-pushed the feat/request.is_subrequest branch from 99bfad0 to ba0d121 Compare January 15, 2025 17:48
@thibaultcha
Copy link
Member

Thank you. It looks like another test in the same suite is missing the new property and causing all these CI failures.


if (r == r->main) {
ngx_str_set(value, "false");
} else {
Copy link
Member

Choose a reason for hiding this comment

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

The linter will also complain about this else which is missing the blank line above. We should actually do this logic like connection.mtls, and move the static true/false strings outside of the mtls getter so that we can have the same value = r == r->main ? true : false logic (see connection.mtls getter).

@thibaultcha
Copy link
Member

A rebase on main will also be necessary as it will fix the other CI issues.

@thibaultcha thibaultcha added the pr/merge-in-progress PR: Merge in progress (do not push) label Jan 16, 2025
@thibaultcha
Copy link
Member

Merged with edits in #666. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr/merge-in-progress PR: Merge in progress (do not push)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New property proposal - is_subrequest
2 participants