-
Notifications
You must be signed in to change notification settings - Fork 2
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
Azure backups #2
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.
Great work here, Sam! Thanks for adding this.
I've left a few comments mostly related to auth and the configuration UX.
Let me know if you need any help implementing the changes and I'll be happy to help!
cmd/e2d/app/run.go
Outdated
AzureAccountName string `env:"E2D_AZURE_ACCOUNT_NAME"` | ||
AzureAccountKey string `env:"E2D_AZURE_ACCOUNT_KEY"` | ||
AzureStorageAccount string `env:"E2D_AZURE_STORAGE_ACCOUNT"` |
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.
What's the difference between AccountName and StorageAccount? It looks like you're reusing the S3 Bucket name for the ContainerName.
My preference would be to allow using URLs like https://<accountname>.blob.core.windows.net/<containername>
and have a regex extract the appropriate parts.
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.
Rather than specifying a URL like that I reused the SnapshotBackupURL which looks similar except it starts with azure://
pkg/snapshot/snapshot_azure.go
Outdated
return nil, err | ||
} | ||
|
||
url := fmt.Sprintf("https://%s.blob.core.windows.net/", config.StorageAccount) |
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.
Ideally we wouldn't build this URL since some users will be using Azure Government, which will use URLs like blob.core.usgovcloudapi.net
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.
This is now set in SnapshotBackupURL. The URL still gets "built" but the hostname is fully customizable.
@@ -52,6 +52,12 @@ type runOptions struct { | |||
AWSSecretKey string `env:"E2D_AWS_SECRET_KEY"` | |||
AWSRoleSessionName string `env:"E2D_AWS_ROLE_SESSION_NAME"` | |||
|
|||
AzureAccountName string `env:"E2D_AZURE_ACCOUNT_NAME"` | |||
AzureAccountKey string `env:"E2D_AZURE_ACCOUNT_KEY"` |
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.
Azure recommends avoiding using Account Keys since they're not unique to a user, and instead using Managed Service Identities.
Unfortunately, like everything Azure, it's not super straightforward to use a MSI since there's multiple types of MSIs:
- System Managed Identities - these are assigned at instance start, and the wireserver will automatically issue this identity to you without any further configuration
- User Managed Identities - you can assign one or more user managed identities to a host, and when requesting tokens from the wireserver, you need to specify the UUID of the identity you wish to get credentials for if there are multiple user managed identities assigned to the host
It might be good to just add a --azure-msi-id
flag to use a specific UUID if the system managed identity isn't sufficient.
There's no need to remove the account key flag, since some users (not us) may likely be using the legacy azure permission model (or are testing from their laptops!)
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.
Done.
Add Azure Managed Identity and move container name and storage account name to the URL.
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.
Awesome! Thanks for making these changes!
I'll set up CI/CD on this project to get a build out so we can test this in the sandbox RG soon.
Add functionality to backup to Azure.