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

Implement Azure resource fetching in the Discovery service #48843

Closed
wants to merge 36 commits into from

Conversation

mvbrock
Copy link
Contributor

@mvbrock mvbrock commented Nov 12, 2024

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.

@mvbrock mvbrock force-pushed the mvbrock/azure-integration-grpc branch 3 times, most recently from 6c2c286 to 7d10b7c Compare November 14, 2024 19:52
@mvbrock mvbrock force-pushed the mvbrock/azure-integration-fetch branch from 6e1c97f to 4c2df44 Compare November 18, 2024 20:35
@mvbrock mvbrock force-pushed the mvbrock/azure-integration-grpc branch from 7d10b7c to 61e626b Compare November 18, 2024 20:38
@mvbrock mvbrock force-pushed the mvbrock/azure-integration-fetch branch 2 times, most recently from dae5182 to b20ff8b Compare November 18, 2024 23:37
Base automatically changed from mvbrock/azure-integration-grpc to master December 2, 2024 18:00
@mvbrock mvbrock force-pushed the mvbrock/azure-integration-fetch branch from 9f7e308 to a3b84e8 Compare December 2, 2024 18:40
@mvbrock mvbrock marked this pull request as ready for review December 4, 2024 22:04
@mvbrock mvbrock requested a review from tigrato December 4, 2024 22:04
@mvbrock mvbrock requested review from rosstimothy and removed request for bernardjkim and hugoShaka December 4, 2024 22:04
@public-teleport-github-review-bot

@mvbrock - this PR will require admin approval to merge due to its size. Consider breaking it up into a series smaller changes.

lib/cloud/azure/roleassignments.go Show resolved Hide resolved
lib/cloud/azure/roleassignments.go Show resolved Hide resolved
lib/cloud/azure/roleassignments.go Show resolved Hide resolved
lib/cloud/azure/roledefinitions.go Show resolved Hide resolved
lib/config/configuration.go Show resolved Hide resolved
lib/srv/discovery/fetchers/azure-sync/azure-sync_test.go Outdated Show resolved Hide resolved
lib/srv/discovery/fetchers/azure-sync/azure-sync_test.go Outdated Show resolved Hide resolved
lib/srv/discovery/fetchers/azure-sync/msggraphclient.go Outdated Show resolved Hide resolved
@hugoShaka
Copy link
Contributor

hugoShaka commented Dec 5, 2024

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:

  • one PR with the protobuf/generated content
  • one PR with the client part
  • one PR with the service/cache collection part
  • one or more PR adding the new logic, services, ... (e.g. the new fetchers in the discovery service)

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.

@mvbrock mvbrock force-pushed the mvbrock/azure-integration-fetch branch 2 times, most recently from a386665 to fd9cc3e Compare December 6, 2024 05:15
api/proto/teleport/legacy/types/types.proto Show resolved Hide resolved
api/proto/teleport/legacy/types/types.proto Outdated Show resolved Hide resolved
lib/srv/discovery/fetchers/azure-sync/virtualmachines.go Outdated Show resolved Hide resolved
lib/srv/discovery/fetchers/azure-sync/virtualmachines.go Outdated Show resolved Hide resolved
lib/srv/discovery/discovery.go Show resolved Hide resolved
lib/srv/discovery/fetchers/azure-sync/reconcile.go Outdated Show resolved Hide resolved
lib/srv/discovery/fetchers/azure-sync/azure-sync.go Outdated Show resolved Hide resolved
lib/srv/discovery/fetchers/azure-sync/msggraphclient.go Outdated Show resolved Hide resolved
@mvbrock mvbrock force-pushed the mvbrock/azure-integration-fetch branch from 4947312 to 21da3f3 Compare December 14, 2024 22:29
lib/srv/discovery/common/reconcile.go Show resolved Hide resolved
lib/srv/discovery/discovery.go Show resolved Hide resolved
lib/srv/discovery/discovery.go Show resolved Hide resolved
Comment on lines +1526 to +1527
// Azure is the Azure configuration for the AccessGraph Sync service.
Azure []AccessGraphAzureSync `yaml:"azure,omitempty"`
Copy link
Contributor

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.

@mvbrock
Copy link
Contributor Author

mvbrock commented Dec 18, 2024

@mvbrock mvbrock closed this Dec 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants