[backend] add executionTrace table #2296

MarineLeM · 2025-01-24T17:56:32Z

Proposed changes

Extract the status_executions field from InjectStatus and create a new table called ExecutionTrace.
Replace the status_executions field across the entire project with references to the new ExecutionTrace table.
Remove any unused fields from the InjectStatus entity.
Refactor the compute Status function

Related issues

Ability to understand my payload traces for multi end points. #1951

openbas-api/src/main/java/io/openbas/executors/Executor.java

codecov · 2025-01-27T15:38:36Z

Codecov Report

Attention: Patch coverage is 18.30601% with 299 lines in your changes missing coverage. Please review.

Project coverage is 36.33%. Comparing base (3854057) to head (72e1416).
Report is 2 commits behind head on release/current.

Files with missing lines	Patch %	Lines
...enbas/rest/inject/service/InjectStatusService.java	28.33%	79 Missing and 7 partials ⚠️
...caldera/service/CalderaResultCollectorService.java	4.25%	45 Missing ⚠️
...as/migration/V3_62__Add_ExecutionTraces_table.java	24.13%	43 Missing and 1 partial ⚠️
.../io/openbas/injectors/caldera/CalderaExecutor.java	0.00%	27 Missing ⚠️
...io/openbas/execution/ExecutionExecutorService.java	0.00%	19 Missing ⚠️
...io/openbas/scheduler/jobs/InjectsExecutionJob.java	0.00%	18 Missing ⚠️
.../openbas/injectors/email/service/EmailService.java	33.33%	9 Missing and 1 partial ⚠️
.../io/openbas/injectors/channel/ChannelExecutor.java	0.00%	6 Missing ⚠️
...nbas/injectors/opencti/service/OpenCTIService.java	0.00%	6 Missing ⚠️
...n/java/io/openbas/injectors/lade/LadeExecutor.java	0.00%	5 Missing ⚠️
... and 14 more

Additional details and impacted files

@@                  Coverage Diff                  @@
##             release/current    #2296      +/-   ##
=====================================================
+ Coverage              36.16%   36.33%   +0.16%     
- Complexity              1653     1659       +6     
=====================================================
  Files                    589      593       +4     
  Lines                  18066    18122      +56     
  Branches                1176     1193      +17     
=====================================================
+ Hits                    6533     6584      +51     
+ Misses                 11207    11205       -2     
- Partials                 326      333       +7

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

openbas-api/src/main/java/io/openbas/injectors/caldera/CalderaExecutor.java

openbas-api/src/main/java/io/openbas/injectors/caldera/CalderaResultCollector.java

...as-api/src/main/java/io/openbas/injectors/caldera/service/CalderaResultCollectorService.java

openbas-api/src/main/java/io/openbas/injectors/email/EmailExecutor.java

openbas-api/src/main/java/io/openbas/migration/V3_62__Add_ExecutionTraces_table.java

openbas-api/src/main/java/io/openbas/rest/inject/InjectApi.java

Signed-off-by: Marine LM <[email protected]>

damgouj

Tests OK for me : Caldera agent, OBAS agent, mail, sms.
Just few comments and maybe "Payload completed" for this one ?

Good work, thanks !
PS : be careful, you have got some tests issues in the CI.

damgouj · 2025-01-28T14:08:58Z

openbas-api/src/main/java/io/openbas/injectors/caldera/CalderaResultCollector.java

+              this.injectRepository,
+              this.injectStatusRepository,
+              this.calderaService,
+              this.injectStatusService);


You pass injectStatusService + injectStatusRepository ? You can't pass the service only ? Because injectStatusService uses injectStatusRepository

openbas-api/src/main/java/io/openbas/migration/V3_62__Add_ExecutionTraces_table.java

damgouj · 2025-01-28T14:34:23Z

openbas-api/src/main/java/io/openbas/rest/inject/service/InjectStatusService.java

+      }
+    }
+
+    if (successCount > 0 && errorCount == 0 && maybePreventedCount == 0 & partialCount == 0) {


& alone in your if, is it normal ? Same below

damgouj · 2025-01-28T15:02:23Z

openbas-api/src/main/java/io/openbas/rest/inject/service/InjectStatusService.java

+    }
+
+    injectStatus.addTrace(
+        ExecutionTraceStatus.INFO, "Process finish", ExecutionTraceAction.PROCESS_FINISH, null);


Process finished ?

openbas-api/src/main/java/io/openbas/rest/inject/service/InjectStatusService.java

