feat: add limit-count plugin #1739

LiteSun · 2021-04-10T15:53:53Z

Please answer these questions before submitting a pull request, or your PR will get closed.

Why submit this pull request?

Bugfix
New feature provided
Improve performance
Backport patches

What changes will this PR take into?

Please update this section with detailed description.

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

juzhiyuan · 2021-04-10T16:17:19Z

Please add your description for this PR

codecov-io · 2021-04-11T02:23:53Z

Codecov Report

Merging #1739 (4325300) into master (419128d) will increase coverage by 0.21%.
The diff coverage is 84.84%.

@@            Coverage Diff             @@
##           master    #1739      +/-   ##
==========================================
+ Coverage   72.57%   72.79%   +0.21%     
==========================================
  Files         113      114       +1     
  Lines        2695     2727      +32     
  Branches      650      655       +5     
==========================================
+ Hits         1956     1985      +29     
- Misses        739      742       +3

Flag	Coverage Δ
frontend-e2e-test	`72.79% <84.84%> (+0.21%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
web/src/components/Plugin/UI/limit-count.tsx	`83.33% <83.33%> (ø)`
web/src/components/Plugin/UI/plugin.tsx	`69.23% <100.00%> (+5.59%)`	⬆️
...ages/Route/components/DebugViews/DebugDrawView.tsx	`79.86% <0.00%> (+1.34%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 419128d...4325300. Read the comment docs.

netlify · 2021-04-11T10:51:43Z

Deploy preview for apisix-dashboard ready!

Built with commit d896705

https://deploy-preview-1739--apisix-dashboard.netlify.app

juzhiyuan

Please update this PR's description

juzhiyuan · 2021-04-11T12:14:01Z

web/src/components/Plugin/UI/limit-count.tsx

+        tooltip={formatMessage({ id: 'component.pluginForm.limit-count.policy.tooltip' })}
+      >
+        <Select onChange={(e: PolicyProps) => { setPoicy(e) }}>
+          {["local", "redis", "redis-cluster"].map(item => (<Select.Option value={item}>{item}</Select.Option>))}


web/src/components/Plugin/UI/limit-count.tsx

moonming · 2021-04-11T23:59:48Z

web/cypress/integration/consumer/create-consumer-with-limit-count-plugin-form.spec.js

+    redis_host: '127.0.0.1',
+    redis_password: 'redis_password',
+    redis_cluster_name: 'redis_cluster_name',
+    redis_cluster_nodes_0: "redis.cluster1",
+    redis_cluster_nodes_1: "redis.cluster1",


What are these default values？

Not a good default values, will update later

web/cypress/integration/consumer/create-consumer-with-limit-count-plugin-form.spec.js

liuxiran · 2021-04-12T07:10:26Z

web/src/components/Plugin/locales/zh-CN.ts

+  // limit-count
+  'component.pluginForm.limit-count.count.tooltip': '指定时间窗口内的请求数量阈值。',
+  'component.pluginForm.limit-count.time_window.tooltip': '时间窗口的大小（以秒为单位），超过这个时间就会重置。',
+  'component.pluginForm.limit-count.key.tooltip': '用来做请求计数的有效值。例如，可以使用主机名（或服务器区域）作为关键字，以便限制每个主机名规定时间内的请求次数。我们也可以使用客户端地址作为关键字，这样我们就可以避免单个客户端规定时间内多次的连接我们的服务。当前接受的 key 有："remote_addr"（客户端 IP 地址）, "server_addr"（服务端 IP 地址）, 请求头中的"X-Forwarded-For" 或 "X-Real-IP", "consumer_name"（consumer 的 username）, "service_id" 。',


May I ask what is the meaning of 服务器区域?

just use 服务器地址

web/src/components/Plugin/UI/limit-count.tsx

liuxiran · 2021-04-12T08:07:08Z

redist Doms lost when edited once again

test step:
1.
configure the limit-count and submit

click enable button to edit it again, you will get the trouble

@LiteSun

LiteSun · 2021-04-12T08:30:14Z

redist Doms lost when edited once again

click enable button to edit it again, you will get the trouble

@LiteSun

Tks for your report. I will check it.

LiteSun · 2021-04-12T09:26:01Z

redist Doms lost when edited once again

@liuxiran fixed.

web/src/components/Plugin/UI/limit-count.tsx

juzhiyuan · 2021-04-14T09:30:15Z

web/src/components/Plugin/UI/limit-count.tsx

+
+  return (<>
+    <Form.Item
+      label="redis_host"


emmm would better have i18n for those fields, for users may not know what's it

juzhiyuan · 2021-04-14T09:35:45Z

web/src/components/Plugin/UI/limit-count.tsx

+      <Form.Item
+        label="rejected_code"
+        name="rejected_code"
+        tooltip={formatMessage({ id: 'component.pluginForm.limit-count.rejected_code.tooltip' })}


default value is 503

juzhiyuan · 2021-04-14T09:37:14Z

web/src/components/Plugin/UI/limit-count.tsx

+      name="redis_port"
+      tooltip={formatMessage({ id: 'component.pluginForm.limit-count.redis_port.tooltip' })}
+    >
+      <InputNumber min={1} />


Suggested change

<InputNumber min={1} />

<InputNumber min={1} max={65535} />

juzhiyuan · 2021-04-14T09:37:40Z

web/src/components/Plugin/UI/limit-count.tsx

+    <Form
+      form={form}
+      {...FORM_ITEM_LAYOUT}
+      initialValues={{ key: 'remote_addr', redis_cluster_nodes: ['', ''], policy, redis_port: 6379, redis_database: 0, redis_timeout: 1000 }}


When you submit data and use local policy, will redis_port be 6379?

No, it won't.

juzhiyuan · 2021-04-14T09:38:34Z

web/src/components/Plugin/UI/limit-count.tsx

+      name="redis_timeout"
+      tooltip={formatMessage({ id: 'component.pluginForm.limit-count.redis_timeout.tooltip' })}
+    >
+      <InputNumber />


juzhiyuan · 2021-04-14T09:39:30Z

web/src/components/Plugin/UI/limit-count.tsx

+  </>)
+}
+
+const RedisClusterForm: React.FC<Props> = () => {


What do these Props for?

juzhiyuan · 2021-04-14T09:41:17Z

web/src/components/Plugin/UI/limit-count.tsx

+
+  return (
+    <>
+      <Form.Item


missing two fields: redis_timeout & redis_password.

https://github.com/apache/apisix/blob/master/apisix/plugins/limit-count.lua#L37

juzhiyuan

Don't forget to resolve those comments :)

feat: add limit-count plugin

d8156d0

LiteSun added the plugin form label Apr 10, 2021

feat: update

3eb2053

LiteSun added 3 commits April 11, 2021 17:15

fix: style

40be120

feat: update i18n

8a86b65

test: add testcase

88bd2e4

LiteSun marked this pull request as ready for review April 11, 2021 10:52

LiteSun requested a review from juzhiyuan April 11, 2021 10:52

fix: lint

2b74398

juzhiyuan reviewed Apr 11, 2021

View reviewed changes

feat: update

c173197

moonming requested changes Apr 12, 2021

View reviewed changes

liuxiran reviewed Apr 12, 2021

View reviewed changes

web/src/components/Plugin/UI/limit-count.tsx Outdated Show resolved Hide resolved

liuxiran reviewed Apr 12, 2021

View reviewed changes

web/src/components/Plugin/UI/limit-count.tsx Outdated Show resolved Hide resolved

LiteSun and others added 3 commits April 12, 2021 16:53

fix: dom lost

2cf677e

Merge branch 'master' into limit-count-plugin-form

bcb2ad0

feat: update

80c3e3e

LiteSun requested review from liuxiran, juzhiyuan, moonming, bzp2010 and Jaycean April 12, 2021 09:26

juzhiyuan reviewed Apr 12, 2021

View reviewed changes

web/src/components/Plugin/UI/limit-count.tsx Outdated Show resolved Hide resolved

LiteSun mentioned this pull request Apr 12, 2021

Added more FE testcase for limit-conn plugin form #1757

Closed

LiteSun added 5 commits April 13, 2021 14:56

Merge branch 'master' into limit-count-plugin-form

0fbc35b

feat: update rule

e416191

feat: update testcase

a1b87a6

test: add create-route-with-limit-count-plugin-form.spec

edc362b

Merge branch 'master' into limit-count-plugin-form

4325300

moonming approved these changes Apr 14, 2021

View reviewed changes

membphis approved these changes Apr 14, 2021

View reviewed changes

juzhiyuan reviewed Apr 14, 2021

View reviewed changes

juzhiyuan approved these changes Apr 15, 2021

View reviewed changes

Merge branch 'master' into limit-count-plugin-form

d896705

LiteSun mentioned this pull request Apr 15, 2021

add missing fields for limit-count plugin (redis cluster) #1773

Closed

starsz approved these changes Apr 15, 2021

View reviewed changes

LiteSun merged commit 10cadaa into apache:master Apr 15, 2021

LiteSun deleted the limit-count-plugin-form branch April 15, 2021 02:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add limit-count plugin #1739

feat: add limit-count plugin #1739

LiteSun commented Apr 10, 2021 •

edited

Loading

juzhiyuan commented Apr 10, 2021

codecov-io commented Apr 11, 2021 •

edited

Loading

netlify bot commented Apr 11, 2021 •

edited

Loading

juzhiyuan left a comment

juzhiyuan Apr 11, 2021

moonming Apr 11, 2021

LiteSun Apr 12, 2021

LiteSun Apr 12, 2021

liuxiran Apr 12, 2021 •

edited

Loading

LiteSun Apr 12, 2021

juzhiyuan Apr 14, 2021

liuxiran commented Apr 12, 2021

LiteSun commented Apr 12, 2021 •

edited

Loading

LiteSun commented Apr 12, 2021 •

edited

Loading

juzhiyuan Apr 14, 2021

juzhiyuan Apr 14, 2021

juzhiyuan Apr 14, 2021

juzhiyuan Apr 14, 2021

LiteSun Apr 18, 2021

juzhiyuan Apr 14, 2021

juzhiyuan Apr 14, 2021

juzhiyuan Apr 14, 2021

juzhiyuan left a comment

feat: add limit-count plugin #1739

feat: add limit-count plugin #1739

Conversation

LiteSun commented Apr 10, 2021 • edited Loading

juzhiyuan commented Apr 10, 2021

codecov-io commented Apr 11, 2021 • edited Loading

Codecov Report

netlify bot commented Apr 11, 2021 • edited Loading

juzhiyuan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liuxiran Apr 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liuxiran commented Apr 12, 2021

LiteSun commented Apr 12, 2021 • edited Loading

LiteSun commented Apr 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juzhiyuan left a comment

Choose a reason for hiding this comment

LiteSun commented Apr 10, 2021 •

edited

Loading

codecov-io commented Apr 11, 2021 •

edited

Loading

netlify bot commented Apr 11, 2021 •

edited

Loading

liuxiran Apr 12, 2021 •

edited

Loading

LiteSun commented Apr 12, 2021 •

edited

Loading

LiteSun commented Apr 12, 2021 •

edited

Loading