Add azurerm_key_vault_certificate_issuer #7074

sirlatrom · 2020-05-25T17:45:57Z

Fixes #7073.

sirlatrom · 2020-06-10T20:19:25Z

I've only been force-pushing to keep up to date with master. Other than that this should be ready.

sirlatrom · 2020-06-13T14:42:55Z

@katbyte @tombuildsstuff I've been using this for a while and seems to work fine; is there anything missing?

website/docs/r/key_vault_certificate_issuer.html.markdown

sirlatrom · 2020-06-14T18:50:30Z

It is required when you have an actual account with the provider, e.g. DigiCert.

…

On Sun, 14 Jun 2020, 19:41 Paul Wiens, ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In website/docs/r/key_vault_certificate_issuer.html.markdown <#7074 (comment)> : > + +resource "azurerm_key_vault_certificate_issuer" "example" { + name = "example-issuer" + org_id = "0000" + key_vault_id = data.azurerm_key_vault.example.id + provider_name = "DigiCert" + account_id = "0000" + password = "example-password" +} +``` + +## Arguments Reference + +The following arguments are supported: + +* `account_id` - (Required) The account number with the third-party Certificate Issuer. This isn't actually required. I have a fork that implements this same resource provider and passing anything other than key_keyvault_id, provider_name, and name isn't required. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#7074 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADH5IMHXU6PNBKEQW4HBI3RWUDS7ANCNFSM4NJV3R5A> .

pwiens · 2020-06-16T20:13:08Z

I would love it if this got merged. I've been waiting for this feature since 2.0

sirlatrom · 2020-06-17T09:01:23Z

Rebased on the main branch and resolved contention from the newly added azurerm_key_vault_certificate datasource.

sirlatrom · 2020-06-17T09:07:29Z

I'd love for this to get merged too.

@pwiens Was it company policy or something that prevented you from contributing your fork?

pwiens · 2020-06-17T12:41:35Z

@sirlatrom This is the first time I've touched go code and I didn't feel comfortable enough putting together a PR with decent tests. I was going to invest some time in doing that but I saw your PR and the issue you opened. Thanks for taking the time to do the work.

jackofallops

Hi @sirlatrom
Thanks for this PR for new resource and data source, it's off to a good start, and apologies for the fairly rushed review here (but I did promise I'd try to get to it today!).

I've made some comments and suggestions below on the code. I've not covered the testing or docs fully yet, but I'll loop back to that after you've had a look at the review so far. (I will mention that the tests for the data source are missing and we would want to see a requiresImport and an update test on the resource.)

azurerm/internal/services/keyvault/key_vault_certificate_issuer_data_source.go

jackofallops · 2020-06-18T13:08:17Z

azurerm/internal/services/keyvault/key_vault_certificate_issuer_data_source.go

+	d.Set("org_id", resp.OrganizationDetails.ID)
+	d.Set("account_id", resp.Credentials.AccountID)


I think these should probably be nil checked before setting, if the response is missing elements for some reason we'll get a panic here.

azurerm/internal/services/keyvault/key_vault_certificate_issuer_resource.go

jackofallops · 2020-06-18T14:01:33Z

azurerm/internal/services/keyvault/key_vault_certificate_issuer_resource.go

+	d.Set("org_id", resp.OrganizationDetails.ID)
+	d.Set("account_id", resp.Credentials.AccountID)
+	d.Set("password", resp.Credentials.Password)


These (OrganizationDetails and Credentials) are potentially nil, so should probably be nil checked before attempting to set the schema attributes to the values contained within to avoid panic.

jackofallops · 2020-06-18T14:14:22Z

azurerm/internal/services/keyvault/key_vault_certificate_issuer_resource.go

+	name := d.Get("name").(string)
+	keyVaultId := d.Get("key_vault_id").(string)


We should get the information to make the Read call from the ID in the state (using d.Id()), rather than the config, this allows us to understand the difference between the config and what Terraform last knew about.

jackofallops · 2020-06-18T14:15:31Z

azurerm/internal/services/keyvault/key_vault_certificate_issuer_resource.go

+		if utils.ResponseWasNotFound(resp.Response) {
+			return fmt.Errorf("KeyVault Certificate Issuer %q (KeyVault URI %q) does not exist", name, keyVaultBaseUri)
+		}
+		return fmt.Errorf("Error making Read request on Azure KeyVault Certificate Issuer %s: %+v", name, err)


As above, re: removing duplication of the word Error from message output

jackofallops · 2020-06-18T14:19:32Z

azurerm/internal/services/keyvault/key_vault_certificate_issuer_resource.go

+	name := d.Get("name").(string)
+	keyVaultId := d.Get("key_vault_id").(string)


As with Read, Delete should use the state for this information.

sirlatrom · 2020-06-25T11:57:21Z

@jackofallops Just missing the datasource tests now, but perhaps you could already now validate the changes to the rest of the PR?

sirlatrom · 2020-06-25T16:19:33Z

@jackofallops I've added a very basic test of the datasource. What is the expected level of detail?

jackofallops · 2020-06-26T12:54:20Z

@jackofallops I've added a very basic test of the datasource. What is the expected level of detail?

Thanks! Usually we create an example resource and ensure that each expected exported attribute in the data source is populated as expected. I'll re-review now, sorry for the delay in looping back.

jackofallops

Hi @sirlatrom
Thanks for the updates and great work so far, a few more changes noted below (sorry, I missed a couple things last pass). I've also left a question around the change to the custom poller for certificate creation, if you can explain the scenario you're looking to mitigate I can review that again.

Thanks again!

azurerm/internal/services/keyvault/key_vault_certificate_issuer_data_source.go

azurerm/internal/services/keyvault/key_vault_certificate_issuer_resource.go

azurerm/internal/services/keyvault/key_vault_certificate_resource.go

azurerm/internal/services/keyvault/tests/key_vault_certificate_issuer_data_source_test.go

azurerm/internal/services/keyvault/tests/key_vault_certificate_issuer_resource_test.go

sirlatrom · 2020-06-26T14:55:06Z

Hi @jackofallops,

A quick answer regarding the poller: Our issuer turns out to include manual processing on the issuer's side, with an initial estimated time returned in a message that would be frail to parse. They do however have a SLA that should fall within the updated timeouts. Alternatively, the timeouts could be exposed as attributes, too.

I'll handle the other review items early next week.

Signed-off-by: Sune Keller <[email protected]>

Signed-off-by: Sune Keller <[email protected]> Co-authored-by: Steve <[email protected]>

Signed-off-by: Sune Keller <[email protected]>

Signed-off-by: Sune Keller <[email protected]> Co-authored-by: Steve <[email protected]>

Signed-off-by: Sune Keller <[email protected]>

Signed-off-by: Sune Keller <[email protected]> Co-authored-by: Tom Harvey <[email protected]>

Signed-off-by: Sune Keller <[email protected]>

sirlatrom · 2020-07-07T11:31:48Z

@tombuildsstuff @jackofallops I've applied your suggestions and also rebased on origin/~~master~~main to ensure the branch is sufficiently up to date.

jackofallops · 2020-07-07T15:12:52Z

Thanks @sirlatrom - I've just fixed a minor issue we introduced along the way, so hopefully travis will be happy now. As soon as that goes green I think we're good to rerun the tests and, all being well, get this merged in.

jackofallops · 2020-07-08T06:54:51Z

Tests re-run and passing after changes:

ghost · 2020-07-10T01:23:08Z

This has been released in version 2.18.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azurerm" {
    version = "~> 2.18.0"
}
# ... other configuration ...

ghost · 2020-08-07T14:51:08Z

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 👉 [email protected]. Thanks!

ghost added size/XL documentation labels May 25, 2020

sirlatrom force-pushed the fix-7073 branch from d8cfdc6 to d291f8c Compare May 29, 2020 08:20

sirlatrom marked this pull request as ready for review May 29, 2020 08:23

jackofallops added new-data-source new-resource service/key-vault Key Vault labels Jun 1, 2020

sirlatrom force-pushed the fix-7073 branch 2 times, most recently from 7aaf9af to 9e68b7c Compare June 3, 2020 14:27

sirlatrom force-pushed the fix-7073 branch 2 times, most recently from 7c53815 to 8d212b5 Compare June 9, 2020 11:45

pwiens reviewed Jun 14, 2020

View reviewed changes

website/docs/r/key_vault_certificate_issuer.html.markdown Outdated Show resolved Hide resolved

sirlatrom force-pushed the fix-7073 branch from bdd1fa3 to ebe19fb Compare June 17, 2020 08:54

jackofallops requested changes Jun 18, 2020

View reviewed changes

jackofallops added the waiting-response label Jun 23, 2020

sirlatrom force-pushed the fix-7073 branch from a1432ea to 46f1f9f Compare June 23, 2020 11:18

ghost removed the waiting-response label Jun 25, 2020

sirlatrom requested a review from jackofallops June 25, 2020 16:20

jackofallops requested changes Jun 26, 2020

View reviewed changes

sirlatrom and others added 16 commits July 7, 2020 13:19

Fix bad merge of datasources list

9037ed4

Signed-off-by: Sune Keller <[email protected]>

Apply suggestions from code review

f8bd62e

Signed-off-by: Sune Keller <[email protected]> Co-authored-by: Steve <[email protected]>

Reword error messages

940540a

Signed-off-by: Sune Keller <[email protected]>

Nil checks

3861039

Signed-off-by: Sune Keller <[email protected]>

Use id from state except during create

23f29f1

