feat(stream_route): support CIDR in ip match by Zheaoli · Pull Request #4980 · apache/apisix

Zheaoli · 2021-09-03T11:51:35Z

What this PR does / why we need it:

Pre-submission checklist:

Did you explain what problem does this PR solve? Or what new features have been added?
Have you added corresponding test cases?
Have you modified the corresponding document?
Is this PR backward compatible? If it is not backward compatible, please discuss on the mailing list first

Zheaoli · 2021-09-04T08:14:52Z

#4441

nic-chen

need test cases

tokers · 2021-09-05T09:12:52Z

-       route.value.remote_addr ~= vars.remote_addr then
-        return false
+    if route.value.remote_addr then
+        local ip = remote_addr_match_cache[route.value.remote_addr]


It's dangerous to use a module-level table to cache the match result, since it may cause the memory leak;

The IP match overheads are not so high, the caching is unnecessary;

Make sense, I'll update mycode

spacewander · 2021-09-06T03:18:45Z

-    if route.value.remote_addr and
-       route.value.remote_addr ~= vars.remote_addr then
-        return false
+    if route.value.remote_addr then


Better to put the init in the create_router instead of the match

Sorry I don't get your means, you mean remove the match function?

spacewander · 2021-09-06T03:20:47Z



-=== TEST 9: set route with invalid host
+=== TEST 9: set route with IP CIDR


New test should be added from the bottom of the file. And it should be in stream-node/sanity.t

spacewander · 2021-09-06T03:21:04Z

    "ext-plugin-proto = 0.3.0",
    "casbin = 1.26.0",
    "api7-snowflake = 2.0-1",
+    "lua-resty-ipmatcher = 0.6.1"


Don't add lua-resty-ipmatcher twice

spacewander · 2021-09-06T07:43:45Z

+
+
+
+=== TEST 11: set stream route (id: 1) which uses upstream_id


The name should show the difference from the others, like CIDR

tzssangglass · 2021-09-06T09:14:01Z

+--- stream_response
+hello world
+--- no_error_log
+[error]


Add blank line at the end

spacewander · 2021-09-07T01:01:30Z

+
+            if not item.value.remote_addr_matcher and item.value.remote_addr then
+                local ip = create_matcher(item.value.remote_addr)
+                if ip then


We can write a new checker in

apisix/apisix/stream/router/ip_port.lua

Line 185 in df0db95

checker = plugin_checker,

to ensure the matcher can be always created?

We can use the methods in https://github.com/apache/apisix/blob/master/apisix/core/ip.lua to do this.

We can use validate_cidr_or_ip to validate.

I think it's unnecessary to use validate_cidr_or_ip here, we just need to check the remote_addr and server_addr is nil or not. the ipmatcher will help to validate the address when create a new match object by using new method

spacewander · 2021-09-09T03:39:12Z


            local route = item.value
+
+            if not item.value.remote_addr_matcher and validate_address(item.value.remote_addr) then


Please add the check to

apisix/apisix/stream/router/ip_port.lua

Line 185 in df0db95

checker = plugin_checker,

spacewander · 2021-09-13T03:40:31Z

+
+
+
+=== TEST 11: set stream route (id: 1) which uses upstream_id and remote address with IP CIDR


Let's add a test that the conf is rejected by validate_cidr_or_ip

Signed-off-by: spacewander <[email protected]>

Zheaoli force-pushed the use-ipmatcher branch from ac5539d to f76a4e9 Compare September 3, 2021 18:47

Zheaoli changed the title ~~[WIP] replace ip match tools by using ipmatcher~~ fix[stream_route]: replace ip match tools by using ipmatcher Sep 4, 2021

Zheaoli changed the title ~~fix[stream_route]: replace ip match tools by using ipmatcher~~ fix(stream_route): replace ip match tools by using ipmatcher Sep 4, 2021

nic-chen reviewed Sep 4, 2021

View reviewed changes

Zheaoli force-pushed the use-ipmatcher branch from 87d9c54 to d01a16f Compare September 5, 2021 08:21

tokers reviewed Sep 5, 2021

View reviewed changes

Zheaoli force-pushed the use-ipmatcher branch from d01a16f to 01e38c8 Compare September 5, 2021 13:38

spacewander reviewed Sep 6, 2021

View reviewed changes

tzssangglass reviewed Sep 6, 2021

View reviewed changes

Zheaoli force-pushed the use-ipmatcher branch 5 times, most recently from 8d29fc1 to b53220c Compare September 6, 2021 16:48

tzssangglass approved these changes Sep 7, 2021

View reviewed changes

spacewander reviewed Sep 7, 2021

View reviewed changes

Zheaoli force-pushed the use-ipmatcher branch 2 times, most recently from d601ef4 to 6f27129 Compare September 8, 2021 09:33

spacewander reviewed Sep 9, 2021

View reviewed changes

replace ip match tools by using ipmatcher

a986e59

Zheaoli force-pushed the use-ipmatcher branch from 49ca149 to a986e59 Compare September 9, 2021 18:46

spacewander reviewed Sep 10, 2021

View reviewed changes

Comment thread apisix/stream/router/ip_port.lua Outdated

Comment thread apisix/stream/router/ip_port.lua Outdated

Comment thread apisix/stream/router/ip_port.lua Outdated

Zheaoli force-pushed the use-ipmatcher branch 2 times, most recently from d311852 to 0720a4a Compare September 12, 2021 14:01

add extra comment

075b646

Zheaoli force-pushed the use-ipmatcher branch from 0720a4a to 075b646 Compare September 12, 2021 14:02

spacewander reviewed Sep 13, 2021

View reviewed changes

t

2848c14

Signed-off-by: spacewander <[email protected]>

fix lint

1ea3e10

Signed-off-by: spacewander <[email protected]>

spacewander changed the title ~~fix(stream_route): replace ip match tools by using ipmatcher~~ feat(stream_route): support CIDR in ip match Sep 17, 2021

spacewander approved these changes Sep 17, 2021

View reviewed changes

spacewander merged commit 1a072f4 into apache:master Sep 17, 2021



		=== TEST 9: set route with invalid host
		=== TEST 9: set route with IP CIDR




		=== TEST 11: set stream route (id: 1) which uses upstream_id


		local route = item.value

		if not item.value.remote_addr_matcher and validate_address(item.value.remote_addr) then

Uh oh!

Conversation

Zheaoli commented Sep 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this PR does / why we need it:

Pre-submission checklist:

Uh oh!

Zheaoli commented Sep 4, 2021

Uh oh!

nic-chen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Zheaoli commented Sep 3, 2021 •

edited

Loading