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

feat: handle_extra_info add set req body #8884

Closed
wants to merge 2 commits into from
Closed

feat: handle_extra_info add set req body #8884

wants to merge 2 commits into from

Conversation

hongdingyi
Copy link

@hongdingyi hongdingyi commented Feb 20, 2023 •
edited
Loading

Description

Fixes # (issue)

Checklist

  • I have explained the need for this PR and the problem it solves
  • I have explained the changes or the new features added to this PR
  • I have added tests corresponding to this change
  • I have updated the documentation to reflect this change
  • I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

@hongdingyi
Copy link
Author

fix:handle_extra_info

@hongdingyi
Copy link
Author

image

@hongdingyi
Copy link
Author

You should add a type。in apisix/apisix-3.1.0/deps/share/lua/5.1/A6/ExtraInfo

@hongdingyi
Copy link
Author

add extra info type

spacewander
spacewander reviewed Feb 21, 2023
View reviewed changes
Copy link
Member

@spacewander spacewander left a comment

Choose a reason for hiding this comment

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

Let's add test in https://github.com/apache/apisix/blob/master/t/plugin/ext-plugin/request-body.t

apisix/plugins/ext-plugin/init.lua
@@ -288,7 +288,14 @@ local function handle_extra_info(ctx, input)

local res
local info_type = req:InfoType()
if info_type == extra_info.Var then
if info_type == extra_info.RespBody + 1 then --4 set_body
Copy link
Member

Choose a reason for hiding this comment

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

Could you add the new field in https://github.com/api7/ext-plugin-proto?
You can read https://github.com/apache/apisix/blob/master/docs/en/latest/internal/plugin-runner.md for the implemented detail.

@rubikplanet
Copy link
Contributor

feature required

@github-actions
Copy link

This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 4 weeks if no further activity occurs. If you think that's incorrect or this pull request should instead be reviewed, please simply write any comment. Even if closed, you can still revive the PR at any time or discuss it on the dev@apisix.apache.org list. Thank you for your contributions.

@github-actions github-actions bot added the stale label Sep 26, 2023
@moonming moonming removed the stale label Oct 9, 2023
@moonming
Copy link
Member

moonming commented Oct 9, 2023

Is this the same as #9990? @monkeyDluffy6017

@monkeyDluffy6017
Copy link
Contributor

@moonming It's two different things

@moonming
Copy link
Member

moonming commented Oct 9, 2023

@moonming It's two different things

So what problems and scenarios does this PR solve? @monkeyDluffy6017

@moonming moonming requested review from shreemaan-abhishek and removed request for monkeyDluffy6017 October 18, 2023 00:56
@shreemaan-abhishek
Copy link
Contributor

@hongdingyi what problem does this PR solve?

@Revolyssup
Copy link
Contributor

This PR requires to add a field in this file https://github.com/api7/ext-plugin-proto/blob/4af76c210e00ead699489cfeb055d8b9bb7596ed/ext-plugin.fbs#L121

@moonming @shreemaan-abhishek @monkeyDluffy6017
As far as I can see, this PR and #9990 are kind of similar. Both set the request's body but at different stage.
The one that is merged sets the request body by signalling it in the response of the RPC. After that the request is sent to the upstream.
This PR on the other hand allows to set the request body using handle_extra_info function which facilitates communication between plugin runner and APISIX while the plugin runner is still processing the request. Here after the request is set, the request won't be sent to upstream immediately as the Reply isn't yet sent by the plugin runner. The plugin runner can do some more things.

Conclusion:
According to me, this PR is not required because after the request is set to something, why should plugin runner do anything else on it. It doesn't seem like something that should happen with handle_extra_info. This can be achieved by the reply of the HTTPReq RPC call.

@hongdingyi In case I am missing some scenario where this actually will be required and just setting the request body using the Reply of the RPC wouldn't justify the use case, please let me know.

@shreemaan-abhishek
Copy link
Contributor

let's wait for @hongdingyi's input. It seems the github actions were not triggered properly so I just made them re-run.

@shreemaan-abhishek shreemaan-abhishek added the wait for update wait for the author's response in this issue/PR label Oct 31, 2023
@github-actions GitHub Actions
Copy link

This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 4 weeks if no further activity occurs. If you think that's incorrect or this pull request should instead be reviewed, please simply write any comment. Even if closed, you can still revive the PR at any time or discuss it on the dev@apisix.apache.org list. Thank you for your contributions.

@github-actions github-actions bot added the stale label Dec 30, 2023
@monkeyDluffy6017
Copy link
Contributor

@hongdingyi are you still working on the pr?

@github-actions github-actions bot removed the stale label Jan 4, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Mar 4, 2024

This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 4 weeks if no further activity occurs. If you think that's incorrect or this pull request should instead be reviewed, please simply write any comment. Even if closed, you can still revive the PR at any time or discuss it on the dev@apisix.apache.org list. Thank you for your contributions.

@github-actions github-actions bot added the stale label Mar 4, 2024
@Revolyssup
Copy link
Contributor

@hongdingyi Can you fix the lint check?

@github-actions github-actions bot removed the stale label Mar 14, 2024
@Revolyssup
Copy link
Contributor

closed because reason not provided

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@spacewander spacewander spacewander left review comments

@shreemaan-abhishek shreemaan-abhishek Awaiting requested review from shreemaan-abhishek

Assignees

@hongdingyi hongdingyi

@Revolyssup Revolyssup

Labels
wait for update wait for the author's response in this issue/PR
Projects
Apache APISIX backlog
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@hongdingyi @rubikplanet @moonming @monkeyDluffy6017 @shreemaan-abhishek @Revolyssup @spacewander