Signed-off-by: Sune Keller <[email protected]>

Expand AdministratorDetails

9225b16

Signed-off-by: Sune Keller <[email protected]>

Remove unnecessary trailing newline

14a7d62

Signed-off-by: Sune Keller <[email protected]>

Add acceptance test for azurerm_key_vault_certificate_issuer data source

9b0b291

Signed-off-by: Sune Keller <[email protected]>

Apply suggestions from code review

a3b5176

Signed-off-by: Sune Keller <[email protected]> Co-authored-by: Steve <[email protected]>

Introduce type KeyVaultCertificateIssuerID and add more nil checks

440679d

Signed-off-by: Sune Keller <[email protected]>

refactor and rework for missing validation in API

b99ddd1

fixed disappears test

9d8ec83

flattenKeyVaultCertificateIssuerAdmins handles nil check

a3b2ecd

Signed-off-by: Sune Keller <[email protected]>

Apply suggestions from code review

d59344d

Signed-off-by: Sune Keller <[email protected]> Co-authored-by: Tom Harvey <[email protected]>

Also let flatten handle nil check in resource

2511496

Signed-off-by: Sune Keller <[email protected]>

Password will always be nil when read, so no use in setting it

dbd4871

Signed-off-by: Sune Keller <[email protected]>

sirlatrom force-pushed the fix-7073 branch from c8ac624 to dbd4871 Compare July 7, 2020 11:20

sirlatrom added 2 commits July 7, 2020 13:24

Remove error return value from flattenKeyVaultCertificateIssuerAdmins

6446928

Signed-off-by: Sune Keller <[email protected]>

Group OrganizationDetails data source Set()s

4304f64

Signed-off-by: Sune Keller <[email protected]>

fix arg count in disappears test error message

562917c

jackofallops approved these changes Jul 8, 2020

View reviewed changes

jackofallops merged commit 98317e5 into hashicorp:master Jul 8, 2020

jackofallops added a commit that referenced this pull request Jul 8, 2020

Updated for #7074

3e4a5b9

sirlatrom deleted the fix-7073 branch July 8, 2020 07:34

ghost locked and limited conversation to collaborators Aug 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add azurerm_key_vault_certificate_issuer #7074

Add azurerm_key_vault_certificate_issuer #7074

sirlatrom commented May 25, 2020

sirlatrom commented Jun 10, 2020

sirlatrom commented Jun 13, 2020

sirlatrom commented Jun 14, 2020 via email

pwiens commented Jun 16, 2020

sirlatrom commented Jun 17, 2020

sirlatrom commented Jun 17, 2020

pwiens commented Jun 17, 2020

jackofallops left a comment

jackofallops Jun 18, 2020

jackofallops Jun 18, 2020

jackofallops Jun 18, 2020

jackofallops Jun 18, 2020

jackofallops Jun 18, 2020

sirlatrom commented Jun 25, 2020

sirlatrom commented Jun 25, 2020

jackofallops commented Jun 26, 2020

jackofallops left a comment

sirlatrom commented Jun 26, 2020

sirlatrom commented Jul 7, 2020

jackofallops commented Jul 7, 2020

jackofallops commented Jul 8, 2020

ghost commented Jul 10, 2020

ghost commented Aug 7, 2020

		d.Set("org_id", resp.OrganizationDetails.ID)
		d.Set("account_id", resp.Credentials.AccountID)

		name := d.Get("name").(string)
		keyVaultId := d.Get("key_vault_id").(string)

Add azurerm_key_vault_certificate_issuer #7074

Add azurerm_key_vault_certificate_issuer #7074

Conversation

sirlatrom commented May 25, 2020

sirlatrom commented Jun 10, 2020

sirlatrom commented Jun 13, 2020

sirlatrom commented Jun 14, 2020 via email

pwiens commented Jun 16, 2020

sirlatrom commented Jun 17, 2020

sirlatrom commented Jun 17, 2020

pwiens commented Jun 17, 2020

jackofallops left a comment

Choose a reason for hiding this comment

jackofallops Jun 18, 2020

Choose a reason for hiding this comment

jackofallops Jun 18, 2020

Choose a reason for hiding this comment

jackofallops Jun 18, 2020

Choose a reason for hiding this comment

jackofallops Jun 18, 2020

Choose a reason for hiding this comment

jackofallops Jun 18, 2020

Choose a reason for hiding this comment

sirlatrom commented Jun 25, 2020

sirlatrom commented Jun 25, 2020

jackofallops commented Jun 26, 2020

jackofallops left a comment

Choose a reason for hiding this comment

sirlatrom commented Jun 26, 2020

sirlatrom commented Jul 7, 2020

jackofallops commented Jul 7, 2020

jackofallops commented Jul 8, 2020

ghost commented Jul 10, 2020

ghost commented Aug 7, 2020