Feedback from review

Updated mock to simplify things.
Updated unit tests to be more robust.

docs/instancemetadata.md

@@ -19,4 +19,4 @@ Environment variables you **must** set:
 * RESOURCEGROUP: the target resource group name
 
 Environment variables you may **optionally** set:
-* HOSTNAME_OVERRIDE: in case default behavior doesn't give a value we want for hostname, pass in an override here
+* HOSTNAME_OVERRIDE: in case default behavior doesn't give a value we want for hostname, pass in an override here.  This can happen when running `aro deploy` inside an Azure Conainer Instance (ACI) where the hostname is not meaningful and we wish to use the host's hostname.

pkg/env/core.go

@@ -48,7 +48,7 @@ func NewCore(ctx context.Context, log *logrus.Entry) (Core, error) {
 	if err != nil {
 		return nil, err
 	}
-	log.Infof("InstanceMetadata: environment=%s subscriptionID=%s tenantID=%s location=%s resourceGroup=%s hostname=%s", im.Environment().Name, im.SubscriptionID(), im.TenantID(), im.Location(), im.ResourceGroup(), im.Hostname())
+	log.Infof("InstanceMetadata: running on %s", im.Environment().Name)
 
 	return &core{
 		InstanceMetadata: im,

pkg/util/env/env.go

@@ -1,37 +0,0 @@
-package env
-
-// Copyright (c) Microsoft Corporation.
-// Licensed under the Apache License 2.0.
-
-import "os"
-
-// Wrapper around env calls so we can mock and test.
-// https://gist.github.com/alexellis/adc67eb022b7fdca31afc0de6529e5ea
-
-type OsEnv struct{}
-
-type Environment struct{}
-
-type EnvironmentSource interface {
-	Getenv(key string) string
-	LookupEnv(key string) (string, bool)
-}
-
-func (Environment) Getenv(source EnvironmentSource, key string) string {
-	return source.Getenv(key)
-}
-
-func (Environment) LookupEnv(source EnvironmentSource, key string) (string, bool) {
-	return source.LookupEnv(key)
-}
-
-func NewOsEnv() OsEnv {
-	return OsEnv{}
-}
-
-func (OsEnv) Getenv(key string) string {
-	return os.Getenv(key)
-}
-func (OsEnv) LookupEnv(key string) (string, bool) {
-	return os.LookupEnv(key)
-}

pkg/util/env/env_test.go

@@ -1,109 +0,0 @@
-package env
-
-// Copyright (c) Microsoft Corporation.
-// Licensed under the Apache License 2.0.
-
-import (
-	"testing"
-)
-
-type TestEnv struct {
-	data map[string]string
-}
-
-func NewTestEnv(envMap map[string]string) TestEnv {
-	return TestEnv{
-		data: envMap,
-	}
-}
-
-func (t *TestEnv) Getenv(key string) string {
-	return t.data[key]
-}
-func (t *TestEnv) LookupEnv(key string) (string, bool) {
-	if out, ok := t.data[key]; ok {
-		return out, true
-	} else {
-		return "", false
-	}
-}
-
-func TestLookupEnv(t *testing.T) {
-	tests := []struct {
-		name        string
-		environment map[string]string
-		lookup      string
-		expect      bool
-	}{
-		{
-			name:        "no environment",
-			environment: make(map[string]string),
-			lookup:      "keyThatDoesNotExist",
-			expect:      false,
-		},
-		{
-			name: "env but missing key",
-			environment: map[string]string{
-				"someKey": "someValue",
-			},
-			lookup: "keyThatDoesNotExist",
-			expect: false,
-		},
-		{
-			name: "lookup existing key",
-			environment: map[string]string{
-				"someKey": "someValue",
-			},
-			lookup: "someKey",
-			expect: true,
-		},
-	}
-
-	for _, test := range tests {
-		testEnv := NewTestEnv(test.environment)
-		_, got := testEnv.LookupEnv(test.lookup)
-		if got != test.expect {
-			t.Errorf("%s: expected %#v got %#v", test.name, test.expect, got)
-		}
-	}
-}
-
-func TestGetenv(t *testing.T) {
-	tests := []struct {
-		name        string
-		environment map[string]string
-		lookup      string
-		expect      string
-	}{
-		{
-			name:        "no environment",
-			environment: make(map[string]string),
-			lookup:      "keyThatDoesNotExist",
-			expect:      "",
-		},
-		{
-			name: "env but missing key",
-			environment: map[string]string{
-				"someKey": "someValue",
-			},
-			lookup: "keyThatDoesNotExist",
-			expect: "",
-		},
-		{
-			name: "lookup existing key",
-			environment: map[string]string{
-				"someKey": "someValue",
-			},
-			lookup: "someKey",
-			expect: "someValue",
-		},
-	}
-
-	for _, test := range tests {
-		testEnv := NewTestEnv(test.environment)
-		got := testEnv.Getenv(test.lookup)
-		if got != test.expect {
-			t.Errorf("%s: expected %#v got %#v", test.name, test.expect, got)
-		}
-	}
-}

pkg/util/instancemetadata/prodenv.go

@@ -1,77 +0,0 @@
-package instancemetadata
-
-// Copyright (c) Microsoft Corporation.
-// Licensed under the Apache License 2.0.
-
-import (
-	"context"
-	"fmt"
-	"os"
-
-	"github.com/Azure/go-autorest/autorest/adal"
-	"github.com/Azure/go-autorest/autorest/azure"
-
-	"github.com/Azure/ARO-RP/pkg/util/env"
-)
-
-type prodenv struct {
-	instanceMetadata
-
-	newServicePrincipalTokenFromMSI func(string, string) (ServicePrincipalToken, error)
-}
-
-func newProdFromEnv(ctx context.Context) (InstanceMetadata, error) {
-	p := &prodenv{
-		newServicePrincipalTokenFromMSI: func(msiEndpoint, resource string) (ServicePrincipalToken, error) {
-			return adal.NewServicePrincipalTokenFromMSI(msiEndpoint, resource)
-		},
-	}
-
-	osenv := env.NewOsEnv()
-	err := p.populateInstanceMetadata(osenv)
-	if err != nil {
-		return nil, err
-	}
-
-	return p, nil
-}
-
-func (p *prodenv) populateInstanceMetadata(osenv env.EnvironmentSource) error {
-
-	for _, key := range []string{
-		"AZURE_ENVIRONMENT",
-		"AZURE_SUBSCRIPTION_ID",
-		"AZURE_TENANT_ID",
-		"LOCATION",
-		"RESOURCEGROUP",
-	} {
-		if _, found := osenv.LookupEnv(key); !found {
-			return fmt.Errorf("environment variable %q unset", key)
-		}
-	}
-
-	// optional env variables
-	// * HOSTNAME_OVERRIDE: defaults to os.Hostname()
-
-	envStr := osenv.Getenv("AZURE_ENVIRONMENT")
-	environment, err := azure.EnvironmentFromName(envStr)
-	if err != nil {
-		return err
-	}
-	p.environment = &environment
-
-	p.subscriptionID = osenv.Getenv("AZURE_SUBSCRIPTION_ID")
-	p.tenantID = osenv.Getenv("AZURE_TENANT_ID")
-	p.location = osenv.Getenv("LOCATION")
-	p.resourceGroup = osenv.Getenv("RESOURCEGROUP")
-	p.hostname = osenv.Getenv("HOSTNAME_OVERRIDE") // empty string returned if not set
-
-	if p.hostname == "" {
-		hostname, err := os.Hostname()
-		if err == nil {
-			p.hostname = hostname
-		}
-	}
-
-	return nil
-}

pkg/util/instancemetadata/prodenv_test.go

@@ -1,145 +0,0 @@
-package instancemetadata
-
-// Copyright (c) Microsoft Corporation.
-// Licensed under the Apache License 2.0.
-
-import (
-	"testing"
-
-	"github.com/Azure/go-autorest/autorest/azure"
-)
-
-type TestEnv struct {
-	data map[string]string
-}
-
-func NewTestEnv(envMap map[string]string) TestEnv {
-	return TestEnv{
-		data: envMap,
-	}
-}
-
-func (t TestEnv) Getenv(key string) string {
-	return t.data[key]
-}
-func (t TestEnv) LookupEnv(key string) (string, bool) {
-	if out, ok := t.data[key]; ok {
-		return out, true
-	} else {
-		return "", false
-	}
-}
-
-func TestProdEnvPopulateInstanceMetadata(t *testing.T) {
-	tests := []struct {
-		name        string
-		environment map[string]string
-		expectErr   bool
-	}{
-		{
-			name:        "no environment",
-			environment: make(map[string]string),
-			expectErr:   true,
-		},
-		{
-			name: "env missing all keys",
-			environment: map[string]string{
-				"someKey": "someValue",
-			},
-			expectErr: true,
-		},
-		{
-			name: "env missing some keys",
-			environment: map[string]string{
-				"RESOURCEGROUP": "my-rg",
-			},
-			expectErr: true,
-		},
-		{
-			name: "env public cloud",
-			environment: map[string]string{
-				"AZURE_ENVIRONMENT":     azure.PublicCloud.Name,
-				"AZURE_SUBSCRIPTION_ID": "some-sub-guid",
-				"AZURE_TENANT_ID":       "some-tenant-guid",
-				"LOCATION":              "some-region",
-				"RESOURCEGROUP":         "my-resourceGroup",
-			},
-			expectErr: false,
-		},
-		{
-			name: "env fairfax",
-			environment: map[string]string{
-				"AZURE_ENVIRONMENT":     azure.USGovernmentCloud.Name,
-				"AZURE_SUBSCRIPTION_ID": "some-sub-guid",
-				"AZURE_TENANT_ID":       "some-tenant-guid",
-				"LOCATION":              "some-region",
-				"RESOURCEGROUP":         "my-resourceGroup",
-			},
-			expectErr: false,
-		},
-		{
-			name: "env mooncake",
-			environment: map[string]string{
-				"AZURE_ENVIRONMENT":     azure.ChinaCloud.Name,
-				"AZURE_SUBSCRIPTION_ID": "some-sub-guid",
-				"AZURE_TENANT_ID":       "some-tenant-guid",
-				"LOCATION":              "some-region",
-				"RESOURCEGROUP":         "my-resourceGroup",
-			},
-			expectErr: false,
-		},
-		{
-			name: "env blackforest",
-			environment: map[string]string{
-				"AZURE_ENVIRONMENT":     azure.GermanCloud.Name,
-				"AZURE_SUBSCRIPTION_ID": "some-sub-guid",
-				"AZURE_TENANT_ID":       "some-tenant-guid",
-				"LOCATION":              "some-region",
-				"RESOURCEGROUP":         "my-resourceGroup",
-			},
-			expectErr: false,
-		},
-		{
-			name: "env with hostname override",
-			environment: map[string]string{
-				"AZURE_ENVIRONMENT":     azure.PublicCloud.Name,
-				"AZURE_SUBSCRIPTION_ID": "some-sub-guid",
-				"AZURE_TENANT_ID":       "some-tenant-guid",
-				"LOCATION":              "some-region",
-				"RESOURCEGROUP":         "my-resourceGroup",
-				"HOSTNAME_OVERRIDE":     "my.over.ride",
-			},
-			expectErr: false,
-		},
-	}
-
-	for _, test := range tests {
-		testEnv := NewTestEnv(test.environment)
-		p := &prodenv{}
-		err := p.populateInstanceMetadata(testEnv)
-		if test.expectErr != (err != nil) {
-			t.Errorf("%s: expected error %#v got %#v", test.name, test.expectErr, err)
-		} else if !test.expectErr {
-			// verify there are values for all required fields
-			if p.environment == nil {
-				t.Errorf("%s: environment expected, found nil", test.name)
-			}
-			if p.subscriptionID == "" {
-				t.Errorf("%s: subscriptionID expected, found empty string", test.name)
-			}
-			if p.tenantID == "" {
-				t.Errorf("%s: tenantID expected, found empty string", test.name)
-			}
-			if p.location == "" {
-				t.Errorf("%s: location expected, found empty string", test.name)
-			}
-			if p.resourceGroup == "" {
-				t.Errorf("%s: resourceGroup expected, found empty string", test.name)
-			}
-			// hostname is always optional to set env var for but we always expect a value
-			if p.hostname == "" {
-				t.Errorf("%s: hostname expected, found empty string", test.name)
-			}
-		}
-	}
-}

pkg/util/instancemetadata/prodfromenv.go

@@ -0,0 +1,73 @@
+package instancemetadata
+
+// Copyright (c) Microsoft Corporation.
+// Licensed under the Apache License 2.0.
+
+import (
+	"context"
+	"fmt"
+	"os"
+
+	"github.com/Azure/go-autorest/autorest/azure"
+)
+
+type prodfromenv struct {
+	instanceMetadata
+
+	Getenv    func(key string) string
+	LookupEnv func(key string) (string, bool)
+}
+
+func newProdFromEnv(ctx context.Context) (InstanceMetadata, error) {
+	p := &prodfromenv{
+		Getenv:    os.Getenv,
+		LookupEnv: os.LookupEnv,
+	}
+
+	err := p.populateInstanceMetadata()
+	if err != nil {
+		return nil, err
+	}
+
+	return p, nil
+}
+
+func (p *prodfromenv) populateInstanceMetadata() error {
+
+	for _, key := range []string{
+		"AZURE_ENVIRONMENT",
+		"AZURE_SUBSCRIPTION_ID",
+		"AZURE_TENANT_ID",
+		"LOCATION",
+		"RESOURCEGROUP",
+	} {
+		if _, found := p.LookupEnv(key); !found {
+			return fmt.Errorf("environment variable %q unset", key)
+		}
+	}
+
+	// optional env variables
+	// * HOSTNAME_OVERRIDE: defaults to os.Hostname()
+
+	envStr := p.Getenv("AZURE_ENVIRONMENT")
+	environment, err := azure.EnvironmentFromName(envStr)
+	if err != nil {
+		return err
+	}
+	p.environment = &environment
+
+	p.subscriptionID = p.Getenv("AZURE_SUBSCRIPTION_ID")
+	p.tenantID = p.Getenv("AZURE_TENANT_ID")
+	p.location = p.Getenv("LOCATION")
+	p.resourceGroup = p.Getenv("RESOURCEGROUP")
+	p.hostname = p.Getenv("HOSTNAME_OVERRIDE") // empty string returned if not set
+
+	if p.hostname == "" {
+		hostname, err := os.Hostname()
+		if err == nil {
+			p.hostname = hostname
+		}
+	}
+
+	return nil
+}

pkg/util/instancemetadata/prodfromenv_test.go

@@ -0,0 +1,158 @@
+package instancemetadata
+
+// Copyright (c) Microsoft Corporation.
+// Licensed under the Apache License 2.0.
+
+import (
+	"os"
+	"testing"
+
+	"github.com/Azure/go-autorest/autorest/azure"
+)
+
+func TestProdEnvPopulateInstanceMetadata(t *testing.T) {
+	hostname, _ := os.Hostname()
+
+	tests := []struct {
+		name        string
+		environment map[string]string
+		expectErr   bool
+		expected    prodfromenv
+	}{
+		{
+			name:        "no environment",
+			environment: make(map[string]string),
+			expectErr:   true,
+			expected:    prodfromenv{},
+		},
+		{
+			name: "env missing all keys",
+			environment: map[string]string{
+				"someKey": "someValue",
+			},
+			expectErr: true,
+			expected:  prodfromenv{},
+		},
+		{
+			name: "env missing some keys",
+			environment: map[string]string{
+				"RESOURCEGROUP": "my-rg",
+			},
+			expectErr: true,
+			expected:  prodfromenv{},
+		},
+		{
+			name: "env valid environment",
+			environment: map[string]string{
+				"AZURE_ENVIRONMENT":     azure.PublicCloud.Name,
+				"AZURE_SUBSCRIPTION_ID": "some-sub-guid",
+				"AZURE_TENANT_ID":       "some-tenant-guid",
+				"LOCATION":              "some-region",
+				"RESOURCEGROUP":         "my-resourceGroup",
+			},
+			expectErr: false,
+			expected: prodfromenv{
+				instanceMetadata: instanceMetadata{
+					environment:    &azure.PublicCloud,
+					subscriptionID: "some-sub-guid",
+					tenantID:       "some-tenant-guid",
+					location:       "some-region",
+					resourceGroup:  "my-resourceGroup",
+					hostname:       hostname,
+				},
+			},
+		},
+		{
+			name: "env invalid environment",
+			environment: map[string]string{
+				"AZURE_ENVIRONMENT":     "ThisEnvDoesNotExist",
+				"AZURE_SUBSCRIPTION_ID": "some-sub-guid",
+				"AZURE_TENANT_ID":       "some-tenant-guid",
+				"LOCATION":              "some-region",
+				"RESOURCEGROUP":         "my-resourceGroup",
+			},
+			expectErr: true,
+			expected: prodfromenv{
+				instanceMetadata: instanceMetadata{
+					environment:    nil,
+					subscriptionID: "some-sub-guid",
+					tenantID:       "some-tenant-guid",
+					location:       "some-region",
+					resourceGroup:  "my-resourceGroup",
+					hostname:       hostname,
+				},
+			},
+		},
+		{
+			name: "env with hostname override",
+			environment: map[string]string{
+				"AZURE_ENVIRONMENT":     azure.PublicCloud.Name,
+				"AZURE_SUBSCRIPTION_ID": "some-sub-guid",
+				"AZURE_TENANT_ID":       "some-tenant-guid",
+				"LOCATION":              "some-region",
+				"RESOURCEGROUP":         "my-resourceGroup",
+				"HOSTNAME_OVERRIDE":     "my.over.ride",
+			},
+			expectErr: false,
+			expected: prodfromenv{
+				instanceMetadata: instanceMetadata{
+					environment:    &azure.PublicCloud,
+					subscriptionID: "some-sub-guid",
+					tenantID:       "some-tenant-guid",
+					location:       "some-region",
+					resourceGroup:  "my-resourceGroup",
+					hostname:       "my.over.ride",
+				},
+			},
+		},
+	}
+
+	for _, test := range tests {
+		p := &prodfromenv{
+			Getenv: func(key string) string {
+				return test.environment[key]
+			},
+			LookupEnv: func(key string) (string, bool) {
+				value, ok := test.environment[key]
+				return value, ok
+			},
+		}
+		err := p.populateInstanceMetadata()
+		if test.expectErr != (err != nil) {
+			t.Errorf("%s: expected error %#v got %#v", test.name, test.expectErr, err)
+		} else if !test.expectErr {
+			// verify there are values for all required fields
+			if p.environment != nil && test.expected.environment != nil {
+				pName := ""
+				eName := ""
+				if p.environment != nil {
+					pName = p.environment.Name
+				}
+				if test.expected.environment != nil {
+					eName = test.expected.environment.Name
+				}
+				if pName != eName {
+					t.Errorf("%s: unexpected environment Name value, expected %#v got %#v", test.name, eName, pName)
+				}
+			} else if p.environment != test.expected.environment {
+				// one of these is nil and the other is not
+				t.Errorf("%s: unexpected environment value, expected %#v got %#v", test.name, test.expected.environment, p.environment)
+			}
+			if p.subscriptionID != test.expected.subscriptionID {
+				t.Errorf("%s: unexpected subscriptionID value, expected %#v got %#v", test.name, test.expected.subscriptionID, p.subscriptionID)
+			}
+			if p.tenantID != test.expected.tenantID {
+				t.Errorf("%s: unexpected tenantID value, expected %#v got %#v", test.name, test.expected.tenantID, p.tenantID)
+			}
+			if p.location != test.expected.location {
+				t.Errorf("%s: unexpected environment value, expected %#v got %#v", test.name, test.expected.location, p.location)
+			}
+			if p.resourceGroup != test.expected.resourceGroup {
+				t.Errorf("%s: unexpected environment value, expected %#v got %#v", test.name, test.expected.resourceGroup, p.resourceGroup)
+			}
+			if p.hostname != test.expected.hostname {
+				t.Errorf("%s: unexpected environment value, expected %#v got %#v", test.name, test.expected.hostname, p.hostname)
+			}
+		}
+	}
+}