Add AD Application and Service Principal providers.

[Stavrus]

Apologies for submitting this PR with 2 providers. I realize the contribution guide asks for 1 PR per, but since these 2 resources are so closely tied to each other I thought it would be more appropriate to review them together.

This PR aims to help fix #16.

This is a first pass at adding the above providers, and thus can be considered WIP. Known issues:

• Key Credentials and Password Credentials cannot detect changes to values other than the key ID.
  • I wasn't quite sure how to handle this case. I could add the other attributes to the hashing function, but this causes other problems:
    • The Azure API can accept start_time and end_time in one format and return them back to you on a read in a different one. This would cause the hashing function to return different values for the credentials read from the tfstate and those read from the user's TF files. Probably solvable by casting the times to a common format in the hashing function, but I felt I was over-complicating things with this solution and held off on implementing it pending further discussion here.
    • The start_time and end_time attributes are optional, but always returned with the value set to a datetime when a read is performed. This would cause a diff to be detected by Terraform if the user didn't specify both of them in their TF file. I could solve this by specifying for both parameters to be required, but I saw Azure rejecting start_time and end_time values of Key Credentials that I couldn't determine the cause of and thus left them optional. Leaving them null was allowing the Key Credential to get accepted. I would need to further investigate these rejections before I would be comfortable setting them to required.
    • If you update an attribute on a credential that is not the key_id it is possible to update that credential so long as the value attribute is nulled for all the credentials before submitting the patch. Since the provider doesn't currently detect these changes, it doesn't perform the nullification.
• If you have more than one credential of a type, and update the key_id of one, the update will fail (e.g. two Password Credentials "a" and "b", and you change "b" to "c"). This is due to the update function not performing the nullification of the value attribute on "a" as described above.
• Changing the key ID on a Key Credential or a Password Credential calculates an invalid display diff, as it displays two credentials changing attributes rather than one credential changing its key_id attribute.
• OData API Errors are not returned to the user.
  • If you attempt to create the resource with invalid data or insufficient permissions you get back a 400 error with a body describing what the problem actually is. It would be useful to propagate this back to the user but I couldn't determine how to properly do this due to my unfamiliarity with autorest.
• Website documentation is a WIP.
  • Missing the import sections for the new resources and the links in the sidebar.
  • The AD Application documentation could probably use some refinement over how to actually assign the correct user permissions for creating applications through the API, as it isn't a very clear process.
  • I also need to verify the templates are actually correct. I ran into a few snags with the terraform-website project to actually build the site, so I figured I could resolve them while discussion on this PR started. I hope this isn't a problem.
• The AD Application provider does not currently support setting RequiredResourceAccess objects.
  • Not necessary for adding support for Feature Request: azurerm_service_principal #16 so I didn't include it in this initial PR.
• The Service Principal provider does not currently support setting Key Credentials nor Password Credentials.
  • I placed the Key Credentials and the Password Credentials in separate files due to the Service Principal having support for setting them. However, I didn't actually implement support for them in the provider because I can set the Password Credential on the application, create the SPN with assigned roles, and then use that SPN to create resources. I'm not sure if setting them in the Service Principal is necessary, but this is an area I need to investigate further so I left it out of the PR.

[taraspos]

Hey, any progress here? :)

[Stavrus]

Hey @trane9991, yes, work is still ongoing on my end to tackle the remaining issues, particularly with the diffing of resources. I got pulled away from this for awhile due to other tasks I had to finish. Sorry to keep you waiting!

[Stavrus]

@tombuildsstuff I think this is now in a good enough state to begin the review process, so I've removed all the [WIP] labeling.

[katbyte]

Hi @Stavrus,

Would it be possible to split this into two separate PRs? One for AD Application and another for service principals, it will make it easier for us to review and get these merged🙂

thanks!

[dominik-lekse]

Thanks @Stavrus for working on this. Looking forward to have this feature available.

[Stavrus]

Hi @katbyte, I've gone ahead and created the AD application PR but I'm afraid that my creation test for service principals requires it to also create an AD application since the two are tied so closely within Azure's APIs. Should I hold off on the SPN PR until the AD application is reviewed and merged?

Also, with the SPN provider, I've been noticing that resources that depend on fresh SPNs run into issues where the Azure APIs haven't caught up with the fact that the SPN exists and error out on me with an SPN not found. For example, the role assignment resource requires an SPN. If I feed this resource an SPN created by my provider it tends to give me the not found error. However, re-running the apply command or inserting a local-exec provisioner into the SPN resource that sleeps for 10 seconds seems to fix the problem. What is the recommended approach here? I could insert the sleep into the provider and hide it from the user, but this seems strange to me. As you can see within this PR, the SPN provider does not set into the tfstate the properties required by other resources (such as role assignment) until it successfully performs a GET on the SPN.

Thanks!

[katbyte]

@Stavrus,

Thank you for splitting it out, I'll try and review it laster today. You can either repurpose this PR and keep it up to date with master or open another one, its your call 🙂

As to waiting for the SPN to become available, it depends on if there is a way to determine its been fully provisioned. If there is an api endpoint you could use WaitForState(). An example of this can be found in the key vault resource: https://github.com/terraform-providers/terraform-provider-azurerm/blob/d819d597531039823c1cf6dffd7f05a131153403/azurerm/resource_arm_key_vault.go#L246

[Stavrus]

Thanks @katbyte! I'll repurpose this PR once the time comes.

I'm still looking into seeing if there's a way to determine when the resource has been fully provisioned, but I came across something interesting while investigating this. It seems the Azure CLI also runs into this issue and their approach is to sleep and retry on consuming resources -- in their case, role assignments. This leads me to suspect that such an API endpoint doesn't exist. I could adjust resources that take in SPNs (e.g. role assignments) to perform the same retry mechanism, but it's a bit clunky since as far as I'm aware I can't tell within the resource whether the SPN ID was passed in as a variable or as the output from the SPN resource. The former would be a waste to perform retries on. Should I still attempt to take this approach?

[katbyte]

@Stavrus,

I think it would be best to keep the retry/waiting logic in the SPN resource. I suggest doing reads in the SPN resource until the correct information is retrieved through a successful read.

[katbyte]

Hi @Stavrus,

As its been some time I'm just going to close this for now and a new PR can be opened once we are ready to move on to step two. 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 👉 [email protected]. Thanks!