Add okta_app_oauth_post_logout_redirect_uri resource #931

jmaness · 2022-01-29T02:44:32Z

Add okta_app_oauth_post_logout_redirect_uri resource with an implementation that mirrors the existing okta_app_oauth_redirect_uri resource. Also added mutex to prevent multiple redirect URI resources from modifying the Okta app concurrently which is potentially rejected by Okta.
Copy MutexKV to an internal package from terraform-plugin-sdk. See Deprecated packages that were removed in v2 hashicorp/terraform-plugin-sdk#453 for details. Other providers such as terraform-plugin-aws [PLACEHOLDER] Internalize or Migrate Import of github.com/hashicorp/terraform-plugin-sdk/helper/mutexkv from Deprecation hashicorp/terraform-provider-aws#13446 have also copied MutexKV to an internal package.
Updated tests and docs with okta_app_oauth_post_logout_redirect_uri resource
Fix minor bug in resource_okta_app_oauth_redirect_uri when attempting to delete a redirect URI. The existing error message erroneously indicated that the Okta app with the specified id does not exist when deleting a redirect URI. The change here logs an info message if the redirect URI no longer exists, and otherwise removes the redirect URI from the Okta app.

…x to prevent multiple redirect URI resources from modifying the Okta app concurrently which is rejected by Okta. Copy MutexKV to internal package Updated tests and docs with okta_app_oauth_post_logout_redirect_uri resource

…a_app_oauth_redirect_uri.go

monde · 2022-02-02T21:27:17Z

Thanks for the PR @jmaness . We are about to release v3.20.4. I'll review the PR with @bogdanprodan-okta next week. Since it is a new resource it rates being in a minor point release. If this gets in it would be in a v3.21.0 release.

monde · 2022-02-08T20:20:43Z

okta/resource_okta_app_oauth_post_logout_redirect_uri.go

