Azure ARM Support

[jen20]

This is a refactoring of the work begun by @aznashwan in #3808. for ARM support (discussion started in #3212).

It was decided that for usability purposes, the ARM API would be added as a new provider. That work is on this branch, along with two of the resources and the requisite documentation. Further work will go onto this branch before merging into master when it is sufficiently useful, and when there is a sufficiently robust set of acceptance tests which can be run during the nightly Travis builds in the same manner as the AWS work done by @phinze.

[alexpilotti]

@jen20 there seems to be a significant copy paste from this PR #3808

Now, not that it really matters, but it would be nice if the commits were done by the original authors of the code. :-)

Due to the nature of this "fork" over the original PR, IMO it would be more correct to keep the previous PR open and refactor the code there.

[jen20]

Hi @alexpilotti - I'm not at all opposed to giving credit for the original work using git commit --amend --author - that is quite appropriate. It turns out that refactoring into a separate provider involves reverting a lot of renames etc if you start from the work on #3808 - that was my original approach.

[alexpilotti]

@jen20 the amended commit author on the patches is ok. It'd be also great to hear @aznashwan's opinion on the refactoring of his code in the form of a discussion on the PR. This is for the sake of getting the best possible combined results and benefit for the users. I'm definitely not opposing the refactoring that you are "suggesting", but some input in the changes at a first glance could be useful.

[jen20]

The reasons for separating this out into a separate provider are primarily user-experience related:

• using ARM, resource_group_name becomes mandatory on anything. We couldn't reflect this in the schema for a single provider without additional ValidateFuncs everywhere, and the original code marked them as optional, leading to badly constructed URIs if it isn't specified. The helper/schema system doesn't have sufficiently complex validation to make this friendly for users - I only found the error after setting up SSL proxying!
• since additional parameters are often required, it isn't realistic to just change out credentials and switch between ASM and ARM in any case - anyone who has adopted the service management provider in Terraform will have some work to do to migrate over to ARM, and so switching out the provider name and the resource prefix doesn't really represent a huge amount of additional work given that.
• the extra indirection of the Choice* range of functions is unnecessarily complex - for both future contributors and the ability to later remove the Service Management code once ARM coverage is complete.
• we significantly reduce the risk of regression both now and in the future by leaving the azure provider alone and separating out the ARM resources.

In addition there are a few other things included on the f-azure-arm branch:

• we now wait for resources to transition to the Succeeded state before proceeding. This is a foundational principle of Terraform providers - they must block until resources are in service. It becomes especially important for compute resources where provisioners must be able to connect, but I observed eventual consistency artefacts even with Virtual Networks under test without that check being present.
• bug fixes to some optional parameters have been made - as an example, it is not required to set a network security group for a subnet in a virtual network, but the original code made bad requests if one was not set.
• bug fixes to authentication for the resource group client mean it is now possible to create resource groups (required to create any other type of resource using ARM unless they are created outside Terraform)
• state is saved correctly for resources upon creation - the original PR did not call read or update after resource creation, leading to bad future plans.

[alexpilotti]

@jen20 as I said I'm not opposing the content of your changes. It's a relatively large PR and as such we were hoping and expecting such comments. The part on which I don't agree is the workflow:

in a typical OSS process a reviewer comments on the commits in a PR and leaves a few days for the author to respond and / or implement the requested changes. If the author does not respond in a reasonable amount of time, it makes of course sense to proceed otherwise.

What happened here, is that we submitted a PR (#3808) that nobody reviewed for a month, just to see it closed and copy / pasted in a different one (changing also the commits author in the process) without any review, which is not particularly correct IMO.

Said that, considering that the goal is to get the best possible Azure support in Terraform, I'm personally ok in continuing our work based on this "forked" PR, provided of course that the commits are amended to match the original author and that a better review dialog is maintained for further activities.

[xied75]

Greetings to all. It seems there isn't much love of Azure here anyway (compared with the vast surface of aws work on TF). Let's "scrum" and move fast. :)

BTW, how many people actually have commit rights for Azure related issues? It will be great to know to speed up things.

[phinze]

