Hi Haiting > Can we get a warning log on broker, even if this is false. This would be useful for existing clusters to turn on this feature.
I think it is a good idea. It may not be easy to implement, but I will try it. I will explain in PR whether this is implemented and why. Thanks Yubiao Feng On Sat, Jul 2, 2022 at 4:26 PM Haiting Jiang <jianghait...@apache.org> wrote: > > > On 2022/06/22 02:52:44 Yubiao Feng wrote: > > Hi, Pulsar community: > > > > I open a pip to discuss "Support the admin API to check unknown request > > parameters" > > > > Proposal Link: https://github.com/apache/pulsar/issues/16135 > > > > ### Motivation > > > > The design of the Admin API is now such that if an incorrect parameter > name > > is submitted, this property (if not required) will be ignored, then > > execution continues, and the response is “204 Success”. This will trick > the > > user into thinking the setup succeeded when it didn't correctly as > expected > > in some cases, as shown below: > > > > User POST request to /{tenant}/{namespace}/{topic}/retention" with > > incorrect parameter: > > ```json > > {"retention_size_in_mb":-1,"retention_time_in_minutes":40320} > > ``` > > > > Which should have been this: > > > > ```json > > {"retentionSizeInMB":-1,"retentionTimeInMinutes":40320} > > ``` > > > > Response: > > > > ```http > > HTTP/1.1 204 No Content > > Date: Mon, 20 Jun 2022 02:54:25 GMT > > broker-address: 127.0.0.1 > > Server: Jetty(9.4.44.v20210927) > > ``` > > > > We can provide an optional mechanism: "fail (HTTP status 400 bad > requests) > > on unknown request parameters". > > > > ## Goal > > > > - scope: > > - ~~Path variables~~(no need for change): This represents the domain. > > The current API has been validated, so no additional modifications are > > required. > > - ~~Query params~~(no support on this proposal): I haven't found an > > elegant way to do it yet, so this proposal does not include Query Param > > validation. > > - *Entity properties*: This proposal only handles requests whose > > Content-type is "application/json" (in fact, this is the only type in our > > project). > > - Configurable(Support dynamic switching). > > > > > > ## Approach > > > > When parsing the request body, any unknown property is considered a bad > > request. The [Jackson unknown property rule]( > > > https://github.com/FasterXML/jackson-databind/blob/de3d0ecbc1fd0a1a6b061e62a198b3ba0d0d163e/src/main/java/com/fasterxml/jackson/databind/DeserializationFeature.java#L121 > ) > > is adopted: > > > > - Case sensitive. > > - Special characters are not ignored. > > - Do not trim Spaces. > > > > If the check fails, return a text/plain response with 400 code. Like > this: > > > > ```http > > HTTP/1.1 400 Bad Request > > Date: Mon, 20 Jun 2022 03:52:10 GMT > > broker-address: 127.0.0.1 > > Content-Type: text/plain > > Content-Length: 432 > > Server: Jetty(9.4.44.v20210927) > > > > Unrecognized field "retention_size_in_mb" (class > > org.apache.pulsar.common.policies.data.RetentionPolicies known > properties: > > "retentionSizeInMB", "retentionTimeInMinutes"]) > > ``` > > > > ## Configuration Changes > > > > broker.conf > > > > ```properties > > # Admin API fail on unknown request parameter in request-body. see > PIP-178. > > Setting this to blank means that this feature is turned off. > > httpRequestsFailOnUnknownPropertiesEnabled=false > > ``` > > Can we get a warning log on broker, even if this is false. > This would be useful for existing clusters to turn on this feature. > > > > > ## Dynamic switching > > Enabling this feature affects all of the broker's HTTP services, > including > > the following: > > > > - /status.html (no post-entity request) > > - /admin [v2,v3] > > - /lookup (no post-entity request) > > - /topics (http client) > > - /metrics (no post-entity request) > > > > Because of the number of apis involved, we provide dynamic configuration. > > When a user discovers any problem, it can be turned on and off > dynamically > > using the Admin API(without restarting Broker), which can reduce impact. > > > > Note: Since admin api v1 is no longer maintained, this feature does not > > affect this part of the functionality. > > > > ```shell > > pulsar-admin brokers update-dynamic-config --config > > httpRequestsFailOnUnknownPropertiesEnabled --value [boolean] > > ``` > > > > Thanks > > Yubiao Feng > > >
