Hi Yubiao,
+1
Is there any intention in the future to change this to the default
behaviour, in 3.0.0? I assume there are going to be some other areas of the
code base and integrations that would need to deal with the new failure
mode. So it does make sense to phase this in gradually. Is that the
motivation to feature flag this change, rather than treat it as a bugfix?
In the suggested example response, it only reports a problem with the first
unrecognised field ("retention_size_in_mb"). I think it would be more
useful if it were able to respond with all the unrecognised fields
("retention_size_in_mb" and "retention_time_in_minutes").
Regard,
Dave
On Sat, Jul 2, 2022 at 9:26 AM 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
> >
>
