-
Notifications
You must be signed in to change notification settings - Fork 124
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
Add a config option to limit request body size #276
Changes from 5 commits
3ac7594
431030e
94dd387
39cce56
a147edf
9b7f1a9
de66166
8c3c0ff
3dda549
e35e02e
d895fd2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -54,6 +54,7 @@ public class PolarisApplicationConfig extends Configuration { | |
private String awsAccessKey; | ||
private String awsSecretKey; | ||
private FileIOFactory fileIOFactory; | ||
private long maxDocumentBytes; | ||
|
||
@JsonProperty("metaStoreManager") | ||
public void setMetaStoreManagerFactory(MetaStoreManagerFactory metaStoreManagerFactory) { | ||
|
@@ -146,6 +147,15 @@ public void setFeatureConfiguration(Map<String, Object> featureConfiguration) { | |
this.configurationStore = new DefaultConfigurationStore(featureConfiguration); | ||
} | ||
|
||
@JsonProperty("maxDocumentBytes") | ||
public void setMaxDocumentBytes(long maxDocumentBytes) { | ||
this.maxDocumentBytes = maxDocumentBytes; | ||
} | ||
|
||
public long getMaxDocumentBytes() { | ||
return maxDocumentBytes; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it should have a default value. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Checking a bit more about class
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you for pulling this up, this actually makes me more strongly interested in 0. Please see my new comment below. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I also find 0-as-unlimited a little confusing. I agree that if we make the default -1 it might imply that 0 is different from -1, but I also think a default of 0 is going to raise some questions. Maybe we should just disallow 0, since There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From the code from StreamReadConstraints, that means both 0 and -1 are considered no limits. But I think it does make sense to have -1 for unlimited and disallow 0. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok I updated it to default to -1, explained what -1 is, and restricted 0 with an exception/useful comment. I agree that -1 is a better indicator of "no limit" and restricting 0 solves some ambiguity. |
||
|
||
public PolarisConfigurationStore getConfigurationStore() { | ||
return configurationStore; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,44 @@ | ||
/* | ||
* Licensed to the Apache Software Foundation (ASF) under one | ||
* or more contributor license agreements. See the NOTICE file | ||
* distributed with this work for additional information | ||
* regarding copyright ownership. The ASF licenses this file | ||
* to you under the Apache License, Version 2.0 (the | ||
* "License"); you may not use this file except in compliance | ||
* with the License. You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, | ||
* software distributed under the License is distributed on an | ||
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
* KIND, either express or implied. See the License for the | ||
* specific language governing permissions and limitations | ||
* under the License. | ||
*/ | ||
package org.apache.polaris.service.throttling; | ||
|
||
import com.fasterxml.jackson.annotation.JsonCreator; | ||
import com.fasterxml.jackson.annotation.JsonProperty; | ||
|
||
/** | ||
* Response object for errors caused by DoS-prevention throttling mechanisms, such as request size | ||
* limits | ||
*/ | ||
public class RequestThrottlingErrorResponse { | ||
andrew4699 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
public enum Error { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated |
||
REQUEST_TOO_LARGE, | ||
; | ||
} | ||
|
||
private final Error error; | ||
|
||
@JsonCreator | ||
public RequestThrottlingErrorResponse(@JsonProperty("error") Error error) { | ||
this.error = error; | ||
} | ||
|
||
public Error getError() { | ||
return error; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
/* | ||
* Licensed to the Apache Software Foundation (ASF) under one | ||
* or more contributor license agreements. See the NOTICE file | ||
* distributed with this work for additional information | ||
* regarding copyright ownership. The ASF licenses this file | ||
* to you under the Apache License, Version 2.0 (the | ||
* "License"); you may not use this file except in compliance | ||
* with the License. You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, | ||
* software distributed under the License is distributed on an | ||
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
* KIND, either express or implied. See the License for the | ||
* specific language governing permissions and limitations | ||
* under the License. | ||
*/ | ||
package org.apache.polaris.service.throttling; | ||
|
||
import com.fasterxml.jackson.core.exc.StreamConstraintsException; | ||
import jakarta.ws.rs.core.MediaType; | ||
import jakarta.ws.rs.core.Response; | ||
import jakarta.ws.rs.ext.ExceptionMapper; | ||
|
||
/** | ||
* Handles exceptions during the request that are a result of stream constraints such as the request | ||
* being too large | ||
*/ | ||
public class StreamReadConstraintsExceptionMapper | ||
andrew4699 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
implements ExceptionMapper<StreamConstraintsException> { | ||
|
||
@Override | ||
public Response toResponse(StreamConstraintsException exception) { | ||
return Response.status(Response.Status.BAD_REQUEST) | ||
.type(MediaType.APPLICATION_JSON_TYPE) | ||
.entity( | ||
new RequestThrottlingErrorResponse( | ||
RequestThrottlingErrorResponse.Error.REQUEST_TOO_LARGE)) | ||
.build(); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maxDocumentBytes
? What is a "Document" in the context of a Polaris instance?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed to maxRequestBodyBytes and updated/moved the comment.