-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
[fix][sec]disable trace in web service #18092
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, we have to cherry-pick it to release branches also
Codecov Report
@@ Coverage Diff @@
## master #18092 +/- ##
============================================
- Coverage 34.91% 27.90% -7.02%
+ Complexity 5707 3694 -2013
============================================
Files 607 393 -214
Lines 53396 43488 -9908
Branches 5712 4472 -1240
============================================
- Hits 18644 12134 -6510
+ Misses 32119 29458 -2661
+ Partials 2633 1896 -737
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch!
@leizhiyuan the related test failed
And I think that we should add this to all the webservers in Pulsar (Proxy, Function Worker) |
After I checked the testcase , I find it seems disableHttpDebugMethods do the same thing. #7907 we can set disableHttpDebugMethods=true to solve the issue? and I can add the disableHttpDebugMethods to Proxy, Function Worker |
Makes sense to me. Before merging the pull it would be good to advise on the dev@. Since this can be considered as breaking change we must be careful on cherry-picking on the release branches, although in this case it's a benefit for all the users/operators. |
agree with you , maybe we can use disableHttpDebugMethods in other component, so we won't break the original scene. but If users want to use the sec feature, they can open the switch. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/pulsarbot rerun-failure-checks |
@leizhiyuan hi, I move this PR to release/2.9.5, if you have any questions, please ping me. thanks. |
The pr had no activity for 30 days, mark with Stale label. |
As discussed on the mailing list https://lists.apache.org/thread/w4jzk27qhtosgsz7l9bmhf1t7o9mxjhp, there is no plan to release 2.9.6, so I am going to remove the release/2.9.6 label |
@leizhiyuan Please either rebase and test, or close the PR. |
Fixes #18091
Master Issue: #18091
Motivation
close TRACE
refactor DisableDebugHttpMethodFilter , use the ConstraintSecurityHandler to achieve this.
because we can not access proxyConfig and WorkerConfig in DisableDebugHttpMethodFilter , the proxy module and worker module depends on broker-common, if I use the interface, PulsarConfiguration ,I need to do some tricky thing.
Modifications
Verifying this change
(Please pick either of the following options)
This change is a trivial rework / code cleanup without any test coverage.
(or)
This change is already covered by existing tests, such as (please describe tests).
(or)
This change added tests and can be verified as follows:
(example:)
Does this pull request potentially affect one of the following parts:
If the box was checked, please highlight the changes
Documentation
doc
doc-required
doc-not-needed
doc-complete
Matching PR in forked repository
PR in forked repository: