clientconfig: Ignore OS_CLOUD if explicit cloud config provided

The current behavior of the 'GetCloudFromYAML' and 'AuthOptions'
utilities is confusing and the source of much pulled hair. As discussed
in the linked bug, the equivalent Python libraries, openstacksdk and
(legacy) os_client_config, both default to reading 'OS_CLOUD' only if
explicit cloud config has not been provided. However, we're actually
overriding what the user provided if 'OS_CLOUD' is set. This is clearly
incorrect. Correct things to prefer user-provide cloud configuration,
falling back to the environment variable only if this is not provided.

While we're here, correctly handle a nil 'opts' arguments in the
'GetCloudFromYAML' function. Previously this was done ad-hoc and was
incomplete.

Signed-off-by: Stephen Finucane <stephenfin@redhat.com>
Closes: #164

Author: stephenfin

openstack/clientconfig/requests.go

@@ -183,6 +183,10 @@ func LoadPublicCloudsYAML() (map[string]Cloud, error) {
 
 // GetCloudFromYAML will return a cloud entry from a clouds.yaml file.
 func GetCloudFromYAML(opts *ClientOpts) (*Cloud, error) {
+	if opts == nil {
+		opts = new(ClientOpts)
+	}
+
 	if opts.YAMLOpts == nil {
 		opts.YAMLOpts = new(YAMLOpts)
 	}
@@ -197,19 +201,18 @@ func GetCloudFromYAML(opts *ClientOpts) (*Cloud, error) {
 	// Determine which cloud to use.
 	// First see if a cloud name was explicitly set in opts.
 	var cloudName string
-	if opts != nil && opts.Cloud != "" {
+	if opts.Cloud != "" {
 		cloudName = opts.Cloud
-	}
-
-	// Next see if a cloud name was specified as an environment variable.
-	// This is supposed to override an explicit opts setting.
-	envPrefix := "OS_"
-	if opts.EnvPrefix != "" {
-		envPrefix = opts.EnvPrefix
-	}
+	} else {
+		// If not, see if a cloud name was specified as an environment variable.
+		envPrefix := "OS_"
+		if opts.EnvPrefix != "" {
+			envPrefix = opts.EnvPrefix
+		}
 
-	if v := env.Getenv(envPrefix + "CLOUD"); v != "" {
-		cloudName = v
+		if v := env.Getenv(envPrefix + "CLOUD"); v != "" {
+			cloudName = v
+		}
 	}
 
 	var cloud *Cloud
@@ -351,16 +354,17 @@ func AuthOptions(opts *ClientOpts) (*gophercloud.AuthOptions, error) {
 	var cloudName string
 	if opts.Cloud != "" {
 		cloudName = opts.Cloud
-	}
-
-	// Next see if a cloud name was specified as an environment variable.
-	envPrefix := "OS_"
-	if opts.EnvPrefix != "" {
-		envPrefix = opts.EnvPrefix
-	}
+	} else {
+		// If not, see if a cloud name was specified as an environment
+		// variable.
+		envPrefix := "OS_"
+		if opts.EnvPrefix != "" {
+			envPrefix = opts.EnvPrefix
+		}
 
-	if v := env.Getenv(envPrefix + "CLOUD"); v != "" {
-		cloudName = v
+		if v := env.Getenv(envPrefix + "CLOUD"); v != "" {
+			cloudName = v
+		}
 	}
 
 	// If a cloud name was determined, try to look it up in clouds.yaml.

openstack/clientconfig/testing/requests_test.go

@@ -84,6 +84,30 @@ func TestGetCloudFromYAML(t *testing.T) {
 	}
 }
 
+func TestGetCloudFromYAMLOSCLOUD(t *testing.T) {
+	os.Setenv("OS_CLOUD", "california")
+
+	clientOpts := &clientconfig.ClientOpts{
+		Cloud: "hawaii",
+	}
+
+	actual, err := clientconfig.GetCloudFromYAML(clientOpts)
+	th.AssertNoErr(t, err)
+	th.AssertDeepEquals(t, &HawaiiCloudYAML, actual)
+
+	os.Unsetenv("OS_CLOUD")
+}
+
+func TestGetCloudFromYAMLMissingClientOpts(t *testing.T) {
+	os.Setenv("OS_CLOUD", "california")
+
+	actual, err := clientconfig.GetCloudFromYAML(nil)
+	th.AssertNoErr(t, err)
+	th.AssertDeepEquals(t, &CaliforniaCloudYAML, actual)
+
+	os.Unsetenv("OS_CLOUD")
+}
+
 func TestAuthOptionsExplicitCloud(t *testing.T) {
 	os.Unsetenv("OS_CLOUD")
 
@@ -116,6 +140,40 @@ func TestAuthOptionsOSCLOUD(t *testing.T) {
 	os.Unsetenv("FOO_CLOUD")
 }
 
+func TestAuthOptionsExplicitCloudAndOSCLOUD(t *testing.T) {
+	os.Setenv("FOO_CLOUD", "hawaii")
+
+	clientOpts := &clientconfig.ClientOpts{
+		EnvPrefix: "FOO_",
+		Cloud:     "california",
+	}
+
+	actual, err := clientconfig.AuthOptions(clientOpts)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// We should have ignored the cloud configuration option
+	th.AssertDeepEquals(t, CaliforniaAuthOpts, actual)
+
+	os.Unsetenv("FOO_CLOUD")
+}
+
+func TestAuthOptionsMissingClientOpts(t *testing.T) {
+	os.Setenv("OS_CLOUD", "hawaii")
+
+	actual, err := clientconfig.AuthOptions(nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// We should have handled the missing config opts and fallen back to
+	// defaults
+	th.AssertDeepEquals(t, HawaiiAuthOpts, actual)
+
+	os.Unsetenv("OS_CLOUD")
+}
+
 func TestAuthOptionsCreationFromCloudsYAML(t *testing.T) {
 	os.Unsetenv("OS_CLOUD")