...as-api/src/main/java/io/openbas/injectors/caldera/service/CalderaResultCollectorService.java

RomuDeuxfois · 2025-01-28T15:02:29Z

openbas-model/src/main/java/io/openbas/database/model/BaseInjectStatus.java

@@ -32,56 +29,20 @@ public abstract class BaseInjectStatus implements Base {
  @NotNull
  private ExecutionStatus name;

-  // region dates tracking
-  @Column(name = "status_executions")


question: Do we need a migration for old execution ?

Why we shouldn't ? Because status_executions in the inject_statuses table would only be used for old executed injects while also having a relation to ExecutionTrace. This is confusing since both represent the same thing, leading to redundancy and unclear data relationships.

RomuDeuxfois · 2025-01-28T15:06:50Z

openbas-api/src/main/java/io/openbas/rest/atomic_testing/form/InjectStatusOutput.java

@@ -32,26 +32,11 @@ public String getName() {

  @Builder.Default
  @JsonProperty("status_traces")
-  private List<InjectStatusExecution> traces = new ArrayList<>();
+  private List<ExecutionTraces> traces = new ArrayList<>();


question: Do we need an ExecutionTracesOutput ?

It can be done in the next chunk since you'll be working on what to send to the frontend. But if you prefer I can clean it now ?

RomuDeuxfois · 2025-01-28T15:09:58Z

openbas-api/src/main/java/io/openbas/rest/exception/DetailedException.java

+import lombok.Getter;
+
+@Getter
+public class DetailedException extends RuntimeException {


nitpick: I am not a big fan of this nomenclature.
But, I don't have any other idea.

RomuDeuxfois · 2025-01-28T15:13:37Z

openbas-api/src/main/java/io/openbas/execution/ExecutionExecutorService.java

@@ -34,6 +38,7 @@ public class ExecutionExecutorService {
  private final OpenBASExecutorContextService openBASExecutorContextService;
  private final InjectStatusRepository injectStatusRepository;

+  @Transactional


issue: Why we have a transactional here ? If we have any error not handle in the try catch, we will loop on error for a specific inject.

RomuDeuxfois · 2025-01-28T15:17:51Z

openbas-api/src/main/java/io/openbas/executors/Injector.java

@@ -96,7 +96,7 @@ public List<DataAttachment> resolveAttachments(
                        doc.getName(), doc.getOriginalFilename(), content, doc.getContentType()));
              } catch (Exception e) {
                String message = "Error getting direct attachment " + doc.getName();
-                execution.addTrace(traceError(message));
+                execution.addTrace(getNewErrorTrace(message, ExecutionTraceAction.EXECUTION));


question: Why having an EXECUTION here ?
It's an internal process, we don't do anything on the inject itself.

openbas-api/src/main/java/io/openbas/injectors/email/service/EmailService.java

RomuDeuxfois · 2025-01-28T16:16:06Z

openbas-model/src/main/java/io/openbas/database/model/InjectTestStatus.java

+  @Column(name = "status_executions")
+  @Convert(converter = InjectStatusExecutionConverter.class)
+  @JsonProperty("status_traces")
+  private List<InjectStatusExecution> traces = new ArrayList<>();


question: Why we don't have the same bahavior as for InjectStatus ?

RomuDeuxfois · 2025-01-28T16:19:25Z

openbas-model/src/main/java/io/openbas/database/model/InjectStatusExecution.java

@@ -12,9 +12,6 @@
 @Setter


nitpick: I am not sure it's a good idea to have two model: one for test and one for real action.

openbas-model/src/main/java/io/openbas/database/model/ExecutionTraceStatus.java

openbas-api/src/main/java/io/openbas/rest/inject/form/InjectExecutionAction.java

savacano28 · 2025-01-29T08:14:51Z

It's great to have this level of granularity in the traces :). 🎉

Tests : ✅

Email, sms, caldera, challenge, channel, obas
Asset group, asset (active, inactive), atomic testing and exercise
Traces complete, finished | info, asset_inactive | maybe prevented, success, error, partial

RomuDeuxfois · 2025-01-29T10:49:22Z

openbas-model/src/main/java/io/openbas/database/model/ExecutionTraceAction.java

+
+  CLEANUP_EXECUTION,
+  COMPLETE, // when inject of one asset is finish, or of one user
+  PROCESS_FINISH // inject is completely done


Should be removed ?

Signed-off-by: Marine LM <[email protected]>