Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[backend] Implement log shipping to Graylog via GELF (#9629) #8410

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

bnazare
Copy link

@bnazare bnazare commented Sep 18, 2024

Proposed changes

  • Shipping of logs to Graylog via GELF
  • Wait for log flushing before application shutdown

Related issues

There are not related issues but this subject has been previously discussed with Linkare within the scope of the OpenCTI implementation for the Centre for Cybersecurity Belgium (https://ccb.belgium.be/).

EDIT: closes #9629 (issue created for tracking)

Checklist

  • I consider the submitted work as finished
  • I tested the code for its functionality
  • I wrote test cases for the relevant uses case (coverage and e2e)
  • I added/update the relevant documentation (either on github or on notion)
  • Where necessary I refactored code to improve the overall quality

Further comments

We added an extra step to the application shutdown so that it now waits for the loggers to flush. This was previously not very relevant but sending data via the network introduces some latency that makes this necessary. Failing to do so results in the loss of some of the last log messages. This is particularly critical in the cases where the application fails as those messages will probably include the details of the relevant error.

No new tests were added for this functionality as that would require setting up an integration testing environment containing at least a Graylog instance plus subordinate MongoDB and Elasticsearch instances. Furthermore, having the test communicate with Graylog, so as to assert that the logs where correctly stored, would not be a trivial implementation. All in all, we estimate the effort required to implement all of this would dwarf the effort put into such a small change.

On the other hand, the changes proposed are all opt-in so they shouldn't break any existing behaviour. The little code that is unavoidable will actually run on every startup and shutdown and thus all existing tests will at least validate that the new functionality has no negative effects when not explicitly enabled.

Copy link

codecov bot commented Sep 18, 2024

Codecov Report

Attention: Patch coverage is 35.82090% with 86 lines in your changes missing coverage. Please review.

Project coverage is 65.17%. Comparing base (0d2a3f8) to head (3fd8920).

Files with missing lines Patch % Lines
...latform/opencti-graphql/src/config/log-shipping.js 31.11% 31 Missing ⚠️
...tform/opencti-graphql/src/config/gelf-transport.js 55.76% 23 Missing ⚠️
...pencti-platform/opencti-graphql/src/config/conf.js 19.23% 21 Missing ⚠️
opencti-platform/opencti-graphql/src/boot.js 0.00% 11 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8410      +/-   ##
==========================================
- Coverage   65.24%   65.17%   -0.08%     
==========================================
  Files         630      632       +2     
  Lines       60273    60406     +133     
  Branches     6763     6764       +1     
==========================================
+ Hits        39325    39368      +43     
- Misses      20948    21038      +90     

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

@SarahBocognano SarahBocognano added the community use to identify PR from community label Sep 24, 2024
@aHenryJard aHenryJard self-assigned this Dec 6, 2024
@labo-flg
Copy link
Member

labo-flg commented Jan 7, 2025

@bnazare thanks for your contribution !
Could you please setup commit signing (https://docs.github.com/en/authentication/managing-commit-signature-verification/signing-commits) so we can merge your code please ?

@aleitao
Copy link

aleitao commented Jan 10, 2025

This PR have a counterpart in OpenCTI-Platform/client-python#786.

@bnazare bnazare force-pushed the feature/log-shipping branch from 3c9b6fe to 55f1339 Compare January 10, 2025 17:48
@bnazare
Copy link
Author

bnazare commented Jan 10, 2025

@bnazare thanks for your contribution ! Could you please setup commit signing (https://docs.github.com/en/authentication/managing-commit-signature-verification/signing-commits) so we can merge your code please ?

Sorry about the delay, they're signed now ;).

@richard-julien richard-julien force-pushed the master branch 5 times, most recently from b414944 to c7f4cb7 Compare January 13, 2025 09:27
@JeremyCloarec
Copy link
Contributor

@bnazare thanks for your contribution ! Could you please setup commit signing (https://docs.github.com/en/authentication/managing-commit-signature-verification/signing-commits) so we can merge your code please ?

Sorry about the delay, they're signed now ;).

Thanks! Some merge conflicts remain to be resolved, but we'll be able to merge your PRs as soon as those are solved.

@labo-flg labo-flg added this to the Release 6.5.0 milestone Jan 17, 2025
@aHenryJard aHenryJard changed the title Implement log shipping to Graylog via GELF [backend] Implement log shipping to Graylog via GELF (#9629) Jan 17, 2025
@labo-flg labo-flg removed this from the Release 6.5.0 milestone Jan 17, 2025
@labo-flg
Copy link
Member

@bnazare could you rebase your PR on latest master please ?
We can merge this rapidly afterwards.

@bnazare bnazare force-pushed the feature/log-shipping branch from 55f1339 to 3fd8920 Compare January 24, 2025 19:01
@bnazare
Copy link
Author

bnazare commented Jan 24, 2025

@labo-flg I've just rebased the PR.

Unfortunately, while testing the results I found an issue with the package "winston-gelf". That package hasn't seen any movement in 6 years and I also couldn't find a more suitable alternative. So, to solve it, I copied its contents into the project and patched it. It's only 40 something lines so it's not a very large addition. Let me know if you'd prefer a different approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community use to identify PR from community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement log shipping to Graylog via GELF
6 participants