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

Bugfixes for consistency issues #1492

Merged
merged 10 commits into from
Sep 26, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -8,15 +8,19 @@ import (
"errors"
"fmt"
"log"
"net/http"
"strings"
"time"

"github.com/hashicorp/go-azure-helpers/lang/pointer"
"github.com/hashicorp/go-azure-helpers/lang/response"
administrativeunitBeta "github.com/hashicorp/go-azure-sdk/microsoft-graph/administrativeunits/beta/administrativeunit"
"github.com/hashicorp/go-azure-sdk/microsoft-graph/common-types/beta"
"github.com/hashicorp/go-azure-sdk/microsoft-graph/common-types/stable"
"github.com/hashicorp/go-azure-sdk/microsoft-graph/directory/stable/administrativeunit"
"github.com/hashicorp/go-azure-sdk/microsoft-graph/directory/stable/administrativeunitmember"
"github.com/hashicorp/go-azure-sdk/sdk/nullable"
"github.com/hashicorp/go-azure-sdk/sdk/odata"
"github.com/hashicorp/go-uuid"
"github.com/hashicorp/terraform-provider-azuread/internal/clients"
"github.com/hashicorp/terraform-provider-azuread/internal/helpers/consistency"
Expand Down Expand Up @@ -188,23 +192,31 @@ func administrativeUnitResourceCreate(ctx context.Context, d *pluginsdk.Resource
id := stable.NewDirectoryAdministrativeUnitID(*administrativeUnit.Id)
updateResp, err := client.UpdateAdministrativeUnit(ctx, id, stable.AdministrativeUnit{
DisplayName: nullable.Value(tempDisplayName),
}, administrativeunit.DefaultUpdateAdministrativeUnitOperationOptions())
}, administrativeunit.UpdateAdministrativeUnitOperationOptions{
RetryFunc: func(resp *http.Response, o *odata.OData) (bool, error) {
return response.WasNotFound(resp), nil
},
})
if err != nil {
if response.WasNotFound(updateResp.HttpResponse) {
return tf.ErrorDiagF(err, "Timed out whilst waiting for new administrative unit to be replicated in Azure AD")
return tf.ErrorDiagF(err, "Timed out whilst waiting for new %s to be replicated in Azure AD", id)
}
return tf.ErrorDiagF(err, "Failed to patch administrative unit after creating")
return tf.ErrorDiagF(err, "Failed to patch %s after creating", id)
}

// Set correct original display name
updateResp, err = client.UpdateAdministrativeUnit(ctx, id, stable.AdministrativeUnit{
DisplayName: nullable.Value(displayName),
}, administrativeunit.DefaultUpdateAdministrativeUnitOperationOptions())
}, administrativeunit.UpdateAdministrativeUnitOperationOptions{
RetryFunc: func(resp *http.Response, o *odata.OData) (bool, error) {
return response.WasNotFound(resp), nil
},
})
if err != nil {
if response.WasNotFound(updateResp.HttpResponse) {
return tf.ErrorDiagF(err, "Timed out whilst waiting for new administrative unit to be replicated in Azure AD")
return tf.ErrorDiagF(err, "Timed out whilst waiting for new %s to be replicated in Azure AD", id)
}
return tf.ErrorDiagF(err, "Failed to patch administrative unit after creating")
return tf.ErrorDiagF(err, "Failed to patch %s after creating", id)
}

// Add members after the administrative unit is created
Expand All @@ -217,7 +229,7 @@ func administrativeUnitResourceCreate(ctx context.Context, d *pluginsdk.Resource
}

if _, err = memberClient.AddAdministrativeUnitMemberRef(ctx, id, addMemberProperties, administrativeunitmember.DefaultAddAdministrativeUnitMemberRefOperationOptions()); err != nil {
return tf.ErrorDiagF(err, "Could not add member %q to administrative unit with object ID: %q", memberId, d.Id())
return tf.ErrorDiagF(err, "Could not add member %q to %s", memberId, id)
}
}
}
Expand Down Expand Up @@ -266,13 +278,13 @@ func administrativeUnitResourceUpdate(ctx context.Context, d *pluginsdk.Resource
}

if _, err := client.UpdateAdministrativeUnit(ctx, id, administrativeUnit, administrativeunit.DefaultUpdateAdministrativeUnitOperationOptions()); err != nil {
return tf.ErrorDiagF(err, "Updating administrative unit with ID: %q", d.Id())
return tf.ErrorDiagF(err, "Updating %s", id)
}

if d.HasChange("members") {
membersResp, err := memberClient.ListAdministrativeUnitMembers(ctx, id, administrativeunitmember.DefaultListAdministrativeUnitMembersOperationOptions())
if err != nil {
return tf.ErrorDiagF(err, "Could not retrieve members for administrative unit with object ID: %q", d.Id())
return tf.ErrorDiagF(err, "Could not retrieve members for %s", id)
}

existingMembers := make([]string, 0)
Expand All @@ -285,7 +297,7 @@ func administrativeUnitResourceUpdate(ctx context.Context, d *pluginsdk.Resource

for _, memberForRemoval := range membersForRemoval {
if _, err = memberClient.RemoveAdministrativeUnitMemberRef(ctx, stable.NewDirectoryAdministrativeUnitIdMemberID(administrativeUnitId, memberForRemoval), administrativeunitmember.DefaultRemoveAdministrativeUnitMemberRefOperationOptions()); err != nil {
return tf.ErrorDiagF(err, "Could not remove members from administrative unit with object ID: %q", d.Id())
return tf.ErrorDiagF(err, "Could not remove members from %s", id)
}
}

Expand All @@ -297,7 +309,7 @@ func administrativeUnitResourceUpdate(ctx context.Context, d *pluginsdk.Resource
}

if _, err = memberClient.AddAdministrativeUnitMemberRef(ctx, id, addMemberProperties, administrativeunitmember.DefaultAddAdministrativeUnitMemberRefOperationOptions()); err != nil {
return tf.ErrorDiagF(err, "Could not add member %q to administrative unit with object ID: %q", memberId, d.Id())
return tf.ErrorDiagF(err, "Could not add member %q to %s", memberId, id)
}
}
}
Expand All @@ -313,11 +325,11 @@ func administrativeUnitResourceRead(ctx context.Context, d *pluginsdk.ResourceDa
resp, err := client.GetAdministrativeUnit(ctx, id, administrativeunit.DefaultGetAdministrativeUnitOperationOptions())
if err != nil {
if response.WasNotFound(resp.HttpResponse) {
log.Printf("[DEBUG] Administrative Unit with ID %q was not found - removing from state", d.Id())
log.Printf("[DEBUG] %s was not found - removing from state", id)
d.SetId("")
return nil
}
return tf.ErrorDiagF(err, "Retrieving administrative unit with object ID: %q", d.Id())
return tf.ErrorDiagF(err, "Retrieving %s", id)
}

administrativeUnit := resp.Model
Expand All @@ -330,7 +342,7 @@ func administrativeUnitResourceRead(ctx context.Context, d *pluginsdk.ResourceDa

membersResp, err := memberClient.ListAdministrativeUnitMembers(ctx, id, administrativeunitmember.DefaultListAdministrativeUnitMembersOperationOptions())
if err != nil {
return tf.ErrorDiagF(err, "Could not retrieve members for administrative unit with object ID: %q", d.Id())
return tf.ErrorDiagF(err, "Could not retrieve members for %s", id)
}

members := make([]string, 0)
Expand All @@ -350,23 +362,24 @@ func administrativeUnitResourceRead(ctx context.Context, d *pluginsdk.ResourceDa

func administrativeUnitResourceDelete(ctx context.Context, d *pluginsdk.ResourceData, meta interface{}) pluginsdk.Diagnostics {
client := meta.(*clients.Client).AdministrativeUnits.AdministrativeUnitClient
id := stable.NewDirectoryAdministrativeUnitID(d.Id())
clientBeta := meta.(*clients.Client).AdministrativeUnits.AdministrativeUnitClientBeta
id := beta.NewAdministrativeUnitID(d.Id())

if _, err := client.DeleteAdministrativeUnit(ctx, id, administrativeunit.DefaultDeleteAdministrativeUnitOperationOptions()); err != nil {
return tf.ErrorDiagF(err, "Deleting administrative unit with object ID: %q", id.AdministrativeUnitId)
if _, err := clientBeta.DeleteAdministrativeUnit(ctx, id, administrativeunitBeta.DefaultDeleteAdministrativeUnitOperationOptions()); err != nil {
return tf.ErrorDiagF(err, "Deleting %s", id)
}

// Wait for administrative unit object to be deleted
if err := consistency.WaitForDeletion(ctx, func(ctx context.Context) (*bool, error) {
if resp, err := client.GetAdministrativeUnit(ctx, id, administrativeunit.DefaultGetAdministrativeUnitOperationOptions()); err != nil {
if resp, err := client.GetAdministrativeUnit(ctx, stable.NewDirectoryAdministrativeUnitID(id.AdministrativeUnitId), administrativeunit.DefaultGetAdministrativeUnitOperationOptions()); err != nil {
if response.WasNotFound(resp.HttpResponse) {
return pointer.To(false), nil
}
return nil, err
}
return pointer.To(true), nil
}); err != nil {
return tf.ErrorDiagF(err, "Waiting for deletion of administrative unit with object ID %q", id.AdministrativeUnitId)
return tf.ErrorDiagF(err, "Waiting for deletion of %s", id)
}

return nil
Expand Down
10 changes: 10 additions & 0 deletions internal/services/administrativeunits/client/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
package client

import (
administrativeunitBeta "github.com/hashicorp/go-azure-sdk/microsoft-graph/administrativeunits/beta/administrativeunit"
"github.com/hashicorp/go-azure-sdk/microsoft-graph/directory/stable/administrativeunit"
"github.com/hashicorp/go-azure-sdk/microsoft-graph/directory/stable/administrativeunitmember"
"github.com/hashicorp/go-azure-sdk/microsoft-graph/directory/stable/administrativeunitscopedrolemember"
Expand All @@ -12,6 +13,7 @@ import (

type Client struct {
AdministrativeUnitClient *administrativeunit.AdministrativeUnitClient
AdministrativeUnitClientBeta *administrativeunitBeta.AdministrativeUnitClient
AdministrativeUnitMemberClient *administrativeunitmember.AdministrativeUnitMemberClient
AdministrativeUnitScopedRoleMemberClient *administrativeunitscopedrolemember.AdministrativeUnitScopedRoleMemberClient
}
Expand All @@ -23,6 +25,13 @@ func NewClient(o *common.ClientOptions) (*Client, error) {
}
o.Configure(administrativeUnitClient.Client)

// Beta API needed to delete administrative units - the stable API is broken and returns 404 with `{"Message":"The OData path is invalid."}`
administrativeUnitClientBeta, err := administrativeunitBeta.NewAdministrativeUnitClientWithBaseURI(o.Environment.MicrosoftGraph)
if err != nil {
return nil, err
}
o.Configure(administrativeUnitClientBeta.Client)

memberClient, err := administrativeunitmember.NewAdministrativeUnitMemberClientWithBaseURI(o.Environment.MicrosoftGraph)
if err != nil {
return nil, err
Expand All @@ -37,6 +46,7 @@ func NewClient(o *common.ClientOptions) (*Client, error) {

return &Client{
AdministrativeUnitClient: administrativeUnitClient,
AdministrativeUnitClientBeta: administrativeUnitClientBeta,
AdministrativeUnitMemberClient: memberClient,
AdministrativeUnitScopedRoleMemberClient: scopedRoleMemberClient,
}, nil
Expand Down
6 changes: 3 additions & 3 deletions internal/services/applications/application_data_source.go
Original file line number Diff line number Diff line change
Expand Up @@ -600,8 +600,8 @@ func applicationDataSourceRead(ctx context.Context, d *pluginsdk.ResourceData, m
d.SetId(id.ID())

tf.Set(d, "api", flattenApplicationApi(app.Api, true))
tf.Set(d, "app_roles", flattenApplicationAppRoles(app.AppRoles))
tf.Set(d, "app_role_ids", flattenApplicationAppRoleIDs(app.AppRoles))
tf.Set(d, "app_roles", applications.FlattenAppRoles(app.AppRoles))
tf.Set(d, "app_role_ids", applications.FlattenAppRoleIDs(app.AppRoles))
tf.Set(d, "application_id", app.AppId.GetOrZero())
tf.Set(d, "client_id", app.AppId.GetOrZero())
tf.Set(d, "device_only_auth_enabled", app.IsDeviceOnlyAuthSupported.GetOrZero())
Expand All @@ -624,7 +624,7 @@ func applicationDataSourceRead(ctx context.Context, d *pluginsdk.ResourceData, m
tf.Set(d, "web", flattenApplicationWeb(app.Web))

if app.Api != nil {
tf.Set(d, "oauth2_permission_scope_ids", flattenApplicationOAuth2PermissionScopeIDs(app.Api.OAuth2PermissionScopes))
tf.Set(d, "oauth2_permission_scope_ids", applications.FlattenOAuth2PermissionScopeIDs(app.Api.OAuth2PermissionScopes))
}

if app.Info != nil {
Expand Down
6 changes: 3 additions & 3 deletions internal/services/applications/application_resource.go
Original file line number Diff line number Diff line change
Expand Up @@ -1633,8 +1633,8 @@ func applicationResourceRead(ctx context.Context, d *pluginsdk.ResourceData, met
}

tf.Set(d, "api", flattenApplicationApi(app.Api, false))
tf.Set(d, "app_role", flattenApplicationAppRoles(app.AppRoles))
tf.Set(d, "app_role_ids", flattenApplicationAppRoleIDs(app.AppRoles))
tf.Set(d, "app_role", applications.FlattenAppRoles(app.AppRoles))
tf.Set(d, "app_role_ids", applications.FlattenAppRoleIDs(app.AppRoles))
tf.Set(d, "application_id", app.AppId.GetOrZero())
tf.Set(d, "client_id", app.AppId.GetOrZero())
tf.Set(d, "description", app.Description.GetOrZero())
Expand All @@ -1659,7 +1659,7 @@ func applicationResourceRead(ctx context.Context, d *pluginsdk.ResourceData, met
tf.Set(d, "web", flattenApplicationWeb(app.Web))

if app.Api != nil {
tf.Set(d, "oauth2_permission_scope_ids", flattenApplicationOAuth2PermissionScopeIDs(app.Api.OAuth2PermissionScopes))
tf.Set(d, "oauth2_permission_scope_ids", applications.FlattenOAuth2PermissionScopeIDs(app.Api.OAuth2PermissionScopes))
}

if app.Info != nil {
Expand Down
7 changes: 2 additions & 5 deletions internal/services/applications/application_resource_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -292,6 +292,8 @@ func TestAccApplication_oauth2PermissionScopes(t *testing.T) {
data.UUID(),
}

// Note: ImportSteps missing here due to inconsistencies with SDKv2 handling of sets

data.ResourceTest(t, r, []acceptance.TestStep{
{
Config: r.basic(data),
Expand All @@ -301,7 +303,6 @@ func TestAccApplication_oauth2PermissionScopes(t *testing.T) {
check.That(data.ResourceName).Key("oauth2_permission_scope_ids.%").HasValue("0"),
),
},
data.ImportStep(),
{
Config: r.oauth2PermissionScopes(data, scopeIDs),
Check: acceptance.ComposeTestCheckFunc(
Expand All @@ -310,7 +311,6 @@ func TestAccApplication_oauth2PermissionScopes(t *testing.T) {
check.That(data.ResourceName).Key("oauth2_permission_scope_ids.%").HasValue("2"),
),
},
data.ImportStep(),
{
Config: r.oauth2PermissionScopesUpdate(data, scopeIDs),
Check: acceptance.ComposeTestCheckFunc(
Expand All @@ -319,7 +319,6 @@ func TestAccApplication_oauth2PermissionScopes(t *testing.T) {
check.That(data.ResourceName).Key("oauth2_permission_scope_ids.%").HasValue("3"),
),
},
data.ImportStep(),
{
Config: r.oauth2PermissionScopes(data, scopeIDs),
Check: acceptance.ComposeTestCheckFunc(
Expand All @@ -328,7 +327,6 @@ func TestAccApplication_oauth2PermissionScopes(t *testing.T) {
check.That(data.ResourceName).Key("oauth2_permission_scope_ids.%").HasValue("2"),
),
},
data.ImportStep(),
{
Config: r.basic(data),
Check: acceptance.ComposeTestCheckFunc(
Expand All @@ -337,7 +335,6 @@ func TestAccApplication_oauth2PermissionScopes(t *testing.T) {
check.That(data.ResourceName).Key("oauth2_permission_scope_ids.%").HasValue("0"),
),
},
data.ImportStep(),
})
}

Expand Down
18 changes: 1 addition & 17 deletions internal/services/applications/applications.go
Original file line number Diff line number Diff line change
Expand Up @@ -684,19 +684,11 @@ func flattenApplicationApi(in *stable.ApiApplication, dataSource bool) []map[str
return []map[string]interface{}{{
"known_client_applications": tf.FlattenStringSlicePtr(in.KnownClientApplications),
"mapped_claims_enabled": mappedClaims,
scopesKey: flattenApplicationOAuth2PermissionScopes(in.OAuth2PermissionScopes),
scopesKey: applications.FlattenOAuth2PermissionScopes(in.OAuth2PermissionScopes),
"requested_access_token_version": accessTokenVersion,
}}
}

func flattenApplicationAppRoleIDs(in *[]stable.AppRole) map[string]string {
return applications.FlattenAppRoleIDs(in)
}

func flattenApplicationAppRoles(in *[]stable.AppRole) []map[string]interface{} {
return applications.FlattenAppRoles(in)
}

func flattenApplicationGroupMembershipClaims(in nullable.Type[string]) []interface{} {
if in.IsNull() {
return []interface{}{}
Expand All @@ -721,14 +713,6 @@ func flattenApplicationImplicitGrant(in *stable.ImplicitGrantSettings) []map[str
}}
}

func flattenApplicationOAuth2PermissionScopeIDs(in *[]stable.PermissionScope) map[string]string {
return applications.FlattenOAuth2PermissionScopeIDs(in)
}

func flattenApplicationOAuth2PermissionScopes(in *[]stable.PermissionScope) []map[string]interface{} {
return applications.FlattenOAuth2PermissionScopes(in)
}

func flattenApplicationOptionalClaims(in *stable.OptionalClaims) []map[string]interface{} {
if in == nil {
return []map[string]interface{}{}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ func directoryRoleEligibilityScheduleRequestResource() *pluginsdk.Resource {
DeleteContext: directoryRoleEligibilityScheduleRequestResourceDelete,

Timeouts: &pluginsdk.ResourceTimeout{
Create: pluginsdk.DefaultTimeout(5 * time.Minute),
Create: pluginsdk.DefaultTimeout(10 * time.Minute),
Read: pluginsdk.DefaultTimeout(5 * time.Minute),
Delete: pluginsdk.DefaultTimeout(5 * time.Minute),
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,20 +34,6 @@ func TestAccRoleEligibilityScheduleRequest_builtin(t *testing.T) {
})
}

func TestAccRoleEligibilityScheduleRequest_custom(t *testing.T) {
data := acceptance.BuildTestData(t, "azuread_directory_role_eligibility_schedule_request", "test")
r := RoleEligibilityScheduleRequestResource{}

data.ResourceTestIgnoreDangling(t, r, []acceptance.TestStep{
{
Config: r.custom(data),
Check: acceptance.ComposeTestCheckFunc(
check.That(data.ResourceName).ExistsInAzure(r),
),
},
})
}

func (r RoleEligibilityScheduleRequestResource) Exists(ctx context.Context, clients *clients.Client, state *terraform.InstanceState) (*bool, error) {
client := clients.DirectoryRoles.DirectoryRoleEligibilityScheduleRequestClient
id := stable.NewRoleManagementDirectoryRoleEligibilityScheduleRequestID(state.ID)
Expand Down Expand Up @@ -89,36 +75,3 @@ resource "azuread_directory_role_eligibility_schedule_request" "test" {
}
`, data.RandomInteger, data.RandomPassword)
}

func (r RoleEligibilityScheduleRequestResource) custom(data acceptance.TestData) string {
return fmt.Sprintf(`
provider "azuread" {}

data "azuread_domains" "test" {
only_initial = true
}

resource "azuread_user" "test" {
user_principal_name = "acctestManager.%[1]d@${data.azuread_domains.test.domains.0.domain_name}"
display_name = "acctestManager-%[1]d"
password = "%[2]s"
}

resource "azuread_custom_directory_role" "test" {
display_name = "acctestCustomRole-%[1]d"
enabled = true
version = "1.0"

permissions {
allowed_resource_actions = ["microsoft.directory/applications/standard/read"]
}
}

resource "azuread_directory_role_eligibility_schedule_request" "test" {
role_definition_id = azuread_custom_directory_role.test.object_id
principal_id = azuread_user.test.object_id
directory_scope_id = "/"
justification = "abc"
}
`, data.RandomInteger, data.RandomPassword)
}
Loading
Loading