Add edition parameter for MS AD

[leibowitz]

Closes #2192
Closes #2357

Add support for the AWS Microsoft AD Edition type (Standard or Enterprise)
https://github.com/aws/aws-sdk-go/blob/master/service/directoryservice/api.go#L8628-L8634

https://aws.amazon.com/about-aws/whats-new/2017/10/introducing-aws-directory-service-for-microsoft-active-directory-standard-edition/
https://aws.amazon.com/directoryservice/pricing/#editions

Note: This parameter only applies to the type = "MicrosoftAD"

[bflad]

@leibowitz thanks for this contribution! Can you check out my below comments and let me know if you have any questions please? We also should add an acceptance test to ensure that this does actually get set in the Terraform state.

e.g. in TestAccAWSDirectoryServiceDirectory_microsoft below the Exists check:

resource.TestCheckResourceAttr("aws_directory_service_discovery.bar", "edition", "Enterprise")

Preferably we'd also create a second acceptance test to ensure that setting Standard does in fact create a Standard edition directory.

[bflad]

This can be simplified to the below 😄

ValidateFunc: validation.StringInSlice([]string{
	directoryservice.DirectoryEditionEnterprise,
	directoryservice.DirectoryEditionStandard,
}, false),

[leibowitz]

I looked around the code to find something similar, but ended up stealing the validation from another attribute. This is much better. Thanks for that

[bflad]

Nitpick: Can you please use the SDK provided constant here? directoryservice.DirectoryEditionEnterprise

[leibowitz]

done

[bflad]

Normally this check wouldn't be necessary, however I think because we the SDK handles both SimpleAD and MicrosoftAD directory types that in fact we do want to keep this nil check in here. 👍

[leibowitz]

Test done

Thanks a lot for the tips 👍 You did it all 😄

[leibowitz]

What is the expected behaviour when edition is specified but the type isn't MicrosoftAD? Should it throw an error? I'm not 100% sure what will happen at the moment, if the AWS API will reject the request or not (probably will)
Update: just tried with SimpleAD, it seems to be ignored. Just like size is ignored when using MicrosoftAD

[bflad]

Thanks for working through this @leibowitz! This LGTM with some minor changes that I have pushed in a followup commit (since it turns out this is much more nuanced than it looks):

• Fixed typos in acceptance testing: Previously (notice _discovery not _directory)

=== RUN   TestAccAWSDirectoryServiceDirectory_microsoft
--- FAIL: TestAccAWSDirectoryServiceDirectory_microsoft (1688.08s)
    testing.go:513: Step 0 error: Check failed: Check 2/2 error: Not found: aws_directory_service_discovery.bar in [root]
=== RUN   TestAccAWSDirectoryServiceDirectory_microsoftStandard
--- FAIL: TestAccAWSDirectoryServiceDirectory_microsoftStandard (1639.87s)
    testing.go:513: Step 0 error: Check failed: Check 2/2 error: Not found: aws_directory_service_discovery.bar in [root]

• Change the edition attribute Default: ... to Computed: true and always d.Set() in the state - this was required because when importing an aws_directory_service_directory, it did not know what the correct value was for SimpleAD type:

=== RUN   TestAccAWSDirectoryServiceDirectory_importBasic
--- FAIL: TestAccAWSDirectoryServiceDirectory_importBasic (472.82s)
	testing.go:513: Step 1 error: ImportStateVerify attributes not equivalent. Difference is shown below. Top is actual, bottom is expected.

		(map[string]string) {
		}


		(map[string]string) (len=1) {
		 (string) (len=7) "edition": (string) (len=10) "Enterprise"
		}

• With edition = "Standard", the API was returning size as "Small", which does not play well with the Default: "Large" for that attribute. Like edition, I marked size as Computed: true without the default to match to existing Terraform and API documentation. Previously:

=== RUN   TestAccAWSDirectoryServiceDirectory_microsoftStandard
--- FAIL: TestAccAWSDirectoryServiceDirectory_microsoftStandard (1704.19s)
    testing.go:513: Step 0 error: After applying this step, the plan was not empty:
        
        DIFF:
        
        DESTROY/CREATE: aws_directory_service_directory.bar
          access_url:                           "d-92672652d7.awsapps.com" => "<computed>"
          alias:                                "d-92672652d7" => "<computed>"
          dns_ip_addresses.#:                   "2" => "<computed>"
          edition:                              "Standard" => "Standard"
          enable_sso:                           "false" => "false"
          name:                                 "corp.notexample.com" => "corp.notexample.com"
          password:                             "<sensitive>" => "<sensitive>" (attribute changed)
          security_group_id:                    "sg-4a3ba335" => "<computed>"
          short_name:                           "corp" => "<computed>"
          size:                                 "Small" => "Large" (forces new resource)
          type:                                 "MicrosoftAD" => "MicrosoftAD"
          vpc_settings.#:                       "1" => "1"
          vpc_settings.0.subnet_ids.#:          "2" => "2"
          vpc_settings.0.subnet_ids.106299362:  "subnet-2c1fde55" => "subnet-2c1fde55"
          vpc_settings.0.subnet_ids.3442655816: "subnet-1e53b355" => "subnet-1e53b355"
          vpc_settings.0.vpc_id:                "vpc-0177fd78" => "vpc-0177fd78"

Now passes everything:

Provider_-_*.log
=== RUN   TestAccAWSDirectoryServiceDirectory_importBasic
--- PASS: TestAccAWSDirectoryServiceDirectory_importBasic (458.95s)
=== RUN   TestAccAWSDirectoryServiceDirectory_tags
--- PASS: TestAccAWSDirectoryServiceDirectory_tags (533.45s)
=== RUN   TestAccAWSDirectoryServiceDirectory_basic
--- PASS: TestAccAWSDirectoryServiceDirectory_basic (567.34s)
=== RUN   TestAccAWSDirectoryServiceDirectory_withAliasAndSso
--- PASS: TestAccAWSDirectoryServiceDirectory_withAliasAndSso (583.32s)
=== RUN   TestAccAWSDirectoryServiceDirectory_connector
--- PASS: TestAccAWSDirectoryServiceDirectory_connector (861.61s)
=== RUN   TestAccAWSDirectoryServiceDirectory_microsoftStandard
--- PASS: TestAccAWSDirectoryServiceDirectory_microsoftStandard (1630.21s)
=== RUN   TestAccAWSDirectoryServiceDirectory_microsoft
--- PASS: TestAccAWSDirectoryServiceDirectory_microsoft (1690.35s)

[leibowitz]

Good catches! Sorry for the typo, not sure where that came from :| Also for the Computed and the problem with Size, plus the default value returned by the API, I'm glad you tested this properly. Really appreciate the work you've put into this!

Like I said before, you did most (if not all) of the work in the end. Thanks again

[bflad]

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

[ghost]

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!