[azurerm_batch_account] - expose required properties when pool_allocation_mode is UserSubscription

[jcorioland]

This PR will fix the bug described in #2793 when user try to create an Azure Batch account with UserSubscription allocation mode.

The resource / data source did not defined the Azure Keyvault reference which is required for that mode.

• Add repro in acceptance tests
• Update batch account resource
• Update the batch account data source
• Add docs for UserSubscription mode

[jcorioland]

@tombuildsstuff @jeffreyCline @katbyte would like your feedback on this:

When deploying an Azure Batch Account in User Subscription mode, a KeyVault is required. Also, the "Microsoft Azure Batch" service principal should be able to access the KeyVault secret, and the KeyVault should be enabled for deployment.

I've documented everything here, with the full TF script that works perfectly.

My problem is for acceptance tests. If I want to be able to create the keyvault in the test, I need to be able to use the AzureAD provider, to get a reference to the "Microsoft Azure Batch" service principal:

data "azuread_service_principal" "batchsp" {
  display_name = "Microsoft Azure Batch"
}

Is it a way to include another provider in the tests sequence?

I did not find a solution, so right now, I've written the tests with a data source force the key vault, assuming that the key vault exists before starting the test and that it has been configured to be usable by Azure Batch. But I am not happy with that.

Any idea how to solve this dependency in the test and make sure it can be run on any subscription from scratch?

Thanks!

[jcorioland]

Hmm checks are failing on:

azurerm/data_source_redis_cache.go:247:43: meta.(*ArmClient).redisPatchSchedulesClient undefined (type *ArmClient has no field or method redisPatchSchedulesClient)
make: *** [test] Error 1
The command "if [[ $MODE == 'unit-tests' ]]; then make test; fi" exited with 2.

¯\(ツ)/¯

[maniSbindra]

Receiving same checks error on my PR #3554 as well. I created a new branch tracking source master (commit d66f52a), and I am getting the same error locally as well.

[stuartleeks]

@jcorioland - see here for the build failure

I got the same issue when rebasing my WIP and that PR seems to fix it. Am hoping that it is reviewed/merged soon :-)

[katbyte]

hi @jcorioland, i've fixed the build but am now getting a test error:

------- Stdout: -------
=== RUN   TestAccDataSourceAzureRMBatchAccount_userSubscription
=== PAUSE TestAccDataSourceAzureRMBatchAccount_userSubscription
=== CONT  TestAccDataSourceAzureRMBatchAccount_userSubscription
--- FAIL: TestAccDataSourceAzureRMBatchAccount_userSubscription (3.48s)
    testing.go:568: Step 0 error: errors during refresh:
        
        Error: KeyVault "azurebatchkv" (Resource Group "batch-custom-img-rg") does not exist
        
          on /opt/teamcity-agent/temp/buildTmp/tf-test195042324/main.tf line 8:
          (source code not available)
        
        
FAIL

[katbyte]

see above comment

[jcorioland]

@katbyte - thanks for the review.

Regarding the tests, this is exactly what I was mentioning in this comment and on the discussion I am having on slack with @tombuildsstuff.

That's being said, I've fixed them by using the deprecated service principal data source:

data "azurerm_azuread_service_principal" "test" {
	display_name = "Microsoft Azure Batch"
}

This allows me to create the keyvault, do the role assignment that are needed in order to create an Azure Batch account in User Subscription mode.

But, best way IMO would be to import the Azure AD provider in tests. I am just not sure how to do it :) If we don't, the test will break when moving to provider v2 anyway. Could you give me any guidance on how to import the Azure AD provider in tests?

[katbyte]

Hi @jcorioland,

I spent some time over the weekend getting things ready to use the AzureAD provider in AzureRM tests. I've opened #3631 to that effect and once its merged you should be able to use it here 🙂

[jcorioland]

hi @tombuildsstuff - I've done updates according to the changes you've requested. Should be good now! :)

[katbyte]

Hi @jcorioland,

Running the tests for this today i'm getting:

------- Stdout: -------
=== RUN   TestAccDataSourceAzureRMBatchAccount_userSubscription
=== PAUSE TestAccDataSourceAzureRMBatchAccount_userSubscription
=== CONT  TestAccDataSourceAzureRMBatchAccount_userSubscription
--- FAIL: TestAccDataSourceAzureRMBatchAccount_userSubscription (256.59s)
    testing.go:568: Step 0 error: errors during apply:
        
        Error: authorization.RoleAssignmentsClient#Create: Failure responding to request: StatusCode=409 -- Original Error: autorest/azure: Service returned an error. Status=409 Code="RoleAssignmentExists" Message="The role assignment already exists."
        
          on /opt/teamcity-agent/temp/buildTmp/tf-test057922371/main.tf line 38:
          (source code not available)
        
        
FAIL

🤔

Might be a problem on our side. I'll try and dig into it later today but any pointers on where to look would be helpful!

[jcorioland]

@katbyte - hmm not sure how to solve that.
This is due to the fact that I need to make sure that the "Microsoft Azure Batch" service principal (which exist in any Azure AD organization) is authorized on the current Azure subscription.

In order to do that, I do the following:

data "azurerm_azuread_service_principal" "test" {
	display_name = "Microsoft Azure Batch"
}

resource "azurerm_role_assignment" "contribrole" {
	scope                = "/subscriptions/%s"
	role_definition_name = "Contributor"
	principal_id         = "${data.azurerm_azuread_service_principal.test.object_id}"
}

BUT, if in your case, the service principal is already contributor on the subscription for any reason, then the role assignment fails because it already exists.

I am not sure how to deal with that. How can I translate in HCL "apply role assignment only if it does not exist already" ?

[jcorioland]

Hey @katbyte @tombuildsstuff ! Was nice to catch up with you at HashiConf this week Tom! From the discussion we had together, I understood you are not waiting for me to change anything on this PR right now. If I mistunderstood, please tell me :) Thanks!

[katbyte]

@jcorioland,

We need to fix the test, either by assuming the role assignment already exists, or possibly use a more restrivie scope such as a resource group?

[jcorioland]

Hi @katbyte,

Unfortunately, the role assignment has to be done at the subscription scope, otherwise:

* azurerm_batch_account.test: Error creating Batch account "testaccbatchdsdsds" (Resource Group "testaccRG-1234-batchaccount"): batch.AccountClient#Create: Failure sending request: StatusCode=400 -- Original Error: Code="InsufficientPermissions" Message="The Batch service does not have the required permissions to access the specified Subscription.\nRequestId:d5ed074f-8b1a-4aec-974c-198ddd11d9ec\nTime:2019-07-12T07:30:32.9063507Z" Target="BatchAccount"

Also documented here.

To move forward, I agree with you, let's assume that the role assignment is created before running tests/terraform.

That's being said, I have:

• Removed the assignment from the test
• Updated the documentation to add a not about this requirement

LMK if it's look good to you.
Thanks!

[katbyte]

Thanks for the revisions @jcorioland! LGTM now 🙂

[ghost]

This has been released in version 1.32.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 = "~> 1.32.0"
}
# ... other configuration ...

[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!