Merge pull request #179 from newrelic/jthurman/appName

chore(client): Sanitize Region override

Author: Jonathan Thurman

internal/client/client.go

@@ -5,10 +5,13 @@ import (
 	"fmt"
 	"os"
 
-	"github.com/newrelic/newrelic-cli/internal/config"
-	"github.com/newrelic/newrelic-cli/internal/credentials"
+	log "github.com/sirupsen/logrus"
+
 	"github.com/newrelic/newrelic-client-go/newrelic"
 	"github.com/newrelic/newrelic-client-go/pkg/region"
+
+	"github.com/newrelic/newrelic-cli/internal/config"
+	"github.com/newrelic/newrelic-cli/internal/credentials"
 )
 
 var (
@@ -64,11 +67,8 @@ func applyOverrides(p *credentials.Profile) *credentials.Profile {
 		return p
 	}
 
-	var out credentials.Profile
-
-	if p == nil {
-		out = credentials.Profile{}
-	} else {
+	out := credentials.Profile{}
+	if p != nil {
 		out = *p
 	}
 
@@ -77,9 +77,30 @@ func applyOverrides(p *credentials.Profile) *credentials.Profile {
 	}
 
 	if envRegion != "" {
-		reg, err := region.Parse(envRegion)
-		if err == nil {
-			out.Region = reg
+		var err error
+		out.Region, err = region.Parse(envRegion)
+
+		if err != nil {
+			switch err.(type) {
+			case region.UnknownError:
+				log.Errorf("error parsing NEW_RELIC_REGION: %s", err)
+				// Ignore the override if they have a default on the profile
+				if p.Region != "" {
+					var e2 error
+					out.Region, e2 = region.Parse(p.Region.String())
+					if e2 != nil {
+						log.Errorf("error parsing default profile: %s", e2)
+						out.Region = region.Default
+					}
+				} else {
+					out.Region = region.Default
+				}
+				log.Errorf("using region %s", out.Region.String())
+			case region.UnknownUsingDefaultError:
+				log.Error(err)
+			default:
+				log.Fatalf("unknown error: %v", err)
+			}
 		}
 	}
 

internal/client/client_integration_test.go

@@ -0,0 +1,87 @@
+// +build integration
+
+package client
+
+import (
+	"io/ioutil"
+	"os"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+
+	"github.com/newrelic/newrelic-client-go/pkg/region"
+
+	"github.com/newrelic/newrelic-cli/internal/credentials"
+)
+
+var overrideEnvVars = []string{
+	"NEW_RELIC_API_KEY",
+	"NEW_RELIC_REGION",
+}
+
+func TestApplyOverrides(t *testing.T) {
+	// Do not run this in parallel, we are messing with the environment
+
+	f, err := ioutil.TempDir("/tmp", "newrelic")
+	assert.NoError(t, err)
+	defer os.RemoveAll(f)
+
+	// Initialize the new configuration directory
+	c, err := credentials.LoadCredentials(f)
+	assert.NoError(t, err)
+
+	// Create an initial profile to work with
+	err = c.AddProfile("testCase1", "us", "apiKeyGoesHere")
+	assert.NoError(t, err)
+	p := c.Profiles["testCase1"]
+	assert.NotNil(t, p)
+
+	// Clear env vars we are going to mess with, and reset on exit
+	for _, v := range overrideEnvVars {
+		if val, ok := os.LookupEnv(v); ok {
+			defer os.Setenv(v, val)
+			os.Unsetenv(v)
+		}
+	}
+
+	// Default case (no overrides)
+	p2 := applyOverrides(&p)
+	assert.NotNil(t, p2)
+	assert.Equal(t, p.APIKey, p2.APIKey)
+	assert.Equal(t, p.Region, p2.Region)
+
+	// Override just the API Key
+	os.Setenv("NEW_RELIC_API_KEY", "anotherAPIKey")
+	p2 = applyOverrides(&p)
+	assert.NotNil(t, p2)
+	assert.Equal(t, "anotherAPIKey", p2.APIKey)
+	assert.Equal(t, p.Region, p2.Region)
+
+	// Both
+	os.Setenv("NEW_RELIC_REGION", "US")
+	p2 = applyOverrides(&p)
+	assert.NotNil(t, p2)
+	assert.Equal(t, "anotherAPIKey", p2.APIKey)
+	assert.Equal(t, region.US, p2.Region)
+
+	// Override just the REGION (valid)
+	os.Unsetenv("NEW_RELIC_API_KEY")
+	p2 = applyOverrides(&p)
+	assert.NotNil(t, p2)
+	assert.Equal(t, p.APIKey, p2.APIKey)
+	assert.Equal(t, region.US, p2.Region)
+
+	// Region lowercase
+	os.Setenv("NEW_RELIC_REGION", "eu")
+	p2 = applyOverrides(&p)
+	assert.NotNil(t, p2)
+	assert.Equal(t, p.APIKey, p2.APIKey)
+	assert.Equal(t, region.EU, p2.Region)
+
+	// Invalid REGION (Should be region.Default)
+	os.Setenv("NEW_RELIC_REGION", "test")
+	p2 = applyOverrides(&p)
+	assert.NotNil(t, p2)
+	assert.Equal(t, p.APIKey, p2.APIKey)
+	assert.Equal(t, region.Default, p2.Region)
+}