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

[BUG] DLS Performance with 2.16 #4670

Closed
pmarjou22 opened this issue Aug 22, 2024 · 8 comments · Fixed by #4741
Closed

[BUG] DLS Performance with 2.16 #4670

pmarjou22 opened this issue Aug 22, 2024 · 8 comments · Fixed by #4741
Labels
bug Something isn't working triaged Issues labeled as 'Triaged' have been reviewed and are deemed actionable.

Comments

@pmarjou22
Copy link

What is the bug?
DLS security filter has a huge impact on performance. I open this ticket following @peternied advice on issue #3776

How can one reproduce the bug?
Launch a plain vanilla opensearch docker

create a role with dls filter :
{"bool":{"filter":[{"match_all":{}},{"bool":{"should":[{"match_phrase":{"currency":"EUR"}},{"match_phrase":{"currency":"molestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestiemolestie"}}],"minimum_should_match":1}}]}}

Iterate search posts in JMeter with a user non admistrator member of the role with a DLS filter. RandomVar is a random string preventing caching.

POST path /sample_data_ecommerce/_search?request_cache=false
POST body
{"size":500,"query":{"bool":{"filter":[{"bool":{"should":[{"match_phrase":{"currency":"EUR"}},{"match_phrase":{"currency":"${randomVar}"}},{"match_phrase":{"currency":"CAD"}},{"match_phrase":{"currency":"POR"}},{"match_phrase":{"currency":"REE"}},{"match_phrase":{"currency":"DSS"}},{"match_phrase":{"currency":"FDF"}},{"match_phrase":{"currency":"KJK"}},{"match_phrase":{"currency":"NBV"}},{"match_phrase":{"currency":"AZE"}},{"match_phrase":{"currency":"WXW"}},{"match_phrase":{"currency":"JDS"}},{"match_phrase":{"currency":"MLA"}},{"match_phrase":{"currency":"TRE"}}],"minimum_should_match":1}}],"must_not":[{"match_phrase":{"customer_full_name":"Frances Davidson"}},{"match_phrase":{"customer_first_name":"Frances"}},{"match_phrase":{"user":"elyssa"}}]}}}

Using 100 indexes (sample ecommerce clones with 2K documents) , we confirmed with JMeter the same behavior that explained in this bug

Version | Admin Rights (Call / Sec) | Limited Rights via DLS (Call / Sec)
2.10 | 33.2 | 21.7
2.13 | 32.6 | 10.2
2.14 | 46.65 | 3.3
2.15 | 45 | 3.1
2.16 | 36.2 | 3.2

What is the expected behavior?
A 30% overhead vs non filtered DLS like in 2.10 opensearch

What is your host/environment?

  • Docker image: opensearchproject/opensearch:2.16.0

Do you have any screenshots?
I'm a first time user of jprofiler

CPU Hot Spot of non admin user
image

Hot Spot Comparison admin / non admin user
image

Admin Memory
image

Non Admin Memory
image

@pmarjou22 pmarjou22 added bug Something isn't working untriaged Require the attention of the repository maintainers and may need to be prioritized labels Aug 22, 2024
@shikharj05
Copy link
Contributor

@pmarjou22 Thanks for filing the issue. The profiler shows both Base64CustomHelper & Base64JDKHelper in hot spot. This could be due to call from SecurityInterceptor to ensureJDKSerialized here

ensureJDKSerialized- https://github.com/opensearch-project/security/blob/main/src/main/java/org/opensearch/security/support/Base64Helper.java#L61-L65

However, this is not a change in 2.16 and present since 2.11. @pmarjou22 - can you share the complete stacktrace which will help confirm the above? Also, have you tried any other version < 2.16 with the same test?

Note:
2.16 had a small refactoring change in DLS- #4490
However, this might not be the cause of regression.

@pmarjou22
Copy link
Author

The snapshots are done on docker fully loaded by jmeter on the non admin user (being attached to a role with DLS)

