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

output/alert: rewrite code for app-layer properties #9511

Closed

Conversation

catenacyber
Copy link
Contributor

Link to redmine ticket:
https://redmine.openinfosecfoundation.org/issues/3827
preliminary work for https://redmine.openinfosecfoundation.org/issues/5053 and app-layer plugins
preliminary work for https://redmine.openinfosecfoundation.org/issues/5977 as well
Part of #8961

Describe changes:

  • Fix setup-app-layer script so that it adds app-layer metadata to alerts

After that, there is still from #8961

  • addition of protocols missing alert metadata (like krb5) + behavioral change for dns alert metadata
  • reusing these SimpleTxLogFunc from a JsonGenericLogger to remove many C files

#9252 with some renaming as per code review + reusing code for output-json-file.c

Same as #9499 but without the force-push on github

Especially fix setup-app-layer script to not forget this part

Ticket: OISF#3827
@codecov
Copy link

codecov bot commented Sep 22, 2023

Codecov Report

Merging #9511 (1e77969) into master (af4bb91) will decrease coverage by 0.03%.
The diff coverage is 96.47%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #9511      +/-   ##
==========================================
- Coverage   82.19%   82.16%   -0.03%     
==========================================
  Files         968      968              
  Lines      274203   274059     -144     
==========================================
- Hits       225373   225193     -180     
- Misses      48830    48866      +36     
Flag Coverage Δ
fuzzcorpus 64.02% <96.47%> (-0.07%) ⬇️
suricata-verify 60.88% <95.29%> (-0.07%) ⬇️
unittests 62.92% <0.00%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

@suricata-qa
Copy link

Information: QA ran without warnings.

Pipeline 16262

Copy link
Member

@jasonish jasonish left a comment

Choose a reason for hiding this comment

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

I want to see this land as soon as possible, but I also want the commit message to explain a little bit more about whats going on. The ticket referenced may be related, but this is more about the actual logging, then the initialization of logging.

@victorjulien
Copy link
Member

Still not really a fan of introducing a new global hard coded table. But I guess if this is a step towards making things more dynamic I can live with it.

@jasonish
Copy link
Member

Still not really a fan of introducing a new global hard coded table. But I guess if this is a step towards making things more dynamic I can live with it.

Yeah, me to. I think without this step, future steps will just take longer to complete tho.

@catenacyber
Copy link
Contributor Author

Replaced by #9768

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

Successfully merging this pull request may close these issues.

4 participants