You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@pulsar.apache.org by Yubiao Feng <yu...@streamnative.io.INVALID> on 2022/06/22 02:52:44 UTC

[DISCUSS] [PIP-179] Support the admin API to check unknown request parameters

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

Re: [DISCUSS] [PIP-179] Support the admin API to check unknown request parameters

Posted by Yubiao Feng <yu...@streamnative.io.INVALID>.

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 <ji...@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
> >
>

Re: [DISCUSS] [PIP-179] Support the admin API to check unknown request parameters

Posted by Yubiao Feng <yu...@streamnative.io.INVALID>.

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
<da...@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 <ji...@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
> > >
> >
>

Re: [DISCUSS] [PIP-179] Support the admin API to check unknown request parameters

Posted by Dave Maughan <da...@streamnative.io.INVALID>.

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 <ji...@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
> >
>

Re: [DISCUSS] [PIP-179] Support the admin API to check unknown request parameters

Posted by Haiting Jiang <ji...@apache.org>.


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
>

Re: [DISCUSS] [PIP-179] Support the admin API to check unknown request parameters

Posted by Yubiao Feng <yu...@streamnative.io.INVALID>.

Hi Michael Marshall

> - 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).

I have checked the API used by function workers, including “admin API v2
Functions”, "function works API SourcesApiV3Resource", "function works API
SinksApiV3Resource" and  "function works API FunctionsApiV3Resource".
Currently, there is no API that uses the request of Post Entity. If it is
used in the future, I will submit PR to solve it.

Thanks
Yubiao Feng

On Thu, Jul 7, 2022 at 11:04 PM Yubiao Feng <yu...@streamnative.io>
wrote:

> 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 <mm...@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 <zi...@apache.org> wrote:
>> >
>> > +1
>> >
>> > Zike Yang
>> >
>> > On Wed, Jun 22, 2022 at 11:26 AM PengHui Li <pe...@apache.org> wrote:
>> >
>> > > +1
>> > >
>> > > Penghui
>> > >
>> > > On Wed, Jun 22, 2022 at 10:53 AM Yubiao Feng
>> > > <yu...@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
>> > > >
>> > >
>>
>

Re: [DISCUSS] [PIP-179] Support the admin API to check unknown request parameters

Posted by Yubiao Feng <yu...@streamnative.io.INVALID>.

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 <mm...@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 <zi...@apache.org> wrote:
> >
> > +1
> >
> > Zike Yang
> >
> > On Wed, Jun 22, 2022 at 11:26 AM PengHui Li <pe...@apache.org> wrote:
> >
> > > +1
> > >
> > > Penghui
> > >
> > > On Wed, Jun 22, 2022 at 10:53 AM Yubiao Feng
> > > <yu...@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
> > > >
> > >
>

Re: [DISCUSS] [PIP-179] Support the admin API to check unknown request parameters

Posted by Michael Marshall <mm...@apache.org>.

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 <zi...@apache.org> wrote:
>
> +1
>
> Zike Yang
>
> On Wed, Jun 22, 2022 at 11:26 AM PengHui Li <pe...@apache.org> wrote:
>
> > +1
> >
> > Penghui
> >
> > On Wed, Jun 22, 2022 at 10:53 AM Yubiao Feng
> > <yu...@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
> > >
> >

Re: [DISCUSS] [PIP-179] Support the admin API to check unknown request parameters

Posted by Anon Hxy <an...@gmail.com>.

+1

Thanks,
Xiaoyu Hou

Zike Yang <zi...@apache.org> 于2022年6月27日周一 13:16写道：

> +1
>
> Zike Yang
>
> On Wed, Jun 22, 2022 at 11:26 AM PengHui Li <pe...@apache.org> wrote:
>
> > +1
> >
> > Penghui
> >
> > On Wed, Jun 22, 2022 at 10:53 AM Yubiao Feng
> > <yu...@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
> > >
> >
>

Re: [DISCUSS] [PIP-179] Support the admin API to check unknown request parameters

Posted by Zike Yang <zi...@apache.org>.

+1

Zike Yang

On Wed, Jun 22, 2022 at 11:26 AM PengHui Li <pe...@apache.org> wrote:

> +1
>
> Penghui
>
> On Wed, Jun 22, 2022 at 10:53 AM Yubiao Feng
> <yu...@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
> >
>

Re: [DISCUSS] [PIP-179] Support the admin API to check unknown request parameters

Posted by PengHui Li <pe...@apache.org>.

+1

Penghui

On Wed, Jun 22, 2022 at 10:53 AM Yubiao Feng
<yu...@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
>