+func resourceAppOAuthPostLogoutRedirectURIDelete(ctx context.Context, d *schema.ResourceData, m interface{}) diag.Diagnostics {
+	appID := d.Get("app_id").(string)
+
+	oktaMutexKV.Lock(appID)


@jmaness is your PR complete? I'm not seeing were oktaMutexKV is initialized with a call to the kv mutex constructor func NewMutexKV()

oktaMutexKV is initialized in provider.go around line 412 at https://github.com/okta/terraform-provider-okta/pull/931/files#diff-a44dd20b4a48cf4cb52debcb9d4397398bccdb8a3e310bafc0ff706d3cb7c499R412

right! I always forget to look and see if the GH diff has collapsed the view of a file on big changes. I see it now.

monde · 2022-02-08T20:26:21Z

okta/resource_okta_app_oauth_redirect_uri.go

 	app := okta.NewOpenIdConnectApplication()
 	err := fetchAppByID(ctx, appID, m, app)
 	if err != nil {
 		return diag.Errorf("failed to get application: %v", err)
 	}
-	if app.Id == "" || contains(app.Settings.OauthClient.RedirectUris, d.Id()) {
+	if app.Id == "" {


@jmaness also, a kv mutex is being brought into the implementation just to fix bug you pointed out:

if app.Id == "" || contains(app.Settings.OauthClient.RedirectUris, d.Id()) {

Can just fixing the bug and not adding the mutex be enough of a fix?

I'll test again without the mutex. If I recall, the issue requiring the mutex was a separate issue from the logic bug.

Issue 1

The concurrency issue requiring the mutex was caused by two terraform operations running in parallel (e.g. adding multiple redirect URI resources) where each operation performed the following:

F - Fetch Okta app

A - Add redirect URI to Okta app JSON object

U - Update Okta app.

Since the critical section (the 3 steps above) isn't protected by a lock, it's possible for terraform to attempt create two redirect URI resources and interleave the operations like this:

F₁

F₂

A₁

A₂

U₂

U₁ (rejected by Okta, maybe by comparing E-Tags or lastUpdateDate)

Issue 2

The logic issue is that if the app contained a redirect URI, it would fail to be deleted and the error "application with id %s does not exist" would be returned. It seems like the logic should be that if the Okta app does not have the redirect URI, then the Okta app does not need to be updated, but otherwise, remove the redirect URI from the Okta app.

@jmaness thanks for explaining the concurrency issue and you actually did experience an issue with the critical path?

I'm asking for my own reference. I am familiar with the kv mutex you referenced from the AWS provider as I used it as reference for a mutex that is used for our rate limit accounting. Different code paths in the provider can make API calls and so having a mutex on a global data structure that is used for accounting makes sense.

I wasn't sure if the threading model in the terraform runtime would actually interrupt an executing function like resourceAppOAuthPostLogoutRedirectURIDelete and thus the need for a mutex there. Let me know what you think and have seen.

I just tested without using a mutex and reproduced the error. Here is an example where one of the redirect URI resources was successfully created, but the other two failed.

Example main.tf

terraform { required_providers { okta = { source = "okta/okta" version = "~> 3.20" } } } provider "okta" { org_name = var.okta_org_name base_url = var.okta_base_url api_token = var.okta_api_token } resource "okta_app_oauth_redirect_uri" "redirect_uri_1" { app_id = var.okta_app_id uri = "https://www.example1.com" } resource "okta_app_oauth_redirect_uri" "redirect_uri_2" { app_id = var.okta_app_id uri = "https://www.example2.com" } resource "okta_app_oauth_redirect_uri" "redirect_uri_3" { app_id = var.okta_app_id uri = "https://www.example3.com" }

terraform apply output

Terraform used the selected providers to generate the following execution plan. Resource actions are indicated with the following symbols: + create Terraform will perform the following actions: # okta_app_oauth_redirect_uri.redirect_uri_1 will be created + resource "okta_app_oauth_redirect_uri" "redirect_uri_1" { + app_id = "0oajksrf3koG0PdWt0h7" + id = (known after apply) + uri = "https://www.example1.com" } # okta_app_oauth_redirect_uri.redirect_uri_2 will be created + resource "okta_app_oauth_redirect_uri" "redirect_uri_2" { + app_id = "0oajksrf3koG0PdWt0h7" + id = (known after apply) + uri = "https://www.example2.com" } # okta_app_oauth_redirect_uri.redirect_uri_3 will be created + resource "okta_app_oauth_redirect_uri" "redirect_uri_3" { + app_id = "0oajksrf3koG0PdWt0h7" + id = (known after apply) + uri = "https://www.example3.com" } Plan: 3 to add, 0 to change, 0 to destroy. Do you want to perform these actions? Terraform will perform the actions described above. Only 'yes' will be accepted to approve. Enter a value: yes okta_app_oauth_redirect_uri.redirect_uri_3: Creating... okta_app_oauth_redirect_uri.redirect_uri_2: Creating... okta_app_oauth_redirect_uri.redirect_uri_1: Creating... okta_app_oauth_redirect_uri.redirect_uri_3: Creation complete after 1s [id=https://www.example3.com] ╷ │ Error: failed to create redirect URI: the API returned an error: Operation on application settings failed.. Causes: errorSummary: The requested action cannot be supported for this application., Status: 403 Forbidden │ │ with okta_app_oauth_redirect_uri.redirect_uri_1, │ on main.tf line 16, in resource "okta_app_oauth_redirect_uri" "redirect_uri_1": │ 16: resource "okta_app_oauth_redirect_uri" "redirect_uri_1" { │ ╵ ╷ │ Error: failed to create redirect URI: the API returned an error: Operation on application settings failed.. Causes: errorSummary: The requested action cannot be supported for this application., Status: 403 Forbidden │ │ with okta_app_oauth_redirect_uri.redirect_uri_2, │ on main.tf line 21, in resource "okta_app_oauth_redirect_uri" "redirect_uri_2": │ 21: resource "okta_app_oauth_redirect_uri" "redirect_uri_2" { │ ╵

Verbose logging output including the Okta API response (with some data redacted)

2022-02-08T19:04:58.996-0500 [INFO] provider.terraform-provider-okta: 2022/02/08 19:04:58 [DEBUG] Okta API Response Details: ---[ RESPONSE ]-------------------------------------- HTTP/2.0 403 Forbidden ... { "errorCode": "E0000039", "errorSummary": "Operation on application settings failed.", "errorLink": "E0000039", "errorId": "oaeVKnbk1sMSomEQYlVuLEsOQ", "errorCauses": [ { "errorSummary": "The requested action cannot be supported for this application." } ] }

With a mutex, the output is:

Terraform used the selected providers to generate the following execution plan. Resource actions are indicated with the following symbols: + create Terraform will perform the following actions: # okta_app_oauth_redirect_uri.redirect_uri_1 will be created + resource "okta_app_oauth_redirect_uri" "redirect_uri_1" { + app_id = "0oajksrf3koG0PdWt0h7" + id = (known after apply) + uri = "https://www.example1.com" } # okta_app_oauth_redirect_uri.redirect_uri_2 will be created + resource "okta_app_oauth_redirect_uri" "redirect_uri_2" { + app_id = "0oajksrf3koG0PdWt0h7" + id = (known after apply) + uri = "https://www.example2.com" } # okta_app_oauth_redirect_uri.redirect_uri_3 will be created + resource "okta_app_oauth_redirect_uri" "redirect_uri_3" { + app_id = "0oajksrf3koG0PdWt0h7" + id = (known after apply) + uri = "https://www.example3.com" } Plan: 3 to add, 0 to change, 0 to destroy. Do you want to perform these actions? Terraform will perform the actions described above. Only 'yes' will be accepted to approve. Enter a value: yes okta_app_oauth_redirect_uri.redirect_uri_1: Creating... okta_app_oauth_redirect_uri.redirect_uri_3: Creating... okta_app_oauth_redirect_uri.redirect_uri_2: Creating... okta_app_oauth_redirect_uri.redirect_uri_1: Creation complete after 0s [id=https://www.example1.com] okta_app_oauth_redirect_uri.redirect_uri_3: Creation complete after 1s [id=https://www.example3.com] okta_app_oauth_redirect_uri.redirect_uri_2: Creation complete after 2s [id=https://www.example2.com] Apply complete! Resources: 3 added, 0 changed, 0 destroyed.

I also tested destroy of multiple redirect URI resources with and without a mutex and reproduced the error when not using a mutex.

Without a mutex

okta_app_oauth_redirect_uri.redirect_uri_1: Refreshing state... [id=https://www.example1.com] okta_app_oauth_redirect_uri.redirect_uri_3: Refreshing state... [id=https://www.example3.com] okta_app_oauth_redirect_uri.redirect_uri_2: Refreshing state... [id=https://www.example2.com] Terraform used the selected providers to generate the following execution plan. Resource actions are indicated with the following symbols: - destroy Terraform will perform the following actions: # okta_app_oauth_redirect_uri.redirect_uri_1 will be destroyed - resource "okta_app_oauth_redirect_uri" "redirect_uri_1" { - app_id = "0oajksrf3koG0PdWt0h7" -> null - id = "https://www.example1.com" -> null - uri = "https://www.example1.com" -> null } # okta_app_oauth_redirect_uri.redirect_uri_2 will be destroyed - resource "okta_app_oauth_redirect_uri" "redirect_uri_2" { - app_id = "0oajksrf3koG0PdWt0h7" -> null - id = "https://www.example2.com" -> null - uri = "https://www.example2.com" -> null } # okta_app_oauth_redirect_uri.redirect_uri_3 will be destroyed - resource "okta_app_oauth_redirect_uri" "redirect_uri_3" { - app_id = "0oajksrf3koG0PdWt0h7" -> null - id = "https://www.example3.com" -> null - uri = "https://www.example3.com" -> null } Plan: 0 to add, 0 to change, 3 to destroy. Do you really want to destroy all resources? Terraform will destroy all your managed infrastructure, as shown above. There is no undo. Only 'yes' will be accepted to confirm. Enter a value: yes okta_app_oauth_redirect_uri.redirect_uri_2: Destroying... [id=https://www.example2.com] okta_app_oauth_redirect_uri.redirect_uri_3: Destroying... [id=https://www.example3.com] okta_app_oauth_redirect_uri.redirect_uri_1: Destroying... [id=https://www.example1.com] okta_app_oauth_redirect_uri.redirect_uri_3: Destruction complete after 1s ╷ │ Error: failed to delete redirect URI: the API returned an error: Operation on application settings failed.. Causes: errorSummary: The requested action cannot be supported for this application., Status: 403 Forbidden │ │ ╵ ╷ │ Error: failed to delete redirect URI: the API returned an error: Operation on application settings failed.. Causes: errorSummary: The requested action cannot be supported for this application., Status: 403 Forbidden │ │ ╵

With a mutex

okta_app_oauth_redirect_uri.redirect_uri_2: Refreshing state... [id=https://www.example2.com] okta_app_oauth_redirect_uri.redirect_uri_1: Refreshing state... [id=https://www.example1.com] okta_app_oauth_redirect_uri.redirect_uri_3: Refreshing state... [id=https://www.example3.com] Terraform used the selected providers to generate the following execution plan. Resource actions are indicated with the following symbols: - destroy Terraform will perform the following actions: # okta_app_oauth_redirect_uri.redirect_uri_1 will be destroyed - resource "okta_app_oauth_redirect_uri" "redirect_uri_1" { - app_id = "0oajksrf3koG0PdWt0h7" -> null - id = "https://www.example1.com" -> null - uri = "https://www.example1.com" -> null } # okta_app_oauth_redirect_uri.redirect_uri_2 will be destroyed - resource "okta_app_oauth_redirect_uri" "redirect_uri_2" { - app_id = "0oajksrf3koG0PdWt0h7" -> null - id = "https://www.example2.com" -> null - uri = "https://www.example2.com" -> null } # okta_app_oauth_redirect_uri.redirect_uri_3 will be destroyed - resource "okta_app_oauth_redirect_uri" "redirect_uri_3" { - app_id = "0oajksrf3koG0PdWt0h7" -> null - id = "https://www.example3.com" -> null - uri = "https://www.example3.com" -> null } Plan: 0 to add, 0 to change, 3 to destroy. Do you really want to destroy all resources? Terraform will destroy all your managed infrastructure, as shown above. There is no undo. Only 'yes' will be accepted to confirm. Enter a value: yes okta_app_oauth_redirect_uri.redirect_uri_1: Destroying... [id=https://www.example1.com] okta_app_oauth_redirect_uri.redirect_uri_2: Destroying... [id=https://www.example2.com] okta_app_oauth_redirect_uri.redirect_uri_3: Destroying... [id=https://www.example3.com] okta_app_oauth_redirect_uri.redirect_uri_3: Destruction complete after 1s okta_app_oauth_redirect_uri.redirect_uri_1: Destruction complete after 1s okta_app_oauth_redirect_uri.redirect_uri_2: Destruction complete after 2s Destroy complete! Resources: 3 destroyed.

Without any of the code changes in this PR, here's the output that demonstrates both the concurrency issue and the logic issue (in the destroy case):

$ terraform apply Terraform used the selected providers to generate the following execution plan. Resource actions are indicated with the following symbols: + create Terraform will perform the following actions: # okta_app_oauth_redirect_uri.redirect_uri_1 will be created + resource "okta_app_oauth_redirect_uri" "redirect_uri_1" { + app_id = "0oajksrf3koG0PdWt0h7" + id = (known after apply) + uri = "https://www.example1.com" } # okta_app_oauth_redirect_uri.redirect_uri_2 will be created + resource "okta_app_oauth_redirect_uri" "redirect_uri_2" { + app_id = "0oajksrf3koG0PdWt0h7" + id = (known after apply) + uri = "https://www.example2.com" } # okta_app_oauth_redirect_uri.redirect_uri_3 will be created + resource "okta_app_oauth_redirect_uri" "redirect_uri_3" { + app_id = "0oajksrf3koG0PdWt0h7" + id = (known after apply) + uri = "https://www.example3.com" } Plan: 3 to add, 0 to change, 0 to destroy. Do you want to perform these actions? Terraform will perform the actions described above. Only 'yes' will be accepted to approve. Enter a value: yes okta_app_oauth_redirect_uri.redirect_uri_2: Creating... okta_app_oauth_redirect_uri.redirect_uri_1: Creating... okta_app_oauth_redirect_uri.redirect_uri_3: Creating... okta_app_oauth_redirect_uri.redirect_uri_2: Creation complete after 1s [id=https://www.example2.com] ╷ │ Error: failed to create redirect URI: the API returned an error: Operation on application settings failed.. Causes: errorSummary: The requested action cannot be supported for this application., Status: 403 Forbidden │ │ with okta_app_oauth_redirect_uri.redirect_uri_1, │ on main.tf line 16, in resource "okta_app_oauth_redirect_uri" "redirect_uri_1": │ 16: resource "okta_app_oauth_redirect_uri" "redirect_uri_1" { │ ╵ ╷ │ Error: failed to create redirect URI: the API returned an error: Operation on application settings failed.. Causes: errorSummary: The requested action cannot be supported for this application., Status: 403 Forbidden │ │ with okta_app_oauth_redirect_uri.redirect_uri_3, │ on main.tf line 26, in resource "okta_app_oauth_redirect_uri" "redirect_uri_3": │ 26: resource "okta_app_oauth_redirect_uri" "redirect_uri_3" { │ ╵ $ terraform destroy okta_app_oauth_redirect_uri.redirect_uri_2: Refreshing state... [id=https://www.example2.com] Terraform used the selected providers to generate the following execution plan. Resource actions are indicated with the following symbols: - destroy Terraform will perform the following actions: # okta_app_oauth_redirect_uri.redirect_uri_2 will be destroyed - resource "okta_app_oauth_redirect_uri" "redirect_uri_2" { - app_id = "0oajksrf3koG0PdWt0h7" -> null - id = "https://www.example2.com" -> null - uri = "https://www.example2.com" -> null } Plan: 0 to add, 0 to change, 1 to destroy. Do you really want to destroy all resources? Terraform will destroy all your managed infrastructure, as shown above. There is no undo. Only 'yes' will be accepted to confirm. Enter a value: yes okta_app_oauth_redirect_uri.redirect_uri_2: Destroying... [id=https://www.example2.com] ╷ │ Error: application with id 0oajksrf3koG0PdWt0h7 does not exist │ │ ╵

Thanks for all the follow up details @jmaness , I'll put some time into this PR tomorrow.

I started looking around at other places, and likely, the same mutex might need to be added in resource_okta_app_saml_app_settings.go, resource_okta_app_saml.go, and a number of others (probably around a dozen or so). Basically, any place that fetches or updates the Okta OAuth app. It might make sense to do that in other PRs though.

monde · 2022-02-10T16:13:41Z

will be in v3.21.0 today

jmaness added 2 commits January 28, 2022 21:31

Fix erroneous references to post logout redirect uris in resource_okt…

9fc451a

…a_app_oauth_redirect_uri.go

monde reviewed Feb 8, 2022

View reviewed changes

bogdanprodan-okta approved these changes Feb 10, 2022

View reviewed changes

monde merged commit 2d06025 into okta:master Feb 10, 2022

monde mentioned this pull request Feb 10, 2022

Prep for v3.21.0 release. #967

Merged

jmaness deleted the feature/add_post_logout_redirect_uris branch February 10, 2022 20:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add okta_app_oauth_post_logout_redirect_uri resource #931

Add okta_app_oauth_post_logout_redirect_uri resource #931

jmaness commented Jan 29, 2022 •

edited

Loading

monde commented Feb 2, 2022

monde Feb 8, 2022

jmaness Feb 8, 2022 •

edited

Loading

monde Feb 8, 2022

monde Feb 8, 2022

jmaness Feb 8, 2022 •

edited

Loading

monde Feb 8, 2022

jmaness Feb 9, 2022

jmaness Feb 9, 2022

jmaness Feb 9, 2022

monde Feb 9, 2022

jmaness Feb 9, 2022

monde commented Feb 10, 2022

Add okta_app_oauth_post_logout_redirect_uri resource #931

Add okta_app_oauth_post_logout_redirect_uri resource #931

Conversation

jmaness commented Jan 29, 2022 • edited Loading

monde commented Feb 2, 2022

Choose a reason for hiding this comment

jmaness Feb 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmaness Feb 8, 2022 • edited Loading

Choose a reason for hiding this comment

Issue 1

Issue 2

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Example main.tf

terraform apply output

Choose a reason for hiding this comment

Without a mutex

With a mutex

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monde commented Feb 10, 2022

jmaness commented Jan 29, 2022 •

edited

Loading

jmaness Feb 8, 2022 •

edited

Loading

jmaness Feb 8, 2022 •

edited

Loading

Example `main.tf`

`terraform apply` output