-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Implement Azure resource fetching in the Discovery service #48843
Conversation
6c2c286
to
7d10b7c
Compare
6e1c97f
to
4c2df44
Compare
7d10b7c
to
61e626b
Compare
dae5182
to
b20ff8b
Compare
9f7e308
to
a3b84e8
Compare
@mvbrock - this PR will require admin approval to merge due to its size. Consider breaking it up into a series smaller changes. |
This PR is quite large. This make reviewing hard, as reviewers we are more likely to miss bugs, and this makes reverting/backporting your PR harder because of the conflicts and potential protobuf breaking changes. Working on a single branch to get the full thing working is positive as this avoids having to backtrack and fix things we forgot on a previous PR. However it might be useful to split the resulting changeset into smaller PRs. You will get a higher quality review and it will take less time to get merged. Teleporters tend to split the PR into at least:
This does not apply to every PR but as a rule of thumb, splitting large PRs into smaller chunks is a best practice and you might want to consider breaking PRs larger than 500 locs. |
a386665
to
fd9cc3e
Compare
4947312
to
21da3f3
Compare
Co-authored-by: Marco Dinis <[email protected]>
// Azure is the Azure configuration for the AccessGraph Sync service. | ||
Azure []AccessGraphAzureSync `yaml:"azure,omitempty"` |
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.
We can leave it as is 👍
I still think we should converge on a single way of setting discovery matchers, and it should use, IMO, the DiscoveryConfig resource.
However, for the sake of consistency, let's have it in both places.
Closing this PR since split PRs have been created and listed below. Copied a few remaining comments here from @marcoandredinis into those PRs as well. |
Part of of https://github.com/gravitational/access-graph/issues/640 to implement Azure resource fetching in the Discovery service, and originating from the Azure integration POC branch master...mvbrock/azure-integration-poc. This PR is dependent on #48628 which specifies the gRPC messages/methods for transmitting Azure resources to the Access Graph. A new Azure fetcher is introduced alongside the AWS fetcher, as well as the addition of Azure role definition/assignment clients in the Cloud library, per https://github.com/gravitational/access-graph/issues/1314.