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

Storage Soft Delete Support #1070

Closed
rohrerb opened this issue Apr 4, 2018 · 12 comments · Fixed by #3807
Closed

Storage Soft Delete Support #1070

rohrerb opened this issue Apr 4, 2018 · 12 comments · Fixed by #3807
Assignees
Labels
enhancement service/storage upstream/microsoft Indicates that there's an upstream issue blocking this issue/PR
Milestone

Comments

@rohrerb
Copy link
Contributor

rohrerb commented Apr 4, 2018

Community Note

  • Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
  • Please do not leave "+1" or "me too" comments, they generate extra noise for issue followers and do not help prioritize the request
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment

Description

Hello,

Could we get support for Storage Soft Delete?

Seperate PR requested to add support on the Azure Go SDK - Azure/azure-sdk-for-go#1504

https://azure.microsoft.com/en-us/blog/soft-delete-for-azure-storage-blobs-now-in-public-preview/
https://docs.microsoft.com/en-us/azure/storage/blobs/storage-blob-soft-delete

Thank you.

@VaijanathB VaijanathB added the upstream/microsoft Indicates that there's an upstream issue blocking this issue/PR label Apr 4, 2018
@mcwumbly
Copy link

Just inlining some info after following this link above

Separate PR requested to add support on the Azure Go SDK - Azure/azure-sdk-for-go#1504

That issue was closed with:

The storage package in this repo is in maintenance mode, it's been replaced by the azblob package. I've ported this issue to its repo, see Azure/azure-storage-blob-go#37

And that issue was closed with:

This is supported in the 2018-03-28 service API version.

Where does that leave things here?

@genevieve
Copy link

@tombuildsstuff Would you mind if we PR this functionality?

@tombuildsstuff
Copy link
Contributor

tombuildsstuff commented Aug 24, 2018

@mcwumbly

The storage package in this repo is in maintenance mode, it's been replaced by the azblob package. I've ported this issue to its repo, see Azure/azure-storage-blob-go#37

Unfortunately we're unable to adopt the replacement library at the current time due to the way it does error handling (which is being tracked here) - once that's fixed we should be able to take a look into this.

@genevieve

@tombuildsstuff Would you mind if we PR this functionality?

Not at all, that'd be great if you wouldn't mind - however based on the comments above this would require either updates to the new Storage SDK (to remove the panic's) or the old one (to add this functionality). I've reached out to Microsoft about their suggestion here, given they own both of these SDK's.

Thanks!

@steffencircle
Copy link

Hi,
is there any update to this ?
We just identified one use-case where soft-delete would be of help for us, so it would be great if we can configure it via terraform directly.

Steffen

@geertn
Copy link

geertn commented Jan 30, 2019

FYI, according to the issue referenced above (as blocker for implementing soft delete) the panic on DecodeString is fixed.

@tombuildsstuff
Copy link
Contributor

@geertn that's true - however there's several other blocking issues preventing us from adopting that SDK at this time unfortunately: Azure/azure-storage-blob-go#86 and Azure/azure-storage-blob-go#47

@landro
Copy link

landro commented Jun 5, 2019

@tombuildsstuff , did you see the updates on Azure/azure-storage-blob-go#86 and Azure/azure-storage-blob-go#47 ? Does this basically mean that support for soft delete can be implemented in the provider in a 1-3 months timeframe?

@tombuildsstuff
Copy link
Contributor

@landro I did - unfortunately that SDK still isn't usable for us, since it (in the case of #86) only supports a single API version and thus we can't guarantee it will work in Azure Regions other than Azure Public (which may not have the latest API Version, e.g. Azure Stack, which we require for the Azure Backend).

We've recently determined that in order to ship 2.0 (#2807) we're going to need to upgrade the Storage SDK to a version which also supports context; as such we've recently started prototyping our own Storage SDK (which uses Azure/go-autorest under the hood, so is fully compatible with the existing Azure SDK for Go.

Whilst this is an approach we were hoping to avoid (insofar as we'd rather use the official SDK than maintain our own) - this appears to be the most pragmatic approach to deliver these features, and 2.0 in general for the moment.

Thanks!

@stuartleeks
Copy link
Contributor

stuartleeks commented Jul 10, 2019

I was just searching for this issue to start a conversation about whether the implementation for this should be a new resource (e.g. azurerm_storage_service_properties) or be created as a property on the existing azurerm_storage_account when I noticed that @benjamin37 has already created a PR and raised the same question!

The API reference show Get/Set calls but no delete - not sure whether that would cause an issue if it were created as a separate resource (which feels a bit cleaner to me)?

Also, in terms of shape, I was thinking something like

resource "azurerm_storage_service_properties" "test" {
  storage_account_id = "/subscriptions/..."
  delete_retention_policy = {
    enabled = true
    days = 10
  }
}

That way this could be generalised to the other properties that are supported, e.g.

resource "azurerm_storage_service_properties" "test" {
  storage_account_id = "/subscriptions/..."
  delete_retention_policy = {
    enabled = true
    days = 10
  }
  cors = {
    allowed_origins = "example.com,test.example.com"
    allowed_methods = "GET,PUT"
    max_age_in_seconds = 3600
    ...
  }
  static_website {
    enabled = true
    index_document = "index.html"
    error_document_404 = "notfound.html"
  }
}

The Get/Set API allows you to set a group of properties (e.g. cors or static_website), so preserving that grouping in Terraform should simplify usage and implementation.

@tombuildsstuff / @katbyte - would love to get your thoughts on this. I.e. Should this be nested in the storage account vs a separate resource (noting that there is no delete API)? And does the resource shape above seem reasonable?

@landro
Copy link

landro commented Nov 20, 2019

Any news regarding storage account soft delete support, @tombuildsstuff ?

katbyte pushed a commit that referenced this issue Dec 17, 2019
I was not sure if it's the right choice to create a new resource for these storage account settings instead of putting it to the storage account resources. I decided to create a separate resource since it's a separate api call and there are many settings that can be done. I'm happy for any feedback regarding this point.

I only took care of the soft delete settings of the storage account. Other settings like Cors rules can also be set with this client call. so this resource can easily be extended.

Rest API Reference: docs.microsoft.com/en-us/rest/api/storagerp/blobservices

(fixes #1070)
@katbyte katbyte added this to the v1.40.0 milestone Dec 17, 2019
@ghost
Copy link

ghost commented Jan 8, 2020

This has been released in version 1.40.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.40.0"
}
# ... other configuration ...

@ghost
Copy link

ghost commented Mar 28, 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 👉 [email protected]. Thanks!

@ghost ghost locked and limited conversation to collaborators Mar 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement service/storage upstream/microsoft Indicates that there's an upstream issue blocking this issue/PR
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants