-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Add User Profile Size Limit Enforced During Profile Updates #137712
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
Conversation
…ential heap memory exhaustion.
|
Hi @ebarlas, I've created a changelog YAML for you. |
legrego
left a comment
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.
@ebarlas, what will Kibana see in terms of HTTP status code & error payload when this limit is hit? I ask because I feel we should monitor this within our SLOs. I want to make sure we have a way to reliably detect this condition, and distinguish it from other potential failure scenarios.
Elasticsearch request processing will fail with an Here's a curl snippet for reference: |
jfreden
left a comment
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.
Good job! This approach LGTM! I've left some comments, let me know what you think.
| Map<String, Object> data = combineMaps(mapFromBytesReference(doc.doc.applicationData()), request.getData()); | ||
| int actualSize = serializationSize(labels) + serializationSize(data); | ||
| if (actualSize > limit) { | ||
| throw new ElasticsearchException( |
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.
I think this should be an IllegalArgumentException and result in a 400 since it's caused by an issue triggered by a series of bad requests. WDYT?
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.
Good point. ElasticsearchStatusException looks like a good option.
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.
@legrego , I updated my comment above to reflect the 400 Bad Request response change.
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.
Thanks @ebarlas
|
|
||
| public class ProfileService { | ||
|
|
||
| public static final Setting<Integer> MAX_SIZE_SETTING = Setting.intSetting( |
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.
Should this be a ByteSizeValue Setting then you can set it to 10mb?
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.
Good catch!
| return XContentHelper.convertToMap(bytesRef, false, XContentType.JSON).v2(); | ||
| } | ||
|
|
||
| static int serializationSize(Map<String, Object> map) { |
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.
Clever!
|
|
||
| static Map<String, Object> mapFromBytesReference(BytesReference bytesRef) { | ||
| if (bytesRef == null || bytesRef.length() == 0) { | ||
| return new HashMap<>(); |
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.
nit: this could be Map.of()
| if (actualSize > limit) { | ||
| throw new ElasticsearchException( | ||
| Strings.format( | ||
| "cannot update profile [%s] because the combined profile size of [%s] bytes exceeds the maximum of [%s] bytes", |
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.
I think using a human readable size here (like 10mb) would be nice.
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.
Much better!
|
Can you take the PR out of draft mode? |
|
Pinging @elastic/es-security (Team:Security) |
jfreden
left a comment
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.
LGTM! Just a comment on the default value that I think was accidentally set to 100MB.
...k/plugin/security/src/main/java/org/elasticsearch/xpack/security/profile/ProfileService.java
Outdated
Show resolved
Hide resolved
…ecurity/profile/ProfileService.java Co-authored-by: Johannes Fredén <109296772+jfreden@users.noreply.github.com>
💚 All backports created successfully
Questions ?Please refer to the Backport tool documentation |
Currently, there are no limits on the size of a user profile. Profiles store username, initials, avatars, etc.
Authorized Kibana observability clients can store an unlimited amount of data in user profile via update-profile.
This change puts a limit on profile size to avoid heap memory pressure and OOM crashes.
A new configuration setting,
xpack.security.profile.max_size, was introduced with a default value of 10 MB to remain safely above the 1 MB request limit size enforced by Kibana.Limit enforcement is implemented with a profile document read before the update, to provide a full view of the profile footprint. This approach is intended to be lightweight. Still, a document read is now incurred for every update request.