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

api: Add Request Info Middleware #4526

Merged
merged 56 commits into from
Jan 21, 2022
Merged

Conversation

CabinfeverB
Copy link
Member

@CabinfeverB CabinfeverB commented Dec 30, 2021

Signed-off-by: Cabinfever_B cabinfeveroier@gmail.com

What problem does this PR solve?

close #4538
This PR is used to support the information-gathering.

This PR should be merged after #4491 and #4495

What is changed and how it works?

This PR creates a middleware to handle the information-gathering for audit middleware and rate limit middleware.
RequestInfo includes below fields

ServiceLabel: service label is used to identify HTTP API and gRPC. According to this identification, A logical service, whether it is gRPC or HTTP, can be controlled by a total limiter. On HTTP API, it is the result of a mapping from the URL path and method. So we can use the names of the mux router to register ServiceLabel. But we must add middlewares into mux route.
Method indicates whether it is gRPC or HTTP
Component: component info of source client
IP: IP of the source client
TimeStamp
URLParam : params from URL Path
BodyParm: params from POST body

For the HTTP API interfaces which need to be audited and limited, we should register their service labels on /server/api/router.go

Benchmark test

for serviceInfoMiddleware in integration test

test BenchmarkDoRequestWithServiceMiddleware thrice and result: 287778 ns/op, 291501 ns/op and 265002 ns/op.
test BenchmarkDoRequestWithoutServiceMiddlewarethrice and result: 232358 ns/op, 277101 ns/op and 213643 ns/op

Based on the above test, serviceMiddleware won't have a big performance impact on the HTTP API.

Check List

Tests

  • Unit test
  • Integration test

Code changes

Side effects

  • Possible performance regression

Release note

Add request info middleware

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>
Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>
Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>
@ti-chi-bot
Copy link
Member

ti-chi-bot commented Dec 30, 2021

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • disksing
  • nolouch

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 release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Dec 30, 2021
Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>
Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>
@CabinfeverB
Copy link
Member Author

/run-all-tests

@CabinfeverB
Copy link
Member Author

/run-all-tests

@CabinfeverB
Copy link
Member Author

/retest

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>
@CabinfeverB
Copy link
Member Author

/run-all-tests

@codecov
Copy link

codecov bot commented Dec 31, 2021

Codecov Report

Merging #4526 (924f302) into master (c7632b8) will increase coverage by 0.17%.
The diff coverage is 95.87%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4526      +/-   ##
==========================================
+ Coverage   74.71%   74.88%   +0.17%     
==========================================
  Files         279      281       +2     
  Lines       27627    27706      +79     
==========================================
+ Hits        20641    20749     +108     
+ Misses       5129     5113      -16     
+ Partials     1857     1844      -13     
Flag Coverage Δ
unittests 74.88% <95.87%> (+0.17%) ⬆️

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

Impacted Files Coverage Δ
server/config/config.go 77.84% <ø> (ø)
server/api/admin.go 51.56% <57.14%> (+0.68%) ⬆️
pkg/apiutil/apiutil.go 71.21% <75.00%> (+3.47%) ⬆️
pkg/requestutil/request_info.go 78.94% <78.94%> (ø)
server/api/router.go 97.32% <99.40%> (-0.11%) ⬇️
pkg/requestutil/context.go 100.00% <100.00%> (ø)
server/api/middleware.go 88.00% <100.00%> (+15.27%) ⬆️
server/server.go 74.75% <100.00%> (+0.57%) ⬆️
server/tso/allocator_manager.go 63.45% <0.00%> (-1.07%) ⬇️
server/election/leadership.go 77.31% <0.00%> (-1.04%) ⬇️
... and 18 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 c7632b8...924f302. Read the comment docs.

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>
Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>
Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>
@CabinfeverB CabinfeverB changed the title add request info middleware api: Add Request Info Middleware Jan 4, 2022
@CabinfeverB CabinfeverB marked this pull request as ready for review January 4, 2022 08:36
@ti-chi-bot ti-chi-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 4, 2022
@CabinfeverB
Copy link
Member Author

/cc @rleungx

@ti-chi-bot ti-chi-bot requested a review from rleungx January 4, 2022 08:37
pkg/requestutil/context.go Show resolved Hide resolved
pkg/requestutil/context_test.go Outdated Show resolved Hide resolved
pkg/requestutil/context_test.go Outdated Show resolved Hide resolved
pkg/requestutil/request_info.go Outdated Show resolved Hide resolved
pkg/requestutil/request_info.go Outdated Show resolved Hide resolved
pkg/requestutil/request_info.go Outdated Show resolved Hide resolved
server/api/middleware.go Show resolved Hide resolved
Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>
@CabinfeverB
Copy link
Member Author

/run-all-tests

@CabinfeverB
Copy link
Member Author

/run-all-tests

@CabinfeverB
Copy link
Member Author

/rebuild

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>
Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>
Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>
Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>
Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>
@CabinfeverB CabinfeverB force-pushed the service_middleware branch 4 times, most recently from 10a564e to b7ec272 Compare January 20, 2022 14:28
Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>
Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>
@ti-chi-bot ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jan 21, 2022
@disksing
Copy link
Contributor

/merge

@ti-chi-bot
Copy link
Member

@disksing: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

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 ti-community-infra/tichi repository.

@ti-chi-bot
Copy link
Member

This pull request has been accepted and is ready to merge.

Commit hash: 924f302

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Jan 21, 2022
@ti-chi-bot ti-chi-bot merged commit 4f5a78a into tikv:master Jan 21, 2022
return route
}

// registerRouteHandleFunc is used to registers a new route which will be registered matcher or service by opts for the URL path
Copy link
Member

Choose a reason for hiding this comment

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

The comment needs to be changed.

disksing pushed a commit to oh-my-tidb/pd that referenced this pull request Feb 8, 2022
* close tikv#4494

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

* close tikv#4494: add priority comment

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

* add request info middleware

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

* close tikv#4494

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

* change service label getter and setter

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

* add lock

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

* add audit middleware

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

* add audit middleware

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

* tikv#4538

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

* fix

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

* fix check

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

* fix check

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

* change service label placement

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

* merge master

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

* add const service label  and could be dynamically turned on and off service middleware

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

* fix statics check

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

* fix statics check

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

* fix statics check

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

* fix statics check

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

* fix function name

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

* add benchmark test

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

* fix r.Body nil panic

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

* add benchmark test

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

* change export

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

* change export

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

* fix statics check

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

* fix typo problem

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

* change service label method

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

* fix statics

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

* fix router

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

* fix router

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

* add http proto

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

* add http proto

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

* change default value

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

* use middleware func shortname

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

* fix comment

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

* change const to iota

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

* change register method

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

* fix

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

* fix

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

* for test

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

* for test

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

* fix race

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

* fix race

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

Co-authored-by: ShuNing <nolouch@gmail.com>
ti-chi-bot added a commit that referenced this pull request Feb 11, 2022
ref #4526, close #4601

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

Co-authored-by: ShuNing <nolouch@gmail.com>
Co-authored-by: Ti Chi Robot <ti-community-prow-bot@tidb.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/api HTTP API. release-note Denotes a PR that will be considered when it comes time to generate release notes. status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Gather http.Request info
5 participants