From ef5b6e93a9d399b1b54d6d6207d7ed44e13f44f0 Mon Sep 17 00:00:00 2001 From: Paul Hinze Date: Tue, 13 Oct 2015 16:57:11 -0500 Subject: [PATCH] provider/azure: fix issues loading config from homedir Issues were: * `settings_file` `ValidateFunc` needs to expand homedir just like the `configure` does, otherwise ~-based paths fail validation * `isFile` was being called before ~-expand so configure was failing as well * `Config` was swallowing error so provider was ending up with `nil`, resulting in crash To fix: * Consolidate settings_file path/contents handling into a single helper called from both `validate` and `configure` funcs * Return err from `Config` To cover: * Added test case to validate w/ tilde-path * Added configure test w/ tilde-path --- builtin/providers/azure/config.go | 2 +- builtin/providers/azure/provider.go | 58 ++++++++-------- builtin/providers/azure/provider_test.go | 85 +++++++++++++++++++++++- 3 files changed, 111 insertions(+), 34 deletions(-) diff --git a/builtin/providers/azure/config.go b/builtin/providers/azure/config.go index cbb23d58b..b096a10c4 100644 --- a/builtin/providers/azure/config.go +++ b/builtin/providers/azure/config.go @@ -98,7 +98,7 @@ func (c Client) getStorageServiceQueueClient(serviceName string) (storage.QueueS func (c *Config) NewClientFromSettingsData() (*Client, error) { mc, err := management.ClientFromPublishSettingsData(c.Settings, c.SubscriptionID) if err != nil { - return nil, nil + return nil, err } return &Client{ diff --git a/builtin/providers/azure/provider.go b/builtin/providers/azure/provider.go index fe100be35..975a93b00 100644 --- a/builtin/providers/azure/provider.go +++ b/builtin/providers/azure/provider.go @@ -64,22 +64,12 @@ func providerConfigure(d *schema.ResourceData) (interface{}, error) { Certificate: []byte(d.Get("certificate").(string)), } - settings := d.Get("settings_file").(string) - - if settings != "" { - if ok, _ := isFile(settings); ok { - settingsFile, err := homedir.Expand(settings) - if err != nil { - return nil, fmt.Errorf("Error expanding the settings file path: %s", err) - } - publishSettingsContent, err := ioutil.ReadFile(settingsFile) - if err != nil { - return nil, fmt.Errorf("Error reading settings file: %s", err) - } - config.Settings = publishSettingsContent - } else { - config.Settings = []byte(settings) - } + settingsFile := d.Get("settings_file").(string) + if settingsFile != "" { + // any errors from readSettings would have been caught at the validate + // step, so we can avoid handling them now + settings, _, _ := readSettings(settingsFile) + config.Settings = settings return config.NewClientFromSettingsData() } @@ -92,31 +82,39 @@ func providerConfigure(d *schema.ResourceData) (interface{}, error) { "or both a 'subscription_id' and 'certificate'.") } -func validateSettingsFile(v interface{}, k string) (warnings []string, errors []error) { +func validateSettingsFile(v interface{}, k string) ([]string, []error) { value := v.(string) - if value == "" { - return + return nil, nil } - var settings settingsData - if err := xml.Unmarshal([]byte(value), &settings); err != nil { - warnings = append(warnings, ` + _, warnings, errors := readSettings(value) + return warnings, errors +} + +const settingsPathWarnMsg = ` settings_file is not valid XML, so we are assuming it is a file path. This support will be removed in the future. Please update your configuration to use -${file("filename.publishsettings")} instead.`) - } else { +${file("filename.publishsettings")} instead.` + +func readSettings(pathOrContents string) (s []byte, ws []string, es []error) { + var settings settingsData + if err := xml.Unmarshal([]byte(pathOrContents), &settings); err == nil { + s = []byte(pathOrContents) return } - if ok, err := isFile(value); !ok { - errors = append(errors, - fmt.Errorf( - "account_file path could not be read from '%s': %s", - value, - err)) + ws = append(ws, settingsPathWarnMsg) + path, err := homedir.Expand(pathOrContents) + if err != nil { + es = append(es, fmt.Errorf("Error expanding path: %s", err)) + return } + s, err = ioutil.ReadFile(path) + if err != nil { + es = append(es, fmt.Errorf("Could not read file '%s': %s", path, err)) + } return } diff --git a/builtin/providers/azure/provider_test.go b/builtin/providers/azure/provider_test.go index 5c720640f..b3feb8392 100644 --- a/builtin/providers/azure/provider_test.go +++ b/builtin/providers/azure/provider_test.go @@ -3,12 +3,14 @@ package azure import ( "io" "io/ioutil" - "log" "os" + "strings" "testing" + "github.com/hashicorp/terraform/config" "github.com/hashicorp/terraform/helper/schema" "github.com/hashicorp/terraform/terraform" + "github.com/mitchellh/go-homedir" ) var testAccProviders map[string]terraform.ResourceProvider @@ -67,20 +69,33 @@ func TestAzure_validateSettingsFile(t *testing.T) { if err != nil { t.Fatalf("Error creating temporary file in TestAzure_validateSettingsFile: %s", err) } + defer os.Remove(f.Name()) fx, err := ioutil.TempFile("", "tf-test-xml") if err != nil { t.Fatalf("Error creating temporary file with XML in TestAzure_validateSettingsFile: %s", err) } + defer os.Remove(fx.Name()) + + home, err := homedir.Dir() + if err != nil { + t.Fatalf("Error fetching homedir: %s", err) + } + fh, err := ioutil.TempFile(home, "tf-test-home") + if err != nil { + t.Fatalf("Error creating homedir-based temporary file: %s", err) + } + defer os.Remove(fh.Name()) _, err = io.WriteString(fx, "") if err != nil { t.Fatalf("Error writing XML File: %s", err) } - - log.Printf("fx name: %s", fx.Name()) fx.Close() + r := strings.NewReplacer(home, "~") + homePath := r.Replace(fh.Name()) + cases := []struct { Input string // String of XML or a path to an XML file W int // expected count of warnings @@ -89,6 +104,7 @@ func TestAzure_validateSettingsFile(t *testing.T) { {"test", 1, 1}, {f.Name(), 1, 0}, {fx.Name(), 1, 0}, + {homePath, 1, 0}, {"", 0, 0}, } @@ -104,6 +120,53 @@ func TestAzure_validateSettingsFile(t *testing.T) { } } +func TestAzure_providerConfigure(t *testing.T) { + home, err := homedir.Dir() + if err != nil { + t.Fatalf("Error fetching homedir: %s", err) + } + fh, err := ioutil.TempFile(home, "tf-test-home") + if err != nil { + t.Fatalf("Error creating homedir-based temporary file: %s", err) + } + defer os.Remove(fh.Name()) + + _, err = io.WriteString(fh, testAzurePublishSettingsStr) + if err != nil { + t.Fatalf("err: %s", err) + } + fh.Close() + + r := strings.NewReplacer(home, "~") + homePath := r.Replace(fh.Name()) + + cases := []struct { + SettingsFile string // String of XML or a path to an XML file + NilMeta bool // whether meta is expected to be nil + }{ + {testAzurePublishSettingsStr, false}, + {homePath, false}, + } + + for _, tc := range cases { + rp := Provider() + raw := map[string]interface{}{ + "settings_file": tc.SettingsFile, + } + + rawConfig, err := config.NewRawConfig(raw) + if err != nil { + t.Fatalf("err: %s", err) + } + + err = rp.Configure(terraform.NewResourceConfig(rawConfig)) + meta := rp.(*schema.Provider).Meta() + if (meta == nil) != tc.NilMeta { + t.Fatalf("expected NilMeta: %t, got meta: %#v", tc.NilMeta, meta) + } + } +} + func TestAzure_isFile(t *testing.T) { f, err := ioutil.TempFile("", "tf-test-file") if err != nil { @@ -129,3 +192,19 @@ func TestAzure_isFile(t *testing.T) { } } } + +// testAzurePublishSettingsStr is a revoked publishsettings file +const testAzurePublishSettingsStr = ` + + + + + + +`