Hi Michael Marshall > I think we should go further and add this configuration option to the function worker and possibly the proxy.
- Proxy: If the broker rejects a request with unknown parameters, the proxy will behave the same as the broker, so the proxy does not need to do additional support. - Function: I am not familiar with function now. I will read the implementation of the function and give you an extra reply (I've been busy lately, so will be a little late). Thanks Yubiao Feng On Thu, Jun 30, 2022 at 12:34 PM Michael Marshall <mmarsh...@apache.org> wrote: > 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 > > > > > > > >
