pulsar
pulsar copied to clipboard
[fix][sec]disable trace in web service
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
- [ ] Make sure that the change passes the CI checks.
(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:)
- Added integration tests for end-to-end deployment with large payloads (10MB)
- Extended integration test for recovery after broker failure
Does this pull request potentially affect one of the following parts:
If the box was checked, please highlight the changes
- [ ] Dependencies (add or upgrade a dependency)
- [ ] The public API
- [ ] The schema
- [ ] The default values of configurations
- [ ] The threading model
- [ ] The binary protocol
- [ ] The REST endpoints
- [ ] The admin CLI options
- [ ] Anything that affects deployment
Documentation
- [ ]
doc
- [ ]
doc-required
- [x]
doc-not-needed
- [ ]
doc-complete
Matching PR in forked repository
PR in forked repository:
Codecov Report
Merging #18092 (74ddac5) into master (6c65ca0) will decrease coverage by
7.01%
. The diff coverage is58.06%
.
@@ 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
Flag | Coverage Δ | |
---|---|---|
unittests | 27.90% <58.06%> (-7.02%) |
:arrow_down: |
Flags with carried forward coverage won't be shown. Click here to find out more.
Impacted Files | Coverage Δ | |
---|---|---|
.../org/apache/pulsar/broker/admin/v2/Namespaces.java | 10.65% <0.00%> (+2.63%) |
:arrow_up: |
.../pulsar/broker/service/AbstractBaseDispatcher.java | 34.35% <ø> (-11.51%) |
:arrow_down: |
.../service/SystemTopicBasedTopicPoliciesService.java | 54.11% <0.00%> (+2.52%) |
:arrow_up: |
.../pulsar/broker/stats/BrokerOperabilityMetrics.java | 98.21% <ø> (+5.56%) |
:arrow_up: |
.../java/org/apache/pulsar/broker/web/WebService.java | 72.72% <0.00%> (-8.56%) |
:arrow_down: |
...g/apache/pulsar/compaction/CompactedTopicImpl.java | 10.71% <0.00%> (ø) |
|
...java/org/apache/pulsar/proxy/server/WebServer.java | 73.37% <0.00%> (-0.97%) |
:arrow_down: |
...broker/delayed/InMemoryDelayedDeliveryTracker.java | 22.00% <50.00%> (+22.00%) |
:arrow_up: |
...ache/pulsar/broker/service/EntryFilterSupport.java | 27.02% <50.00%> (+2.02%) |
:arrow_up: |
...apache/pulsar/proxy/server/DirectProxyHandler.java | 63.63% <50.00%> (ø) |
|
... and 319 more |
@leizhiyuan the related test failed
WebServiceTest.testDisableHttpTraceAndTrackMethods
expected [405] but found [403]
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. https://github.com/apache/pulsar/pull/7907
we can set disableHttpDebugMethods=true to solve the issue?
and I can add the disableHttpDebugMethods to 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.
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.
/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.