provider/azure: added a number of storage and networking resources. #2052

aznashwan · 2015-05-22T23:17:29Z

This patch contains adaptations, additions and documentation for the following Azure resources:

DNS server refs
hosted services
local network connections
security group rules as a separate resource
storage services
storage containers
storage queues
storage blobs

To run the acceptance tests, make sure you have the appropriate environment variables the provider's creation requires, TF_ACC and an existing storage account set in the Western part of the US in AZURE_STORAGE set before running.

radeksimko · 2015-05-23T03:59:18Z

#2053

svanharmelen · 2015-05-26T20:54:26Z

builtin/providers/azure/config.go

+
+	// unfortunately; because of how Azure's network API works; doing networking operations
+	// concurrently is very hazardous, and we need a mutex.
+	mutex *sync.Mutex


I actually don't think this gives you the behaviour you would expect. Have a look at the Terraform internals around plugin communication to see for yourself, but I'll think you'll agree with me...

Once we loose the mutex here, having a custom AzureClient doesn't make sense anymore, so we could stick to just management.Client instead.

Oh yes it does, trial & error being my witness.
The mutex is wholly necessary as per how the API works; you'd get into the awful jam in which two network resources would be in a sort-of 'creation race condition', in which the last one to send its network config back to Azure would be the only one which'd get created...

I fully understand how the API works with regards to virtual networks, and indeed the API for virtual networks cannot be used concurrently in a safe way.

But did you actually check the Terraform internals with regards to plugin communication as I suggested? I don't think you did, so let me explain...

