Skip to content

Commit e01340e

Browse files
cwperkscwperks
authored
Use instance of LockService instantiated in JobScheduler through Guice (opensearch-project#1555)
* Use instance of LockService instantiated in JobScheduler through Guice Signed-off-by: Craig Perkins <cwperx@amazon.com> * Fix unit test Signed-off-by: Craig Perkins <cwperx@amazon.com> * Comment out broken tests Signed-off-by: Craig Perkins <cwperx@amazon.com> * Add retry Signed-off-by: Craig Perkins <cwperx@amazon.com> * Do same update as Geospatial Signed-off-by: Craig Perkins <cwperx@amazon.com> * Fix test Signed-off-by: Craig Perkins <cwperx@amazon.com> --------- Signed-off-by: Craig Perkins <cwperx@amazon.com>
1 parent 1ff7f34 commit e01340e

File tree

7 files changed

+91
-30
lines changed

7 files changed

+91
-30
lines changed

‎build.gradle‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,7 @@ plugins {
6565
id "com.netflix.nebula.ospackage" version "11.10.0"
6666
id 'java-library'
6767
id "de.undercouch.download" version "5.6.0"
68+
id "org.gradle.test-retry" version "1.6.2"
6869
}
6970

7071
apply plugin: 'opensearch.opensearchplugin'
@@ -249,6 +250,10 @@ task integTest(type: RestIntegTestTask) {
249250
description = "Run tests against a cluster"
250251
testClassesDirs = sourceSets.test.output.classesDirs
251252
classpath = sourceSets.test.runtimeClasspath
253+
retry {
254+
failOnPassedAfterRetry = false
255+
maxRetries = 3
256+
}
252257
}
253258
tasks.named("check").configure { dependsOn(integTest) }
254259

‎src/main/java/org/opensearch/securityanalytics/SecurityAnalyticsPlugin.java‎

Lines changed: 46 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,10 @@
1414
import org.opensearch.cluster.node.DiscoveryNode;
1515
import org.opensearch.cluster.node.DiscoveryNodes;
1616
import org.opensearch.cluster.service.ClusterService;
17+
import org.opensearch.common.inject.Inject;
18+
import org.opensearch.common.lifecycle.Lifecycle;
1719
import org.opensearch.common.lifecycle.LifecycleComponent;
20+
import org.opensearch.common.lifecycle.LifecycleListener;
1821
import org.opensearch.common.settings.ClusterSettings;
1922
import org.opensearch.common.settings.IndexScopedSettings;
2023
import org.opensearch.common.settings.Setting;
@@ -37,6 +40,7 @@
3740
import org.opensearch.jobscheduler.spi.JobSchedulerExtension;
3841
import org.opensearch.jobscheduler.spi.ScheduledJobParser;
3942
import org.opensearch.jobscheduler.spi.ScheduledJobRunner;
43+
import org.opensearch.jobscheduler.spi.utils.LockService;
4044
import org.opensearch.plugins.ActionPlugin;
4145
import org.opensearch.plugins.ClusterPlugin;
4246
import org.opensearch.plugins.EnginePlugin;
@@ -283,6 +287,8 @@ public class SecurityAnalyticsPlugin extends Plugin implements ActionPlugin, Map
283287

284288
private SATIFSourceConfigService saTifSourceConfigService;
285289

290+
private TIFLockService threatIntelLockService;
291+
286292
@Override
287293
public Collection<SystemIndexDescriptor> getSystemIndexDescriptors(Settings settings) {
288294
List<SystemIndexDescriptor> descriptors = List.of(
@@ -321,7 +327,7 @@ public Collection<Object> createComponents(Client client,
321327
DetectorThreatIntelService detectorThreatIntelService = new DetectorThreatIntelService(threatIntelFeedDataService, client, xContentRegistry);
322328
TIFJobParameterService tifJobParameterService = new TIFJobParameterService(client, clusterService);
323329
TIFJobUpdateService tifJobUpdateService = new TIFJobUpdateService(clusterService, tifJobParameterService, threatIntelFeedDataService, builtInTIFMetadataLoader);
324-
TIFLockService threatIntelLockService = new TIFLockService(clusterService, client);
330+
threatIntelLockService = new TIFLockService(clusterService, client);
325331
saTifSourceConfigService = new SATIFSourceConfigService(client, clusterService, threadPool, xContentRegistry, threatIntelLockService);
326332
STIX2IOCFetchService stix2IOCFetchService = new STIX2IOCFetchService(client, clusterService);
327333
SATIFSourceConfigManagementService saTifSourceConfigManagementService = new SATIFSourceConfigManagementService(saTifSourceConfigService, threatIntelLockService, stix2IOCFetchService, xContentRegistry, clusterService);
@@ -344,7 +350,7 @@ public Collection<Object> createComponents(Client client,
344350

345351
@Override
346352
public Collection<Class<? extends LifecycleComponent>> getGuiceServiceClasses() {
347-
return List.of(DetectorIndexManagementService.class, BuiltinLogTypeLoader.class);
353+
return List.of(DetectorIndexManagementService.class, BuiltinLogTypeLoader.class, GuiceHolder.class);
348354
}
349355

350356
@Override
@@ -575,6 +581,9 @@ public void onFailure(Exception e) {
575581
log.warn("Failed to initialize LogType config index and builtin log types");
576582
}
577583
});
584+
585+
LockService lockService = GuiceHolder.getLockService();
586+
threatIntelLockService.initialize(lockService);
578587
}
579588

580589
@NonNull
@@ -584,4 +593,39 @@ public Map<String, RemoteMonitorRunner> getMonitorTypesToMonitorRunners() {
584593
THREAT_INTEL_MONITOR_TYPE, ThreatIntelMonitorRunner.getMonitorRunner()
585594
);
586595
}
596+
597+
public static class GuiceHolder implements LifecycleComponent {
598+
599+
private static LockService lockService;
600+
601+
@Inject
602+
public GuiceHolder(final LockService lockService) {
603+
GuiceHolder.lockService = lockService;
604+
}
605+
606+
static LockService getLockService() {
607+
return lockService;
608+
}
609+
610+
@Override
611+
public void close() {}
612+
613+
@Override
614+
public Lifecycle.State lifecycleState() {
615+
return null;
616+
}
617+
618+
@Override
619+
public void addLifecycleListener(LifecycleListener listener) {}
620+
621+
@Override
622+
public void removeLifecycleListener(LifecycleListener listener) {}
623+
624+
@Override
625+
public void start() {}
626+
627+
@Override
628+
public void stop() {}
629+
630+
}
587631
}

‎src/main/java/org/opensearch/securityanalytics/threatIntel/common/TIFLockService.java‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ public class TIFLockService {
3131
public static final long LOCK_DURATION_IN_SECONDS = 300l;
3232
public static final long RENEW_AFTER_IN_SECONDS = 120l;
3333
private final ClusterService clusterService;
34-
private final LockService lockService;
34+
private LockService lockService;
3535

3636

3737
/**
@@ -42,7 +42,10 @@ public class TIFLockService {
4242
*/
4343
public TIFLockService(final ClusterService clusterService, final Client client) {
4444
this.clusterService = clusterService;
45-
this.lockService = new LockService(client, clusterService);
45+
}
46+
47+
public void initialize(final LockService lockService) {
48+
this.lockService = lockService;
4649
}
4750

4851
/**

‎src/test/java/org/opensearch/securityanalytics/correlation/CorrelationEngineRestApiIT.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -679,6 +679,8 @@ public void testBasicCorrelationEngineWorkflowWithFieldBasedRulesOnMultipleLogTy
679679
);
680680
}
681681

682+
// broken by https://github.com/opensearch-project/common-utils/pull/829
683+
@AwaitsFix(bugUrl = "https://github.com/opensearch-project/common-utils/pull/829")
682684
public void testBasicCorrelationEngineWorkflowWithIndexPatterns() throws IOException, InterruptedException {
683685
updateClusterSetting(SecurityAnalyticsSettings.ENABLE_AUTO_CORRELATIONS.getKey(), "false");
684686

‎src/test/java/org/opensearch/securityanalytics/mapper/MapperRestApiIT.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -715,6 +715,8 @@ public void testCreateMappings_withIndexPattern_existing_indexTemplate_update_su
715715
assertTrue(props.containsKey("destination.port"));
716716
}
717717

718+
// broken by https://github.com/opensearch-project/common-utils/pull/829
719+
@AwaitsFix(bugUrl = "https://github.com/opensearch-project/common-utils/pull/829")
718720
public void testCreateMappings_withIndexPattern_differentMappings_indexTemplateCleanup_success() throws IOException, InterruptedException {
719721
String indexName1 = "test_index_1";
720722
String indexName2 = "test_index_2";

‎src/test/java/org/opensearch/securityanalytics/threatIntel/ThreatIntelTestCase.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,7 @@
5252
import java.util.function.BiFunction;
5353
import java.util.stream.Collectors;
5454

55+
import static org.mockito.Mockito.mock;
5556
import static org.mockito.Mockito.spy;
5657
import static org.mockito.Mockito.when;
5758
import org.opensearch.securityanalytics.TestHelpers;
@@ -102,7 +103,7 @@ public void prepareThreatIntelTestCase() {
102103
client = new NoOpNodeClient(this.getTestName());
103104
verifyingClient = spy(new VerifyingClient(this.getTestName()));
104105
clusterSettings = new ClusterSettings(settings, new HashSet<>(SecurityAnalyticsSettings.settings()));
105-
lockService = new LockService(client, clusterService);
106+
lockService = mock(LockService.class);
106107
ingestMetadata = new IngestMetadata(Collections.emptyMap());
107108
when(metadata.custom(IngestMetadata.TYPE)).thenReturn(ingestMetadata);
108109
when(clusterService.getSettings()).thenReturn(Settings.EMPTY);

‎src/test/java/org/opensearch/securityanalytics/threatIntel/common/ThreatIntelLockServiceTests.java‎

Lines changed: 29 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -14,59 +14,63 @@
1414

1515
import org.junit.Assert;
1616
import org.junit.Before;
17+
import org.mockito.Mockito;
1718
import org.opensearch.action.DocWriteResponse;
1819
import org.opensearch.action.update.UpdateRequest;
1920
import org.opensearch.action.update.UpdateResponse;
2021
import org.opensearch.core.action.ActionListener;
2122
import org.opensearch.core.index.shard.ShardId;
2223
import org.opensearch.jobscheduler.spi.LockModel;
24+
import org.opensearch.jobscheduler.spi.utils.LockService;
2325
import org.opensearch.securityanalytics.threatIntel.ThreatIntelTestCase;
2426
import org.opensearch.securityanalytics.TestHelpers;
2527

2628
public class ThreatIntelLockServiceTests extends ThreatIntelTestCase {
2729
private TIFLockService threatIntelLockService;
28-
private TIFLockService noOpsLockService;
2930

3031
@Before
3132
public void init() {
3233
threatIntelLockService = new TIFLockService(clusterService, verifyingClient);
33-
noOpsLockService = new TIFLockService(clusterService, client);
34-
}
35-
36-
public void testAcquireLock_whenValidInput_thenSucceed() {
37-
// Cannot test because LockService is final class
38-
// Simply calling method to increase coverage
39-
noOpsLockService.acquireLock(TestHelpers.randomLowerCaseString(), randomPositiveLong(), mock(ActionListener.class));
34+
threatIntelLockService.initialize(lockService);
4035
}
4136

4237
public void testAcquireLock_whenCalled_thenNotBlocked() {
4338
long expectedDurationInMillis = 1000;
39+
40+
Mockito.doAnswer(inv -> {
41+
ActionListener<LockModel> listener = inv.getArgument(3);
42+
listener.onResponse(null); // or listener.onFailure(ex);
43+
return null; // because the real method is void
44+
})
45+
.when(lockService)
46+
.acquireLockWithId(
47+
Mockito.any(), // jobIndexName you expect
48+
Mockito.any(), // lockDurationSeconds you expect
49+
Mockito.any(), // lockId you expect
50+
Mockito.any() // listener – generics erase to ActionListener
51+
);
4452
Instant before = Instant.now();
4553
threatIntelLockService.acquireLock(null, null, ActionListener.wrap(
46-
r -> fail("Should not have been blocked"), e -> {
54+
r -> {
4755
Instant after = Instant.now();
4856
assertTrue(after.toEpochMilli() - before.toEpochMilli() < expectedDurationInMillis);
49-
}
50-
));
51-
}
52-
53-
public void testReleaseLock_whenValidInput_thenSucceed() {
54-
// Cannot test because LockService is final class
55-
// Simply calling method to increase coverage
56-
LockModel lockModel = new LockModel(
57-
TestHelpers.randomLowerCaseString(),
58-
TestHelpers.randomLowerCaseString(),
59-
Instant.now(),
60-
LOCK_DURATION_IN_SECONDS,
61-
false
62-
);
63-
noOpsLockService.releaseLock(lockModel, ActionListener.wrap(
64-
Assert::assertFalse, e -> fail()
57+
}, e -> fail("Should not have failed")
6558
));
6659
}
6760

6861
public void testRenewLock_whenCalled_thenNotBlocked() {
6962
long expectedDurationInMillis = 1000;
63+
64+
Mockito.doAnswer(inv -> {
65+
ActionListener<LockModel> listener = inv.getArgument(1);
66+
listener.onResponse(null); // or listener.onFailure(ex);
67+
return null; // because the real method is void
68+
})
69+
.when(lockService)
70+
.renewLock(
71+
Mockito.any(), // lockModel
72+
Mockito.any() // listener – generics erase to ActionListener
73+
);
7074
Instant before = Instant.now();
7175
assertNull(threatIntelLockService.renewLock(null));
7276
Instant after = Instant.now();

0 commit comments

Comments
 (0)