2.16 non admin user
image

2.11 non admin user
image

2.10 non admin user
image

@pmarjou22
Copy link
Author

The 2_16 non admin JProfiler file
2_16_nonadminuserwithDLS.zip

@cwperks cwperks added triaged Issues labeled as 'Triaged' have been reviewed and are deemed actionable. and removed untriaged Require the attention of the repository maintainers and may need to be prioritized labels Aug 26, 2024
@cwperks
Copy link
Member

cwperks commented Aug 26, 2024

[Triage] @pmarjou22 Thank you for filing this issue. I'm surprised to see references to Base64CustomHelper on 2.16 since the custom serialization logic was removed in 2.14.

Marking this issue as triaged.

@acidul
Copy link

acidul commented Sep 16, 2024

[Triage] @pmarjou22 Thank you for filing this issue. I'm surprised to see references to Base64CustomHelper on 2.16 since the custom serialization logic was removed in 2.14.

Hello @cwperks, do you have a PR or commit id for this removal you are talking about? I still see Base64CustomHelper in 2.16

@acidul
Copy link

acidul commented Sep 18, 2024

Hi

I've analyzed the 2_16 non admin JProfiler file @pmarjou22 provided.

In the case of an opensearch cluster only composed with 2.16 nodes, I noticed some serializations are made with custom serializer. See :

Base64Helper.serializeObject((Serializable) dlsQueries)

as the default serializer is the custom one :

public static String serializeObject(final Serializable object) {

then a lot of time is spent to deserialize (with custom deserializer) then re-serialize with jdk, in

public static String ensureJDKSerialized(final String string) {

I performed some tests to complete what @pmarjou22 did :

  • 2.16, as it has been released, using a mix of custom and jdk de/serializers
  • 2.16 enforcing the usage of custom de/serializer
  • 2.16 enforcing the usage of JDK de/serializer

This is the result made with jmeter :
Version | de/serializer | ensureJDKSerialized | Limited Rights via DLS (Call / Sec)
2.16 | mix custo & jdk | deserialize custo THEN deserialize jdk OR serialize jdk | 3.4
2.16' | custo only | noop | 12.0
2.16'' | jdk only | noop | 27.1

Here is the JProfiler of the last run (jdk only) :
2_16_nonadminuserwithDLS_jdk_only.zip

image

The changes I made are concentrated in Base64Helper :

    public static String serializeObject(final Serializable object, final boolean useJDKSerialization) {
        // Force JDK serializer usage

        //return useJDKSerialization ? Base64JDKHelper.serializeObject(object) : Base64CustomHelper.serializeObject(object);
        return Base64JDKHelper.serializeObject(object);
    }
    public static Serializable deserializeObject(final String string, final boolean useJDKDeserialization) {
        // Force JDK deserializer usage

        //return useJDKDeserialization ? Base64JDKHelper.deserializeObject(string) : Base64CustomHelper.deserializeObject(string);
        return Base64JDKHelper.deserializeObject(string);
    }
    public static String ensureJDKSerialized(final String string) {
        // Noop operation
        return string;
        
        /*Serializable serializable;
        ...

I precise, in production we never mix different versions of opensearch in our cluster (we only do cold upgrade, never rolling upgrade).

Do you have any feedback about this?
Thanks

@pmarjou22
Copy link
Author

Thanks a lot @acidul for the analysis. @peternied, is ensureJDKSerialized feature a mandatory check for security ? The serialization + deserialization that are done seems to be realy costly for non admin users having roles with DLS

@cwperks
Copy link
Member

cwperks commented Sep 18, 2024

@pmarjou22 Its necessary in mixed clusters where some nodes use custom serialization and others use jdk serialization, but not in homogenous clusters (all nodes of the same version).

This issue is actively being looked into and is related to this issue as well: #4494

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working triaged Issues labeled as 'Triaged' have been reviewed and are deemed actionable.
Projects
None yet
4 participants