Hi @xied75, thanks for the comment. It's true that AWS has more activity today, but Azure is definitely on the upswing! Currently we're having HashiCorp core team members facilitate review and merge of Azure-related code, but we're always keeping an eye out for active community members to invite to the core team. It's important that we get this base ARM structure laid down properly, but once that's in we'll be able to move more quickly by parallelizing across resources.

[jen20]

Hi @alexpilotti! We discussed internally and decided the scope of the changes was such that it was easier to make them internally than to communicate across the probably 3 or 4 round trips that would be required. The authorship of the original move across to this branch has been adjusted to accurately reflect @aznashwan's work (along with some of the changes which got it into a working state).

If @aznashwan has some time to review this prior to merge that would be appreciated!

Now that we have base support for the provider in place, it should be reasonably straightforward to proceed on the resources on a piecemeal basis on individual pull requests which are much easier to review and feed back on using the normal processes observed in the Terraform repository.

Once again though, we'd like to thank @aznashwan and others involved at CloudBase for the contribution - it's a strong start to getting complete coverage of the the ARM API!

[jen20]

It turns out the upstream Azure/azure-sdk-for-go has some issues which look unlikely to be resolved in the time frame we're talking about. I've taken the (temporary) measure of forking the upstream to hashicorp/azure-sdk-for-go, and done the work associated with import paths. This only affects the azurerm provider - the azure provider using the service management API is unchanged.

[aznashwan]

Hello everyone!

First off; I do very much appreciate the authorship acknowledgements (I'd even argue that there is extra code accidentally attributed to me, @jen20), though it was never really about that.
My main pain point is the man-hours poured into the hefty refactor and the seemingly overnight decision to revert to the separate provider model (which happened so fast it just rubbed in the feeling of wasted work a little).

Overall, I'd still argue that having the two providers merged would not have been that user unfriendly (realistically speaking, there is only the added resource_group_name parameter on the 6 overlapping resources which simply ConflictsWith: []string{"use_asm_api"}, the rest of which can be really easily handled through code...). I do very much, however, respect your [I'm sure not at all easy to take] decision of switching architectures while it's still remotely feasible...

Either way, I am indeed happy to see things progressing rapidly once more, and will lay out a handful of notes I have compiled on this PR (mostly small things). I'll also be adapting the handful of resources not pulled in here in anticipation of this one's merging and the true resource-ing beginning 😄

[aznashwan]

pathorcontents.Read gives back the contents as its first return value, so the variable's name is misleading...
Also, as @jen20 pointed out on my old PR, wasPath should have been checked and a proper warning returned if it was indeed an unsourced file.

[jen20]

OK, it seems that Azure/azure-sdk-for-go#246 has been fixed, so we can remove our fork of the Azure Go SDK. My guess is that MS have turned on the preview endpoint again, which is fine, at least in the short term. There should only be one last set of changes (based on @aznashwan's feedback as well as some additional work) before we are good to merge this stuff and we'll have reasonable exemplars for resource types.

[jen20]

Just rebased this onto master to resolve some conflicts. The acceptance tests are now passing and resources track the natural Azure ID rather than the name. There is still some tidying up to do, however.

[jen20]

After clean up work:

make testacc TEST=./builtin/providers/azurerm
go generate ./...
TF_ACC=1 go test ./builtin/providers/azurerm -v  -timeout 90m
=== RUN TestProvider
--- PASS: TestProvider (0.00s)
=== RUN TestProvider_impl
--- PASS: TestProvider_impl (0.00s)
=== RUN TestAccAzureRMResourceGroup_basic
--- PASS: TestAccAzureRMResourceGroup_basic (51.89s)
=== RUN TestAccAzureRMVirtualNetwork_basic
--- PASS: TestAccAzureRMVirtualNetwork_basic (57.15s)
=== RUN TestParseAzureResourceID
--- PASS: TestParseAzureResourceID (0.00s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/azurerm    109.056s

[phinze]

This is unnecessary, yeah?

[jen20]

It is - following the pattern of all other provider bins though. We should probably get rid of all of them.

[phinze]

+1 lets omit it here and circle around to remove the others

[phinze]

LGTM!

[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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.