Initial support for Microsoft Graph with opt-in beta #373

manicminer · 2020-12-14T02:47:17Z

This PR introduces support for Microsoft Graph using https://github.com/manicminer/hamilton, alongside the existing Azure Active Directory Graph API implementation, and aims to maintain forward and backward compatibility. Users can opt-in to using MS Graph by setting the use_microsoft_graph provider attribute or the AAD_USE_MICROSOFT_GRAPH environment variable.

This is broken into multiple commits to aid review.

Authentication

Authentication for MS Graph is handled by hamilton/auth similarly to go-azure-helpers. Different auth methods can be configured and are tried in order, until one succeeds.
- Service principal auth is always tried first, if configured
- If managed identity is enabled, it will raise an error on failure (current behavior)
- Azure CLI is the last attempted method and serves as a fallback
Authentication via hamilton/auth is only performed when MS Graph is enabled, to avoid erroring if no API permissions are granted to the principal.
Authentication via go-azure-helpers is always performed, even when MS Graph is enabled. Both methods must work for provider configuration to succeed. This will be removed in v2.0.
Client configuration is sourced from the AuthConfig supplied by go-azure-helpers. This will be switched in v2.0.
All national clouds are supported
The US Gov cloud is now two clouds in MS Graph - L4 and L5.

Resources

All resource CRUD functions have been replaced with temporary shim functions to conditionally call the corresponding aadgraph or msgraph function.
A number of resource.StateChangeConf{}.WaitForState() method usages have been removed since MS Graph has been observed to be consistent in most cases.
Lots of deprecations and new fields to accommodate new API schema and nomenclature

Documentation

Several documentation fixes, deprecations added
Upgrade guide for v2.0 and Microsoft Graph, with section explaining the MS Graph Beta

tombuildsstuff

a few minor comments but this otherwise LGTM 👍

docs/resources/application.md

tombuildsstuff · 2021-05-17T09:22:43Z

docs/resources/application_oauth2_permission_scope.md

+## Attributes Reference
+
+In addition to all arguments above, the following attributes are exported:
+
+*No additional attributes are exported*


The ID will be?

I have tried to avoid mentioning the resource ID too much as it's a frequent source of confusion - instead there are contextual attributes like scope_id in this case.

internal/services/applications/application_password_resource.go

internal/tf/import.go

tombuildsstuff · 2021-05-17T10:31:11Z

vendor/github.com/manicminer/hamilton/msgraph/groups.go

+	var status int
+	for _, owner := range *group.Owners {
+		// don't fail if an owner already exists
+		checkOwnerAlreadyExists := func(resp *http.Response, o *odata.OData) bool {


wouldn't this indicate a bug?

This mostly helps to work around API inconsistency where newly created groups (and other objects) sometimes inherit an owner but sometimes don't. Also the group read operation has inconsistencies where owners sometimes take awhile to show up after adding, this helps work around that without excessive/fruitless retry logic in the calling app.

tombuildsstuff · 2021-05-17T10:31:29Z

vendor/github.com/manicminer/hamilton/msgraph/groups.go

+		}
+
+		// despite the above check, sometimes owners are just gone
+		checkOwnerGone := func(resp *http.Response, o *odata.OData) bool {


(as above) wouldn't this indicate a bug?

Similarly sometimes the read operation on a group indicates the presence of an owner when it's actually already been removed, so you end up with a GET showing an owner, but DELETEing that owner gets you a 403.

tombuildsstuff · 2021-05-17T10:32:54Z

vendor/github.com/manicminer/hamilton/odata/odata.go

+	if err := json.Unmarshal(data, &e); err != nil {
+		return err
+	}
+	for _, k := range []string{"error", "odata.error"} {


can this also be in @odata.error?

I haven't seen that but maybe? AFAICT @odata.error isn't in the odata standard, although I think i'd probably expect it to have astring value if it showed up anywhere.

…ice_principal_password For both resources: - Since `value` is to be removed, generate a value for AAD Graph if one is not specified in configuration, mimicking characteristics of msgraph-generated passwords - As `end_date` / `end_date_relative` are also being removed, default `end_date_relative` to 17520h (2 years) which mimics MS Graph

…rmission_scope

manicminer · 2021-05-19T20:06:22Z

Test Results

Applications - AAD Graph

Applications - MS Graph

Domains - AAD Graph

Domains - MS Graph

Groups - AAD Graph

Groups - MS Graph

Service Principals - AAD Graph

Service Principals - MS Graph

Users - AAD Graph

Users - MS Graph

ken5scal · 2021-05-20T02:20:15Z

Great work!

simongottschlag · 2021-05-20T05:18:07Z

😁🎉🚀💐🥇⭐️🙌🥳

ghost · 2021-05-20T13:30:31Z

This has been released in version 1.5.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azuread" {
    version = "~> 1.5.0"
}
# ... other configuration ...

JonZeolla · 2021-05-21T02:20:07Z

Awesome work!

github-actions · 2021-06-28T02:09:17Z

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

ghost added size/XXL dependencies labels Dec 14, 2020

manicminer added api/microsoft-graph enhancement provider labels Dec 14, 2020

manicminer added this to the v1.2.0 milestone Dec 14, 2020

manicminer force-pushed the feature/enable-microsoft-graph branch 4 times, most recently from 66a9a37 to 33946d2 Compare December 14, 2020 11:42

manicminer marked this pull request as ready for review December 15, 2020 14:59

manicminer requested a review from a team December 15, 2020 14:59

manicminer force-pushed the feature/enable-microsoft-graph branch from 0d4a908 to e06f028 Compare December 15, 2020 15:33

manicminer force-pushed the feature/resource-packages branch from 7e061a3 to 3a9c0a4 Compare December 16, 2020 15:11

manicminer force-pushed the feature/enable-microsoft-graph branch from 34c6ac0 to 14b1fa8 Compare December 16, 2020 15:14

manicminer force-pushed the feature/resource-packages branch 2 times, most recently from d690b7a to 9225d08 Compare December 16, 2020 17:45

manicminer force-pushed the feature/enable-microsoft-graph branch 3 times, most recently from c505e72 to 6f7c4d8 Compare December 16, 2020 17:50

manicminer force-pushed the feature/resource-packages branch from 9225d08 to 5dc0f38 Compare January 11, 2021 18:05

manicminer force-pushed the feature/enable-microsoft-graph branch from 6f7c4d8 to 6ea7423 Compare January 11, 2021 18:05

manicminer force-pushed the feature/resource-packages branch 2 times, most recently from 439ac7f to 8e0e9c2 Compare January 13, 2021 23:23

Base automatically changed from feature/resource-packages to main January 14, 2021 19:32

manicminer modified the milestones: v1.2.0, v1.3.0 Jan 14, 2021

hashicorp deleted a comment Jan 14, 2021

manicminer force-pushed the feature/enable-microsoft-graph branch 2 times, most recently from f228887 to 1ab05ac Compare January 16, 2021 00:25

manicminer added 3 commits May 12, 2021 00:01

Merge branch 'main' into feature/enable-microsoft-graph

f680f3f

Proper deprecation for azuread_application_oauth2_permission resource

c7fa48e

Migration guide fixups

5cdc32a

tombuildsstuff approved these changes May 17, 2021

View reviewed changes

manicminer added 10 commits May 17, 2021 12:51

Absolute URL probably betetr for linking between docs sections

2cc48e0

Authentication and environment info in migration guide

c6ce6c1

Changelog for #373

0681240

Update Hamilton to v0.13.0

e56ebf2

Upgrade Plugin SDK for Terraform 0.15 support

02a6a58

Application role/scope values can be empty

44e470f

AAD Graph test fixes

d08ab44

API-specific default for application / service principal password expiry

1015701

Import check fixes for application_app_role and application_oauth2_pe…

4cd4d74

…rmission_scope

manicminer force-pushed the feature/enable-microsoft-graph branch from c70a8bc to ebaf2ab Compare May 19, 2021 21:24

Fix dependencies and broken unit test, add missing validation

b2de421

manicminer force-pushed the feature/enable-microsoft-graph branch from ebaf2ab to b2de421 Compare May 19, 2021 21:42

manicminer added 2 commits May 19, 2021 23:10

Update dependencies

141b076

Fix text mangling for vendored files

1aea423

manicminer merged commit e9f389f into main May 19, 2021

manicminer deleted the feature/enable-microsoft-graph branch May 19, 2021 23:23

manicminer restored the feature/enable-microsoft-graph branch May 20, 2021 01:03

manicminer deleted the feature/enable-microsoft-graph branch May 20, 2021 01:04

github-actions bot locked as resolved and limited conversation to collaborators Jun 28, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial support for Microsoft Graph with opt-in beta #373

Initial support for Microsoft Graph with opt-in beta #373

manicminer commented Dec 14, 2020 •

edited

Loading

tombuildsstuff left a comment

tombuildsstuff May 17, 2021

manicminer May 17, 2021

tombuildsstuff May 17, 2021

manicminer May 17, 2021 •

edited

Loading

tombuildsstuff May 17, 2021

manicminer May 17, 2021

tombuildsstuff May 17, 2021

manicminer May 17, 2021

manicminer commented May 19, 2021

ken5scal commented May 20, 2021

simongottschlag commented May 20, 2021

ghost commented May 20, 2021

JonZeolla commented May 21, 2021

github-actions bot commented Jun 28, 2021

Initial support for Microsoft Graph with opt-in beta #373

Initial support for Microsoft Graph with opt-in beta #373

Conversation

manicminer commented Dec 14, 2020 • edited Loading

Authentication

Resources

Documentation

tombuildsstuff left a comment

Choose a reason for hiding this comment

tombuildsstuff May 17, 2021

Choose a reason for hiding this comment

manicminer May 17, 2021

Choose a reason for hiding this comment

tombuildsstuff May 17, 2021

Choose a reason for hiding this comment

manicminer May 17, 2021 • edited Loading

Choose a reason for hiding this comment

tombuildsstuff May 17, 2021

Choose a reason for hiding this comment

manicminer May 17, 2021

Choose a reason for hiding this comment

tombuildsstuff May 17, 2021

Choose a reason for hiding this comment

manicminer May 17, 2021

Choose a reason for hiding this comment

manicminer commented May 19, 2021

Test Results

Applications - AAD Graph

Applications - MS Graph

Domains - AAD Graph

Domains - MS Graph

Groups - AAD Graph

Groups - MS Graph

Service Principals - AAD Graph

Service Principals - MS Graph

Users - AAD Graph

Users - MS Graph

ken5scal commented May 20, 2021

simongottschlag commented May 20, 2021

ghost commented May 20, 2021

JonZeolla commented May 21, 2021

github-actions bot commented Jun 28, 2021

manicminer commented Dec 14, 2020 •

edited

Loading

manicminer May 17, 2021 •

edited

Loading