Implement Azure resource fetching in the Discovery service #48843

mvbrock · 2024-11-12T20:20:22Z

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.

public-teleport-github-review-bot · 2024-12-04T22:04:50Z

@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

lib/cloud/azure/roledefinitions.go

lib/config/configuration.go

lib/srv/discovery/fetchers/azure-sync/azure-sync.go

lib/srv/discovery/fetchers/azure-sync/azure-sync_test.go

tigrato · 2024-12-05T11:43:54Z

lib/srv/discovery/fetchers/azure-sync/msggraphclient.go

we already have a entraid graph client in lib/msgraph

I will see if it can be used or slightly modified to work here.

I should probably do this in a followup PR since some significant changes may need to be made to the entraid graph client.

tigrato · 2024-12-05T11:44:56Z

lib/srv/discovery/fetchers/azure-sync/principals.go

+	accessgraphv1alpha "github.com/gravitational/teleport/gen/proto/go/accessgraph/v1alpha"
+)
+
+const groupType = "#microsoft.graph.group"


all this can be replaced with msgraph

hugoShaka · 2024-12-05T22:00:17Z

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.

marcoandredinis · 2024-12-06T10:18:50Z

api/proto/teleport/legacy/types/types.proto

+  // Azure is a configuration for Azure Access Graph service poll service.
+  repeated AccessGraphAzureSync Azure = 3 [(gogoproto.jsontag) = "azure,omitempty"];


Can we please update the PollInterval comment? I assume it will be applied to Azure Sync as well.

marcoandredinis · 2024-12-06T10:21:46Z

api/proto/teleport/legacy/types/types.proto

+  repeated string Regions = 1 [(gogoproto.jsontag) = "regions,omitempty"];
+  string SubscriptionID = 2 [(gogoproto.jsontag) = "subscription_id,omitempty"];
+  string Integration = 3 [(gogoproto.jsontag) = "integration,omitempty"];


Can you please add comments to those?
I assume Integration is required but for the others, can we use the wildcard to indicate all the regions or all the subscriptions?

What about resource groups? Should that also be used to filter out resources?
I imagine someone trying TAG and maybe they only want a couple of ResourceGroups to see how it works, before applying to everything

Added the suggestion for resource group configuration in https://github.com/gravitational/access-graph/issues/1376

marcoandredinis · 2024-12-06T10:24:40Z

lib/srv/discovery/fetchers/azure-sync/virtualmachines.go

+)
+
+func (a *Fetcher) fetchVirtualMachines(ctx context.Context) ([]*accessgraphv1alpha.AzureVirtualMachine, error) {
+	// Fetch the VMs


Suggested change

// Fetch the VMs

💅 I don't think this comment is helpful.

marcoandredinis · 2024-12-06T10:24:54Z

lib/srv/discovery/fetchers/azure-sync/virtualmachines.go

+		return nil, trace.Wrap(err)
+	}
+
+	// Return the VMs as protobuf messages


Suggested change

// Return the VMs as protobuf messages

💅 I don't think this comment is helpful.

marcoandredinis · 2024-12-06T10:26:52Z

lib/srv/discovery/fetchers/azure-sync/virtualmachines.go

+
+func (a *Fetcher) fetchVirtualMachines(ctx context.Context) ([]*accessgraphv1alpha.AzureVirtualMachine, error) {
+	// Fetch the VMs
+	vms, err := a.vmClient.ListVirtualMachines(ctx, "*")


Can we have a variable saying that "*" is used to get all resource groups?

marcoandredinis · 2024-12-06T10:53:30Z

lib/srv/discovery/discovery.go

+	s.muDynamicTAGAWSFetchers.Lock()
+	s.dynamicTAGAWSFetchers[dc.GetName()] = awsSyncMatchers
+	s.muDynamicTAGAWSFetchers.Unlock()


No azure dynamic matchers for TAG Sync?

marcoandredinis · 2024-12-06T10:56:26Z

lib/srv/discovery/fetchers/azure-sync/reconcile.go

+		reconcile(old.VirtualMachines, new.VirtualMachines, azureVmKey, azureVmWrap),
+		reconcile(old.Principals, new.Principals, azurePrincipalsKey, azurePrincipalsWrap),
+		reconcile(old.RoleDefinitions, new.RoleDefinitions, azureRoleDefKey, azureRoleDefWrap),
+		reconcile(old.RoleAssignments, new.RoleAssignments, azureRoleAssignKey, azureRoleAssignWrap),


Just a 💅
If we always order the type of resources alphabetically, it will be easier to spot issues/typos

marcoandredinis · 2024-12-06T10:57:34Z

lib/srv/discovery/fetchers/azure-sync/azure-sync.go

+	featNameVms             = "azure/virtualmachines"
+)
+
+const FetcherConcurrency = 5


Can you please add a godoc here?
Do we have concurrency in fetching resources? Or is it resources and regions?

marcoandredinis · 2024-12-06T10:57:47Z

lib/srv/discovery/fetchers/azure-sync/azure-sync.go

+	}, nil
+}
+
+type Features struct {


Missing godoc

marcoandredinis · 2024-12-06T11:01:40Z

lib/srv/discovery/fetchers/azure-sync/msggraphclient.go

We have lib/msgraph. Can we use that instead?
If something's missing there, can we add it instead of replicating most of the code here?

…ciliation

mvbrock force-pushed the mvbrock/azure-integration-grpc branch 3 times, most recently from 6c2c286 to 7d10b7c Compare November 14, 2024 19:52

mvbrock force-pushed the mvbrock/azure-integration-fetch branch from 6e1c97f to 4c2df44 Compare November 18, 2024 20:35

mvbrock force-pushed the mvbrock/azure-integration-grpc branch from 7d10b7c to 61e626b Compare November 18, 2024 20:38

mvbrock force-pushed the mvbrock/azure-integration-fetch branch 2 times, most recently from dae5182 to b20ff8b Compare November 18, 2024 23:37

mvbrock mentioned this pull request Nov 27, 2024

Adding Azure integration protobuf messages and gRPC methods #48628

Merged

Base automatically changed from mvbrock/azure-integration-grpc to master December 2, 2024 18:00

mvbrock force-pushed the mvbrock/azure-integration-fetch branch from 9f7e308 to a3b84e8 Compare December 2, 2024 18:40

mvbrock marked this pull request as ready for review December 4, 2024 22:04

mvbrock requested a review from tigrato December 4, 2024 22:04

github-actions bot added discovery size/xl labels Dec 4, 2024

github-actions bot requested review from bernardjkim and hugoShaka December 4, 2024 22:04

mvbrock requested review from rosstimothy and removed request for bernardjkim and hugoShaka December 4, 2024 22:04

mvbrock requested a review from marcoandredinis December 4, 2024 22:05

tigrato reviewed Dec 5, 2024

View reviewed changes

mvbrock force-pushed the mvbrock/azure-integration-fetch branch 2 times, most recently from a386665 to fd9cc3e Compare December 6, 2024 05:15

marcoandredinis reviewed Dec 6, 2024

View reviewed changes

mvbrock added 4 commits December 14, 2024 16:25

Adding Azure integration gRPC messages and RPC methods

1883230

Make derive

b113b78

More PR feedback and generating protobuf code

92c1979

Make derive

abaeebb

mvbrock added 29 commits December 14, 2024 16:26

Rebase conflicts

6cf5e40

Did not fully fetch from origin/master when rebasing

081a526

Removing azure config field and keeping poll_interval as-is

c51afd1

Correct from parent branch

3faf1f1

Adding Azure integration gRPC messages and RPC methods

61f3335

More PR feedback and generating protobuf code

34d6b4f

Adding partial implementation

e76cc02

Adding more fetching

577cee6

Adding Azure role definitions and assignments clients

3779998

Creating local consumer interface for Azure clients in the sync package

974617b

Using the common deduplication function, and unit testing azure recon…

2f2c6ef

…ciliation

Adding unit test for azure sync

0c33017

Add proper config parsing

6184486

Initializing Azure watcher and fixing Azure creds

edf5ac9

Post-rebase grpc and derived

0d54f2a

Fixing rebase

e2b2e23

Make derived

8194d1d

Fix imports

bbfc3b8

Fix grpc/derived after rebase

8a47506

Make grpc

e1b7ab2

Fix error handling for bidi client

bf049cc

Go mod tidy on integrations

0eb22d2

Fixing race condition in error collection

e1d6ecf

Fixing json mistake in grpc

fad9a52

Linting

f327038

Adding license

c900515

Godoc

dde6053

Removing UMI client ID

3c7e908

PR feedback

21da3f3

mvbrock force-pushed the mvbrock/azure-integration-fetch branch from 4947312 to 21da3f3 Compare December 14, 2024 22:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Azure resource fetching in the Discovery service #48843

Implement Azure resource fetching in the Discovery service #48843

mvbrock commented Nov 12, 2024

public-teleport-github-review-bot bot commented Dec 4, 2024

tigrato Dec 5, 2024

mvbrock Dec 5, 2024

mvbrock Dec 5, 2024

tigrato Dec 5, 2024

hugoShaka commented Dec 5, 2024 •

edited

Loading

marcoandredinis Dec 6, 2024

marcoandredinis Dec 6, 2024

marcoandredinis Dec 6, 2024

mvbrock Dec 6, 2024

marcoandredinis Dec 6, 2024

marcoandredinis Dec 6, 2024

marcoandredinis Dec 6, 2024

marcoandredinis Dec 6, 2024

marcoandredinis Dec 6, 2024

marcoandredinis Dec 6, 2024

marcoandredinis Dec 6, 2024

marcoandredinis Dec 6, 2024

		// Azure is a configuration for Azure Access Graph service poll service.
		repeated AccessGraphAzureSync Azure = 3 [(gogoproto.jsontag) = "azure,omitempty"];

Implement Azure resource fetching in the Discovery service #48843

Are you sure you want to change the base?

Implement Azure resource fetching in the Discovery service #48843

Conversation

mvbrock commented Nov 12, 2024

public-teleport-github-review-bot bot commented Dec 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hugoShaka commented Dec 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hugoShaka commented Dec 5, 2024 •

edited

Loading