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

lua: add request info dynamic metadata api #3627

Closed
wants to merge 8 commits into from
Closed

lua: add request info dynamic metadata api #3627

wants to merge 8 commits into from

Conversation

dio
Copy link
Member

@dio dio commented Jun 14, 2018

lua: add request info dynamic metadata API

Description:
This patch allows to get and set the dynamic metadata of a request via Lua.

Setting metadata,

request_handle:requestInfo():dynamicMetadata():set("envoy.lb", "foo", "bar")

Getting metadata,

-- returns a table
request_handle:requestInfo():dynamicMetadata():get("envoy.lb")
request_handle:requestInfo():dynamicMetadata():get("envoy.lb")["foo"]

Risk Level: Low

Testing: Unit and integration tests

Docs Changes:

  • Added request info object API
  • Added dynamic metadata object API

Release Notes:

  • Added request info dynamic metadata Lua API

Fixes #3618

dio added 2 commits June 13, 2018 22:32
This patch add access to request info's dynamic metadata from lua.

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
@stale
Copy link

stale bot commented Jun 21, 2018

This pull request has been automatically marked as stale because it has not had activity in the last 7 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!

@stale stale bot added the stale stalebot believes this issue/PR has not been touched recently label Jun 21, 2018
@mattklein123
Copy link
Member

@dio is this still WIP or does this need a reviewer?

@stale stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Jun 22, 2018
@dio
Copy link
Member Author

dio commented Jun 22, 2018

@mattklein123 yes it is still WIP I will add docs, support for accepting a table as param then I'll ask for a review.

Two questions,

  1. Do you think this is still considered an enhancement? Since there is an effort here: 827c0a5 for making it accessible via config.
  2. While this is part of the review, what do you think about the proposed API?

Thanks!

dio added 2 commits June 23, 2018 13:42
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
@dio dio changed the title WIP: lua: add request info dynamic metadata api lua: add request info dynamic metadata api Jun 23, 2018
dio added 3 commits June 23, 2018 15:43
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
}

int RequestInfoWrapper::luaProtocol(lua_State* state) {
switch (request_info_.protocol().value()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I have not looked at the entire code. But one quick comment here, this whole switch can be replaced with this method call

Copy link
Member Author

Choose a reason for hiding this comment

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

Gotcha! Thanks @ramaraochavali!

@mattklein123
Copy link
Member

Do you think this is still considered an enhancement?

@dio what do you mean? Are you asking if I think this feature is no longer necessary? TBH I don't know I haven't been tracking all of this very closely. In general I would prefer there is only a single way to do something where reasonable, but if there are good reasons to have multiple ways am open to it. Thoughts?

@mattklein123 mattklein123 self-assigned this Jun 27, 2018
@ramaraochavali
Copy link
Contributor

Not sure what @dio is referring to here. But exposing Protocol Identification attributes as described in the issue #3704 is definitely required for us and I think would be a reasonable addition.

@dio
Copy link
Member Author

dio commented Jun 28, 2018

Sorry for introducing confusion. Will breakdown this PR into:

  1. RequestInfo with protocol()
  2. RequestInfo with dynamicMetadata()
  3. Connection with secure() (whether the current conn using SSL or not).

I’ll continue this hopefully tonight. Thanks

@ramaraochavali
Copy link
Contributor

@dio sure..no problem.. Thanks for doing #1 and #3. lmk if it becomes too much work for you.

@dio
Copy link
Member Author

dio commented Jun 30, 2018

@ramaraochavali @mattklein123

I have put PRs related to #3704:

  1. requestInfo():protocol() in lua: add requestInfo():protocol() API #3760
  2. connection():ssl() in lua: add connection():ssl() API #3761

While this PR will be layered on top of #3760 if approved and merged.

Thanks.

@dio
Copy link
Member Author

dio commented Jul 2, 2018

Closing this for now in favor of #3760 for preparing the requestInfo wrapper.

Related: #3761

@dio dio closed this Jul 2, 2018
@dio
Copy link
Member Author

dio commented Jul 6, 2018

Resubmitted as #3800 for adding dynamic metadata API

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.

3 participants