Skip to content

[Netskope Alerts] Add text multi-field to netskope.alerts.breach.description field #13977

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

Merged
merged 9 commits into from
May 26, 2025

Conversation

leandrojmp
Copy link
Contributor

@leandrojmp leandrojmp commented May 23, 2025

Proposed commit message

The value of the field netskope.alerts.breach.date is a date in unix epoch format, this field is incorrectly mapped as a double, but it should be mapped as a date as it represents the date when the breach was identified.

For this to work as a date a new date processor is required and added as well.

The value of the field netskope.alerts.breach.description can also be a lenghty description of the breach, but it is mapped only as a keyword, since sometimes the user may want to search for strings in this field it should be a multi-field with a .text field mapped as a match_only_field

One thing to add is that this is a breaking change, the mapping of the field netskope.alerts.breach.date is being changed from double to date and this will lead to a Kibana Conflict, which makes the field unusable until the conflict is solved.

I think that is very unlikely that this field is being used with the current mapping.

Also, the changelog specs does not allow to use multiple types, so since this is correcting the mappings and also adding a new processor, I'm tagging as an enhancement and bumped the minor version, I don't think this change justify bumping the major version.

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.
  • I have verified that any added dashboard complies with Kibana's Dashboard good practices

Related issues

@leandrojmp leandrojmp requested a review from a team as a code owner May 23, 2025 05:24
@leandrojmp
Copy link
Contributor Author

The test cases were generated running elastic-package test pipeline -g and the README.md file was updated running elastic-package build.

@andrewkroh andrewkroh added Integration:netskope Netskope Team:Security-Service Integrations Security Service Integrations team [elastic/security-service-integrations] labels May 23, 2025
@elasticmachine
Copy link

Pinging @elastic/security-service-integrations (Team:Security-Service Integrations)

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.

The manifest specifies v8.13.0 as the minimum version, so the test expectations should be rendered with that stack version.

@leandrojmp
Copy link
Contributor Author

The manifest specifies v8.13.0 as the minimum version, so the test expectations should be rendered with that stack version.

Just did it, using elastic-package stack up --version 8.13.0, not sure exactly what this changed.

@efd6
Copy link
Contributor

efd6 commented May 25, 2025

The URI parts and user agent processors are a bit different between the two versions.

@efd6
Copy link
Contributor

efd6 commented May 25, 2025

/test

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.

As you note, this is a breaking change. That should be reflected in the changelog version bump. But also, do we want to split this into to parts: one part for the description mapping (which is not breaking and so is just a minor bump); and the other which is arguably a breaking bugfix. This would allow people to pick up the enhancement without any breakage. It would require this being broken into two PRs so that separate versions get published.

@elastic-vault-github-plugin-prod

🚀 Benchmarks report

To see the full report comment with /test benchmark fullreport

@leandrojmp
Copy link
Contributor Author

As you note, this is a breaking change. That should be reflected in the changelog version bump. But also, do we want to split this into to parts: one part for the description mapping (which is not breaking and so is just a minor bump); and the other which is arguably a breaking bugfix. This would allow people to pick up the enhancement without any breakage. It would require this being broken into two PRs so that separate versions get published.

No problem, I can change this PR to just change the mapping for the description field and then create a new one for the mapping of the date field with the new processor.

The change in the mapping on the description field I would consider an enhancement, as the original mapping is not being changed, just a new .text mapping is being added.

But regarding to the versions, should both bump the minor version? I don't think that the breaking change on the date filter for the new PR would be enough to justify bumping this to version 2, there is some discussion on how to deal with these changes here: #13861

@efd6
Copy link
Contributor

efd6 commented May 26, 2025

/test

@leandrojmp leandrojmp changed the title [Netskope Alerts] Fix Mappings for Breach date and Breach description fields and adds parse for Breach date field. [Netskope Alerts] Add text multi-field to netskope.alerts.breach.description field May 26, 2025
@efd6
Copy link
Contributor

efd6 commented May 26, 2025

But regarding to the versions, should both bump the minor version? I don't think that the breaking change on the date filter for the new PR would be enough to justify bumping this to version 2

What would be enough?

@leandrojmp
Copy link
Contributor Author

leandrojmp commented May 26, 2025

What would be enough?

Not exactly sure, something more complex, I guess. The main issue is that bumping major versions on breaking changes can lead to a faster increase of the integration version, which can give the false impression that a lot of things is changing.

If for example we find another 2 breaking next week, this could lead the integration from version 1.X to 4.X in a couple of days.

This is related to what is mentioned on the discussion linked.

Incrementing major versions often for such small but impactful changes can cause disruption in the community and with end users in general.

Copy link

@elasticmachine
Copy link

💚 Build Succeeded

History

@efd6
Copy link
Contributor

efd6 commented May 26, 2025

Not exactly sure, something more complex, I guess.

Yeah, this is the issue. That is a vibe concept, but we need to have a solid construction. That solid construction is provided by semver. This change would break people, so it's a breaking change. Absent a policy that excludes this kind of breakage from the set that we consider to be breaking, I think we need to at least consider it. This is something that we are discussing internally.

The main issue is that bumping major versions on breaking changes can lead to a faster increase of the integration version, which can give the false impression that a lot of things is changing.

I think this is an education issue; major means no more and no less than, "We are breaking our continuation of API/behaviour contract." That people interpret it to mean progress is an issue with their interpretation.

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.

Suggested commit message body

The value of the field netskope.alerts.breach.description can be a
lengthy description of the breach, but it is mapped only as a keyword.
Since sometimes the user may want to search for strings in this field
it should be a multi-field with a .text field mapped as a
match_only_field.

@leandrojmp
Copy link
Contributor Author

@efd6 do I need to change anything else here to have this merged?

Once this is merged I will make the other PR.

@efd6
Copy link
Contributor

efd6 commented May 26, 2025

If you're happy with that commit message, I'll go ahead and merge with it.

@leandrojmp
Copy link
Contributor Author

If you're happy with that commit message, I'll go ahead and merge with it.

That's ok! We can merge it.

@efd6 efd6 merged commit 186f3f9 into elastic:main May 26, 2025
8 checks passed
@elastic-vault-github-plugin-prod

Package netskope - 1.24.0 containing this change is available at https://epr.elastic.co/package/netskope/1.24.0/

v1v added a commit to v1v/integrations that referenced this pull request May 26, 2025
* main: (42 commits)
  [jamf_pro] Fix `flattened` field types for non-object values (elastic#13985)
  [Netskope Alerts] Add text multi-field to netskope.alerts.breach.description field (elastic#13977)
  zscaler_zia: add strict field template mode for tcp and http_endpoint input data streams (elastic#13904)
  apm: Add config for tail-based sampling discard on write (elastic#13950)
  [CI] Add dev/coverage into backport script (elastic#13987)
  Update configuration updatecli for 8.x snapshot (elastic#13981)
  [Prometheus] Add username, password, and SSL related fields for query dataset (elastic#13969)
  o365: Ignore failures in rename processors for organization fields (elastic#13983)
  aws.firewall: Document ingested log types of AWS Network Firewall (elastic#13978)
  mimecast: resolve field data type conflicts between data streams (elastic#13825)
  [Infoblox NIOS] Handle the parsing of IPv6 address (elastic#13947)
  [Cribl] Fix handling of metric event type (elastic#13930)
  zscaler_zpa: fix handling of multiple remote IPs, and event categorisation (elastic#13755)
  Adding agentless deployment to the sublime security integration (elastic#13963)
  [integration/system] add use_performance_counters in system integration (elastic#13150)
  crowdstrike,m365_defender,microsoft_defender_{cloud,endpoint},sentinel_one: normalise severity handling (elastic#13955)
  [forgerock] Map `forgerock.response.elapsedTime` as a long not a date (elastic#13959)
  github: squelch errors from pagination ends (elastic#13965)
  cisco_secure_endpoint: squelch errors from pagination ends (elastic#13964)
  [Cloud Security] Cloud Asset Inventory:  fixed cloud formation URL (elastic#13971)
  ...
v1v added a commit that referenced this pull request May 26, 2025
* feature/use-google-secrets: (43 commits)
  use -ci account
  [jamf_pro] Fix `flattened` field types for non-object values (#13985)
  [Netskope Alerts] Add text multi-field to netskope.alerts.breach.description field (#13977)
  zscaler_zia: add strict field template mode for tcp and http_endpoint input data streams (#13904)
  apm: Add config for tail-based sampling discard on write (#13950)
  [CI] Add dev/coverage into backport script (#13987)
  Update configuration updatecli for 8.x snapshot (#13981)
  [Prometheus] Add username, password, and SSL related fields for query dataset (#13969)
  o365: Ignore failures in rename processors for organization fields (#13983)
  aws.firewall: Document ingested log types of AWS Network Firewall (#13978)
  mimecast: resolve field data type conflicts between data streams (#13825)
  [Infoblox NIOS] Handle the parsing of IPv6 address (#13947)
  [Cribl] Fix handling of metric event type (#13930)
  zscaler_zpa: fix handling of multiple remote IPs, and event categorisation (#13755)
  Adding agentless deployment to the sublime security integration (#13963)
  [integration/system] add use_performance_counters in system integration (#13150)
  crowdstrike,m365_defender,microsoft_defender_{cloud,endpoint},sentinel_one: normalise severity handling (#13955)
  [forgerock] Map `forgerock.response.elapsedTime` as a long not a date (#13959)
  github: squelch errors from pagination ends (#13965)
  cisco_secure_endpoint: squelch errors from pagination ends (#13964)
  ...
@leandrojmp
Copy link
Contributor Author

Hello @efd6

As asked, just created the other PR here #14008

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Integration:netskope Netskope Team:Security-Service Integrations Security Service Integrations team [elastic/security-service-integrations]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants