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

x-pack/filebeat/input/entityanalytics/provider/azuread: add registered owner/user handling #36092

Merged
merged 1 commit into from
Jul 24, 2023

Conversation

efd6
Copy link
Contributor

@efd6 efd6 commented Jul 18, 2023

What does this PR do?

Add support for including registered owner and registered user data to device documents.

Why is it important?

Part of the entity analytics roadmap.

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 — needs additional tests
  • I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Author's Checklist

  • [ ]

How to test this PR locally

Related issues

Use cases

Screenshots

Logs

@efd6 efd6 added enhancement Filebeat Filebeat Team:Security-External Integrations backport-skip Skip notification from the automated backport with mergify labels Jul 18, 2023
@efd6 efd6 self-assigned this Jul 18, 2023
@efd6 efd6 force-pushed the st5940-azure-device-people branch from 4d7445b to ebb32fa Compare July 18, 2023 01:04
@efd6 efd6 changed the title x-pack/filebeat/input/entityanalytics/azuread: add registered owner/user handling x-pack/filebeat/input/entityanalytics/provider/azuread: add registered owner/user handling Jul 18, 2023
@efd6 efd6 force-pushed the st5940-azure-device-people branch from ebb32fa to 59d7674 Compare July 18, 2023 01:09
@elasticmachine
Copy link
Collaborator

elasticmachine commented Jul 18, 2023

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2023-07-21T08:20:32.655+0000

  • Duration: 74 min 28 sec

Test stats 🧪

Test Results
Failed 0
Passed 3056
Skipped 178
Total 3234

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /package : Generate the packages and run the E2E tests.

  • /beats-tester : Run the installation tests with beats-tester.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@efd6 efd6 force-pushed the st5940-azure-device-people branch 2 times, most recently from 57d9c9c to f8a4dd2 Compare July 19, 2023 02:31
@efd6 efd6 marked this pull request as ready for review July 19, 2023 03:04
@efd6 efd6 requested a review from a team as a code owner July 19, 2023 03:04
@elasticmachine
Copy link
Collaborator

Pinging @elastic/security-external-integrations (Team:Security-External Integrations)

@efd6 efd6 requested a review from taylor-swanson July 19, 2023 03:04
@efd6
Copy link
Contributor Author

efd6 commented Jul 19, 2023

run elasticsearch-ci/docs

@mergify
Copy link
Contributor

mergify bot commented Jul 20, 2023

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 st5940-azure-device-people upstream/st5940-azure-device-people
git merge upstream/main
git push upstream st5940-azure-device-people

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.

Changes LGTM, but I see there are still some WIP/TODO items in the description. Did you have anything additional needing to be added (docs perhaps)?

@efd6
Copy link
Contributor Author

efd6 commented Jul 21, 2023

Neglected to remove from the description. This is ready.

@efd6 efd6 force-pushed the st5940-azure-device-people branch from f8a4dd2 to d24f281 Compare July 21, 2023 08:20
@efd6 efd6 merged commit b5a811e into elastic:main Jul 24, 2023
@jamiehynds
Copy link

@efd6 we are currently looking at splitting the data streams between user and device data within AzureAD. Can you confirm if a user is only ingesting device data, will they also get the registered owner and registered user data, or is that coming from the user data?

@efd6
Copy link
Contributor Author

efd6 commented Aug 9, 2023

@jamiehynds The provider exposes the entire data set as an atomic entity, so yes, if the integration is only asking for device data, the associated user data will still be present.

Splitting it means that there will be duplicated storage of the state and duplicated work, so it might bu worth considering having something in the provider logic to deduplicate and points, i.e. if two requests are going to the same endpoint, one for users and one for devices, then only one kv store is created that is shared between the two. I'm not entirely sure how we would achieve this off the top of my head.

@taylor-swanson
Copy link
Contributor

I replied in a separate email, but another thought I had was adding toggles for enabling or disabling collection of user and device data. That way, if someone only wanted to collect user data, but not device data, they would turn off device collection. There would still only be one data stream. This assumes, of course, that device data isn't dependent on user data. I suppose if that were the case, then we could make user data mandatory and device data optional.

@efd6
Copy link
Contributor Author

efd6 commented Aug 10, 2023

This assumes, of course, that device data isn't dependent on user data. I suppose if that were the case, then we could make user data mandatory and device data optional.

It's entirely possible for us to dynamically determine which endpoints must be hit to satisfy the users' requests. The question that does arise though is how fine grained do we want to make this. I can see a case where a user wants only device, and no user information (even user information associated with devices), so we could conceivably have

  • all
  • user only
  • device only (but enriched with associated users)
  • device only (no user data)

How fine do we want this?

@jamiehynds
Copy link

@efd6 those 4 options seem to cover all/most scenarios without having to make it overly complex. We could set the default to 'all' and let users adjust from there if needed (I'd imagine the vast majority will want both users and devices).

If we go with this approach, are changes required on the provider side, or does it fall towards the integration itself?

cc @piyush-elastic

@efd6
Copy link
Contributor Author

efd6 commented Aug 13, 2023

Changes will be required on the provider side to allow this.

@jamiehynds
Copy link

@efd6 with FF tomorrow, how would the provider side changes impact the integration development? If these changes were part of 8.11, could Crest use an 8.11 snapshot to develop the integration early on in the release cycle?

@efd6
Copy link
Contributor Author

efd6 commented Aug 14, 2023

We can configure this however we want. If we default to "all", then addition of the configuration would be a non-breaking enhancement since the non-configured case would be what we are already doing. So, yes, they can build against the code that exists with a view to finer control in the future.

@jamiehynds
Copy link

Sounds good, thanks @efd6. I'll get Crest to start on the integration side using the "all" option and create an issue to focus on the granular controls so we can track separately.

Scholar-Li pushed a commit to Scholar-Li/beats that referenced this pull request Feb 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.10-candidate backport-skip Skip notification from the automated backport with mergify enhancement Filebeat Filebeat
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants