From 85ac171d385d7dd141d0eb7333c161aa98e2d492 Mon Sep 17 00:00:00 2001 From: Tom Bamford Date: Tue, 11 Jun 2024 16:19:59 +0100 Subject: [PATCH 1/3] internal concurrency lock when updating azuread_application / azuread_application_registration --- .../applications/application_redirect_uris_resource.go | 2 +- .../applications/application_registration_resource.go | 3 +++ internal/services/applications/application_resource.go | 9 ++++++--- 3 files changed, 10 insertions(+), 4 deletions(-) diff --git a/internal/services/applications/application_redirect_uris_resource.go b/internal/services/applications/application_redirect_uris_resource.go index 215e4809d..4d1a79f11 100644 --- a/internal/services/applications/application_redirect_uris_resource.go +++ b/internal/services/applications/application_redirect_uris_resource.go @@ -199,7 +199,7 @@ func (r ApplicationRedirectUrisResource) Update() sdk.ResourceFunc { applicationId := parse.NewApplicationID(id.ApplicationId) var model ApplicationRedirectUrisModel - if err := metadata.Decode(&model); err != nil { + if err = metadata.Decode(&model); err != nil { return fmt.Errorf("decoding: %+v", err) } diff --git a/internal/services/applications/application_registration_resource.go b/internal/services/applications/application_registration_resource.go index 947bdc4fd..e2e5e12a7 100644 --- a/internal/services/applications/application_registration_resource.go +++ b/internal/services/applications/application_registration_resource.go @@ -353,6 +353,9 @@ func (r ApplicationRegistrationResource) Update() sdk.ResourceFunc { return fmt.Errorf("decoding: %+v", err) } + tf.LockByName(applicationResourceName, id.ApplicationId) + defer tf.UnlockByName(applicationResourceName, id.ApplicationId) + properties := msgraph.Application{ DirectoryObject: msgraph.DirectoryObject{ Id: &id.ApplicationId, diff --git a/internal/services/applications/application_resource.go b/internal/services/applications/application_resource.go index 166093d88..22332ef8a 100644 --- a/internal/services/applications/application_resource.go +++ b/internal/services/applications/application_resource.go @@ -1106,7 +1106,7 @@ func applicationResourceCreate(ctx context.Context, d *pluginsdk.ResourceData, m // See https://github.com/hashicorp/terraform-provider-azuread/issues/914 if acceptMappedClaims != nil { api.AcceptMappedClaims = acceptMappedClaims - if _, err := client.Update(ctx, msgraph.Application{ + if _, err = client.Update(ctx, msgraph.Application{ DirectoryObject: msgraph.DirectoryObject{ Id: app.Id, }, @@ -1119,7 +1119,7 @@ func applicationResourceCreate(ctx context.Context, d *pluginsdk.ResourceData, m if len(ownersExtra) > 0 { // Add any remaining owners after the application is created app.Owners = &ownersExtra - if _, err := client.AddOwners(ctx, app); err != nil { + if _, err = client.AddOwners(ctx, app); err != nil { return tf.ErrorDiagF(err, "Could not add owners to application with object ID: %q", id.ApplicationId) } } @@ -1133,7 +1133,7 @@ func applicationResourceCreate(ctx context.Context, d *pluginsdk.ResourceData, m // Upload the application image if imageContentType != "" && len(imageData) > 0 { - _, err := client.UploadLogo(ctx, id.ApplicationId, imageContentType, imageData) + _, err = client.UploadLogo(ctx, id.ApplicationId, imageContentType, imageData) if err != nil { return tf.ErrorDiagF(err, "Could not upload logo image for application with object ID: %q", id.ApplicationId) } @@ -1151,6 +1151,9 @@ func applicationResourceUpdate(ctx context.Context, d *pluginsdk.ResourceData, m return tf.ErrorDiagPathF(err, "id", "Parsing ID") } + tf.LockByName(applicationResourceName, id.ApplicationId) + defer tf.UnlockByName(applicationResourceName, id.ApplicationId) + displayName := d.Get("display_name").(string) // Perform this check at apply time to catch any duplicate names created during the same apply From 55fada45e8ccf289424b80a3304ed81167ed8b89 Mon Sep 17 00:00:00 2001 From: Tom Bamford Date: Tue, 11 Jun 2024 16:20:31 +0100 Subject: [PATCH 2/3] azuread_application: work around very buggy API when instantiating from template The `/instantiate` operation can return a 404 whlst also processing the request completely normally. This leads to orphaned objects in the directory, and a resource that cannot successfully Create. Work around this by polling for the application and service principal that you'd expect to see created out-of-band, whenever a 404 is received. Also set a temporary `displayName` for the application, as this is the only means we have to identify the resulting object is the one we are looking for. Unfortunately this means that this workaround cannot be implemented for the `azuread_application_from_template` resource, since that resource intentionally avoids changing the implicitly created `user_impersonation` scope - this will get created with nonsensical display names / descriptions in the consent flow. Since the whole point of the standalone resource is to inherit scopes from the template, this makes it infeasible to add this workaround there. --- .../applications/application_resource.go | 91 ++++++++++++++++++- .../services/applications/client/client.go | 5 + 2 files changed, 92 insertions(+), 4 deletions(-) diff --git a/internal/services/applications/application_resource.go b/internal/services/applications/application_resource.go index 22332ef8a..ffc2a96f5 100644 --- a/internal/services/applications/application_resource.go +++ b/internal/services/applications/application_resource.go @@ -954,15 +954,97 @@ func applicationResourceCreate(ctx context.Context, d *pluginsdk.ResourceData, m } if templateId != "" { + // Validate the template exists + if _, status, err := appTemplatesClient.Get(ctx, templateId, odata.Query{}); err != nil { + if status == http.StatusNotFound { + return tf.ErrorDiagPathF(err, "template_id", "Could not find application template with ID %q", templateId) + } + return tf.ErrorDiagF(err, "Could not retrieve application template with ID %q", templateId) + } + + // Generate a temporary display name to assert uniqueness when handling buggy 404 when instantiating + uuid, err := uuid.GenerateUUID() + if err != nil { + return tf.ErrorDiagF(err, "Failed to generate a UUID") + } + tempDisplayName := fmt.Sprintf("TERRAFORM_INSTANTIATE_%s", uuid) + // Instantiate application from template gallery and return via the update function properties := msgraph.ApplicationTemplate{ ID: pointer.To(templateId), - DisplayName: pointer.To(displayName), + DisplayName: pointer.To(tempDisplayName), } - result, _, err := appTemplatesClient.Instantiate(ctx, properties) + // When the /instantiate operation returns 404, it has probably created the application anyway. There is no way to tell this + // other than polling for the application object which is created out-of-band, so we create it with a quasi-unique temporary + // displayName and then poll for it. + result, status, err := appTemplatesClient.Instantiate(ctx, properties) if err != nil { - return tf.ErrorDiagF(err, "Could not instantiate application from template") + if status != http.StatusNotFound { + return tf.ErrorDiagF(err, "Could not instantiate application from template") + } + + deadline, ok := ctx.Deadline() + if !ok { + return tf.ErrorDiagF(errors.New("context has no deadline"), "internal-error: context has no deadline") + } + + // Since the API response can't be trusted, we'll have to take on responsibility for ensuring + // the application object and service principal objects were created as expected. + pollingResult, err := (&pluginsdk.StateChangeConf{ //nolint:staticcheck + Pending: []string{"Waiting"}, + Target: []string{"Found"}, + Timeout: time.Until(deadline), + MinTimeout: 5 * time.Second, + Refresh: func() (interface{}, string, error) { + // List applications with matching applicationTemplateId and displayName (using the temporary display name we generated above) + filter := fmt.Sprintf("applicationTemplateId eq '%s' and displayName eq '%s'", odata.EscapeSingleQuote(templateId), odata.EscapeSingleQuote(tempDisplayName)) + applicationsResult, _, err := client.List(ctx, odata.Query{Filter: filter}) + if err != nil { + return nil, "Error", err + } + if applicationsResult == nil { + return nil, "Waiting", nil + } + for _, application := range *applicationsResult { + if id := application.ID(); id != nil && application.AppId != nil && application.ApplicationTemplateId != nil && *application.ApplicationTemplateId == templateId && application.DisplayName != nil && *application.DisplayName == tempDisplayName { + // We should ensure the service principal was also created + servicePrincipalsClient := meta.(*clients.Client).Applications.ServicePrincipalsClient + + // List service principals for the created application + servicePrincipalsFilter := fmt.Sprintf("appId eq '%s'", odata.EscapeSingleQuote(*application.AppId)) + servicePrincipalsResult, _, err := servicePrincipalsClient.List(ctx, odata.Query{Filter: servicePrincipalsFilter}) + if err != nil { + return nil, "Error", err + } + if servicePrincipalsResult == nil { + return nil, "Waiting", nil + } + for _, servicePrincipal := range *servicePrincipalsResult { + // Validate the appId and applicationTemplateId match the application + if servicePrincipalId := servicePrincipal.ID(); servicePrincipalId != nil && servicePrincipal.AppId != nil && *servicePrincipal.AppId == *application.AppId && servicePrincipal.ApplicationTemplateId != nil && *servicePrincipal.ApplicationTemplateId == templateId { + return msgraph.ApplicationTemplate{ + Application: &application, + ServicePrincipal: &servicePrincipal, + }, "Found", nil + } + } + } + } + return nil, "Waiting", nil + }, + }).WaitForStateContext(ctx) + + if err != nil { + return tf.ErrorDiagF(err, "Could not instantiate application from template") + } + if pollingResult == nil { + return tf.ErrorDiagF(errors.New("attempted to poll for application and service principal but they were not found"), "Could not instantiate application from template") + } + + if template, ok := pollingResult.(msgraph.ApplicationTemplate); ok { + result = &template + } } if result.Application == nil { @@ -976,7 +1058,8 @@ func applicationResourceCreate(ctx context.Context, d *pluginsdk.ResourceData, m id := parse.NewApplicationID(*result.Application.ID()) d.SetId(id.ID()) - // The application was created out of band, so we'll update it just as if it was imported + // The application was created out of band, so we'll update it just as if it was imported. This will also + // set the correct displayName for the application. return applicationResourceUpdate(ctx, d, meta) } diff --git a/internal/services/applications/client/client.go b/internal/services/applications/client/client.go index b6bcdd9a7..a99d73ee0 100644 --- a/internal/services/applications/client/client.go +++ b/internal/services/applications/client/client.go @@ -13,6 +13,7 @@ type Client struct { ApplicationsClientBeta *msgraph.ApplicationsClient ApplicationTemplatesClient *msgraph.ApplicationTemplatesClient DirectoryObjectsClient *msgraph.DirectoryObjectsClient + ServicePrincipalsClient *msgraph.ServicePrincipalsClient } func NewClient(o *common.ClientOptions) *Client { @@ -31,10 +32,14 @@ func NewClient(o *common.ClientOptions) *Client { directoryObjectsClient := msgraph.NewDirectoryObjectsClient() o.ConfigureClient(&directoryObjectsClient.BaseClient) + servicePrincipalsClient := msgraph.NewServicePrincipalsClient() + o.ConfigureClient(&servicePrincipalsClient.BaseClient) + return &Client{ ApplicationsClient: applicationsClient, ApplicationsClientBeta: applicationsClientBeta, ApplicationTemplatesClient: applicationTemplatesClient, DirectoryObjectsClient: directoryObjectsClient, + ServicePrincipalsClient: servicePrincipalsClient, } } From b7952978638c4034ac459cd45cb050e8031ca372 Mon Sep 17 00:00:00 2001 From: Tom Bamford Date: Tue, 11 Jun 2024 16:26:37 +0100 Subject: [PATCH 3/3] REBASE! dependencies: updating to `REBASE ME!` of `github.com/manicminer/hamilton` - depends on https://github.com/manicminer/hamilton/pull/285 --- go.mod | 2 ++ go.sum | 4 ++-- .../manicminer/hamilton/msgraph/application_templates.go | 5 ++--- vendor/modules.txt | 3 ++- 4 files changed, 8 insertions(+), 6 deletions(-) diff --git a/go.mod b/go.mod index 49c951b47..f87f4a167 100644 --- a/go.mod +++ b/go.mod @@ -63,3 +63,5 @@ require ( ) go 1.21.3 + +replace github.com/manicminer/hamilton => github.com/MarkDordoy/hamilton v0.17.1-0.20240611151114-899c6ce169f6 diff --git a/go.sum b/go.sum index c6eace182..bde093446 100644 --- a/go.sum +++ b/go.sum @@ -1,5 +1,7 @@ dario.cat/mergo v1.0.0 h1:AGCNq9Evsj31mOgNPcLyXc+4PNABt905YmuqPYYpBWk= dario.cat/mergo v1.0.0/go.mod h1:uNxQE+84aUszobStD9th8a29P2fMDhsBdgRYvZOxGmk= +github.com/MarkDordoy/hamilton v0.17.1-0.20240611151114-899c6ce169f6 h1:yRxIRrSebI7v7BspqjteIZKrnNQDWhRA68NVeU8cTmI= +github.com/MarkDordoy/hamilton v0.17.1-0.20240611151114-899c6ce169f6/go.mod h1:u80g9rPtJpCG7EC0iayttt8UfeAp6jknClixgZGE950= github.com/Microsoft/go-winio v0.6.1 h1:9/kr64B9VUZrLm5YYwbGtUJnMgqWVOdUAXu6Migciow= github.com/Microsoft/go-winio v0.6.1/go.mod h1:LRdKpFKfdobln8UmuiYcKPot9D2v6svN5+sAH+4kjUM= github.com/ProtonMail/go-crypto v0.0.0-20230828082145-3c4c8a2d2371 h1:kkhsdkhsCvIsutKu5zLMgWtgh9YxGCNAw8Ad8hjwfYg= @@ -111,8 +113,6 @@ github.com/kr/text v0.1.0 h1:45sCR5RtlFHMR4UwH9sdQ5TC8v0qDQCHnXt+kaKSTVE= github.com/kr/text v0.1.0/go.mod h1:4Jbv+DJW3UT/LiOwJeYQe1efqtUx/iVham/4vfdArNI= github.com/kylelemons/godebug v1.1.0 h1:RPNrshWIDI6G2gRW9EHilWtl7Z6Sb1BR0xunSBf0SNc= github.com/kylelemons/godebug v1.1.0/go.mod h1:9/0rRGxNHcop5bhtWyNeEfOS8JIWk580+fNqagV/RAw= -github.com/manicminer/hamilton v0.70.0 h1:XMgVcwVtUGq2aBXqVAynaUDZdPXCXC8/rd7D5Zsg3UM= -github.com/manicminer/hamilton v0.70.0/go.mod h1:u80g9rPtJpCG7EC0iayttt8UfeAp6jknClixgZGE950= github.com/mattn/go-colorable v0.1.9/go.mod h1:u6P/XSegPjTcexA+o6vUJrdnUu04hMope9wVRipJSqc= github.com/mattn/go-colorable v0.1.12/go.mod h1:u5H1YNBxpqRaxsYJYSkiCWKzEfiAb1Gb520KVy5xxl4= github.com/mattn/go-colorable v0.1.13 h1:fFA4WZxdEF4tXPZVKMLwD8oUnCTTo08duU7wxecdEvA= diff --git a/vendor/github.com/manicminer/hamilton/msgraph/application_templates.go b/vendor/github.com/manicminer/hamilton/msgraph/application_templates.go index 1aeac9db4..ca24bf8c5 100644 --- a/vendor/github.com/manicminer/hamilton/msgraph/application_templates.go +++ b/vendor/github.com/manicminer/hamilton/msgraph/application_templates.go @@ -95,9 +95,8 @@ func (c *ApplicationTemplatesClient) Instantiate(ctx context.Context, applicatio } resp, status, _, err := c.BaseClient.Post(ctx, PostHttpRequestInput{ - Body: body, - ConsistencyFailureFunc: RetryOn404ConsistencyFailureFunc, - ValidStatusCodes: []int{http.StatusCreated}, + Body: body, + ValidStatusCodes: []int{http.StatusCreated}, Uri: Uri{ Entity: fmt.Sprintf("/applicationTemplates/%s/instantiate", *applicationTemplate.ID), }, diff --git a/vendor/modules.txt b/vendor/modules.txt index 00d5ddebe..f96b5e7f8 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -209,7 +209,7 @@ github.com/hashicorp/terraform-svchost # github.com/hashicorp/yamux v0.1.1 ## explicit; go 1.15 github.com/hashicorp/yamux -# github.com/manicminer/hamilton v0.70.0 +# github.com/manicminer/hamilton v0.70.0 => github.com/MarkDordoy/hamilton v0.17.1-0.20240611151114-899c6ce169f6 ## explicit; go 1.21 github.com/manicminer/hamilton/errors github.com/manicminer/hamilton/internal/utils @@ -428,3 +428,4 @@ google.golang.org/protobuf/types/known/timestamppb ## explicit; go 1.19 software.sslmate.com/src/go-pkcs12 software.sslmate.com/src/go-pkcs12/internal/rc2 +# github.com/manicminer/hamilton => github.com/MarkDordoy/hamilton v0.17.1-0.20240611151114-899c6ce169f6