Skip to content
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

Updates azurerm_storage_blob on content_md5 changes #7786

Merged
merged 10 commits into from Nov 19, 2020
Merged

Updates azurerm_storage_blob on content_md5 changes #7786

merged 10 commits into from Nov 19, 2020

Conversation

Humoiz
Copy link
Contributor

@Humoiz Humoiz commented Jul 16, 2020

Implementing feature mentioned in the feature request: #1990

Problem statement: Currently in order to update the blob, user has to change the blob name to trigger the update.

This PR adds new field content_md5 to have azurerm_storage_blob update on content_md5 changes. It forces a new deployment if the value for content_md5 is changed. This is only supported for blob type Block.

(fixes #1990)

@Stanpol
Copy link

Stanpol commented Jul 21, 2020

@Humoiz could you please request a review from @katbyte?
It would be great to have this PR merged in the coming milestones.

@Humoiz
Copy link
Contributor Author

Humoiz commented Jul 21, 2020

@katbyte - Can you please review this PR?

@jackofallops
Copy link
Member

Hi @Humoiz - Apologies, I somehow missed the notification on this one. Could you add a passing test to cover this new functionality and show it working? (there's a few examples elsewhere in the provider for including a small file as testdata)

Thanks

@3mard
Copy link

3mard commented Sep 14, 2020

Hi @Humoiz
Thanks for your great work, if you are busy I can take care of the testing

@ghost ghost removed the waiting-response label Sep 14, 2020
@Humoiz
Copy link
Contributor Author

Humoiz commented Sep 14, 2020

Hi @Humoiz
Thanks for your great work, if you are busy I can take care of the testing

Thanks! I will address the testing this week.

@ghost ghost added size/L and removed size/M labels Sep 19, 2020
@Humoiz
Copy link
Contributor Author

Humoiz commented Sep 19, 2020

Hi @Humoiz - Apologies, I somehow missed the notification on this one. Could you add a passing test to cover this new functionality and show it working? (there's a few examples elsewhere in the provider for including a small file as testdata)

Thanks

Added the tests.

Copy link
Collaborator

@katbyte katbyte left a 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 @Humoiz - overall this looks good but these tests are failing due to public accessnot being allowed on the storage account:


TestAccAzureRMStorageBlob_blockFromInlineContent
--
TestAccAzureRMStorageBlob_blockFromPublicBlob
TestAccAzureRMStorageBlob_requiresImport
TestAccAzureRMStorageBlob_updateBlockFromInlineContent


should be a simple config change to fix this 🙂

@Humoiz
Copy link
Contributor Author

Humoiz commented Sep 21, 2020

TestAccAzureRMStorageBlob_updateBlockFromInlineContent

@katbyte - Thank you for pointing this out. I have updated TestAccAzureRMStorageBlob_updateBlockFromInlineContent test to have the container blob level access. I have not updated the other tests as they are existing tests and not in scope for this PR.

@ghost ghost removed the waiting-response label Sep 21, 2020
@Humoiz Humoiz requested a review from katbyte September 21, 2020 00:16
Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Humoiz - i think its the flag that needs to be changed:

=== CONT  TestAccAzureRMStorageBlob_blockEmptyAzureADAuth
    TestAccAzureRMStorageBlob_updateBlockFromInlineContent: testing.go:684: Step 0 error: errors during apply:
        
        Error: failed creating container: containers.Client#Create: Failure sending request: StatusCode=409 -- Original Error: autorest/azure: Service returned an error. Status=<nil> Code="PublicAccessNotPermitted" Message="Public access is not permitted on this storage account.\nRequestId:2ee4abf8-601e-0005-1f61-903875000000\nTime:2020-09-21T21:53:23.8541083Z"
        
          on /opt/teamcity-agent/temp/buildTmp/tf-test589256406/main.tf line 16:
          (source code not available)
        
        
    TestAccAzureRMStorageBlob_blockFromInlineContent: testing.go:684: Step 0 error: errors during apply:
        
        Error: failed creating container: containers.Client#Create: Failure sending request: StatusCode=409 -- Original Error: autorest/azure: Service returned an error. Status=<nil> Code="PublicAccessNotPermitted" Message="Public access is not permitted on this storage account.\nRequestId:3ad9340a-001e-0075-2f61-90d775000000\nTime:2020-09-21T21:53:23.9088950Z"
        
          on /opt/teamcity-agent/temp/buildTmp/tf-test588258110/main.tf line 16:
          (source code not available)
        
        
    TestAccAzureRMStorageBlob_blockFromPublicBlob: testing.go:684: Step 0 error: errors during apply:
        
        Error: failed creating container: containers.Client#Create: Failure sending request: StatusCode=409 -- Original Error: autorest/azure: Service returned an error. Status=<nil> Code="PublicAccessNotPermitted" Message="Public access is not permitted on this storage account.\nRequestId:e3cfbba0-101e-0054-0661-90cddc000000\nTime:2020-09-21T21:53:23.8093032Z"
        
          on /opt/teamcity-agent/temp/buildTmp/tf-test884844187/main.tf line 16:
          (source code not available)
        
        
    TestAccAzureRMStorageBlob_blockFromLocalFileWithContentMd5: testing.go:684: Step 0 error: errors during apply:
        
        Error: failed creating container: containers.Client#Create: Failure sending request: StatusCode=409 -- Original Error: autorest/azure: Service returned an error. Status=<nil> Code="PublicAccessNotPermitted" Message="Public access is not permitted on this storage account.\nRequestId:70ee7031-e01e-00c9-6f61-90f196000000\nTime:2020-09-21T21:53:48.5732074Z"
        
          on /opt/teamcity-agent/temp/buildTmp/tf-test894169348/main.tf line 16:
          (source code not available)
        
        
    TestAccAzureRMStorageBlob_requiresImport: testing.go:684: Step 0 error: errors during apply:
        
        Error: failed creating container: containers.Client#Create: Failure sending request: StatusCode=409 -- Original Error: autorest/azure: Service returned an error. Status=<nil> Code="PublicAccessNotPermitted" Message="Public access is not permitted on this storage account.\nRequestId:68d53229-101e-0038-3561-90c398000000\nTime:2020-09-21T21:53:48.4836020Z"
        
          on /opt/teamcity-agent/temp/buildTmp/tf-test072062594/main.tf line 16:
          (source code not available)

@Humoiz
Copy link
Contributor Author

Humoiz commented Sep 22, 2020

@Humoiz - i think its the flag that needs to be changed:

=== CONT  TestAccAzureRMStorageBlob_blockEmptyAzureADAuth
    TestAccAzureRMStorageBlob_updateBlockFromInlineContent: testing.go:684: Step 0 error: errors during apply:
        
        Error: failed creating container: containers.Client#Create: Failure sending request: StatusCode=409 -- Original Error: autorest/azure: Service returned an error. Status=<nil> Code="PublicAccessNotPermitted" Message="Public access is not permitted on this storage account.\nRequestId:2ee4abf8-601e-0005-1f61-903875000000\nTime:2020-09-21T21:53:23.8541083Z"
        
          on /opt/teamcity-agent/temp/buildTmp/tf-test589256406/main.tf line 16:
          (source code not available)
        
        
    TestAccAzureRMStorageBlob_blockFromInlineContent: testing.go:684: Step 0 error: errors during apply:
        
        Error: failed creating container: containers.Client#Create: Failure sending request: StatusCode=409 -- Original Error: autorest/azure: Service returned an error. Status=<nil> Code="PublicAccessNotPermitted" Message="Public access is not permitted on this storage account.\nRequestId:3ad9340a-001e-0075-2f61-90d775000000\nTime:2020-09-21T21:53:23.9088950Z"
        
          on /opt/teamcity-agent/temp/buildTmp/tf-test588258110/main.tf line 16:
          (source code not available)
        
        
    TestAccAzureRMStorageBlob_blockFromPublicBlob: testing.go:684: Step 0 error: errors during apply:
        
        Error: failed creating container: containers.Client#Create: Failure sending request: StatusCode=409 -- Original Error: autorest/azure: Service returned an error. Status=<nil> Code="PublicAccessNotPermitted" Message="Public access is not permitted on this storage account.\nRequestId:e3cfbba0-101e-0054-0661-90cddc000000\nTime:2020-09-21T21:53:23.8093032Z"
        
          on /opt/teamcity-agent/temp/buildTmp/tf-test884844187/main.tf line 16:
          (source code not available)
        
        
    TestAccAzureRMStorageBlob_blockFromLocalFileWithContentMd5: testing.go:684: Step 0 error: errors during apply:
        
        Error: failed creating container: containers.Client#Create: Failure sending request: StatusCode=409 -- Original Error: autorest/azure: Service returned an error. Status=<nil> Code="PublicAccessNotPermitted" Message="Public access is not permitted on this storage account.\nRequestId:70ee7031-e01e-00c9-6f61-90f196000000\nTime:2020-09-21T21:53:48.5732074Z"
        
          on /opt/teamcity-agent/temp/buildTmp/tf-test894169348/main.tf line 16:
          (source code not available)
        
        
    TestAccAzureRMStorageBlob_requiresImport: testing.go:684: Step 0 error: errors during apply:
        
        Error: failed creating container: containers.Client#Create: Failure sending request: StatusCode=409 -- Original Error: autorest/azure: Service returned an error. Status=<nil> Code="PublicAccessNotPermitted" Message="Public access is not permitted on this storage account.\nRequestId:68d53229-101e-0038-3561-90c398000000\nTime:2020-09-21T21:53:48.4836020Z"
        
          on /opt/teamcity-agent/temp/buildTmp/tf-test072062594/main.tf line 16:
          (source code not available)

@katbyte - The existing and new storage tests fails on my local environment. Unfortunately the failure does not give me any details on why it failed so I can not tests my new test changes. However, the same configuration works for me when ran outside of the tests. It would be helpful if you can exactly point me in the code on what needs to be changed to have these tests pass.

This is the configuration I am using in the new tests and it works when I run the configuration outside of the tests.

resource "azurerm_resource_group" "test" {
  name     = "acctestRG"
  location = "West US"
}

resource "azurerm_storage_account" "test" {
  name                     = "acctestacc2de4"
  resource_group_name      = azurerm_resource_group.test.name
  location                 = azurerm_resource_group.test.location
  account_tier             = "Standard"
  account_replication_type = "LRS"
}

resource "azurerm_storage_container" "test" {
  name                  = "test"
  storage_account_name  = azurerm_storage_account.test.name
  container_access_type = "blob"
}

resource "azurerm_storage_blob" "test" {
  name                   = "example.vhd"
  storage_account_name   = azurerm_storage_account.test.name
  storage_container_name = azurerm_storage_container.test.name
  type                   = "Block"
  source_content         = "Wubba Lubba Dub Dub"
  content_md5            = "${md5("Wubba Lubba Dub Dub")}"
}

@Humoiz Humoiz requested a review from katbyte October 8, 2020 07:19
@Stanpol

This comment has been minimized.

@berberich

This comment has been minimized.

@3mard

This comment has been minimized.

@jackofallops
Copy link
Member

Apologies this has taken so long to get back to. I'll be looking at the this later this week.

@ghost ghost added size/M and removed size/L labels Nov 11, 2020
Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM @jackofallops 👍

Copy link
Member

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey @Humoiz

Apologies for the delayed re-review here!

Taking a look through this mostly LGTM - based on the Giovanni SDK conditionally setting the checksum based on whether the value is non-nil - I believe we'll need to conditionally set the ContentMD5 field rather than always setting this to an empty string, incase the behaviour of the API changes - WDYT? That said, if we can fix up the remaining minor comments then this otherwise LGTM 👍

Thanks!

azurerm/internal/services/storage/blobs.go Outdated Show resolved Hide resolved
azurerm/internal/services/storage/blobs.go Outdated Show resolved Hide resolved
azurerm/internal/services/storage/blobs.go Show resolved Hide resolved
website/docs/r/storage_blob.html.markdown Outdated Show resolved Hide resolved
azurerm/internal/services/storage/blobs.go Outdated Show resolved Hide resolved
azurerm/internal/services/storage/blobs.go Outdated Show resolved Hide resolved
* Conditionally setting the ContentMD5 field in requests, whilst overkill
  this avoids an expectation on the behaviour of the API ignoring an empty
  string, since Giovanni conditionally sends this if it's not-nil
* Fixes a typo in the docs
* Wraps a couple of exceptions to be clearer
@tombuildsstuff
Copy link
Member

@Humoiz I hope you don't mind, but so that we can get this merged into this release, I've pushed a commit to resolve the comments above

@tombuildsstuff tombuildsstuff dismissed their stale review November 19, 2020 11:00

dismissing since changes have been pushed

Copy link
Member

@jackofallops jackofallops left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@tombuildsstuff tombuildsstuff added this to the v2.37.0 milestone Nov 19, 2020
@tombuildsstuff
Copy link
Member

Tests look good 👍

@tombuildsstuff tombuildsstuff merged commit f6b8185 into hashicorp:master Nov 19, 2020
tombuildsstuff added a commit that referenced this pull request Nov 19, 2020
@Humoiz Humoiz deleted the hussain/storage_blob_etag_changes branch November 19, 2020 13:59
@jpveritone
Copy link

I'm using azure provider version 1.44.0. Will this only be available in latest 2.x version?

@ghost
Copy link

ghost commented Nov 20, 2020

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

@3mard
Copy link

3mard commented Nov 20, 2020

@jpveritone I believe so you need to be on version >= 2.37.0

@ghost
Copy link

ghost commented Dec 19, 2020

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!

@hashicorp hashicorp locked as resolved and limited conversation to collaborators Dec 19, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Have azurerm_storage_blob updates on etag/hash changes
8 participants