x-pack/filebeat/input/entityanalytics/provider/activedirectory: don't publish unmodified updates #41179

efd6 · 2024-10-09T02:44:22Z

Proposed commit message

See title.

Checklist

My code follows the style guidelines of this project
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have made corresponding change to the default configuration files
I have added tests that prove my fix is effective or that my feature works
I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Disruptive User Impact

Author's Checklist

[ ]

How to test this PR locally

Related issues

Use cases

Screenshots

Logs

mergify · 2024-10-09T02:44:58Z

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @efd6? 🙏.
For such, you'll need to label your PR with:

The upcoming major version of the Elastic Stack
The upcoming minor version of the Elastic Stack (if you're not pushing a breaking change)

To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

backport-8./d is the label to automatically backport to the 8./d branch. /d is the digit

mergify · 2024-10-09T02:44:58Z

backport-8.x has been added to help with the transition to the new branch 8.x.
If you don't need it please use backport-skip label and remove the backport-8.x label.

elasticmachine · 2024-10-09T05:18:35Z

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

mergify · 2024-10-11T07:15:01Z

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b non-mod-entities upstream/non-mod-entities
git merge upstream/main
git push upstream non-mod-entities

chrisberkhout

Is this to avoid duplicates or to avoid redundant work?

Does this mean that a computer added after the last full sync won't be ingested until the next full sync?

efd6 · 2024-10-14T20:08:22Z

Is this to avoid duplicates or to avoid redundant work?

To avoid redundant work.

Does this mean that a computer added after the last full sync won't be ingested until the next full sync?

No, it should be added with an update. I will reassess.

efd6 · 2024-10-14T21:33:58Z

/test

chrisberkhout

As discussed elsewhere, the way forward here may be to:

do all the additions from incremental changes
do the deletes only on full sync

Redundant additions could be skipped during full sync but I think it's better to do them anyway, as a way to periodically recover from missed updates.

chrisberkhout · 2024-10-15T08:53:38Z

x-pack/filebeat/input/entityanalytics/provider/activedirectory/activedirectory.go

 				p.logger.Errorw("Error running incremental update", "error", err)
 				p.metrics.updateError.Inc()
 			}
 			p.metrics.updateTotal.Inc()
 			p.metrics.updateProcessingTime.Update(time.Since(start).Nanoseconds())
 			updateTimer.Reset(p.cfg.UpdateInterval)
 			p.logger.Debugf("Next update expected at: %v", time.Now().Add(p.cfg.UpdateInterval))
+			last = start


If time was used I think it would be better to get it from state.whenChanged in order to minimize issues from disagreements between client and server time (specifically, when the server doesn't provide all data up to the time of a request).

It's a little more subtle than that, but I think I have got what you want in my next change.

efd6 · 2024-10-16T06:24:25Z

@chrisberkhout PTAL Requires careful review.

x-pack/filebeat/input/entityanalytics/provider/activedirectory/activedirectory.go

chrisberkhout

Activedirectory could use a final tweak of the time value.

Jamf I'm not sure whether the changes are necessary. Also it seems to not handle deletes.

x-pack/filebeat/input/entityanalytics/provider/activedirectory/activedirectory.go

x-pack/filebeat/input/entityanalytics/provider/jamf/jamf.go

chrisberkhout · 2024-10-17T08:53:59Z

x-pack/filebeat/input/entityanalytics/provider/jamf/jamf.go

@@ -350,7 +351,7 @@ func (p *jamfInput) runFullSync(inputCtx v2.Context, store *kvstore.Store, clien
 // runIncrementalUpdate will run an incremental update. The process is similar
 // to full synchronization, except only users which have changed (newly
 // discovered, modified, or deleted) will be published.


only users which have changed (newly discovered, modified, or deleted) will be published

I don't see deletes being handled anywhere.

In doFetchComputers it will always fetch everything and return whatever is new or modified compared to the store (unless its a fullSync, in which case it returns nothing).

I don't see any mention of deletes in the GET /api/preview/computers documentation.

mergify · 2024-10-18T08:23:19Z

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b non-mod-entities upstream/non-mod-entities
git merge upstream/main
git push upstream non-mod-entities

…: don't publish unmodified updates

This leaves a small window when it is possible to send redundant data, but it is simpler and safer than tightening the bounds.

Only mark users as deleted on the basis of full sync since this is the only time that we can know that a user is absent. Also remove users from the store on close if they have been marked as deleted.

chrisberkhout

Looks good 👍

There's a duplicate line in the changelog.

Also the PR description and changelog could say more clearly what the changed behavior is:

I think "don't publish unmodified updates" is only happening in that the timestamp handling is improved. It's not actually skipping publication of things received from the server but already known (because the incremental sync doesn't get them and the full sync should to do a full publish anyway).
The other, more important change is that the old version was deleting anything that didn't appear in the new incremental sync response, and now it will only do the deletions during the full sync and only of things that the server actually doesn't have anymore.

chrisberkhout · 2024-10-22T14:17:37Z

CHANGELOG.next.asciidoc

@@ -164,6 +164,8 @@ https://github.com/elastic/beats/compare/v8.8.1\...main[Check the HEAD diff]
 - Fixed failed job handling and removed false-positive error logs in the GCS input. {pull}41142[41142]
 - Bump github.com/elastic/go-sfdc dependency used by x-pack/filebeat/input/salesforce. {pull}41192[41192]
 - Log bad handshake details when websocket connection fails {pull}41300[41300]
+- Don't send redundant documents for non-modified entities in the Jamf and Active Directory entityanalytics input. {pull}41179[41179]


Suggested change

- Don't send redundant documents for non-modified entities in the Jamf and Active Directory entityanalytics input. {pull}41179[41179]

…ve modification time and deletion logic (#41179) This improves the update time stamps of modified events by using the documents' whenChanged fields in the case of returned documents, and the current time when a document is identified as having been deleted. The latest of these is used to determine the time filter for the next Active Directory query. Documents are marked as deleted only when they are found to not exist in full sync collection, and are removed from the state store when they are identified as deleted. The change in behaviour to not use updates to identify corrects behaviour that would cause older but not deleted entities to be deleted from the index. (cherry picked from commit 6b54074) # Conflicts: # x-pack/filebeat/input/entityanalytics/provider/activedirectory/activedirectory.go

…ve modification time and deletion logic (#41179) This improves the update time stamps of modified events by using the documents' whenChanged fields in the case of returned documents, and the current time when a document is identified as having been deleted. The latest of these is used to determine the time filter for the next Active Directory query. Documents are marked as deleted only when they are found to not exist in full sync collection, and are removed from the state store when they are identified as deleted. The change in behaviour to not use updates to identify corrects behaviour that would cause older but not deleted entities to be deleted from the index. (cherry picked from commit 6b54074)

…/activedirectory: don't publish unmodified updates (#41386) * x-pack/filebeat/input/entityanalytics/provider/activedirectory: improve modification time and deletion logic (#41179) This improves the update time stamps of modified events by using the documents' whenChanged fields in the case of returned documents, and the current time when a document is identified as having been deleted. The latest of these is used to determine the time filter for the next Active Directory query. Documents are marked as deleted only when they are found to not exist in full sync collection, and are removed from the state store when they are identified as deleted. The change in behaviour to not use updates to identify corrects behaviour that would cause older but not deleted entities to be deleted from the index. (cherry picked from commit 6b54074) * remove irrelevant changelog entry --------- Co-authored-by: Dan Kortschak <[email protected]>

…ve modification time and deletion logic (#41179) This improves the update time stamps of modified events by using the documents' whenChanged fields in the case of returned documents, and the current time when a document is identified as having been deleted. The latest of these is used to determine the time filter for the next Active Directory query. Documents are marked as deleted only when they are found to not exist in full sync collection, and are removed from the state store when they are identified as deleted. The change in behaviour to not use updates to identify corrects behaviour that would cause older but not deleted entities to be deleted from the index. (cherry picked from commit 6b54074) # Conflicts: # x-pack/filebeat/input/entityanalytics/provider/activedirectory/activedirectory.go

…r/activedirectory: improve modification time and deletion logic (#41385) * x-pack/filebeat/input/entityanalytics/provider/activedirectory: improve modification time and deletion logic (#41179) This improves the update time stamps of modified events by using the documents' whenChanged fields in the case of returned documents, and the current time when a document is identified as having been deleted. The latest of these is used to determine the time filter for the next Active Directory query. Documents are marked as deleted only when they are found to not exist in full sync collection, and are removed from the state store when they are identified as deleted. The change in behaviour to not use updates to identify corrects behaviour that would cause older but not deleted entities to be deleted from the index. (cherry picked from commit 6b54074) # Conflicts: # x-pack/filebeat/input/entityanalytics/provider/activedirectory/activedirectory.go * fix cherry pick failures --------- Co-authored-by: Dan Kortschak <[email protected]>

efd6 added Filebeat Filebeat bugfix Team:Security-Service Integrations Security Service Integrations Team labels Oct 9, 2024

efd6 self-assigned this Oct 9, 2024

botelastic bot added needs_team Indicates that the issue/PR needs a Team:* label and removed needs_team Indicates that the issue/PR needs a Team:* label labels Oct 9, 2024

mergify bot added the backport-8.x Automated backport to the 8.x branch with mergify label Oct 9, 2024

efd6 force-pushed the non-mod-entities branch from e0c6f45 to 73f01b5 Compare October 9, 2024 02:45

efd6 added the backport-8.15 Automated backport to the 8.15 branch with mergify label Oct 9, 2024

efd6 marked this pull request as ready for review October 9, 2024 05:18

efd6 requested a review from a team as a code owner October 9, 2024 05:18

efd6 force-pushed the non-mod-entities branch from 73f01b5 to e87a06f Compare October 13, 2024 22:24

chrisberkhout reviewed Oct 14, 2024

View reviewed changes

efd6 requested a review from chrisberkhout October 14, 2024 23:17

chrisberkhout requested changes Oct 15, 2024

View reviewed changes

chrisberkhout reviewed Oct 16, 2024

View reviewed changes

x-pack/filebeat/input/entityanalytics/provider/activedirectory/activedirectory.go Outdated Show resolved Hide resolved

chrisberkhout requested changes Oct 17, 2024

View reviewed changes

efd6 force-pushed the non-mod-entities branch 2 times, most recently from ddd9e66 to 9e7674c Compare October 17, 2024 22:40

efd6 changed the title ~~x-pack/filebeat/input/entityanalytics/provider/{jamf,activedirectory}: don't publish unmodified updates~~ x-pack/filebeat/input/entityanalytics/provider/activedirectory: don't publish unmodified updates Oct 17, 2024

efd6 added 2 commits October 18, 2024 18:54

x-pack/filebeat/input/entityanalytics/provider/{jamf,activedirectory}…

2476c89

…: don't publish unmodified updates

revise approach to use modification times to decide publication

686ea7e

This leaves a small window when it is possible to send redundant data, but it is simpler and safer than tightening the bounds.

efd6 added 5 commits October 18, 2024 18:54

use time of modification to determine update time

a4387f3

revise approach to handling deleted users

0a18a90

Only mark users as deleted on the basis of full sync since this is the only time that we can know that a user is absent. Also remove users from the store on close if they have been marked as deleted.

address pr comment

6603e89

address pr comments for activedirectory

67d0a91

back out jamf changes

3709f8a

efd6 force-pushed the non-mod-entities branch from 9e7674c to 3709f8a Compare October 18, 2024 08:25

chrisberkhout approved these changes Oct 22, 2024

View reviewed changes

efd6 added 2 commits October 23, 2024 07:17

remove stale changelog entry

1f17004

improve clarity of changelog

644b3af

efd6 merged commit 6b54074 into elastic:main Oct 22, 2024
22 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

x-pack/filebeat/input/entityanalytics/provider/activedirectory: don't publish unmodified updates #41179

x-pack/filebeat/input/entityanalytics/provider/activedirectory: don't publish unmodified updates #41179

efd6 commented Oct 9, 2024

mergify bot commented Oct 9, 2024

mergify bot commented Oct 9, 2024

elasticmachine commented Oct 9, 2024

mergify bot commented Oct 11, 2024

chrisberkhout left a comment

efd6 commented Oct 14, 2024 •

edited

Loading

efd6 commented Oct 14, 2024

chrisberkhout left a comment

chrisberkhout Oct 15, 2024

efd6 Oct 16, 2024

efd6 commented Oct 16, 2024

chrisberkhout left a comment

chrisberkhout Oct 17, 2024

mergify bot commented Oct 18, 2024

chrisberkhout left a comment

chrisberkhout Oct 22, 2024

x-pack/filebeat/input/entityanalytics/provider/activedirectory: don't publish unmodified updates #41179

x-pack/filebeat/input/entityanalytics/provider/activedirectory: don't publish unmodified updates #41179

Conversation

efd6 commented Oct 9, 2024

Proposed commit message

Checklist

Disruptive User Impact

Author's Checklist

How to test this PR locally

Related issues

Use cases

Screenshots

Logs

mergify bot commented Oct 9, 2024

mergify bot commented Oct 9, 2024

elasticmachine commented Oct 9, 2024

mergify bot commented Oct 11, 2024

chrisberkhout left a comment

Choose a reason for hiding this comment

efd6 commented Oct 14, 2024 • edited Loading

efd6 commented Oct 14, 2024

chrisberkhout left a comment

Choose a reason for hiding this comment

chrisberkhout Oct 15, 2024

Choose a reason for hiding this comment

efd6 Oct 16, 2024

Choose a reason for hiding this comment

efd6 commented Oct 16, 2024

chrisberkhout left a comment

Choose a reason for hiding this comment

chrisberkhout Oct 17, 2024

Choose a reason for hiding this comment

mergify bot commented Oct 18, 2024

chrisberkhout left a comment

Choose a reason for hiding this comment

chrisberkhout Oct 22, 2024

Choose a reason for hiding this comment

efd6 commented Oct 14, 2024 •

edited

Loading