Hi Dave
> Is there any intention in the future to change this to the default
behaviour, in 3.0.0?
There are no plans for that right now
> Is that the motivation to feature flag this change, rather than treat it
as a bugfix?
Yes, the design goal now is that rejection of unknown request parameters
will be treated as a dynamically enabled feature.
> 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.
Yes, correct. This proposal will apply to all post entity validation of the
Admin API in Broker.
On Thu, Jul 7, 2022 at 7:04 PM Dave Maughan
<dave.maug...@streamnative.io.invalid> wrote:
> 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
> > >
> >
>
