Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
poorbarcode opened this issue Jun 20, 2022 · 4 comments
Closed
Labels

Comments

@poorbarcode
Copy link
Contributor

poorbarcode commented Jun 20, 2022

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:

{"retention_size_in_mb":-1,"retention_time_in_minutes":40320}

Which should have been this:

{"retentionSizeInMB":-1,"retentionTimeInMinutes":40320}

Response:

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 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/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

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

pulsar-admin brokers update-dynamic-config --config httpRequestsFailOnUnknownPropertiesEnabled --value [boolean]

Note: Since admin api v1 is no longer maintained, this feature does not affect this part of the functionality.

@poorbarcode poorbarcode changed the title [PIP-178] Support admin API fail on unknown request parameter [PIP-178] Support the admin API to check unknown request parameter Jun 20, 2022
@poorbarcode poorbarcode changed the title [PIP-178] Support the admin API to check unknown request parameter [PIP-178] Support the admin API to check unknown request parameters Jun 20, 2022
@Jason918
Copy link
Contributor

PIP ID is used by #16042

@poorbarcode poorbarcode changed the title [PIP-178] Support the admin API to check unknown request parameters [PIP-179] Support the admin API to check unknown request parameters Jun 21, 2022
@poorbarcode
Copy link
Contributor Author

PIP ID is used by #16042

I have corrected it. Thanks

@poorbarcode
Copy link
Contributor Author

poorbarcode commented Jun 22, 2022

@poorbarcode
Copy link
Contributor Author

complete.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants