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

server: add Self Protection mechanism #4466

Closed
wants to merge 19 commits into from
Closed

server: add Self Protection mechanism #4466

wants to merge 19 commits into from

Conversation

CabinfeverB
Copy link
Member

@CabinfeverB CabinfeverB commented Dec 15, 2021 •
edited
Loading

What problem does this PR solve?

This PR is used for fix #4373 including add audit logger and rate limiter for HTTP API
(close #4207, #2971, #3111, #3383)

This PR is split to three small ones:
- self protection framework #4469
- implement QPS rate limiter and Audit logger #4470
- add config #4471

What is changed and how it works?

  • add SelfProtectionHandler which implement http.Handler and has method to add gRPC interceptor
  • implement ApiRateLimiter and Auditlogger in SelfProtectionHandler
  • add SelfProtectionHandler middleware in server
  • add component name in Header for pdctl

Check List

Tests

  • Unit test (to do)
  • Integration test (to do)
  • Manual test (add detailed scripts or steps below) (to do)

Code changes

  • Has configuration change

Side effects

  • Possible performance regression

Release note

To be written

@CabinfeverB
add Self Protection Middleware Framework(tikv#4373)
3761aae 
Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>
@CabinfeverB
add Self Protection Middleware Framework(tikv#4373)
3c20341 
Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>
@CabinfeverB
add Self Protection Middleware Framework(tikv#4373) and fix some typo…
89d2eb5 
… in last commit

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>
@CabinfeverB
adjust Self Protection Middleware Framework(tikv#4373)
1e7d7c7 
Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>
@CabinfeverB
Fix tikv#4373 : add rate limiter
3713e62 
Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>
@CabinfeverB
Fix tikv#4373 : add rate limiter
e55b021 
Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>
@CabinfeverB
Fix tikv#4373 : add audit logger
4b37f31 
Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>
@CabinfeverB
Fix tikv#4373 : add config
7bd6353 
Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>
@CabinfeverB
Fix tikv#4373 : change HTTP service source
16bbac9 
Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>
@CabinfeverB
Fix tikv#4373 : change HTTP service source
fdcee1e 
Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>
@CabinfeverB
Fix tikv#4373 : fix some lint problem
4461424 
Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>
@CabinfeverB
Merge branch 'master' into api_rate_limiter
9b28a20
@CabinfeverB
Fix tikv#4373 : add middleware in server and add pdctl component
098acb7 
Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>
@ti-chi-bot
Copy link
Member

[REVIEW NOTIFICATION]

This pull request has not been approved.

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

@ti-chi-bot ti-chi-bot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Dec 15, 2021
@codecov
Copy link

codecov bot commented Dec 15, 2021 •
edited
Loading

Codecov Report

Merging #4466 (e104967) into master (e7c795f) will decrease coverage by 0.56%.
The diff coverage is 8.67%.

❗ Current head e104967 differs from pull request most recent head 2a10ed3. Consider uploading reports for the commit 2a10ed3 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4466      +/-   ##
==========================================
- Coverage   74.96%   74.40%   -0.57%     
==========================================
  Files         264      266       +2     
  Lines       27796    28013     +217     
==========================================
+ Hits        20837    20842       +5     
- Misses       5115     5312     +197     
- Partials     1844     1859      +15
Flag Coverage Δ
unittests 74.40% <8.67%> (-0.57%) ⬇️

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

Impacted Files Coverage Δ
pkg/apiutil/apiutil.go 36.61% <0.00%> (-25.29%) ⬇️
server/config/config.go 77.75% <ø> (ø)
tools/pd-ctl/pdctl/command/global.go 67.67% <0.00%> (-1.40%) ⬇️
server/middleware/self_protection.go 5.06% <5.06%> (ø)
server/server.go 71.26% <34.61%> (-1.13%) ⬇️
server/api/server.go 100.00% <100.00%> (ø)
server/middleware/metrics.go 100.00% <100.00%> (ø)
pkg/tempurl/tempurl.go 45.00% <0.00%> (-15.00%) ⬇️
server/region_syncer/client.go 78.90% <0.00%> (-4.69%) ⬇️
server/schedulers/random_merge.go 60.00% <0.00%> (-3.34%) ⬇️
... and 16 more

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 e7c795f...2a10ed3. Read the comment docs.

This was referenced Dec 16, 2021
Closed
Closed
Closed
@ti-chi-bot
Copy link
Member

@CabinfeverB: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@ti-chi-bot ti-chi-bot added do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Dec 16, 2021
rleungx
rleungx reviewed Dec 16, 2021
View reviewed changes
server/api/server.go Outdated
@@ -37,6 +37,7 @@ func NewHandler(ctx context.Context, svr *server.Server) (http.Handler, server.S
router.PathPrefix(apiPrefix).Handler(negroni.New(
serverapi.NewRuntimeServiceValidator(svr, group),
serverapi.NewRedirector(svr),
svr.SelfProtectionHandler,
Copy link
Member

Choose a reason for hiding this comment

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

seem we need negroni.Handler.

@CabinfeverB
Fix tikv#4373 : add comment
1c186dd 
Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>
@CabinfeverB
Fix tikv#4373 : add comment
2a10ed3 
Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>
@CabinfeverB
Fix tikv#4373 : add comment
fb3bf37 
Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>
@CabinfeverB
Fix tikv#4373 : add comment
112aac3 
Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>
nolouch
nolouch reviewed Dec 23, 2021
View reviewed changes
conf/config.toml
@@ -200,3 +200,12 @@

## When enabled, usage data will be sent to PingCAP for improving user experience.
# enable-telemetry = true

[self-protection]
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider a better name? For example [middleware]

server/middleware/metrics.go
// See the License for the specific language governing permissions and
// limitations under the License.

package middleware
Copy link
Contributor

Choose a reason for hiding this comment

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

better move to pkg?

@CabinfeverB
Copy link
Member Author

close this PR to change spliting small PRs

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

@rleungx rleungx rleungx left review comments

@nolouch nolouch nolouch left review comments

Assignees
No one assigned
Labels
do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Provide PD self-protection mechanism PD HTTP API self-protection
4 participants
@CabinfeverB @ti-chi-bot @nolouch @rleungx