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.

Sign up for GitHub

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

Jump to bottom

[trendmicro] Add support of Deep Security version 20 #9124

Merged
merged 4 commits into from Feb 26, 2024
Merged

[trendmicro] Add support of Deep Security version 20 #9124

merged 4 commits into from Feb 26, 2024

Conversation

mohitjha-elastic
Copy link
Contributor

Type of change

  • Enhancement

What does this PR do?

  1. Add Support of Deep Securiry Version 20.
  2. Enhance the Code and ECS mappings.
  3. Update the minimum kibana version to 8.11.0.
  4. Create new dashboard on the basis of Deep Security version 20.

Checklist

  • I have reviewed tips for building integrations and this pull request is aligned with them.
  • I have verified that all data streams collect metrics or logs.
  • I have added an entry to my package's changelog.yml file.
  • I have verified that Kibana version constraints are current according to guidelines.

All changes

  • Change follows the contributing guidelines
  • Supported versions of the monitoring target are documented
  • Supported operating systems are documented (if applicable)
  • Integration or System tests exist
  • Documentation exists
  • Fields follow ECS and naming conventions
  • At least a manual test with ES / Kibana / Agent has been performed.
  • Required Kibana version set to: ^8.11.0

How to test this PR locally

Clone integrations repo.
Install the elastic package locally.
Start the elastic stack using the elastic package.
Move to integrations/packages/trendmicro directory.
Run the following command to run tests.
elastic-package test -v

Automated Test

test-log-trendmicro.txt

Screenshots

image
image

@mohitjha-elastic
Add support of Deep Security version 20.
35da6d4 
Enhance the code and ECS mappings.
Update the minimum kibana version to 8.11.0
Add support of new dashboard.
@mohitjha-elastic mohitjha-elastic requested a review from a team as a code owner February 12, 2024 12:32
@jamiehynds jamiehynds requested a review from a team February 19, 2024 10:41
jamiehynds
jamiehynds reviewed Feb 19, 2024
View reviewed changes
packages/trendmicro/manifest.yml Outdated
title: "Trendmicro"
version: "1.8.4"
description: "collect Trendmicro Deep Security events with elastic agent."
title: Trend Micro
Copy link

Choose a reason for hiding this comment

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

Could we adjust the title to 'Trend Micro Deep Security' to avoid confusion with other Trend products.

jamiehynds
jamiehynds reviewed Feb 19, 2024
View reviewed changes
packages/trendmicro/manifest.yml Outdated
description: "collect Trendmicro Deep Security events with elastic agent."
title: Trend Micro
version: "1.9.0"
description: Collect logs from Trend Micro with Elastic Agent.
Copy link

Choose a reason for hiding this comment

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

Change to Trend Micro Deep Security

