hashicorp / terraform-provider-aws

The AWS Provider enables Terraform to manage AWS resources.
https://registry.terraform.io/providers/hashicorp/aws
Mozilla Public License 2.0
9.61k stars 9k forks source link

[Bug]: adding `["COGNITO"]` as `supported_identity_providers` triggers deletion of `token_validity_units` #34285

Open garysassano opened 8 months ago

garysassano commented 8 months ago

Terraform Core Version

1.5.0

AWS Provider Version

5.23.0

Affected Resource(s)

aws_cognito_user_pool_client

Expected Behavior

Before:

resource "aws_cognito_user_pool_client" "grafana" {
  name            = "grafana-client"
  generate_secret = false
  user_pool_id    = aws_cognito_user_pool.grafana.id
  # supported_identity_providers         = ["COGNITO"]
  callback_urls                        = ["http://localhost:3000/login/generic_oauth"]
  logout_urls                          = ["http://localhost:3000/login/generic_oauth"]
  allowed_oauth_flows_user_pool_client = true
  allowed_oauth_flows                  = ["code"]
  allowed_oauth_scopes                 = ["openid", "email", "profile"]
}

After:

resource "aws_cognito_user_pool_client" "grafana" {
  name                                 = "grafana-client"
  generate_secret                      = false
  user_pool_id                         = aws_cognito_user_pool.grafana.id
  supported_identity_providers         = ["COGNITO"]
  callback_urls                        = ["http://localhost:3000/login/generic_oauth"]
  logout_urls                          = ["http://localhost:3000/login/generic_oauth"]
  allowed_oauth_flows_user_pool_client = true
  allowed_oauth_flows                  = ["code"]
  allowed_oauth_scopes                 = ["openid", "email", "profile"]
}

I expected nothing to happen when executing terraform apply.

Actual Behavior

Terraform asks me to delete token_validity_units, and throws error if I approve.

tf-apply

Workaround: add token_validity_units to the resource configuration even if you don't need to change default values.

resource "aws_cognito_user_pool_client" "grafana" {
  name                                 = "grafana-client"
  generate_secret                      = false
  supported_identity_providers         = ["COGNITO"]
  user_pool_id                         = aws_cognito_user_pool.grafana.id
  callback_urls                        = ["http://localhost:3000/login/generic_oauth"]
  logout_urls                          = ["http://localhost:3000/login/generic_oauth"]
  allowed_oauth_flows_user_pool_client = true
  allowed_oauth_flows                  = ["code"]
  allowed_oauth_scopes                 = ["openid", "email", "profile"]
  token_validity_units {
    access_token  = "minutes"
    id_token      = "minutes"
    refresh_token = "days"
  }
}

Relevant Error/Panic Output Snippet

No response

Terraform Configuration Files

see above

Steps to Reproduce

see above

Debug Output

No response

Panic Output

No response

Important Factoids

No response

References

No response

Would you like to implement a fix?

None

github-actions[bot] commented 8 months ago

Community Note

Voting for Prioritization

Volunteering to Work on This Issue

jtyrus commented 8 months ago

I can take a look at this

jtyrus commented 8 months ago

Not able to reproduce and my state file (with or without supported_identity_providers) doesn't show token_validity_units.

Looks like there was a change in 5.24.0 for this, but I wasn't able to reproduce using 5.23.0.

30662

Are you updating existing user pool clients made a while ago? Can probably make this backwards compatible.

Terraform will perform the following actions:

  # aws_cognito_user_pool_client.test will be updated in-place
  ~ resource "aws_cognito_user_pool_client" "test" {
        id                                            = "5gk7j025k0304g2pm8hr7he7qk"
        name                                          = "mypool-client"
      ~ supported_identity_providers                  = [
          + "COGNITO",
        ]
        # (16 unchanged attributes hidden)
    }

Plan: 0 to add, 1 to change, 0 to destroy.
garysassano commented 8 months ago

This is my current configuration:

resource "aws_cognito_user_pool_client" "grafana" {
  name                = "grafana-client"
  user_pool_id        = aws_cognito_user_pool.grafana.id
  generate_secret     = false
  explicit_auth_flows = ["ALLOW_REFRESH_TOKEN_AUTH", "ALLOW_ADMIN_USER_PASSWORD_AUTH", "ALLOW_USER_PASSWORD_AUTH"]
  token_validity_units {
    refresh_token = "days"
    access_token  = "minutes"
    id_token      = "minutes"
  }
  callback_urls                        = ["http://localhost:3000/login/generic_oauth"]
  logout_urls                          = ["http://localhost:3000/login/generic_oauth"]
  allowed_oauth_flows_user_pool_client = true
  allowed_oauth_flows                  = ["code"]
  allowed_oauth_scopes                 = ["openid", "email", "profile"]
  supported_identity_providers         = ["COGNITO"]
}

And if I remove token_validity_units, Terraform asks me to delete it even if it's the default value. I'm using AWS provider v5.24.0, so I don't think there was any recent change for this resouce.

jtyrus commented 7 months ago

Ahh got it I see the issue. I was able to reproduce the error using.

token_validity_units {
    refresh_token = "days"
    access_token  = "minutes"
    id_token      = "minutes"
  }

The issue is the default units for access_token and id_token are hours with a value of 1, but the minimum time 5 minutes. So when you set it to minutes without setting the length, you get the error because you're trying to set it to 1 minute. Try this

access_token_validity = 5
  id_token_validity = 5
  token_validity_units {
    refresh_token = "days"
    access_token  = "minutes"
    id_token      = "minutes"
  }

Or use days, hours, hours which is the default..

Not sure this warrants a change since you're editing 1 half of default behavior and the docs do specify the minimum is 5 minutes.

garysassano commented 7 months ago

@jtyrus But I didn't even want to set it in the first place... Since I'm using the defaults, I shouldn't be forced to put that in my Terraform code.

jtyrus commented 7 months ago

I wasn't getting an error when I didn't include it.

But I'm testing more and it looks like if I remove everything after first defining the units and validity errors still pop up. Which leads me to believe it's a state issue around the validity.

jtyrus commented 7 months ago

@garysassano Have a fix I think will work, adding/updating tests now. If you're pressed for time manually adding those fields should work for now, default is.

  refresh_token_validity = 1
  access_token_validity = 24
  id_token_validity = 24
  token_validity_units {
    refresh_token = "days"
    access_token  = "hours"
    id_token      = "hours"
  }

Still wasn't able to repro your original scenario, but if you're creating new user pool clients you don't have to include those.

ckaenzig commented 3 months ago

Hi. I'm also hitting this issue with terraform 1.7.5 and the aws provider version 5.42.0.

I also created the user pool client initially without COGNITO in supported_identity_providers (I use a third party OIDC provider) and later added it. My terraform code doesn't have (and never had) the token_validity_units block.

Subsequent plans or applys show that it wants to remove the token_valididty_units block:

`Terraform will perform the following actions:

  # aws_cognito_user_pool_client.dev_test will be updated in-place
  ~ resource "aws_cognito_user_pool_client" "dev_test" {
        id                                            = "4ifaj1l7okv7s38aelq8tu31u6"
        name                                          = "test"
        # (18 unchanged attributes hidden)

      - token_validity_units {
          - access_token  = "minutes" -> null
          - id_token      = "minutes" -> null
          - refresh_token = "days" -> null
        }
    }

Plan: 0 to add, 1 to change, 0 to destroy.

If I apply this, I get an error from AWS:

aws_cognito_user_pool_client.dev_test: Modifying... [id=4ifaj1l7okv7s38aelq8tu31u6]
β•·
β”‚ Error: updating Cognito User Pool Client (4ifaj1l7okv7s38aelq8tu31u6)
β”‚ 
β”‚   with aws_cognito_user_pool_client.dev_test,
β”‚   on dev.tf line 29, in resource "aws_cognito_user_pool_client" "dev_test":
β”‚   29: resource "aws_cognito_user_pool_client" "dev_test" {
β”‚ 
β”‚ InvalidParameterException: Invalid range for token validity.

I hope this can help.