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

Add Azure Blob storage backend #173

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

rco-ableton
Copy link
Contributor

Allows tfmigrate to use an Azure Blob to store history.

Resolves #21.

Allows tfmigrate to use an Azure Blob to store history.
@rco-ableton rco-ableton marked this pull request as ready for review April 10, 2024 14:42
@rco-ableton
Copy link
Contributor Author

I forgot to update the table of contents again! 🙈 Fixed in 37c7e15.

@minamijoyo
Copy link
Owner

Hi @rco-ableton, Thank you for working on this! I appreciate your contribution to the missing feature for Azure users.

Before starting the code review, I'd like to discuss the user interface design and acceptance testing.

User interface design

The configuration items for s3 and gcs history storage are based on the backend section of tfstate. This is because it is a common use case for many users to store their migration history near tfstate. This does not mean all authentication methods should be implemented from the beginning. We want to start with the minimum number of authentication methods necessary, as implementing them will require testing. However, we would like to align the names of configuration items, environment variables, etc., with those of the tfstate backend unless there is a particular reason not to do so.
https://developer.hashicorp.com/terraform/language/settings/backends/azurerm

Based on the above, my proposal is as follows:

tfmigrate {
  migration_dir = "./tfmigrate"
  history {
    storage "azurerm" {
      access_key           = "<storage access key>"
      storage_account_name = "storage"
      container_name       = "tfmigrate-test"
      key                  = "tfmigrate/history.json"
    }
  }
}
  • The type of storage is azurerm instead of azure.
  • The name of the storage account is storage_account_name instead of account_name.
  • The name of the history file is key instead of blob_name. In addition, this argument should be required.
  • The access_key can be read from the environment variable ARM_ACCESS_KEY instead of TFMIGRATE_AZURE_STORAGE_ACCESS_KEY.

That said, I am an AWS user and have no experience with Azure, so please feel free to point out any mistakes or strangeness from common practice in Azure.

Acceptance testing

Acceptance testing is essential to ensure that nothing is broken so that multiple cloud providers can be supported and stable. We are currently mocking s3 with localstack and gcs with fake-gcs-server. Please take a look at docker-compose.yml for details. Is there an equivalent for Azure storage? I did some quick research and found Azurite. Do you know how about this one? Or is there any alternative? What do you think of it?
https://github.com/Azure/Azurite

@rco-ableton
Copy link
Contributor Author

Hi @minamijoyo! Apologies for the small delay in replying to you.

I'm far from an Azure expert, but your proposed interface changes make sense to me, and I've pushed some more commits to address them (we can squash all of these commits down into the original if you prefer).

As for acceptance testing, I've never tried using Azurite, but I will take a look and see if I can get it working!

@minamijoyo
Copy link
Owner

Hi @rco-ableton, thank you for working on this!

I signed up for Azure and tested this branch with the Azure blob storage. Overall, it works well, but I found some issues.

(1) The access_key attribute works, but the environment variable ARM_ACCESS_KEY is not.
(2) An error returns if the history file does not exist at the specified key, but it should be treated as an uninitialized empty file instead of an error, considering the case when history management starts. The s3 and gcs implementations do so.

README.md Show resolved Hide resolved
@rco-ableton
Copy link
Contributor Author

(1) The access_key attribute works, but the environment variable ARM_ACCESS_KEY is not.

Ah, my mistake, I completely overlooked this suggestion in your original comment. Fixed in 92a2f0a.

(2) An error returns if the history file does not exist at the specified key, but it should be treated as an uninitialized empty file instead of an error, considering the case when history management starts. The s3 and gcs implementations do so.

Sounds good! This should be fixed as of 4d8fc6c.

I've also addressed the missing edits to README.md in f88f0d2. If it's convenient for you, I'd like to rebase this branch and squash these changes into the correct commits so we have a cleaner history.

In the meantime I plan to look at getting Azurite working today 🙂

@rco-ableton
Copy link
Contributor Author

After some investigation I was able to get Azurite up and running, and have updated docker-compose.yml and test.yml to boot it and create a test storage container using the Azure CLI—see 9878d87.

I think this should be enough to start adding acceptance tests in the code itself, but I'm not sure exactly what needs to be updated. I see some references to LOCALSTACK_ENDPOINT in terraform_test.go and test_helper.go, but I'm not familiar enough with these tests to be sure how to integrate the Azure things there. I guess it would make sense to be able to select which backend is being tested (e.g. via an environment variable)? Or is there a simpler way to use the new backend?

By the way, I will be mostly on vacation throughout May, so please feel free to edit any of my changes as you see fit in the meantime—otherwise I'm happy to pick this up when I'm back!

storage/azure/client.go Outdated Show resolved Hide resolved
@minamijoyo
Copy link
Owner

Before writing automated acceptance tests, I'm unsure how to test with Azurite manually. In other words, how can we set the request endpoint to Azurite and resolve the hostname in the docker environment? Because the endpoint's hostname includes the storage account name.

After reading the document for Azurite, I found that it can change the URL format to a subpath by specifying --disableProductStyleUrl.
https://github.com/Azure/Azurite?tab=readme-ov-file#disable-product-style-url

In addition, the azure-sdk-for-go azblob client has a NewClientFromConnectionString() function that can be passed a connection string directly.
https://pkg.go.dev/github.com/Azure/azure-sdk-for-go/sdk/storage/azblob#NewClientFromConnectionString

On the other hand, in Terraform's implementation of the azurerm backend, I could not find a way to specify the connection string directly; there is a parameter called endpoint, but it seems to have a different use. Am I correct?
https://developer.hashicorp.com/terraform/language/settings/backends/azurerm#endpoint

Note that tfmigrate doesn't work with the local tfstate backend. The only way I could come up with for testing is to mock tfmigrate's azurerm history storage with azurite, while mocking terraform's s3 tfstate backend with localstack. It's a bit tricky, but it probably works.

I have yet to have time to try it out, so please correct me if I'm wrong.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request] Azure history storage
2 participants