You are using a mutex to guard concurrent use of a management.Client within the plugin, to prevent problems with API endpoints that do not support being used concurrently. But there is no concurrency within the plugin itself. The concurrency is build into the core of Terraform and it will create a new instance of the plugin (most likely in it's own goroutine) for every call that can be made concurrently.

So in this case if you create a Terraform config with two virtual networks in there, Terraform core will actually call the plugin twice, resulting in two separate instances of the plugin that will then both have their own mutex.

Considering all this, you'll now probably understand that having the mutex here will not solve your problem as it will only guard an already synchronous instance of the plugin, from using the management.Client concurrently.

I just noticed you'd made a comment about this same topic on my WIP PR, so let me move this discussion over there so we discuss this only once...

aznashwan · 2015-06-02T15:04:22Z

❗ There is some duplicate work also present in this pull request.

Reviews are vastly appreciated on the non-duplicated resources; namely:

dns server definitions
hosted services
local network connections
security group rules (as separate entities)
storage services
storage blobs
storage containers

Any other elements should be considered identical to those from Sander's PR (for which reviews are also vastly appreciated).

phinze · 2015-06-03T22:55:23Z

builtin/providers/azure/resource_azure_dns_server.go

+		Exists: resourceAzureDnsServerExists,
+		Delete: resourceAzureDnsServerDelete,
+
+		SchemaVersion: 1,


No need to set this explicitly - we're letting them be zero-indexed, with a bump to 1 when the first state migration is necessary.

Noted and fixed.

phinze · 2015-06-04T01:44:19Z

builtin/providers/azure/resource_azure_local_network.go

+	vpnGateway := d.Get("vpn_gateway_address").(string)
+	var prefixes []string
+	if nprefixes := d.Get("address_space_prefixes.#").(int); nprefixes > 0 {
+		prefixes = []string{}


this line is unnecessary; append will properly handle the nil slice declared above (edit: though this comment has been since superceded by the above comment about avoiding dot-notation in d.Get calls)

Wow; was not aware of that.
Noted and fixed.

phinze · 2015-06-04T02:02:40Z

Okay - first of all, this is generally looking great!!

Here's my current review status:

  [x] dns server definitions
  [x] hosted services
  [x] local network connections
  [x] security group rules (as separate entities)
  [ ] storage services
  [ ] storage blobs
  [ ] storage containers

I decided to pause here because (a) I'm out of time 😉 and (b) I wanted to give you a chance to see and respond to some of the generally applicable recommendations without duplicating a bunch of "here too" comments.

Let me know how things look on your side and I'll check back in with you tomorrow morning. 👍

aznashwan · 2015-06-04T10:06:04Z

Thanks a lot for the review.
I've either addressed or am knee deep into addressing all of these.

I haven't pushed up the rebased branch yet as I'm ironing our some stuff + writing tests in the meantime. But I hope to get that in by the end of the day.

Consider everything you pointed out noted and fixed and thanks again for your time!

phinze · 2015-06-12T17:44:14Z

builtin/providers/azure/provider_test.go

+// has been created beforehand as the creation of one takes a lot
+// and would greatly impede the multitude of tests which rely on one.
+// NOTE: the storage container should be located in `West US`.
+var testAccStorageServiceName = os.Getenv("AZURE_STORAGE")


What's the behavior when this is not set?

In other places we've:

Had the provider's setup fail fast with a message when it's not sert.

Had the tests that rely on the value being set either skip or fail fast.

From what I can tell the value just ends up empty right now, would be nice to do something more explicit here.

Currently, all of the tests run the PreCheck function present below; which fails if the appropriate combination of environment variables used for the creation of the Provider and AZURE_STORAGE is not set.
That function is ran in each test; so absolutely no test will be run without it.

Not sure if I like the consts in this file as normally they (terraform-security-group for example) are just part of the test configs.

Having just a single call to get the AZURE_STORAGE from the env and use the var testAccStorageServiceName everywhere looks like a nice improvement! And indeed I see no problem with this approach as it's indeed tested to exist before getting to this point...

The constants are a bit dodgy; yes, but I placed them there for a couple of reasons:

it may seem superficial, but they aid the changing of these values in the rare cases where one might like to run the tests but somehow already has a terraform-security-group security group up there (it actually happened to me).

they greatly aid practicality in tests of 'sub-resources' that all stem from the same super-resource and thus there's no point in fetching that particular value for each when it's always the same for all of them (the storage elements example).

phinze · 2015-06-12T19:10:10Z

builtin/providers/azure/resource_azure_hosted_service.go

+func resourceAzureHostedServiceCreate(d *schema.ResourceData, meta interface{}) error {
+	azureClient := meta.(*Client)
+	mgmtClient := azureClient.mgmtClient
+	hostedServiceClient := hostedservice.NewClient(mgmtClient)


Thought: should we DRY up the setup required here by pre-allocating these clients and dangling them off of a struct that's passed as meta?

We do it for AWS and it seems to work out pretty well: https://github.com/hashicorp/terraform/blob/master/builtin/providers/aws/config.go

I agree, though not a blocker

Noted for the future patch (hopefully) adding a bunch more resources.
(but not on the storage sub-resources; unfortunately creating a client for them directly requires the storage service the resource will be deployed on).

phinze · 2015-06-12T19:57:40Z

This is looking really great.

Seems pretty ready to merge from my perspective. Great job!!

Hey @catsby do you think you could give this a quick once over before we press the big green button? Just for a double checking.

catsby · 2015-06-12T21:35:37Z

Quick once-over done, I think we're go for button pressing

provider/azure: added a number of storage and networking resources.

phinze · 2015-06-12T21:49:47Z

💥 🎊 😀

ghost · 2020-05-02T02:06:54Z

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.

aznashwan force-pushed the azure branch from a0a86ae to 4e92dbe Compare May 23, 2015 00:03

radeksimko mentioned this pull request May 23, 2015

WIP: Add Azure provider (peer-review PR) #2053

Merged

aznashwan force-pushed the azure branch from 4e92dbe to ed696d5 Compare May 25, 2015 19:07

svanharmelen reviewed May 26, 2015
View reviewed changes

svanharmelen added provider/azure-classic enhancement labels May 28, 2015

aznashwan force-pushed the azure branch from ed696d5 to c1e7bff Compare May 28, 2015 15:51

aznashwan force-pushed the azure branch 2 times, most recently from e6de361 to 9b97e32 Compare June 3, 2015 22:51

phinze reviewed Jun 3, 2015
View reviewed changes

phinze reviewed Jun 4, 2015
View reviewed changes

aznashwan force-pushed the azure branch from 9b97e32 to 489ee1b Compare June 10, 2015 23:17

aznashwan added 3 commits June 11, 2015 21:27

Merge resources unto upstream.

9670e69

Minor fixups, refactors and test updates.

137cb97

Added documentation.

329424c

aznashwan force-pushed the azure branch from 489ee1b to 329424c Compare June 11, 2015 18:27

aznashwan changed the title ~~WIP: Azure provider (not ready for merge)~~ provider/azure: added a number of storage and networking resources. Jun 11, 2015

Added Storage Queue resource.

ebfbef0

phinze reviewed Jun 12, 2015
View reviewed changes

aznashwan added 2 commits June 12, 2015 20:57

Removed obsolete TODO's.

bd371a3

Added documentation for running tests.

548a17d

phinze reviewed Jun 12, 2015
View reviewed changes

Removed redundant casting checks.

82a7f08

aznashwan force-pushed the azure branch from dd50a1a to 82a7f08 Compare June 12, 2015 19:25

phinze added a commit that referenced this pull request Jun 12, 2015

Merge pull request #2052 from aznashwan/azure

1ebe117

provider/azure: added a number of storage and networking resources.

phinze merged commit 1ebe117 into hashicorp:master Jun 12, 2015

ghost locked and limited conversation to collaborators May 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

provider/azure: added a number of storage and networking resources. #2052

provider/azure: added a number of storage and networking resources. #2052

aznashwan commented May 22, 2015

radeksimko commented May 23, 2015

svanharmelen May 26, 2015

aznashwan May 28, 2015

svanharmelen May 28, 2015

aznashwan commented Jun 2, 2015

phinze Jun 3, 2015

aznashwan Jun 4, 2015

phinze Jun 4, 2015

aznashwan Jun 4, 2015

phinze commented Jun 4, 2015

aznashwan commented Jun 4, 2015

phinze Jun 12, 2015

aznashwan Jun 12, 2015

svanharmelen Jun 12, 2015

aznashwan Jun 12, 2015

phinze Jun 12, 2015

catsby Jun 12, 2015

aznashwan Jun 12, 2015

phinze commented Jun 12, 2015

catsby commented Jun 12, 2015

phinze commented Jun 12, 2015

ghost commented May 2, 2020

provider/azure: added a number of storage and networking resources. #2052

provider/azure: added a number of storage and networking resources. #2052

Conversation

aznashwan commented May 22, 2015

radeksimko commented May 23, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aznashwan commented Jun 2, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phinze commented Jun 4, 2015

aznashwan commented Jun 4, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phinze commented Jun 12, 2015

catsby commented Jun 12, 2015

phinze commented Jun 12, 2015

ghost commented May 2, 2020