-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Storage: adding a shim layer for Resource Manager #14220
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.
hi @magodo
Thanks for this PR.
I've taken a look through here and left some comments inline - so if we can work through and fix those up then this is otherwise looking good on that front. That said, unfortunately we're blocked from exposing this functionality to users until the Resource Manager API is feature-compatible, so we'll need to ensure that this functionality can't be enabled for the moment (and/or hold on this PR) until the upstream service implements this functionality. Given we've been waiting on the service to implement this functionality for an extended period (so there's no guarantee it'll be fixed anytime soon) I think it's worth doing both in this case for the moment?
Thanks!
@tombuildsstuff Thank you for the review. I've resolved most of them, while keeping some for further discussion. Meanwhile, I've found that the cross planes tests were actually not taking effect, and I've made a fix for that. 💤 TF_ACC=1 go test -v -timeout=20h ./internal/services/storage -run="TestAccStorageContainer_resourceManager|TestAccStorageContainer_dataPlane"
=== RUN TestAccStorageContainer_resourceManagerBasic
=== PAUSE TestAccStorageContainer_resourceManagerBasic
=== RUN TestAccStorageContainer_resourceManagerUpdate
=== PAUSE TestAccStorageContainer_resourceManagerUpdate
=== RUN TestAccStorageContainer_resourceManagerMetaData
=== PAUSE TestAccStorageContainer_resourceManagerMetaData
=== RUN TestAccStorageContainer_resourceManagerRoot
storage_container_resource_test.go:260:
--- SKIP: TestAccStorageContainer_resourceManagerRoot (0.00s)
=== RUN TestAccStorageContainer_resourceManagerWeb
=== PAUSE TestAccStorageContainer_resourceManagerWeb
=== RUN TestAccStorageContainer_dataPlaneThenResourceManager
=== PAUSE TestAccStorageContainer_dataPlaneThenResourceManager
=== RUN TestAccStorageContainer_resourceManagerThenDataPlane
=== PAUSE TestAccStorageContainer_resourceManagerThenDataPlane
=== CONT TestAccStorageContainer_resourceManagerBasic
=== CONT TestAccStorageContainer_resourceManagerWeb
=== CONT TestAccStorageContainer_resourceManagerThenDataPlane
=== CONT TestAccStorageContainer_dataPlaneThenResourceManager
=== CONT TestAccStorageContainer_resourceManagerMetaData
=== CONT TestAccStorageContainer_resourceManagerUpdate
--- PASS: TestAccStorageContainer_resourceManagerWeb (105.00s)
--- PASS: TestAccStorageContainer_resourceManagerBasic (106.34s)
--- PASS: TestAccStorageContainer_dataPlaneThenResourceManager (124.17s)
--- PASS: TestAccStorageContainer_resourceManagerThenDataPlane (126.36s)
--- PASS: TestAccStorageContainer_resourceManagerUpdate (135.13s)
--- PASS: TestAccStorageContainer_resourceManagerMetaData (163.88s)
PASS
ok github.com/hashicorp/terraform-provider-azurerm/internal/services/storage 163.943s
💤 TF_ACC=1 go test -v -timeout=20h ./internal/services/storage -run="TestAccStorageShare_resourceManager|TestAccStorageShare_dataPlane"
=== RUN TestAccStorageShare_resourceManagerBasic
=== PAUSE TestAccStorageShare_resourceManagerBasic
=== RUN TestAccStorageShare_resourceManagerMetaData
=== PAUSE TestAccStorageShare_resourceManagerMetaData
=== RUN TestAccStorageShare_resourceManagerAcl
storage_share_resource_test.go:244:
--- SKIP: TestAccStorageShare_resourceManagerAcl (0.00s)
=== RUN TestAccStorageShare_resourceManagerAclGhostedRecall
storage_share_resource_test.go:269:
--- SKIP: TestAccStorageShare_resourceManagerAclGhostedRecall (0.00s)
=== RUN TestAccStorageShare_resourceManagerUpdateQuota
=== PAUSE TestAccStorageShare_resourceManagerUpdateQuota
=== RUN TestAccStorageShare_resourceManagerLargeQuota
=== PAUSE TestAccStorageShare_resourceManagerLargeQuota
=== RUN TestAccStorageShare_resourceManagerNfsProtocol
=== PAUSE TestAccStorageShare_resourceManagerNfsProtocol
=== RUN TestAccStorageShare_dataPlaneThenResourceManagerMetaData
=== PAUSE TestAccStorageShare_dataPlaneThenResourceManagerMetaData
=== RUN TestAccStorageShare_resourceManagerThenDataPlaneMetaData
=== PAUSE TestAccStorageShare_resourceManagerThenDataPlaneMetaData
=== RUN TestAccStorageShare_dataPlaneThenResourceManagerAcl
storage_share_resource_test.go:395:
--- SKIP: TestAccStorageShare_dataPlaneThenResourceManagerAcl (0.00s)
=== RUN TestAccStorageShare_resourceManagerThenDataPlaneAcl
storage_share_resource_test.go:423:
--- SKIP: TestAccStorageShare_resourceManagerThenDataPlaneAcl (0.00s)
=== CONT TestAccStorageShare_resourceManagerBasic
=== CONT TestAccStorageShare_resourceManagerNfsProtocol
=== CONT TestAccStorageShare_dataPlaneThenResourceManagerMetaData
=== CONT TestAccStorageShare_resourceManagerThenDataPlaneMetaData
=== CONT TestAccStorageShare_resourceManagerUpdateQuota
=== CONT TestAccStorageShare_resourceManagerLargeQuota
=== CONT TestAccStorageShare_resourceManagerMetaData
--- PASS: TestAccStorageShare_resourceManagerNfsProtocol (108.46s)
--- PASS: TestAccStorageShare_resourceManagerBasic (110.00s)
--- PASS: TestAccStorageShare_resourceManagerLargeQuota (194.12s)
--- PASS: TestAccStorageShare_dataPlaneThenResourceManagerMetaData (204.61s)
--- PASS: TestAccStorageShare_resourceManagerThenDataPlaneMetaData (207.69s)
--- PASS: TestAccStorageShare_resourceManagerMetaData (211.44s)
--- PASS: TestAccStorageShare_resourceManagerUpdateQuota (377.89s)
PASS
ok github.com/hashicorp/terraform-provider-azurerm/internal/services/storage 377.927s
💤 TF_ACC=1 go test -v -timeout=20h ./internal/services/storage -run="TestAccStorageQueue_resourceManager|TestAccStorageQueue_dataPlane"
=== RUN TestAccStorageQueue_resourceManagerBasic
=== PAUSE TestAccStorageQueue_resourceManagerBasic
=== RUN TestAccStorageQueue_resourceManagerMetaData
=== PAUSE TestAccStorageQueue_resourceManagerMetaData
=== RUN TestAccStorageQueue_dataPlaneThenResourceManagerMetaData
=== PAUSE TestAccStorageQueue_dataPlaneThenResourceManagerMetaData
=== RUN TestAccStorageQueue_resourceManagerThenDataPlaneMetaData
=== PAUSE TestAccStorageQueue_resourceManagerThenDataPlaneMetaData
=== CONT TestAccStorageQueue_resourceManagerBasic
=== CONT TestAccStorageQueue_dataPlaneThenResourceManagerMetaData
=== CONT TestAccStorageQueue_resourceManagerMetaData
=== CONT TestAccStorageQueue_resourceManagerThenDataPlaneMetaData
--- PASS: TestAccStorageQueue_resourceManagerBasic (97.85s)
--- PASS: TestAccStorageQueue_dataPlaneThenResourceManagerMetaData (117.89s)
--- PASS: TestAccStorageQueue_resourceManagerThenDataPlaneMetaData (180.53s)
--- PASS: TestAccStorageQueue_resourceManagerMetaData (187.26s)
PASS
ok github.com/hashicorp/terraform-provider-azurerm/internal/services/storage 187.298s |
I've resolved the left two comments:
Please take another look! |
…erraform-provider-azurerm into storage_mgmt_shim_layer
(as discussed offline) we also need Resource Manager API's for the other (non Blob File/Share File) API calls too e.g. terraform-provider-azurerm/internal/services/storage/storage_account_resource.go Lines 1219 to 1244 in 3f2512d
Else we're unable to read these once Network Rules are enabled |
@magodo is there an update from the Service Team on when these fixes will go live? |
... I hit comment but 🙈 |
|
Just saw an update in the internal ticket opened for resolving this the issues I listed above. |
@tombuildsstuff Merged with the main branch, and all the related tests are passing. Though there is no news here as all those issues are still in progress, except Azure/azure-rest-api-specs#17007, that seems to be fixed in the 2021-09-01, where I have another PR to update the API version to it: #17523. Test💤 TF_ACC=1 go test -v -timeout=20h ./internal/services/storage -run="TestAccStorageShare_resourceManager|TestAccStorageShare_dataPlane"=== RUN TestAccStorageShare_resourceManagerBasic 💤 TF_ACC=1 go test -v -timeout=20h ./internal/services/storage -run="TestAccStorageContainer_resourceManager|TestAccStorageContainer_dataPlane" === RUN TestAccStorageContainer_resourceManagerBasic 💤 TF_ACC=1 go test -v -timeout=20h ./internal/services/storage -run="TestAccStorageQueue_resourceManager|TestAccStorageQueue_dataPlane" === RUN TestAccStorageQueue_resourceManagerBasic |
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
@tombuildsstuff A tiny update to the #14220 (comment). With #17523 merged, I now opt in the shim layer for the table client. However, the API has some issue, which makes the ACL update failed, which is tracked in Azure/azure-rest-api-specs#17007 (comment) (I've also asked the service team internally). |
@magodo - any updates from the service team on this? |
@katbyte Unfortunately, there is no update from the service team at the moment. |
@magodo since this is still waiting on the Service Team, rather than leaving this PR open indefinitely I'm going to temporarily close this PR for the moment - once the Service Team's shipped the changes we can re-open this / fix up the merge conflicts and look to get this merged, however. |
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. |
Adding shim mgmt layer for container, queue, shares. Due to the mgmt plane API of the table still lacks of some feature (namely the
signedIdentities
), the table resource is still data plane only.This PR also introduced a new feature toggle for the
storage
in the provider block:use_resource_manager
, which controls whether to use data plane API or the mgmt plane API for the affected resources.The affected resources include:
Remark
azurerm_storage_container
is not able to destroy a "root" container. The test case is commented. The tracking issue is: Storage mgmt API of container can't destroy a "root" container Azure/azure-rest-api-specs#16783azurerm_storage_share
doesn't work. The related test cases are commented. The tracking issue is: Storage Share Mgmt APIGET
doesn't return some set properties Azure/azure-rest-api-specs#16782StorageQueuesWrapper
to remove theUpdateServiceProperties()
andGetServiceProperties()
. The reason is that these two methods don't technically belong to the queue resource, but to the queue service. The only place that is using these two methods are in theazurerm_storage_account
. Currently, due to the mgmt API of the queue service lacks of theminuteMetrics
,hourMetrics
and thelogging
, we have to keep using the data plane API. However, once this gap disappears, we can use the mgmt API of the queue service, just as what has been done for the file service and the blob service in the implementation of theazurerm_storage_account
(where they are using the mgmt plane client to set/get service properties). Until then, we can further remove these two methods from theDataPlaneStorageQueueWrapper
.Test
Probably is able to fix issue #2977.