Skip to content

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

Merged
juststillthinking merged 21 commits into
apache:masterfrom
Revolyssup:revolyssup/fix9600
Jul 12, 2023
Merged

change: refactor logic for enabling L4/L7 proxy#9607
juststillthinking merged 21 commits into
apache:masterfrom
Revolyssup:revolyssup/fix9600

Conversation

@Revolyssup

@Revolyssup Revolyssup commented Jun 6, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

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)

@Revolyssup

Revolyssup commented Jun 6, 2023

Copy link
Copy Markdown
Contributor Author

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

leslie-tsang
leslie-tsang previously approved these changes Jun 6, 2023
View reviewed changes
Comment thread conf/config-default.yaml Outdated
@leslie-tsang
leslie-tsang self-requested a review June 6, 2023 08:55
kingluo
kingluo reviewed Jun 7, 2023
View reviewed changes
Comment thread apisix/cli/ops.lua Outdated
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

@kingluo kingluo Jun 7, 2023

Copy link
Copy Markdown
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.

@Revolyssup Revolyssup Jun 7, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

Comment thread apisix/cli/schema.lua Outdated
proxy_mode = {
type = "string",
enum = {"http", "stream", "http&stream"},
},

@kingluo kingluo Jun 7, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

fix the indent.

@Revolyssup Revolyssup Jun 7, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

Revolyssup added 4 commits June 7, 2023 15:49
@Revolyssup
apply suggestions
da42c40 
Signed-off-by: revolyssup <[email protected]>
@Revolyssup
Merge branch 'revolyssup/fix9600' of github.com:Revolyssup/apisix int…
12382e2 
…o revolyssup/fix9600
@Revolyssup
fix doc lint
7b2b148 
Signed-off-by: revolyssup <[email protected]>
@Revolyssup
fix lint
80de85b 
Signed-off-by: revolyssup <[email protected]>
@juststillthinking juststillthinking changed the title fix: Refactor logic for enabling L4/L7 proxy change: refactor logic for enabling L4/L7 proxy Jun 12, 2023
juststillthinking
juststillthinking reviewed Jun 12, 2023
View reviewed changes
Comment thread conf/config-default.yaml Outdated
juststillthinking
juststillthinking reviewed Jun 12, 2023
View reviewed changes
Comment thread conf/config-default.yaml
juststillthinking
juststillthinking reviewed Jun 12, 2023
View reviewed changes
Comment thread docs/en/latest/stream-proxy.md Outdated
@juststillthinking

juststillthinking commented Jun 12, 2023

Copy link
Copy Markdown
Contributor

Hi @Revolyssup, test cases are needed

@juststillthinking

juststillthinking commented Jun 12, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

Revolyssup commented Jun 12, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

@Revolyssup

Revolyssup commented Jun 15, 2023

Copy link
Copy Markdown
Contributor Author

@monkeyDluffy6017 Please approve CI again.

@juststillthinking

juststillthinking commented Jun 15, 2023

Copy link
Copy Markdown
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

juststillthinking
juststillthinking reviewed Jun 21, 2023
View reviewed changes
Comment thread docs/en/latest/stream-proxy.md
juststillthinking
juststillthinking reviewed Jun 21, 2023
View reviewed changes
Comment thread apisix/cli/ngx_tpl.lua Outdated
@juststillthinking

juststillthinking commented Jun 21, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Please search only: and delete the related code

@juststillthinking

juststillthinking commented Jun 21, 2023

Copy link
Copy Markdown
Contributor

Please make the ci pass

@juststillthinking juststillthinking added wait for update wait for the author's response in this issue/PR and removed need test cases labels Jun 21, 2023
@Revolyssup

Revolyssup commented Jun 21, 2023

Copy link
Copy Markdown
Contributor Author

Please search only: and delete related code

done

@juststillthinking

juststillthinking commented Jun 26, 2023

Copy link
Copy Markdown
Contributor

@Revolyssup Please make the ci pass

@Revolyssup

Revolyssup commented Jun 27, 2023

Copy link
Copy Markdown
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?

@juststillthinking

juststillthinking commented Jun 28, 2023

Copy link
Copy Markdown
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

@Revolyssup
fix merge conflicts
6c692f5 
Signed-off-by: revolyssup <[email protected]>
@Revolyssup
fix failing CI
408b254 
Signed-off-by: revolyssup <[email protected]>
@Revolyssup

Revolyssup commented Jun 29, 2023

Copy link
Copy Markdown
Contributor Author

@monkeyDluffy6017 approve CI please

@Revolyssup

Revolyssup commented Jun 29, 2023

Copy link
Copy Markdown
Contributor Author

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

@Revolyssup
fix lint
f659ea7 
Signed-off-by: revolyssup <[email protected]>
juststillthinking
juststillthinking reviewed Jul 6, 2023
View reviewed changes
Comment thread conf/config-default.yaml Outdated
Revolyssup added 3 commits July 7, 2023 09:35
@Revolyssup
remove duplication
b3f8dff 
Signed-off-by: Ashish Tiwari <[email protected]>
@Revolyssup
sync config-default with master
1b87366 
Signed-off-by: Ashish Tiwari <[email protected]>
@Revolyssup
solve merge conflict
ebf9ddc 
Signed-off-by: Ashish Tiwari <[email protected]>
@juststillthinking juststillthinking added approved and removed wait for update wait for the author's response in this issue/PR labels Jul 10, 2023
@Sn0rt

Sn0rt commented Jul 11, 2023

Copy link
Copy Markdown
Contributor

@leslie-tsang pls take a review .

@juststillthinking
juststillthinking merged commit a45c395 into apache:master Jul 12, 2023
@AlinsRan AlinsRan mentioned this pull request Sep 5, 2023
Merged
@juststillthinking juststillthinking mentioned this pull request Oct 15, 2023
Merged
5 tasks
@kayx23 kayx23 mentioned this pull request Mar 26, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kingluo kingluo kingluo approved these changes
@leslie-tsang leslie-tsang Awaiting requested review from leslie-tsang
+2 more reviewers
@Sn0rt Sn0rt Sn0rt left review comments
@juststillthinking juststillthinking juststillthinking approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

discussion: refactor L4 and/or L7 Proxy Enablement

5 participants

@Revolyssup @juststillthinking @Sn0rt @kingluo @leslie-tsang