-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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: allow fine-grain control of API requests #36441
Conversation
Pinging @elastic/security-external-integrations (Team:Security-External Integrations) |
@jamiehynds Please take a look at the intended behaviour documented here. |
if err != nil { | ||
return updatedUsers, updatedDevices, err | ||
} | ||
p.logger.Debugf("Received %d devices from API", len(changedUsers)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
p.logger.Debugf("Received %d devices from API", len(changedUsers)) | |
p.logger.Debugf("Received %d devices from API", len(changedDevices)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. That was incorrect in the previous code. Good catch.
case "", "all", "users", "devices": | ||
default: | ||
return errors.New("dataset must be 'all', 'users', 'devices' or empty") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additional comment: If validation is required , then can we extend the unit test here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That feels like testing that addition works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not super important may be..!
[float] | ||
===== `dataset` | ||
|
||
The datasets to collect from the API. This can be one of "all", "users" or "devices", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@efd6 this approach looks ok to me and we can set the default to 'all'. To be safe we can add a section to the integration docs to outline the three options, and the fact that we'll include registered users/owners as part of the device data collection.
FYI @piyush-elastic
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
…ol of API requests This adds support for specifying which of users/devices to collect from the AzureAD API endpoints in order to reduce network costs for users who do not need a full set of entities. The current change does not change the behaviour of device collection of registered owners and registered users; when the "devices" dataset is selected there user entities will still be collected as they are considered here as an attribute of the device, rather than a component of the users dataset.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This approach LGTM
I think testing is good enough, although an extra unit test case for config validation wouldn't hurt (could just be an invalid value to make sure that branch is counted in coverage).
…ain control of API requests (elastic#36441) This adds support for specifying which of users/devices to collect from the AzureAD API endpoints in order to reduce network costs for users who do not need a full set of entities. The current change does not change the behaviour of device collection of registered owners and registered users; when the "devices" dataset is selected there user entities will still be collected as they are considered here as an attribute of the device, rather than a component of the users dataset.
Proposed commit message
This adds support for specifying which of users/devices to collect from the AzureAD API endpoints in order to reduce network costs for users who do not need a full set of entities.
The current change does not change the behaviour of device collection of registered owners and registered users; when the "devices" dataset is selected there user entities will still be collected as they are considered here as an attribute of the device, rather than a component of the users dataset.
Checklist
CHANGELOG.next.asciidoc
orCHANGELOG-developer.next.asciidoc
.Author's Checklist
How to test this PR locally
Related issues
Use cases
Screenshots
Logs