-
Notifications
You must be signed in to change notification settings - Fork 4.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix Msi support for stream analytics job #22339
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the PR @ThreeFive-O, overall looks good but we have a test failure:
------- Stdout: -------
=== RUN TestAccStreamAnalyticsJob_jobStorageAccount_Msi
testcase.go:120: Step 2/3 error: Error running apply: exit status 1
Error: updating Streaming Job (Subscription: "*******"
Resource Group Name: "acctestRG-230706172940933015"
Streaming Job Name: "acctestjob-230706172940933015"): streamingjobs.StreamingJobsClient#Update: Failure responding to request: StatusCode=422 -- Original Error: autorest/azure: Service returned an error. Status=422 Code="422" Message="The Job Storage Account has AuthenticationMode set to MSI, but MSI has not been enabled for the streaming job. Please enable MSI on the streaming job and try again." Details=[{"code":"422","correlationId":"616d865f-8faf-6d54-ea43-7880f774757a","message":"The Job Storage Account has AuthenticationMode set to MSI, but MSI has not been enabled for the streaming job. Please enable MSI on the streaming job and try again.","requestId":"87b8abb6-b1fc-438c-b248-748dc58690fe"}]
with azurerm_stream_analytics_job.test,
on terraform_plugin_test.tf line 30, in resource "azurerm_stream_analytics_job" "test":
30: resource "azurerm_stream_analytics_job" "test" {
--- FAIL: TestAccStreamAnalyticsJob_jobStorageAccount_Msi (191.14s)
FAIL
@katbyte Forgot to add the system managed identity in the test block. Also used gofmt on the two GO files. Please let the tests run again. |
@katbyte Could you please run the CI to make sure there are no more test errors. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for updating @ThreeFive-O. The tests look good (see below), however I wonder if there's a way we can accommodate both Create and Update operations here, by requiring/allowing account_key
along with authentication_mode = "Msi"
when creating a new resource, then handling the change of authentication setup as part of the Create operation (e.g. create it with ConnectionString
then switch it to Msi
)? This would avoid requiring the user to perform a two-step apply which is something we try to avoid wherever possible. WDYT?
@manicminer Unfortunately I don't know how to solve the issue for "Stream analytics job with MSI with job storage account"
Now if we want to combine step 2 and 4 into one logic as you suggest, there would have to be a long polling on the storage account until it receives the required RBAC permission for the MSI which is step 3. Idea: If you have any pointers on how to solve such a deadlock scenario please let me know. |
@ThreeFive-O Sorry, I overlooked the permissions aspect of configuring the storage account. In this case, I think we'd need to look at having a virtual resource for configuring the job storage account for the stream analytics job. This would enable configurations with a dependency on the role assignment, for example: resource "azurerm_resource_group" "test" {
name = "acctestRG-%d"
location = "%[3]s"
}
resource "azurerm_storage_account" "test" {
name = "acctestacc%[2]s"
resource_group_name = azurerm_resource_group.test.name
location = azurerm_resource_group.test.location
account_tier = "Standard"
account_replication_type = "LRS"
}
resource "azurerm_stream_analytics_job" "test" {
name = "acctestjob-%[4]d"
resource_group_name = azurerm_resource_group.test.name
location = azurerm_resource_group.test.location
streaming_units = 3
content_storage_policy = "JobStorageAccount"
job_storage_account {
account_name = azurerm_storage_account.test.name
account_key = azurerm_storage_account.test.primary_access_key
}
identity {
type = "SystemAssigned"
}
transformation_query = <<QUERY
SELECT *
INTO [YourOutputAlias]
FROM [YourInputAlias]
QUERY
lifecycle {
ignore_changes = [job_storage_account]
}
}
resource "azurerm_role_assignment" "test" {
scope = azurerm_storage_account.test.id
role_definition_name = "Storage Blob Data Contributor"
principal_id = azurerm_stream_analytics_job.test.identity[0].principal_id
}
resource "azurerm_stream_analytics_job_storage_account" "test" {
authentication_mode = "Msi"
account_name = azurerm_storage_account.test.name
} It might be necessary to use |
@manicminer I was thinking something along those lines but I wasn't sure how to implement such a virtual resource. As regards to your code suggestion, a reference to the stream analytics job is also required for the virtual resource. resource "azurerm_stream_analytics_job_storage_account" "test" {
authentication_mode = "Msi"
account_name = azurerm_storage_account.test.name
stream_analytics_job_id = azurerm_stream_analytics_job.test.id
} Now two questions:
|
There are quite a lot of virtual resources around the provider you can look at for reference, e.g. azurerm_virtual_network_dns_servers We should use the ID for reference where possible, the use of resource group / name is a legacy pattern. We likely don't want to handle the role assignment here, this would cross boundaries of responsibility and also should be up to the practitioner to decide and configure explicitly. |
@manicminer Thank you for the pointers and your help |
@ThreeFive-O Just checking in to see if you've maybe had time to work on this further? |
@ThreeFive-O any chance you'd had another chance to look at this one? |
@tombuildsstuff Unfortunately not. I was assigned to another project and couldn't follow up. |
Closing since this would require re-work and no one in the team currently has capacity to look into it. Support for this will be tracked in #19351. |
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
Fix #19152
Fix #19351
Allow
Msi
option forjob_storage_account
authentication_mode
.A stream analytics job can't be directly created using
Msi
option because of cyclic dependency between job and storage account, it can be changed latter on once the job exists.Therefore
Msi
should be supported.This PR adds the functionality and even throws an error when using
Msi
option in the create phase.