Skip to content

Commit d4a0b7d

Browse files
author
elasticsearchmachine
committed
Merge remote-tracking branch 'origin/main' into lucene_snapshot
2 parents 7478842 + 918a9cc commit d4a0b7d

File tree

18 files changed

+788
-73
lines changed

18 files changed

+788
-73
lines changed

‎build-tools-internal/src/main/resources/checkstyle_suppressions.xml‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,8 @@
3535
<!-- Intentionally have multi line string for a bulk request, otherwise this needs to fallback to string concatenation -->
3636
<suppress files="modules[/\\]data-streams[/\\]src[/\\]javaRestTest[/\\]java[/\\]org[/\\]elasticsearch[/\\]datastreams[/\\]TsdbDataStreamRestIT.java" checks="LineLength" />
3737
<suppress files="qa[/\\]rolling-upgrade[/\\]src[/\\]javaRestTest[/\\]java[/\\]org[/\\]elasticsearch[/\\]upgrades[/\\]TsdbIT.java" checks="LineLength" />
38+
<suppress files="qa[/\\]rolling-upgrade[/\\]src[/\\]javaRestTest[/\\]java[/\\]org[/\\]elasticsearch[/\\]upgrades[/\\]TsdbIndexingRollingUpgradeIT.java" checks="LineLength" />
39+
<suppress files="qa[/\\]rolling-upgrade[/\\]src[/\\]javaRestTest[/\\]java[/\\]org[/\\]elasticsearch[/\\]upgrades[/\\]LogsdbIndexingRollingUpgradeIT.java" checks="LineLength" />
3840

3941
<!-- Gradle requires inputs to be seriablizable -->
4042
<suppress files="build-tools-internal[/\\]src[/\\]main[/\\]java[/\\]org[/\\]elasticsearch[/\\]gradle[/\\]internal[/\\]precommit[/\\]TestingConventionRule.java" checks="RegexpSinglelineJava" />

‎distribution/tools/entitlement-agent/impl/src/main/java/org/elasticsearch/entitlement/instrumentation/impl/InstrumenterImpl.java‎

Lines changed: 82 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,10 @@
1515
import org.objectweb.asm.ClassReader;
1616
import org.objectweb.asm.ClassVisitor;
1717
import org.objectweb.asm.ClassWriter;
18+
import org.objectweb.asm.FieldVisitor;
1819
import org.objectweb.asm.MethodVisitor;
1920
import org.objectweb.asm.Opcodes;
21+
import org.objectweb.asm.RecordComponentVisitor;
2022
import org.objectweb.asm.Type;
2123

2224
import java.io.IOException;
@@ -73,7 +75,13 @@ public byte[] instrumentClass(String className, byte[] classfileBuffer) {
7375
}
7476

7577
class EntitlementClassVisitor extends ClassVisitor {
76-
final String className;
78+
79+
private static final String ENTITLEMENT_ANNOTATION = "EntitlementInstrumented";
80+
81+
private final String className;
82+
83+
private boolean isAnnotationPresent;
84+
private boolean annotationNeeded = true;
7785

7886
EntitlementClassVisitor(int api, ClassVisitor classVisitor, String className) {
7987
super(api, classVisitor);
@@ -85,25 +93,85 @@ public void visit(int version, int access, String name, String signature, String
8593
super.visit(version, access, name + classNameSuffix, signature, superName, interfaces);
8694
}
8795

96+
@Override
97+
public AnnotationVisitor visitAnnotation(String descriptor, boolean visible) {
98+
if (visible && descriptor.equals(ENTITLEMENT_ANNOTATION)) {
99+
isAnnotationPresent = true;
100+
annotationNeeded = false;
101+
}
102+
return cv.visitAnnotation(descriptor, visible);
103+
}
104+
105+
@Override
106+
public void visitNestMember(String nestMember) {
107+
addClassAnnotationIfNeeded();
108+
super.visitNestMember(nestMember);
109+
}
110+
111+
@Override
112+
public void visitPermittedSubclass(String permittedSubclass) {
113+
addClassAnnotationIfNeeded();
114+
super.visitPermittedSubclass(permittedSubclass);
115+
}
116+
117+
@Override
118+
public void visitInnerClass(String name, String outerName, String innerName, int access) {
119+
addClassAnnotationIfNeeded();
120+
super.visitInnerClass(name, outerName, innerName, access);
121+
}
122+
123+
@Override
124+
public FieldVisitor visitField(int access, String name, String descriptor, String signature, Object value) {
125+
addClassAnnotationIfNeeded();
126+
return super.visitField(access, name, descriptor, signature, value);
127+
}
128+
129+
@Override
130+
public RecordComponentVisitor visitRecordComponent(String name, String descriptor, String signature) {
131+
addClassAnnotationIfNeeded();
132+
return super.visitRecordComponent(name, descriptor, signature);
133+
}
134+
88135
@Override
89136
public MethodVisitor visitMethod(int access, String name, String descriptor, String signature, String[] exceptions) {
137+
addClassAnnotationIfNeeded();
90138
var mv = super.visitMethod(access, name, descriptor, signature, exceptions);
91-
boolean isStatic = (access & ACC_STATIC) != 0;
92-
var key = new MethodKey(
93-
className,
94-
name,
95-
Stream.of(Type.getArgumentTypes(descriptor)).map(Type::getInternalName).toList(),
96-
isStatic
97-
);
98-
var instrumentationMethod = instrumentationMethods.get(key);
99-
if (instrumentationMethod != null) {
100-
// LOGGER.debug("Will instrument method {}", key);
101-
return new EntitlementMethodVisitor(Opcodes.ASM9, mv, isStatic, descriptor, instrumentationMethod);
102-
} else {
103-
// LOGGER.trace("Will not instrument method {}", key);
139+
if (isAnnotationPresent == false) {
140+
boolean isStatic = (access & ACC_STATIC) != 0;
141+
var key = new MethodKey(
142+
className,
143+
name,
144+
Stream.of(Type.getArgumentTypes(descriptor)).map(Type::getInternalName).toList(),
145+
isStatic
146+
);
147+
var instrumentationMethod = instrumentationMethods.get(key);
148+
if (instrumentationMethod != null) {
149+
// LOGGER.debug("Will instrument method {}", key);
150+
return new EntitlementMethodVisitor(Opcodes.ASM9, mv, isStatic, descriptor, instrumentationMethod);
151+
} else {
152+
// LOGGER.trace("Will not instrument method {}", key);
153+
}
104154
}
105155
return mv;
106156
}
157+
158+
/**
159+
* A class annotation can be added via visitAnnotation; we need to call visitAnnotation after all other visitAnnotation
160+
* calls (in case one of them detects our annotation is already present), but before any other subsequent visit* method is called
161+
* (up to visitMethod -- if no visitMethod is called, there is nothing to instrument).
162+
* This includes visitNestMember, visitPermittedSubclass, visitInnerClass, visitField, visitRecordComponent and, of course,
163+
* visitMethod (see {@link ClassVisitor} javadoc).
164+
*/
165+
private void addClassAnnotationIfNeeded() {
166+
if (annotationNeeded) {
167+
// logger.debug("Adding {} annotation", ENTITLEMENT_ANNOTATION);
168+
AnnotationVisitor av = cv.visitAnnotation(ENTITLEMENT_ANNOTATION, true);
169+
if (av != null) {
170+
av.visitEnd();
171+
}
172+
annotationNeeded = false;
173+
}
174+
}
107175
}
108176

109177
static class EntitlementMethodVisitor extends MethodVisitor {

‎distribution/tools/entitlement-agent/impl/src/test/java/org/elasticsearch/entitlement/instrumentation/impl/InstrumenterTests.java‎

Lines changed: 95 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -9,20 +9,24 @@
99

1010
package org.elasticsearch.entitlement.instrumentation.impl;
1111

12+
import org.elasticsearch.common.Strings;
1213
import org.elasticsearch.entitlement.api.EntitlementChecks;
1314
import org.elasticsearch.entitlement.api.EntitlementProvider;
1415
import org.elasticsearch.entitlement.instrumentation.InstrumentationService;
15-
import org.elasticsearch.entitlement.instrumentation.MethodKey;
1616
import org.elasticsearch.logging.LogManager;
1717
import org.elasticsearch.logging.Logger;
1818
import org.elasticsearch.test.ESTestCase;
1919
import org.junit.Before;
20+
import org.objectweb.asm.Type;
2021

2122
import java.lang.reflect.InvocationTargetException;
2223
import java.lang.reflect.Method;
23-
import java.util.Map;
24+
import java.util.Arrays;
25+
import java.util.stream.Collectors;
2426

2527
import static org.elasticsearch.entitlement.instrumentation.impl.ASMUtils.bytecode2text;
28+
import static org.elasticsearch.entitlement.instrumentation.impl.InstrumenterImpl.getClassFileInfo;
29+
import static org.hamcrest.Matchers.is;
2630

2731
/**
2832
* This tests {@link InstrumenterImpl} in isolation, without a java agent.
@@ -60,6 +64,10 @@ public static class ClassToInstrument implements Testable {
6064
public static void systemExit(int status) {
6165
assertEquals(123, status);
6266
}
67+
68+
public static void anotherSystemExit(int status) {
69+
assertEquals(123, status);
70+
}
6371
}
6472

6573
static final class TestException extends RuntimeException {}
@@ -76,8 +84,11 @@ public static class TestEntitlementManager implements EntitlementChecks {
7684
*/
7785
volatile boolean isActive;
7886

87+
int checkSystemExitCallCount = 0;
88+
7989
@Override
8090
public void checkSystemExit(Class<?> callerClass, int status) {
91+
checkSystemExitCallCount++;
8192
assertSame(InstrumenterTests.class, callerClass);
8293
assertEquals(123, status);
8394
throwIfActive();
@@ -90,18 +101,11 @@ private void throwIfActive() {
90101
}
91102
}
92103

93-
public void test() throws Exception {
94-
// This test doesn't replace ClassToInstrument in-place but instead loads a separate
95-
// class ClassToInstrument_NEW that contains the instrumentation. Because of this,
96-
// we need to configure the Transformer to use a MethodKey and instrumentationMethod
97-
// with slightly different signatures (using the common interface Testable) which
98-
// is not what would happen when it's run by the agent.
99-
100-
MethodKey k1 = instrumentationService.methodKeyForTarget(ClassToInstrument.class.getMethod("systemExit", int.class));
101-
Method v1 = EntitlementChecks.class.getMethod("checkSystemExit", Class.class, int.class);
102-
var instrumenter = new InstrumenterImpl("_NEW", Map.of(k1, v1));
104+
public void testClassIsInstrumented() throws Exception {
105+
var classToInstrument = ClassToInstrument.class;
106+
var instrumenter = createInstrumenter(classToInstrument, "systemExit");
103107

104-
byte[] newBytecode = instrumenter.instrumentClassFile(ClassToInstrument.class).bytecodes();
108+
byte[] newBytecode = instrumenter.instrumentClassFile(classToInstrument).bytecodes();
105109

106110
if (logger.isTraceEnabled()) {
107111
logger.trace("Bytecode after instrumentation:\n{}", bytecode2text(newBytecode));
@@ -112,22 +116,96 @@ public void test() throws Exception {
112116
newBytecode
113117
);
114118

119+
getTestChecks().isActive = false;
120+
115121
// Before checking is active, nothing should throw
116-
callStaticSystemExit(newClass, 123);
122+
callStaticMethod(newClass, "systemExit", 123);
117123

118124
getTestChecks().isActive = true;
119125

120126
// After checking is activated, everything should throw
121-
assertThrows(TestException.class, () -> callStaticSystemExit(newClass, 123));
127+
assertThrows(TestException.class, () -> callStaticMethod(newClass, "systemExit", 123));
128+
}
129+
130+
public void testClassIsNotInstrumentedTwice() throws Exception {
131+
var classToInstrument = ClassToInstrument.class;
132+
var instrumenter = createInstrumenter(classToInstrument, "systemExit");
133+
134+
InstrumenterImpl.ClassFileInfo initial = getClassFileInfo(classToInstrument);
135+
var internalClassName = Type.getInternalName(classToInstrument);
136+
137+
byte[] instrumentedBytecode = instrumenter.instrumentClass(internalClassName, initial.bytecodes());
138+
byte[] instrumentedTwiceBytecode = instrumenter.instrumentClass(internalClassName, instrumentedBytecode);
139+
140+
logger.trace(() -> Strings.format("Bytecode after 1st instrumentation:\n%s", bytecode2text(instrumentedBytecode)));
141+
logger.trace(() -> Strings.format("Bytecode after 2nd instrumentation:\n%s", bytecode2text(instrumentedTwiceBytecode)));
142+
143+
Class<?> newClass = new TestLoader(Testable.class.getClassLoader()).defineClassFromBytes(
144+
ClassToInstrument.class.getName() + "_NEW_NEW",
145+
instrumentedTwiceBytecode
146+
);
147+
148+
getTestChecks().isActive = true;
149+
getTestChecks().checkSystemExitCallCount = 0;
150+
151+
assertThrows(TestException.class, () -> callStaticMethod(newClass, "systemExit", 123));
152+
assertThat(getTestChecks().checkSystemExitCallCount, is(1));
153+
}
154+
155+
public void testClassAllMethodsAreInstrumentedFirstPass() throws Exception {
156+
var classToInstrument = ClassToInstrument.class;
157+
var instrumenter = createInstrumenter(classToInstrument, "systemExit", "anotherSystemExit");
158+
159+
InstrumenterImpl.ClassFileInfo initial = getClassFileInfo(classToInstrument);
160+
var internalClassName = Type.getInternalName(classToInstrument);
161+
162+
byte[] instrumentedBytecode = instrumenter.instrumentClass(internalClassName, initial.bytecodes());
163+
byte[] instrumentedTwiceBytecode = instrumenter.instrumentClass(internalClassName, instrumentedBytecode);
164+
165+
logger.trace(() -> Strings.format("Bytecode after 1st instrumentation:\n%s", bytecode2text(instrumentedBytecode)));
166+
logger.trace(() -> Strings.format("Bytecode after 2nd instrumentation:\n%s", bytecode2text(instrumentedTwiceBytecode)));
167+
168+
Class<?> newClass = new TestLoader(Testable.class.getClassLoader()).defineClassFromBytes(
169+
ClassToInstrument.class.getName() + "_NEW_NEW",
170+
instrumentedTwiceBytecode
171+
);
172+
173+
getTestChecks().isActive = true;
174+
getTestChecks().checkSystemExitCallCount = 0;
175+
176+
assertThrows(TestException.class, () -> callStaticMethod(newClass, "systemExit", 123));
177+
assertThat(getTestChecks().checkSystemExitCallCount, is(1));
178+
179+
assertThrows(TestException.class, () -> callStaticMethod(newClass, "anotherSystemExit", 123));
180+
assertThat(getTestChecks().checkSystemExitCallCount, is(2));
181+
}
182+
183+
/** This test doesn't replace ClassToInstrument in-place but instead loads a separate
184+
* class ClassToInstrument_NEW that contains the instrumentation. Because of this,
185+
* we need to configure the Transformer to use a MethodKey and instrumentationMethod
186+
* with slightly different signatures (using the common interface Testable) which
187+
* is not what would happen when it's run by the agent.
188+
*/
189+
private InstrumenterImpl createInstrumenter(Class<?> classToInstrument, String... methodNames) throws NoSuchMethodException {
190+
Method v1 = EntitlementChecks.class.getMethod("checkSystemExit", Class.class, int.class);
191+
var methods = Arrays.stream(methodNames).map(name -> {
192+
try {
193+
return instrumentationService.methodKeyForTarget(classToInstrument.getMethod(name, int.class));
194+
} catch (NoSuchMethodException e) {
195+
throw new RuntimeException(e);
196+
}
197+
}).collect(Collectors.toUnmodifiableMap(name -> name, name -> v1));
198+
199+
return new InstrumenterImpl("_NEW", methods);
122200
}
123201

124202
/**
125203
* Calling a static method of a dynamically loaded class is significantly more cumbersome
126204
* than calling a virtual method.
127205
*/
128-
private static void callStaticSystemExit(Class<?> c, int status) throws NoSuchMethodException, IllegalAccessException {
206+
private static void callStaticMethod(Class<?> c, String methodName, int status) throws NoSuchMethodException, IllegalAccessException {
129207
try {
130-
c.getMethod("systemExit", int.class).invoke(null, status);
208+
c.getMethod(methodName, int.class).invoke(null, status);
131209
} catch (InvocationTargetException e) {
132210
Throwable cause = e.getCause();
133211
if (cause instanceof TestException n) {

‎distribution/tools/plugin-cli/src/main/java/org/elasticsearch/plugins/cli/SyncPluginsAction.java‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525
import java.nio.file.Path;
2626
import java.util.ArrayList;
2727
import java.util.Comparator;
28+
import java.util.HashMap;
2829
import java.util.Iterator;
2930
import java.util.List;
3031
import java.util.Locale;
@@ -60,7 +61,7 @@ public SyncPluginsAction(Terminal terminal, Environment env) {
6061
* @throws UserException if a plugins config file is found.
6162
*/
6263
public static void ensureNoConfigFile(Environment env) throws UserException {
63-
final Path pluginsConfig = env.configFile().resolve("elasticsearch-plugins.yml");
64+
final Path pluginsConfig = env.configFile().resolve(ELASTICSEARCH_PLUGINS_YML);
6465
if (Files.exists(pluginsConfig)) {
6566
throw new UserException(
6667
ExitCodes.USAGE,
@@ -207,9 +208,8 @@ private List<InstallablePlugin> getPluginsToUpgrade(
207208
Optional<PluginsConfig> cachedPluginsConfig,
208209
List<PluginDescriptor> existingPlugins
209210
) {
210-
final Map<String, String> cachedPluginIdToLocation = cachedPluginsConfig.map(
211-
config -> config.getPlugins().stream().collect(Collectors.toMap(InstallablePlugin::getId, InstallablePlugin::getLocation))
212-
).orElse(Map.of());
211+
final Map<String, String> cachedPluginIdToLocation = new HashMap<>();
212+
cachedPluginsConfig.ifPresent(config -> config.getPlugins().forEach(p -> cachedPluginIdToLocation.put(p.getId(), p.getLocation())));
213213

214214
return pluginsToMaybeUpgrade.stream().filter(eachPlugin -> {
215215
final String eachPluginId = eachPlugin.getId();

‎distribution/tools/plugin-cli/src/test/java/org/elasticsearch/plugins/cli/SyncPluginsActionTests.java‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -157,6 +157,22 @@ public void test_getPluginChanges_withOfficialPluginToUpgrade_returnsPluginToUpg
157157
assertThat(pluginChanges.upgrade.get(0).getId(), equalTo("analysis-icu"));
158158
}
159159

160+
/**
161+
* Check that when there is an official plugin in the config file and in the cached config, then we
162+
* calculate that the plugin does not need to be upgraded.
163+
*/
164+
public void test_getPluginChanges_withOfficialPluginCachedConfigAndNoChanges_returnsNoChanges() throws Exception {
165+
createPlugin("analysis-icu");
166+
config.setPlugins(List.of(new InstallablePlugin("analysis-icu")));
167+
168+
final PluginsConfig cachedConfig = new PluginsConfig();
169+
cachedConfig.setPlugins(List.of(new InstallablePlugin("analysis-icu")));
170+
171+
final PluginChanges pluginChanges = action.getPluginChanges(config, Optional.of(cachedConfig));
172+
173+
assertThat(pluginChanges.isEmpty(), is(true));
174+
}
175+
160176
/**
161177
* Check that if an unofficial plugins' location has not changed in the cached config, then we
162178
* calculate that the plugin does not need to be upgraded.

‎docs/changelog/115640.yaml‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
pr: 115640
2+
summary: Fix NPE on plugin sync
3+
area: Infra/CLI
4+
type: bug
5+
issues:
6+
- 114818

0 commit comments

Comments
 (0)