Update policy parser to allow static methods for entitlement creation #121706
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jdconrad
added
:Core/Infra/Core
jdconrad
added
auto-backport
Collaborator
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
Contributor
Author
|
@elasticmachine run elasticsearch-ci/part-3-entitlements |
rjernst
reviewed
Feb 4, 2025
Member
rjernst
left a comment
rjernst
left a comment
There was a problem hiding this comment.
Looks good, a couple thoughts
| try { | ||
| return (Entitlement) entitlementConstructor.newInstance(parameterValues); | ||
| if (entitlementConstructor != null) { | ||
| return (Entitlement) entitlementConstructor.newInstance(parameterValues); |
Member
There was a problem hiding this comment.
Would it be simpler to keep a MethodHandle and invoke that? Then there would be no difference between the ctor and static method?
Contributor
Author
There was a problem hiding this comment.
I did a quick prototype of this, but it adds additional complexity for error handling, and doesn't save much in lookup. I'm still open to this, but think I would want to do it as a different PR where we look at simplifying the entirety of the method a bit more.
| Policy expected = new Policy( | ||
| "test-policy.yaml", | ||
| List.of(new Scope("entitlement-module-name", List.of(new FileEntitlement("test/path/to/file", "read_write")))) | ||
| List.of(new Scope("entitlement-module-name", List.of(FileEntitlement.create("test/path/to/file", "read_write")))) |
Member
There was a problem hiding this comment.
It's probably worth having explicit test(s) that check the behavior when eg a ctor and method are both annotated, similar to ManyConstructorsEntitlement already here. ie let's not rely on FileEntitlement, let's have explicit tests for this feature of the parser.
rjernst
approved these changes
Feb 4, 2025
| ); | ||
| } | ||
|
|
||
| public void testMultipleMethodsAnnotated() throws IOException { |
Member
There was a problem hiding this comment.
Can we also have a error test for a non-static method having the annotation?
Contributor
Author
|
@elasticmachine run elasticsearch-ci/part-1-entitlements |
ldematte
approved these changes
Feb 5, 2025
Contributor
ldematte
left a comment
ldematte
left a comment
There was a problem hiding this comment.
LGTM 2
GH status for the CI is "stuck", CI actually passed, I think you are free to merge 👍
jdconrad
added a commit
to jdconrad/elasticsearch
that referenced
this pull request
Feb 5, 2025
…elastic#121706) This updates the PolicyParser to allow static methods to have an ExternalEntitlement annotation. This removes a limitation where constructors cannot properly support type-erasure with different types of data structures for internal entitlement generation and external entitlement generation (for example List<Object> from the parser and List<SomeData> from an internal builder). We continue to enforce that only one constructor/method may be annotated with ExternalEntitlement per Entitlement class.
jdconrad
added a commit
to jdconrad/elasticsearch
that referenced
this pull request
Feb 5, 2025
…elastic#121706) This updates the PolicyParser to allow static methods to have an ExternalEntitlement annotation. This removes a limitation where constructors cannot properly support type-erasure with different types of data structures for internal entitlement generation and external entitlement generation (for example List<Object> from the parser and List<SomeData> from an internal builder). We continue to enforce that only one constructor/method may be annotated with ExternalEntitlement per Entitlement class.
Collaborator
jdconrad
added a commit
to jdconrad/elasticsearch
that referenced
this pull request
Feb 5, 2025
…elastic#121706) This updates the PolicyParser to allow static methods to have an ExternalEntitlement annotation. This removes a limitation where constructors cannot properly support type-erasure with different types of data structures for internal entitlement generation and external entitlement generation (for example List<Object> from the parser and List<SomeData> from an internal builder). We continue to enforce that only one constructor/method may be annotated with ExternalEntitlement per Entitlement class.
elasticsearchmachine
pushed a commit
that referenced
this pull request
Feb 5, 2025
…#121706) (#121796) This updates the PolicyParser to allow static methods to have an ExternalEntitlement annotation. This removes a limitation where constructors cannot properly support type-erasure with different types of data structures for internal entitlement generation and external entitlement generation (for example List<Object> from the parser and List<SomeData> from an internal builder). We continue to enforce that only one constructor/method may be annotated with ExternalEntitlement per Entitlement class.
elasticsearchmachine
pushed a commit
that referenced
this pull request
Feb 5, 2025
…#121706) (#121797) This updates the PolicyParser to allow static methods to have an ExternalEntitlement annotation. This removes a limitation where constructors cannot properly support type-erasure with different types of data structures for internal entitlement generation and external entitlement generation (for example List<Object> from the parser and List<SomeData> from an internal builder). We continue to enforce that only one constructor/method may be annotated with ExternalEntitlement per Entitlement class.
elasticsearchmachine
pushed a commit
that referenced
this pull request
Feb 5, 2025
…#121706) (#121795) This updates the PolicyParser to allow static methods to have an ExternalEntitlement annotation. This removes a limitation where constructors cannot properly support type-erasure with different types of data structures for internal entitlement generation and external entitlement generation (for example List<Object> from the parser and List<SomeData> from an internal builder). We continue to enforce that only one constructor/method may be annotated with ExternalEntitlement per Entitlement class.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This updates the
PolicyParserto allow static methods to have anExternalEntitlementannotation. This removes a limitation where constructors cannot properly support type-erasure with different types of data structures for internal entitlement generation and external entitlement generation (for exampleList<Object>from the parser andList<SomeData>from an internal builder). We continue to enforce that only one constructor/method may be annotated withExternalEntitlementperEntitlementclass.For testing I converted
FileEntitlementto use a static method for the policy parser as this is going to be replaced byFilesEntitlementin the near future.