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.

Sign up for GitHub

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

Jump to bottom

lua: add new methods to access network connection streamInfo() & dynamicMetadata() #22246

Closed
wants to merge 1 commit into from
Closed

lua: add new methods to access network connection streamInfo() & dynamicMetadata() #22246

wants to merge 1 commit into from

Conversation

agrawroh
Copy link
Contributor

Background

Currently, there seems to be no way to access the dynamic metadata set by any network filters in LUA or any other HTTP filters. streamInfo() is accessible on the connection and could be used to fetch the dynamicMetadata() from the network filters but it's is currently not exposed in HTTP LUA filter.

Use Case

We want to read the TLV values set by the proxy_protocol filter in the dynamic metadata in the LUA filter so that we can propagate these TLV pairs to the upstream services for some further processing.

Changes

This PR adds a new connectionStreamInfo() methods to LUA filter that could be used to access the Stream Info object on the connection and then fetch the dynamic metadata from network filters like this:

connectionStreamInfo():dynamicMetadata():get("envoy.proxy_protocol")

Commit Message: add new methods to access network connection streamInfo() & dynamicMetadata().
Additional Description: See Background Section.
Risk Level: Low
Testing: Unit Tests
Docs Changes: Added the description of the new methods added in LUA.
Release Notes: Added
Platform Specific Features: N/A

Signed-off-by: Rohit Agrawal [email protected]

@repokitteh-read-only
Copy link

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #22246 was opened by agrawroh.

see: more, trace.

@agrawroh agrawroh force-pushed the add-cx-stream-info branch 2 times, most recently from 8937634 to 5c71b9c Compare July 19, 2022 00:02
@agrawroh
Copy link
Contributor Author

cc @ggreenway

@agrawroh
Copy link
Contributor Author

/retest

@repokitteh-read-only
Copy link

Retrying Azure Pipelines:
Check envoy-presubmit isn't fully completed, but will still attempt retrying.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #22246 (comment) was created by @agrawroh.

see: more, trace.

@agrawroh agrawroh force-pushed the add-cx-stream-info branch from 5c71b9c to af7181b Compare July 19, 2022 19:15
@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@github-actions github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Aug 18, 2022
@dynajoe
Copy link

dynajoe commented Aug 19, 2022

My team is interested in seeing this PR get through. We too need to get proxy protocol dynamic metadata at the HCM to forward to upstream. Is there an alternative?

@github-actions github-actions bot removed the stale stalebot believes this issue/PR has not been touched recently label Aug 19, 2022
@wbpcode
Copy link
Member

wbpcode commented Aug 24, 2022

/assign

@agrawroh agrawroh marked this pull request as ready for review August 24, 2022 16:20
wbpcode
wbpcode reviewed Aug 25, 2022
View reviewed changes
Copy link
Member

@wbpcode wbpcode left a comment

Choose a reason for hiding this comment

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

thanks for working on this. it make sense to me.

source/extensions/filters/http/lua/lua_filter.cc
Comment on lines +137 to +139
lua_state_.registerType<ConnectionStreamInfoWrapper>();
lua_state_.registerType<ConnectionDynamicMetadataMapWrapper>();
lua_state_.registerType<ConnectionDynamicMetadataMapIterator>();
Copy link
Member

Choose a reason for hiding this comment

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

should the stream info and dynamic metadata need independent type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good question. I wasn't aware of any implications for re-using the one we have for HTTP-level stream info object so I created separate ones for the listener-level stream info. If you think it's fine to reuse the existing ones then we can remove it.

Copy link
Member

Choose a reason for hiding this comment

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

That's a good question. I wasn't aware of any implications for re-using the one we have for HTTP-level stream info object so I created separate ones for the listener-level stream info. If you think it's fine to reuse the existing ones then we can remove it.

Yeah, I think it would be better to just reuse this wrapper.

@mattklein123
Copy link
Member

@wbpcode friendly ping on review.

wbpcode
wbpcode reviewed Sep 2, 2022
View reviewed changes
Copy link
Member

@wbpcode wbpcode left a comment •
edited
Loading

Choose a reason for hiding this comment

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

It's LGTM overall, except I think it would be better to reuse the exist type wrapper. Thanks.

And sorry for the delayed review.

/wait-any

source/extensions/filters/http/lua/lua_filter.cc
Comment on lines +137 to +139
lua_state_.registerType<ConnectionStreamInfoWrapper>();
lua_state_.registerType<ConnectionDynamicMetadataMapWrapper>();
lua_state_.registerType<ConnectionDynamicMetadataMapIterator>();
Copy link
Member

Choose a reason for hiding this comment

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

That's a good question. I wasn't aware of any implications for re-using the one we have for HTTP-level stream info object so I created separate ones for the listener-level stream info. If you think it's fine to reuse the existing ones then we can remove it.

Yeah, I think it would be better to just reuse this wrapper.

@github-actions
Copy link

github-actions bot commented Oct 2, 2022

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@github-actions github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Oct 2, 2022
@github-actions
Copy link

github-actions bot commented Oct 9, 2022

This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@github-actions github-actions bot closed this Oct 9, 2022
@dynajoe
Copy link

dynajoe commented Nov 28, 2022

@agrawroh what's the next step to enable access to metadata?

@wbpcode wbpcode removed the stale stalebot believes this issue/PR has not been touched recently label Jan 29, 2023
@fedya-at-db fedya-at-db mentioned this pull request Mar 7, 2024
Closed
@agrawroh agrawroh mentioned this pull request Apr 11, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wbpcode wbpcode wbpcode left review comments

@mattklein123 mattklein123 Awaiting requested review from mattklein123 mattklein123 is a code owner

Assignees

@wbpcode wbpcode

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@agrawroh @dynajoe @wbpcode @mattklein123