Separate Painless Whitelist Loading from the Painless Definition #26540
Conversation
jdconrad
added
:Plugin Lang Painless
:Core/Infra/Scripting
| constant.clazz == float.class || | ||
| constant.clazz == double.class || | ||
| constant.clazz == String.class; | ||
| constant.clazz == byte.class || |
There was a problem hiding this comment.
I think this was better aligned before?
There was a problem hiding this comment.
Intellij formatting error.
| addStruct(whitelist.javaClassLoader, whitelistStruct); | ||
|
|
||
| Struct painlessStruct = structsMap.get(whitelistStruct.painlessTypeName); | ||
| javaClassesToPainlessStructs.put(painlessStruct.clazz, painlessStruct); |
There was a problem hiding this comment.
We should fail if the put replaced an existing entry? Or rather, not replace if the mapping already exists (ie where a 2 different whiteslists are whitelisting methods on the same class). But surely they can't be allowed to have different names in painless for the same java class (one would overwrite the other here).
There was a problem hiding this comment.
Good catch, added an error.
| structsMap.put("def", new Struct("def", Object.class, org.objectweb.asm.Type.getType(Object.class))); | ||
|
|
||
| try { | ||
| for (Whitelist whitelist : whitelists) { |
There was a problem hiding this comment.
Can you add a comment why we need this first loop over the classes, and then a second loop over all the methods/fields?
| } | ||
| } catch (Exception exception) { | ||
| throw new IllegalArgumentException("error loading whitelist(s) " + origin, exception); |
There was a problem hiding this comment.
Is this message accurate? Aren't the whitelists already loaded before this method is even called?
There was a problem hiding this comment.
Message is accurate. The whitelist shouldn't be considered finished loading until the validation can be run.
| assert currentClass != null; | ||
| addSignature(currentClass, line); | ||
| } | ||
| for (Struct painlessStruct : structsMap.values()) { |
There was a problem hiding this comment.
A short comment explaining what this loop does would be nice (to map the class hierarchy of java to whitelisted methods in subclasses in painless?)
| javaHandle = MethodHandles.publicLookup().in(ownerStruct.clazz).unreflectConstructor(javaConstructor); | ||
| } catch (IllegalAccessException exception) { | ||
| throw new IllegalArgumentException("constructor not defined for owner struct [" + ownerStructName + "] " + | ||
| " with constructor parameters " + whitelistConstructor.painlessParameterTypeNames); |
There was a problem hiding this comment.
Is it intentional to drop the original exception? If so, please add a comment explaining why.
| try { | ||
| javaMethodHandle = MethodHandles.publicLookup().in(javaImplClass).unreflect(javaMethod); | ||
| } catch (IllegalAccessException exception) { | ||
| throw new IllegalArgumentException("method handle not found for method with name " + |
There was a problem hiding this comment.
Same here, original exception is dropped.
There was a problem hiding this comment.
Good catch... was unintentional.
| // sanity check, look for missing covariant/generic override | ||
| if (owner.clazz.isInterface() && child.clazz == Object.class) { | ||
| /*if (owner.clazz.isInterface() && child.clazz == Object.class) { |
There was a problem hiding this comment.
Is this meant to be commented out? Please remove if it is no longer relevant.
There was a problem hiding this comment.
I want to follow up in a different PR. Added a long explanation in a TODO. Will remove the elements after I've gone through them again. I think some are still relevant, but the code is buggy at this time.
| * Structs will automatically extend other white-listed structs if the Java class they represent is a | ||
| * subclass of other structs including Java interfaces. | ||
| */ | ||
| public static final class Struct { |
There was a problem hiding this comment.
I think we should consider (in a followup) renaming Struct to Class. We use the same name everywhere else as the java class (eg Method), so I think we should be consistent, and it will decrease confusion.
|
@rjernst Thanks for the review. |
* master: added comment fix line length violation Moved the check to fetch phase. This basically means that we throw a better error message instead of an AOBE and not adding more restrictions. inner hits: Do not allow inner hits that use _source and have a non nested object field as parent Separate Painless Whitelist Loading from the Painless Definition (elastic#26540) convert more admin requests to writeable (elastic#26566) Handle release of 5.6.1 Allow `InputStreamStreamInput` array size validation where applicable (elastic#26692) Update global checkpoint with permit after recovery
* master: (278 commits) Move pre-6.0 node checkpoint to SequenceNumbers Invalid JSON request body caused endless loop (elastic#26680) added comment fix line length violation Moved the check to fetch phase. This basically means that we throw a better error message instead of an AOBE and not adding more restrictions. inner hits: Do not allow inner hits that use _source and have a non nested object field as parent Separate Painless Whitelist Loading from the Painless Definition (elastic#26540) convert more admin requests to writeable (elastic#26566) Handle release of 5.6.1 Allow `InputStreamStreamInput` array size validation where applicable (elastic#26692) Update global checkpoint with permit after recovery Filter pre-6.0 nodes for checkpoint invariants Skip bad request REST test on pre-6.0 Reenable BWC tests after disabling for backport Add global checkpoint tracking on the primary [Test] Fix reference/cat/allocation/line_8 test failure [Docs] improved description for fs.total.available_in_bytes (elastic#26657) Fix discovery-file plugin to use custom config path fix testSniffNodes to use the new error message Add check for invalid index in WildcardExpressionResolver (elastic#26409) ...
* master: (67 commits) Restoring from snapshot should force generation of a new history uuid (elastic#26694) test: Use a single primary shard so that the exception can caught in the same way Move pre-6.0 node checkpoint to SequenceNumbers Invalid JSON request body caused endless loop (elastic#26680) added comment fix line length violation Moved the check to fetch phase. This basically means that we throw a better error message instead of an AOBE and not adding more restrictions. inner hits: Do not allow inner hits that use _source and have a non nested object field as parent Separate Painless Whitelist Loading from the Painless Definition (elastic#26540) convert more admin requests to writeable (elastic#26566) Handle release of 5.6.1 Allow `InputStreamStreamInput` array size validation where applicable (elastic#26692) Update global checkpoint with permit after recovery Filter pre-6.0 nodes for checkpoint invariants Skip bad request REST test on pre-6.0 Reenable BWC tests after disabling for backport Add global checkpoint tracking on the primary [Test] Fix reference/cat/allocation/line_8 test failure [Docs] improved description for fs.total.available_in_bytes (elastic#26657) Fix discovery-file plugin to use custom config path ...
) Adds several small whitelist data structures and a new Whitelist class to separate the idea of loading a whitelist from the actual Painless Definition class. This is the first step of many in allowing users to define custom whitelists per context. Also supports the idea of loading multiple whitelists from different sources for a single context.
clintongormley
added
:Core/Infra/Scripting
Adds several small whitelist data structures and a new Whitelist class to separate the idea of loading a whitelist from the actual Painless Definition class. This is the first step of many in allowing users to define custom whitelists per context. Also supports the idea of loading multiple whitelists from different sources for a single context.
This is not as large a change as it looks -- ~250 lines are small formatting changes to the whitelist resource files. ~300 lines are documentation. (Sorry @rjernst)