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

change: refactor logic for enabling L4/L7 proxy #9607

Merged
merged 21 commits into from Jul 12, 2023

Conversation

Revolyssup
Copy link
Contributor

@Revolyssup Revolyssup commented Jun 6, 2023

Description

Fixes #9600
This PR refactors the logic as explained in the issue above.

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)

Signed-off-by: revolyssup <ashishjaitiwari15112000@gmail.com>
@Revolyssup
Copy link
Contributor Author

@leslie-tsang @monkeyDluffy6017 Please review and make suggestions.

leslie-tsang
leslie-tsang previously approved these changes Jun 6, 2023
conf/config-default.yaml Outdated Show resolved Hide resolved
Co-authored-by: leslie <leslie@apache.org>
local stream = "stream"
if yaml_conf.apisix.proxy_mode then
-- check for "http" as prefix
if string.sub(yaml_conf.apisix.proxy_mode,1,#http) ~= http then
Copy link
Contributor

Choose a reason for hiding this comment

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

Literally compare with "http", "stream" and "http&stream" here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

proxy_mode = {
type = "string",
enum = {"http", "stream", "http&stream"},
},
Copy link
Contributor

Choose a reason for hiding this comment

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

fix the indent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Signed-off-by: revolyssup <ashishjaitiwari15112000@gmail.com>
Signed-off-by: revolyssup <ashishjaitiwari15112000@gmail.com>
Signed-off-by: revolyssup <ashishjaitiwari15112000@gmail.com>
@monkeyDluffy6017 monkeyDluffy6017 changed the title fix: Refactor logic for enabling L4/L7 proxy change: refactor logic for enabling L4/L7 proxy Jun 12, 2023
@monkeyDluffy6017
Copy link
Contributor

Hi @Revolyssup, test cases are needed

@monkeyDluffy6017
Copy link
Contributor

monkeyDluffy6017 commented Jun 12, 2023

You need to remove the configuration stream_proxy.only in all files, like

  1. https://github.com/apache/apisix/blob/master/t/cli/test_tls_over_tcp.sh#L26
  2. https://github.com/apache/apisix/blob/master/apisix/cli/ops.lua#L274

Revolyssup and others added 5 commits June 12, 2023 10:11
Co-authored-by: Liu Wei <375636559@qq.com>
Co-authored-by: Liu Wei <375636559@qq.com>
Signed-off-by: revolyssup <ashishjaitiwari15112000@gmail.com>
Signed-off-by: revolyssup <ashishjaitiwari15112000@gmail.com>
Signed-off-by: revolyssup <ashishjaitiwari15112000@gmail.com>
@Revolyssup
Copy link
Contributor Author

Revolyssup commented Jun 12, 2023

You need to remove the configuration stream_proxy.only in all files, like

  1. https://github.com/apache/apisix/blob/master/t/cli/test_tls_over_tcp.sh#L26
  2. https://github.com/apache/apisix/blob/master/apisix/cli/ops.lua#L274

I have changed all test files to respect proxy_mode and removed stream_proxy.only

Co-authored-by: Liu Wei <375636559@qq.com>
@Revolyssup
Copy link
Contributor Author

@monkeyDluffy6017 Please approve CI again.

@monkeyDluffy6017
Copy link
Contributor

@Revolyssup I find that many places in our code rely on the stream_proxy variable to control the stream switch, so if you want to use proxy_mode to control it, you'll need to change these places, like

  1. https://github.com/apache/apisix/blob/master/apisix/cli/ngx_tpl.lua#L127
  2. https://github.com/apache/apisix/blob/master/apisix/admin/init.lua#L177

@Revolyssup
Copy link
Contributor Author

@monkeyDluffy6017 I could find only those two places left to make that change. Can you approve CI now? Or point out if there is some other place I missed. I searched for stream_proxy in repo and have made the change in control statements

@monkeyDluffy6017
Copy link
Contributor

monkeyDluffy6017 commented Jun 21, 2023

Please search only: and delete the related code

@monkeyDluffy6017
Copy link
Contributor

Please make the ci pass

@monkeyDluffy6017 monkeyDluffy6017 added wait for update wait for the author's response in this issue/PR and removed need test cases labels Jun 21, 2023
Signed-off-by: revolyssup <ashishjaitiwari15112000@gmail.com>
@Revolyssup
Copy link
Contributor Author

Please search only: and delete related code

done

Signed-off-by: revolyssup <ashishjaitiwari15112000@gmail.com>
@monkeyDluffy6017
Copy link
Contributor

@Revolyssup Please make the ci pass

@Revolyssup
Copy link
Contributor Author

@Revolyssup Please make the ci pass

@monkeyDluffy6017 Yeah, I am trying to figure out this is failing Details
Do you have a clue?

@monkeyDluffy6017
Copy link
Contributor

It says failed to enable stream proxy and http proxy. This should be caused by your changes, perhaps you should check the corresponding test cases

Signed-off-by: revolyssup <ashishjaitiwari15112000@gmail.com>
Signed-off-by: revolyssup <ashishjaitiwari15112000@gmail.com>
@Revolyssup
Copy link
Contributor Author

@monkeyDluffy6017 approve CI please

@Revolyssup
Copy link
Contributor Author

Most tests failed because of unrelated single reason. I think we need to re run the tests.

Signed-off-by: revolyssup <ashishjaitiwari15112000@gmail.com>
Signed-off-by: Ashish Tiwari <ashishjaitiwari15112000@gmail.com>
Signed-off-by: Ashish Tiwari <ashishjaitiwari15112000@gmail.com>
Signed-off-by: Ashish Tiwari <ashishjaitiwari15112000@gmail.com>
@monkeyDluffy6017 monkeyDluffy6017 added approved and removed wait for update wait for the author's response in this issue/PR labels Jul 10, 2023
@Sn0rt
Copy link
Contributor

Sn0rt commented Jul 11, 2023

@leslie-tsang pls take a review .

@monkeyDluffy6017 monkeyDluffy6017 merged commit a45c395 into apache:master Jul 12, 2023
40 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

discussion: refactor L4 and/or L7 Proxy Enablement
5 participants