Skip to content
This repository has been archived by the owner on Mar 28, 2020. It is now read-only.

Add cloud environment support for ABS backup #1981

Merged
merged 1 commit into from
Aug 9, 2018
Merged

Conversation

feiskyer
Copy link
Contributor

@feiskyer feiskyer commented Aug 2, 2018

PR #1842 adds support for ABS (Azure Blob Service) backup. But it always using AzurePublicCloud, so that other sovereign clouds (e.g. mooncake) won't work.

This PR adds an extra option cloud to ABS credential and use storage.NewBasicClientOnSovereignCloud() to fix the issue. If cloud is not set, then AzurePublicCloud is used (same as before).

Example ABS credential now is

apiVersion: v1
kind: Secret
metadata:
  name: abs-credentials
type: Opaque
stringData:
  storage-account: <storage-account-name>
  storage-key: <storage-key>
  cloud: AzurePublicCloud

@feiskyer
Copy link
Contributor Author

feiskyer commented Aug 2, 2018

cc @xiang90

Copy link

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

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

Other than question about the autorest package, this LGTM, but IANTM.

@@ -18,6 +18,7 @@ import (
"fmt"

"github.com/Azure/azure-sdk-for-go/storage"
"github.com/Azure/go-autorest/autorest/azure"
Copy link

Choose a reason for hiding this comment

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

Right now it looks like go-autorest is a transient dependency, this seems to make it a direct dependency.
Should this include an update to Gopkg.toml?

Copy link
Contributor Author

@feiskyer feiskyer Aug 9, 2018

Choose a reason for hiding this comment

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

Sorry, not farmiliar with dep. Run dep ensure doesn't add that. Should I add it explicitly?

Copy link
Contributor

Choose a reason for hiding this comment

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

No need to add go-autorest. it is vendored already via https://github.com/coreos/etcd-operator/blame/master/Gopkg.lock#L17

@fanminshi
Copy link
Contributor

I don't see a doc on how to use the ABS as the backup backend. Could you add a doc under doc/user/backup on how to do backup using ABS? We can do that at a separate pr.

@fanminshi
Copy link
Contributor

lgtm on this code.

@mwieczorek
Copy link

@fanminshi there’s
https://github.com/coreos/etcd-operator/blob/master/doc/design/abs_backup.md
But of course it should be updated and maybe moved to doc/user folder?

@fanminshi
Copy link
Contributor

@mwieczorek good find. I think we should leave the design doc alone. Let's just copy and paste the design content and update it with more details on how to use ABS as the backend for public cloud, sovereign cloud, and etc.

@fanminshi fanminshi mentioned this pull request Aug 9, 2018
@fanminshi fanminshi merged commit 0aec6b1 into coreos:master Aug 9, 2018
@feiskyer feiskyer deleted the abs branch August 10, 2018 01:44
@feiskyer
Copy link
Contributor Author

@fanminshi @mwieczorek Thanks. Let me add the user guide for it in a separate PR.

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

Successfully merging this pull request may close these issues.

None yet

4 participants