I think this optional configuration is a good idea. I agree that a 204 is misleading for a malformed request.
Additionally, I think we should go further and add this configuration option to the function worker and possibly the proxy (which handles a single post call) as well, since they also handle http requests with parameters. Thanks, Michael On Mon, Jun 27, 2022 at 12:16 AM Zike Yang <z...@apache.org> wrote: > > +1 > > Zike Yang > > On Wed, Jun 22, 2022 at 11:26 AM PengHui Li <peng...@apache.org> wrote: > > > +1 > > > > Penghui > > > > On Wed, Jun 22, 2022 at 10:53 AM Yubiao Feng > > <yubiao.f...@streamnative.io.invalid> 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 > > > ``` > > > > > > ## 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 > > > > >
