Skip to content

Conversation

@ldematte
Copy link
Contributor

@ldematte ldematte commented Jan 29, 2025

This PR fixes an issue that become apparent with #121017: when we initialize Entitlements, and we get the list of methods to instrument, and instrument them, we need to get them from all the "versioned" interfaces compatible with the current runtime version. We also need to use the most specific (latest) "versioned" interface in the instrumentation code.

Alternative to #121132 which uses ASM and the actual inheritance chain to navigate the classes (interfaces) to extract check$ methods from.

@ldematte ldematte requested a review from a team January 29, 2025 15:07
@elasticsearchmachine elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label Jan 29, 2025
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

@prdoyle
Copy link
Contributor

prdoyle commented Jan 29, 2025

In general, reflection-based code is easier to read and maintain than ASM code. I'd lean toward reflection if there's no compelling reason to use ASM.

@ldematte
Copy link
Contributor Author

ldematte commented Jan 30, 2025

In general, reflection-based code is easier to read and maintain than ASM code. I'd lean toward reflection if there's no compelling reason to use ASM.

I was thinking the same and thought that reflection could be equivalent but Iwas wrong in assuming that: as this test failure #120811 (comment) demonstrates, we have to be very careful when using reflection this early.
If the interface contains methods with types that are not available, things will break; that's the very reason we went with ASM here in the first place after all.

So I closed #121132 in favor of this one, "porting" all the comments here and addressing them.

@ldematte ldematte added v9.0.0 auto-backport Automatically create backport pull requests when merged labels Jan 30, 2025
Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine but one question.


methodsToInstrument.put(methodToInstrument, checkMethod);
var classFileInfo = InstrumenterImpl.getClassFileInfo(currentClass);
ClassReader reader = new ClassReader(classFileInfo.bytecodes());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need asm here? Since we have Class objects already, can't we use reflection to find all of the methods?

Copy link
Contributor Author

@ldematte ldematte Jan 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking the same, but every time we use reflection (e.g. in #120811 (comment)), we end up regretting it and go back to ASM; for example in the linked issue I tried to use reflection first, only to end up with NoClassDefFoundError errors in tests due to the fact that our interface uses types that may not (are not) always available (at least at this time).
It might be fine in this case if we use it only for finding the base classes, but since we need to use ASM to find all the methods anyway, why risk?

@ldematte
Copy link
Contributor Author

ldematte commented Feb 3, 2025

Serverless failure is due to a lack of policy - fixed in 3451

@ldematte ldematte requested a review from a team February 3, 2025 09:02
Copy link
Contributor

@prdoyle prdoyle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather see some protection against visiting interfaces multiple times, but we can merge this as-is I think.

Also, reflection would probably be simpler than asm, but that's also not a hill I want to die on.

@ldematte
Copy link
Contributor Author

ldematte commented Feb 3, 2025

I'd rather see some protection against visiting interfaces multiple times, but we can merge this as-is I think.

That I've added, implementing your suggestion. See line 55 of InstrumentationServiceImpl.
As for reflection, I've tried and it gave trouble, see my previous comments.

@ldematte ldematte added the v9.0.0 label Feb 4, 2025
@ldematte ldematte merged commit fdbd079 into elastic:main Feb 4, 2025
22 checks passed
@ldematte ldematte deleted the entitlements/fix-multiple-version-checker-initialization-2 branch February 4, 2025 13:32
ldematte added a commit to ldematte/elasticsearch that referenced this pull request Feb 4, 2025
@elasticsearchmachine
Copy link
Collaborator

💔 Backport failed

Status Branch Result
9.0
8.18 Commit could not be cherrypicked due to conflicts
8.x Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 121192

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto-backport Automatically create backport pull requests when merged :Core/Infra/Core Core issues without another label >non-issue Team:Core/Infra Meta label for core/infra team v8.18.1 v8.19.0 v9.0.0 v9.0.1 v9.1.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants