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/activedirectory: new package #37919

Merged
merged 3 commits into from
Mar 22, 2024

Conversation

efd6
Copy link
Contributor

@efd6 efd6 commented Feb 8, 2024

Proposed commit message

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.

Author's Checklist

  • Testing requires setting up an active directory test server (or pointing the test at an existing server). There are notes in the test file explaining how pointing the tests at the server is handled. I used instructions here for setting up a local VM test server.

How to test this PR locally

Related issues

Use cases

Screenshots

Logs

@efd6 efd6 added enhancement Filebeat Filebeat backport-skip Skip notification from the automated backport with mergify Team:Security-Service Integrations Security Service Integrations Team 8.13-candidate labels Feb 8, 2024
@efd6 efd6 self-assigned this Feb 8, 2024
@botelastic 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 Feb 8, 2024
@elasticmachine
Copy link
Collaborator

elasticmachine commented Feb 8, 2024

💚 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: 2024-03-21T21:04:26.401+0000

  • Duration: 139 min 0 sec

Test stats 🧪

Test Results
Failed 0
Passed 3352
Skipped 185
Total 3537

💚 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 i8559-activedirectory-fe branch 2 times, most recently from 56019f0 to a376c7d Compare February 12, 2024 20:13
@efd6 efd6 marked this pull request as ready for review February 12, 2024 20:14
@efd6 efd6 requested a review from a team as a code owner February 12, 2024 20:14
@elasticmachine
Copy link
Collaborator

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

Copy link
Contributor

mergify bot commented Feb 12, 2024

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 i8559-activedirectory-fe upstream/i8559-activedirectory-fe
git merge upstream/main
git push upstream i8559-activedirectory-fe

Copy link
Contributor

mergify bot commented Feb 19, 2024

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 i8559-activedirectory-fe upstream/i8559-activedirectory-fe
git merge upstream/main
git push upstream i8559-activedirectory-fe

Copy link
Contributor

mergify bot commented Feb 21, 2024

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 i8559-activedirectory-fe upstream/i8559-activedirectory-fe
git merge upstream/main
git push upstream i8559-activedirectory-fe

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.

Change LGTM 👍🏼 Just added few nits/clarifications.

[float]
===== `ad_user`

The client user name. Used for authentication. Field is required.
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this user need any additional permissions to fetch other users? Should they be documented?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will add.


// 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.
Copy link
Contributor

Choose a reason for hiding this comment

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

How are user deletes handled? Unlike okta, this doesn't seem to have a Deleted/DEPROVISIONED status.
So, until the next full sync happens, there could be stale(deleted) users in the state?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AD doesn't have a notion of deleted. I was thinking about this and wasn't sure what we should do. At the very least, "deleted" should be removed from the docs, but I'll take a look at possible approaches to see if we can reconstruct this functionality.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added a method for handling deletion. It's not a complete approach, but I think it is the best we can get.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks Dan. That makes sense

@efd6 efd6 requested a review from kcreddy March 21, 2024 21:04
@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

cc @efd6

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

cc @efd6

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

cc @efd6

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

cc @efd6

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

cc @efd6

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

cc @efd6

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

cc @efd6

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

cc @efd6

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

cc @efd6

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

cc @efd6

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 👍🏼

@efd6 efd6 merged commit 8bf2581 into elastic:main Mar 22, 2024
43 of 46 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.13-candidate backport-skip Skip notification from the automated backport with mergify enhancement Filebeat Filebeat Team:Security-Service Integrations Security Service Integrations Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants