Add subject alternative names to azurerm_key_vault_certificate

[draggeta]

Because of hacktober I decided to just try my hand at this. Either way. This allows you to specify subject alternative names for the azurerm_key_vault_certificate resource. All combinations of FQDNs, email addresses and UPNs are possible.

Test results:

PS > go test -timeout 180m -v -ldflags="-X=github.com/terraform-providers/terraform-provider-azurerm/version.ProviderVersion=acc" github.com\terraform-providers\terraform-provider-azurerm\azurerm -run ^TestAccAzureRMKeyVaultCertificate_basicGenerateSans$
=== RUN   TestAccAzureRMKeyVaultCertificate_basicGenerateSans
--- PASS: TestAccAzureRMKeyVaultCertificate_basicGenerateSans (222.49s)
PASS
ok      github.com/terraform-providers/terraform-provider-azurerm/azurerm       232.421s

Closes #479
Also fixes #1309

[draggeta]

This is now somehow failing on linting with gofmt -s. I've written to the file with -w and it still outputs no diff in git for me. Both on a Ubuntu and Windows device.

@tombuildsstuff do you have any idea how I can fix this?

[katbyte]

There is a utils.ExpandStringArray that does exactly this

[katbyte]

Hi @draggeta,

We have a make fmt command we use to run, an gofmt and a make lint CI runs to check gofmt and more

I ran it and pushed up the changes

[tombuildsstuff]

@draggeta we're currently using Go 1.10 on this project (but Go 1.11 is the latest) - we'll be moving to this in the near future; of particular note between the two versions is how go fmt can return different results (which I believe may be causing the issue here?)

[tombuildsstuff]

hey @draggeta

I've taken a look through and left a couple of comments but this otherwise LGTM; if we can fix those up we should be able to run the tests and get this merged :)

Thanks!

[draggeta]

@tombuildsstuff Well, that explains a lot, I'm using 1.11 😄

As for the changes. I implemented them, but now I always end up with 1 item for the SAN when importing or when it is not defined. That was why I had written the code like I did. I had forgotten about it.

Am I correct in my assumption that this should be fixed with a DiffSuppressFunc?

[tombuildsstuff]

@draggeta heh - thanks for pushing those changes too :)

Am I correct in my assumption that this should be fixed with a DiffSuppressFunc?

Kinda; since this is a list we'll make this Computed instead (which we can do here) - given one item's being returned from Azure this means an item's being returned even if there's no items in that list; it also means we'll want to ensure we always set the items in that block (e.g. upns) - I'll push an updated review, but this otherwise looks good to me 👍

[tombuildsstuff]

in retrospect (since they're returned from the API) - we'll want to always flatten these fields (even if there's no items returned) - as such we should be able to make this:

sanOutput["emails"] = utils.FlattenStringArray(san.Emails)
sanOutput["dns_names"] = utils.FlattenStringArray(san.DNSNames)
sanOutput["upns"] = utils.FlattenStringArray(san.Upns)

[tombuildsstuff]

hey @draggeta

Thanks for pushing those changes - this now LGTM 👍 - I'll kick off the test suite shortly

Thanks!

[tombuildsstuff]

Ignoring a known test failure - the tests pass - thanks for this @draggeta :):

[tombuildsstuff]

hi @draggeta

Just to let you know that this has been released as a part of v1.18 of the AzureRM Provider (the full changelog is available here). You can upgrade to this by specifying the version in the provider block (as shown below) and then running terraform init -upgrade

provider "azurerm" {
  version = "=1.18.0"
}

Thanks!

[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. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 hashibot-feedback@hashicorp.com. Thanks!