@jamiehynds jamiehynds requested a review from a team February 19, 2024 10:44
@kcreddy kcreddy added the enhancement New feature or request label Feb 19, 2024
@mohitjha-elastic
Resolve review comments.
07d01be 
1. Change Trend Micro to Trend Micro Deep Security everywhere.
2. Split the descriptions in new lines.
3. Update the changelog as per Jamie's suggestion.
4. Add some related.* fields in pipeline.
5. Add script to remove CS from threat.tactic.id and threat.technique.id.
kcreddy
kcreddy reviewed Feb 21, 2024
View reviewed changes
...s/trendmicro/data_stream/deep_security/_dev/test/pipeline/test-trendmicro.json-expected.json
},
"related": {
"hosts": [
"4"
Copy link
Contributor

Choose a reason for hiding this comment

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

host.name makes more sense than host.id

Copy link
Contributor

Choose a reason for hiding this comment

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

Rest LGTM 👍🏼

Copy link
Contributor Author

Choose a reason for hiding this comment

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

host.name makes more sense than host.id

host.name in the pipeline test is something that is coming from filebeat, we are not doing mapping of that field in the pipeline. However, I have removed that from the pipeline tests.
Thanks for the catch @kcreddy

@efd6
Copy link
Contributor

efd6 commented Feb 21, 2024

/test

@efd6
Copy link
Contributor

efd6 commented Feb 21, 2024

Either the manifest.yml or the CODEOWNERS needs to be updated.

Error: owner "elastic/security-service-integrations" defined in "packages/trendmicro/manifest.yml" is not in ".github/CODEOWNERS"

@efd6
Copy link
Contributor

efd6 commented Feb 22, 2024

/test

@elasticmachine
Copy link

🚀 Benchmarks report

To see the full report comment with /test benchmark fullreport

efd6
efd6 approved these changes Feb 22, 2024
View reviewed changes
Copy link
Contributor

@efd6 efd6 left a comment

Choose a reason for hiding this comment

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

LGTM, but waiting for @kcreddy and @jamiehynds

kcreddy
kcreddy approved these changes Feb 22, 2024
View reviewed changes
Copy link
Contributor

@kcreddy kcreddy left a comment

Choose a reason for hiding this comment

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

LGTM

@kcreddy
Copy link
Contributor

kcreddy commented Feb 22, 2024

Looks like SonarQube complains about lack of coverage for all ingest pipelines.
Screenshot 2024-02-22 at 1 09 31 PM

@kcreddy
Copy link
Contributor

kcreddy commented Feb 23, 2024

/test

@elasticmachine
Copy link

💚 Build Succeeded

History

@elastic-sonarqube Elastic SonarQube
Copy link

Quality Gate failed Quality Gate failed

Failed conditions

77.3% 77.3% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube

@kcreddy
Copy link
Contributor

kcreddy commented Feb 23, 2024

For Sonarqube, I think we need to add more log samples covering all ingest pipelines to satisfy the coverage.
@efd6 have you come across PRs where SonarQube failing recently? May I know what was proposed?

@efd6
Copy link
Contributor

efd6 commented Feb 23, 2024 •
edited

Yes, possibly a majority, but those have been minor changes rather than large additions. Looking at the uncovered lines, I think this is probably OK, but please take a look and confirm.

jamiehynds
jamiehynds reviewed Feb 23, 2024
View reviewed changes
packages/trendmicro/manifest.yml
owner:
github: elastic/sec-deployment-and-devices
type: community
Copy link

Choose a reason for hiding this comment

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

@mohitjha-elastic can we change this to elastic please, as we'll support the integration from here given the major updates we've done to the package.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jamiehynds This is actually old code.
Its changed in the latest commit to elastic.

@kcreddy
Copy link
Contributor

kcreddy commented Feb 23, 2024

Yes, possibly a majority, but those have been minor changes rather than large additions. Looking at the uncovered lines, I think this is probably OK, but please take a look and confirm

@efd6 Thanks! I was also under same impression. Since merging is blocked, can we skip the SonarQube or force it to pass ? How was that handled in previous case?

@efd6
Copy link
Contributor

efd6 commented Feb 23, 2024

The merge is waiting on elastic/sec-deployment-and-devices approval.

@jamiehynds
Copy link

The merge is waiting on elastic/sec-deployment-and-devices approval.

Would you mind approving @taylor-swanson? Apologies for the mix up on ownership.

taylor-swanson
taylor-swanson approved these changes Feb 23, 2024
View reviewed changes
Copy link
Contributor

@taylor-swanson taylor-swanson left a comment

Choose a reason for hiding this comment

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

LGTM

@efd6 efd6 merged commit 20be9a6 into elastic:main Feb 26, 2024
4 of 5 checks passed
@elasticmachine
Copy link

Package trendmicro - 2.0.0 containing this change is available at https://epr.elastic.co/search?package=trendmicro

1 similar comment
@elasticmachine
Copy link

Package trendmicro - 2.0.0 containing this change is available at https://epr.elastic.co/search?package=trendmicro

gizas pushed a commit that referenced this pull request Mar 13, 2024
@mohitjha-elastic @gizas
[trendmicro] Add support of Deep Security version 20 (#9124)
c5d29bf 
* Add support of Deep Security version 20.
* Enhance the code and ECS mappings.
* Update the minimum kibana version to 8.11.0
* Add support of new dashboard.
* Update codeowner.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kcreddy kcreddy kcreddy approved these changes

@jamiehynds jamiehynds jamiehynds left review comments

@efd6 efd6 efd6 approved these changes

@taylor-swanson taylor-swanson taylor-swanson approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request Integration:TrendMicro TrendMicro
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@mohitjha-elastic @efd6 @elasticmachine @kcreddy @jamiehynds @taylor-swanson