New Resource: aws_cognito_identity_provider #3601

pawelsocha · 2018-03-02T14:51:53Z

Overview

New resource to create identity provider for Cognito user pool.

This PR solves #3279

Tests

TF_ACC=1 go test ./... -v -run=TestAccAWSCognitoIdentityProvider_basic -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAWSCognitoIdentityProvider_basic
--- PASS: TestAccAWSCognitoIdentityProvider_basic (42.71s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	42.747s```

pawelsocha · 2018-03-07T19:50:26Z

@bflad maybe you know how to handle perpetual diffs for TypeMap variable?

leonfs · 2018-04-16T12:27:43Z

Any idea on when this PR will get merged?

At the moment as soon as you change/add/remove a Provider through the console terraform, for some unexpected reason, tries to recreate the user pool.

pawelsocha · 2018-04-16T12:36:59Z

@leonfs thanks for your feedback. I will check this :-)

leonfs · 2018-04-16T20:58:12Z

@pawelsocha No worries, thanks for making this PR.

Though the bug I mentioned earlier does not have anything to do with your code but with the current implementation of the aws_cognito_user_pool_client.

I suspect that after adding an identity provider (let's say Google) to the user pool through AWS console, the api call to AWS from Terraform returns an array with Google as value in the suppoted-Idenityt_providers field. Therefore, terraform state is different from what it is in AWS.

For some reason Terraform tries to recreate the whole resource (it shouldn't as it's a value that can change after creation).

I'm wondering if with this PR the problem will actually be solved. Or it's a different problem.

pawelsocha · 2018-04-17T07:23:55Z

@leonfs yeap, something is wrong.
I trying to catch this mismatch between plan and apply.

My example:

In aws_cognito_user_pool:
Mismatch reason: attribute mismatch: schema.1914614485.string_attribute_constraints.0.max_length

So, debugging in progress :-)

pawelsocha · 2018-04-17T10:46:50Z

@leonfs Terraform recreate aws_cognito_user_pool_client resource every apply/plan because it's affected by TypeSet implementation in TF: hashicorp/terraform#15420 (comment)

And also in resource source attribute have ForceNew set to true:

		"schema": {
				Type:     schema.TypeSet,
				Optional: true,
				Computed: true,
				ForceNew: true,

leonfs · 2018-04-17T19:47:03Z

@pawelsocha That's interesting. What I don't understand this is when a change in the schema attributes happen.

What triggered for me the change was adding/changing and identity provider (Google). What link could exist between those 2 things?

pawelsocha · 2018-04-17T19:48:39Z

@leonfs change is forced by resource definition in code.

leonfs · 2018-04-17T20:07:30Z

@pawelsocha - Not really sure if I get what you are saying.

I can see that there is a field ForceNew: true but that means that if the schema attribute has changed then the whole resource aws_cognito_user_pool would be recreated.

But when I change/add the identity provider I don't change the schema attribute. So why would that force the recreation?

pawelsocha · 2018-04-17T20:10:47Z

@leonfs when tf recreating aws_cognito_user_pool generate new resource ID and this id is used in identity provider.

leonfs · 2018-04-17T20:50:19Z

@pawelsocha Not really sure about what you are saying. Does not really make sense. TF should not recreate the aws_cognito_user_pool in the first place, and that's the problem. But the good news is that I've found a solution to the problem by digging a bit more into the plan output.

It looks like AWS adds a custom attribute called identities as soon as you add an identity provider to the user pool. This is what's making the set of schemas to change.

The problem is fixed by adding the following schema attribute.

schema {
    attribute_data_type      = "String"
    developer_only_attribute = false
    mutable                  = true
    name                     = "identities"

    string_attribute_constraints {}
  }

Notice the empty string_attribute_constraint. This is a bug in the cognito module. You can't not include it as that would generate a change in the plan on every apply.

See issues:

In any case - thanks for the feedback and looking forward to your PR getting merged.

pawelsocha · 2018-04-17T20:59:46Z

@leonfs :-) Today I had only a few hours to investigate this problem. But, in my case terraform recreate the whole resource and create new ID (exactly like in #3891)
And I found this info: https://github.com/terraform-providers/terraform-provider-aws/blob/b8857d88b3cc0fe2cb208c8a36508ec531251155/aws/structure.go#L3005-L3007

Now I know. Aws add some extra attributes to the resource.

One question: in your plan - terraform shows change in the schema with identity?

skoop22 · 2018-05-02T22:08:02Z

@pawelsocha
I had the same issue when creating a workaround for this not yet existing feature.
AWS adds identities to the schema "Automagically" could not find any doc on it ...
As a workaround I added identities to my aws_cognito_user_pool.

  resource "aws_cognito_user_pool" "cognito_userpool" {
  name = "${var.cognito_user_pool_name}"

  tags {
    Name = "Cognito Userpool"
    CreatedBy = "terraform"
    Environment = "${var.environment}"
    Application = "${var.application}"
  }
  schema {
    attribute_data_type = "String"
    name = "email"
    required = true
    mutable = true
    string_attribute_constraints {
      min_length = 0
      max_length = 2048
    }
  }
  schema {
    attribute_data_type = "String"
    name = "identities"
    mutable = true
    string_attribute_constraints {
    }
  }
}

resource "aws_cognito_user_pool_domain" "cognito_domain" {
  user_pool_id = "${aws_cognito_user_pool.cognito_userpool.id}"
  domain = "${var.cognito_user_pool_name}"
}


data "template_file" "cognito_userpool_template" {
  template = "${file("cognito_userpool_identityprovider_template/cognito_userpool_identityprovider_template.js")}"
  vars {
    userPoolId = "${aws_cognito_user_pool.cognito_userpool.id}"
    environment = "${var.environment}"
    metaDataUrl = "${var.saml_metadataurl}"

  }
}

resource "null_resource" "create_cognito_userpool_federation" {
  triggers {
    cognito_user_pool_id = "${aws_cognito_user_pool.cognito_userpool.id}"
  }

  provisioner "local-exec" {
    command = "aws cognito-idp create-identity-provider --region ${var.aws_region} --cli-input-json '${data.template_file.cognito_userpool_template.rendered}'"
  }
}

pawelsocha · 2018-05-04T06:48:50Z

@skoop22 Thanks for your research.
Now I thinking how to solve this problem. The first thing is filter schema attributes in the aws_cognito_user_pool resource. Second, add identity provider directly to the aws_cognito_user_pool resource.

But, I'm not a terraform developer.

ldenman · 2018-05-04T18:13:39Z

aws/resource_aws_cognito_identity_provider.go

+ log.Print("[DEBUG] Updating Cognito Identity Provider")
+
+ params := &cognitoidentityprovider.UpdateIdentityProviderInput{
+ UserPoolId: aws.String(d.Get("UserPoolId").(string)),


Just wondering... is "d.Get("UserPoolId") correct? Or should it be: d.Get("user_pool_id")?

Yes, I will change it.

ldenman · 2018-05-04T18:15:13Z

aws/resource_aws_cognito_identity_provider.go

+
+ "user_pool_id": {
+ Type: schema.TypeString,
+ Required: true,


Should user_pool_id be ForceNew? If the pool id changes, wouldn't a new provider need to be created?

…rceNew to user_pool_id

skoop22 · 2018-05-16T12:47:08Z

Any news on this one I want to remove my workaround :)

bflad

Hi @pawelsocha! 👋 Thanks for submitting this! I left some feedback below which I'll try to address myself right now. If I can get this fixed up tonight, I'll get this in for the v1.21.0 release.

bflad · 2018-05-31T00:11:14Z

aws/resource_aws_cognito_identity_provider.go

+ "provider_details": {
+ Type: schema.TypeMap,
+ Optional: true,
+ Computed: true,


Not sure if this was intentional, but it does not seem like we should be ignoring this from the API if its not in the Terraform configuration. I'll double check from the acceptance testing. Also, the API/SDK documentation seems to imply this should be Required: true.

bflad · 2018-05-31T00:21:45Z

aws/resource_aws_cognito_identity_provider.go

+ return fmt.Errorf("Error creating Cognito Identity Provider: %s", err)
+ }
+
+ d.SetId(*name)


In order to easily support import, let's prepend the resource ID with the user pool ID:

d.SetId(fmt.Sprintf("%s:%s", userPoolID, providerName))

Then with configuring the passthrough Importer in the resource, we can support import via terraform import aws_cognito_identity_provider.example xxx_yyyyyy:example

bflad · 2018-05-31T00:23:25Z

aws/resource_aws_cognito_identity_provider.go

+ })
+
+ if err != nil {
+ if awsErr, ok := err.(awserr.Error); ok && awsErr.Code() == "ResourceNotFoundException" {


Minor nitpick: we have a helper function to simplify this logic: isAWSErr(err, cognitoidentity.ErrCodeResourceNotFoundException, "")

bflad · 2018-05-31T00:24:37Z

aws/resource_aws_cognito_identity_provider.go

+
+ if err != nil {
+ if awsErr, ok := err.(awserr.Error); ok && awsErr.Code() == "ResourceNotFoundException" {
+ d.SetId("")


When removing a resource from the state, we prefer to provide a warning log, e.g.

log.Printf("[WARN] Cognito Identity Provider %q not found, removing from state", d.Id())

bflad · 2018-05-31T00:25:47Z

aws/resource_aws_cognito_identity_provider.go

+ return err
+ }
+
+ ip := ret.IdentityProvider


We should perform a nil check here in the unlikely scenario that the API does not error and also omits this attribute.

bflad · 2018-05-31T00:31:15Z

aws/resource_aws_cognito_identity_provider_test.go

+ Steps: []resource.TestStep{
+ {
+ Config: testAccAWSCognitoIdentityProviderConfig_basic(),
+ Check: resource.ComposeAggregateTestCheckFunc(


This is missing an testAccCheckAWSCognitoIdentityProviderExists() function, which we usually call first in a set of checks to help determine if the physical resource exists in the API as well as pass back the API response object back as a variable we can later reference in other checks.

bflad · 2018-05-31T00:33:01Z

aws/resource_aws_cognito_identity_provider_test.go

+ return nil
+}
+
+func testAccAWSCognitoIdentityProviderConfig_basic() string {


Minor nitpick: the formatting is fairly inconsistent throughout this test configuration which makes it harder to read

bflad · 2018-05-31T00:37:12Z

aws/structure.go

+ return m
+}
+
+func flattenCognitoIdentityProviderMap(config map[string]*string) map[string]string {


The AWS SDK already provides a helper function to perform this: aws.StringValueMap()

bflad · 2018-05-31T00:38:39Z

aws/structure.go

@@ -3035,6 +3035,23 @@ func flattenCognitoIdentityPoolRoles(config map[string]*string) map[string]strin
 return m
 }

+func expandCognitoIdentityProviderMap(config map[string]interface{}) map[string]*string {


We already have a (admittedly hard to find) helper function to perform this already: stringMapToPointers()

bflad · 2018-05-31T00:40:07Z

website/docs/r/cognito_identity_provider.html.markdown

+
+```hcl
+resource "aws_cognito_user_pool" "example" {
+ name = "example-pool"


Minor nitpick: the formatting is slightly off in the example configuration here and below with email

bflad · 2018-05-31T01:42:01Z

After implementing the above feedback, I was able to get the acceptance testing working by ignoring the identities schema attribute in the user pool resource -- I believe this will be the correct path forward rather than forcing everyone to define that attribute in their schema if they also implement this resource.

make testacc TEST=./aws TESTARGS='-run=TestAccAWSCognitoIdentityProvider_basic'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSCognitoIdentityProvider_basic -timeout 120m
=== RUN   TestAccAWSCognitoIdentityProvider_basic
--- PASS: TestAccAWSCognitoIdentityProvider_basic (16.53s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	16.567s

bflad · 2018-05-31T04:01:51Z

This has been released in version 1.21.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

pawelsocha · 2018-05-31T06:06:58Z

@bflad thanks for your review. it's lots of knowledge for me.

ghost · 2020-04-05T17:42:15Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

Start work on new resource aws_cognito_identity_provider

8c52fe1

ghost added the size/XS Managed by automation to categorize the size of a PR. label Mar 2, 2018

bflad added new-resource Introduces a new resource. service/cognito labels Mar 2, 2018

Add code

7756cf2

ghost added size/L Managed by automation to categorize the size of a PR. and removed size/XS Managed by automation to categorize the size of a PR. labels Mar 6, 2018

Add documentation

a8b5a3d

Cleanup code, add extra tests, fix typos in manual

17ac7cc

ghost added the size/L Managed by automation to categorize the size of a PR. label Mar 7, 2018

pawelsocha changed the title ~~[WIP] new resource aws_cognito_identity_provider~~ new resource aws_cognito_identity_provider Mar 7, 2018

radeksimko changed the title ~~new resource aws_cognito_identity_provider~~ New Resource: aws_cognito_identity_provider Mar 23, 2018

bflad mentioned this pull request Apr 26, 2018

Support for Cognito Identity Provider #3279

Closed

ldenman reviewed May 4, 2018

View reviewed changes

Accoring to ldenman comment: fix Get with wrong variable name, add Fo…

e0bc329

…rceNew to user_pool_id

ghost added the size/L Managed by automation to categorize the size of a PR. label May 4, 2018

bflad reviewed May 31, 2018

View reviewed changes

bflad added this to the v1.21.0 milestone May 31, 2018

bflad merged commit e0bc329 into hashicorp:master May 31, 2018

bflad added a commit that referenced this pull request May 31, 2018

resource/aws_cognito_identity_provider: Address PR #3601 feedback

c57f30a

bflad added a commit that referenced this pull request May 31, 2018

Update CHANGELOG for #3601

28e72dc

pawelsocha deleted the aws_cognito_identity_provider branch May 31, 2018 06:17

ghost locked and limited conversation to collaborators Apr 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Resource: aws_cognito_identity_provider #3601

New Resource: aws_cognito_identity_provider #3601

pawelsocha commented Mar 2, 2018 •

edited

Loading

pawelsocha commented Mar 7, 2018

leonfs commented Apr 16, 2018

pawelsocha commented Apr 16, 2018

leonfs commented Apr 16, 2018

pawelsocha commented Apr 17, 2018

pawelsocha commented Apr 17, 2018

leonfs commented Apr 17, 2018

pawelsocha commented Apr 17, 2018

leonfs commented Apr 17, 2018

pawelsocha commented Apr 17, 2018

leonfs commented Apr 17, 2018 •

edited

Loading

pawelsocha commented Apr 17, 2018 •

edited

Loading

skoop22 commented May 2, 2018

pawelsocha commented May 4, 2018

ldenman May 4, 2018

pawelsocha May 4, 2018

ldenman May 4, 2018 •

edited

Loading

pawelsocha May 4, 2018

skoop22 commented May 16, 2018

bflad left a comment

bflad May 31, 2018

bflad May 31, 2018

bflad May 31, 2018

bflad May 31, 2018

bflad May 31, 2018

bflad May 31, 2018

bflad May 31, 2018

bflad May 31, 2018

bflad May 31, 2018

bflad May 31, 2018

bflad commented May 31, 2018

bflad commented May 31, 2018

pawelsocha commented May 31, 2018

ghost commented Apr 5, 2020

New Resource: aws_cognito_identity_provider #3601

New Resource: aws_cognito_identity_provider #3601

Conversation

pawelsocha commented Mar 2, 2018 • edited Loading

Overview

Tests

pawelsocha commented Mar 7, 2018

leonfs commented Apr 16, 2018

pawelsocha commented Apr 16, 2018

leonfs commented Apr 16, 2018

pawelsocha commented Apr 17, 2018

pawelsocha commented Apr 17, 2018

leonfs commented Apr 17, 2018

pawelsocha commented Apr 17, 2018

leonfs commented Apr 17, 2018

pawelsocha commented Apr 17, 2018

leonfs commented Apr 17, 2018 • edited Loading

pawelsocha commented Apr 17, 2018 • edited Loading

skoop22 commented May 2, 2018

pawelsocha commented May 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ldenman May 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skoop22 commented May 16, 2018

bflad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bflad commented May 31, 2018

bflad commented May 31, 2018

pawelsocha commented May 31, 2018

ghost commented Apr 5, 2020

pawelsocha commented Mar 2, 2018 •

edited

Loading

leonfs commented Apr 17, 2018 •

edited

Loading

pawelsocha commented Apr 17, 2018 •

edited

Loading

ldenman May 4, 2018 •

edited

Loading