Repository: Azure/ARO-RP
Pull request #1527 (2021-06-03 19:33:08)

Support creating InstanceMetadata from env
### Which issue this PR addresses:
    
Inability to access instance metadata endpoint inside ACI.

### What this PR does / why we need it:

In Ev2 deployments the deployer is running in Azure Container Instance
(ACI) and does _not_ have access to the instance metadata we have used
to date.  But we have the info to populate env vars.  So we will do that
and attempt to use env vars first.  If we get everything we need, log
what happened and do not use metadata endpoint.  Else fallback to
existing behavior.

### Test plan for issue:

Deploying in INT for all supported clouds.

### Is there any documentation that needs to be updated for this PR?

No?

mjudeikis: Please do not merge it. Gonna check it in the morning. Logging should not be instantiated again. Will check tomorrow :) 
jewzaam: @mjudeikis yes, this never calls `GetEntry()` on its own, always requires it be called elsewhere.  So we only ever have a single (if only called once) instance.  And if not set things fail.
mjudeikis: Did this whule reviewing: https://github.com/mjudeikis/ARO-RP/commit/a24741ef99a4557b6958c1eecbb901ea76cb9a53 you can use if you want.
In addition `if p.populateInstanceMetadataFromEnvironment() {` needs to be gated by `EV2` env variable. Otherwise it will introduce flakes into `TestPopulateInstanceMetadata` test. And it would be good to add test cases for this function as we modified its behavior. 
jewzaam: @mjudeikis changes merged except unit tests.  Will see how I can get that going with use of mock, challenge from what I read last night being mocking getenv is not a thing but.. we'll see.  If you're OK with current state without tests I ask we consider merging now.  I will work on tests today regardless.
jewzaam: @mjudeikis @m1kola ready for review with unit tests.

Patch:
diff --git a/docs/instancemetadata.md b/docs/instancemetadata.md
new file mode 100644
index 000000000..017c0d532
--- /dev/null
+++ b/docs/instancemetadata.md
@@ -0,0 +1,22 @@
+Documentation is a work in progress.
+
+# aro deploy
+
+## Run via ADO
+
+Environment is picked up from instance metadata endpoint, no env vars are required to be set.
+
+## Run via Ev2
+
+Ev2 will run `aro deploy` in ACI which cannot access instance metadata endpoint (169.254.169.254).  We have all the info needed already so we make it available via environment variables.  In addition, a variable indicating we're running in Ev2 is required to ensure current non-Ev2 behavior is not impacted.
+
+Environment variables you **must** set:
+* AZURE_EV2: any value other than empty string
+* AZURE_ENVIRONMENT: the cloud environment name from go-autorest environment names
+* AZURE_SUBSCRIPTION_ID: the target subscription ID
+* AZURE_TENANT_ID: the target tenant ID
+* LOCATION: the target location
+* 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.  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.
diff --git a/pkg/env/core.go b/pkg/env/core.go
index 81f0d9f5f..bf5ec4163 100644
--- a/pkg/env/core.go
+++ b/pkg/env/core.go
@@ -39,7 +39,7 @@ func NewCore(ctx context.Context, log *logrus.Entry) (Core, error) {
 		log.Info("running in local development mode")
 	}
 
-	im, err := instancemetadata.New(ctx, isLocalDevelopmentMode)
+	im, err := instancemetadata.New(ctx, log, isLocalDevelopmentMode)
 	if err != nil {
 		return nil, err
 	}
@@ -48,7 +48,7 @@ func NewCore(ctx context.Context, log *logrus.Entry) (Core, error) {
 	if err != nil {
 		return nil, err
 	}
-	log.Infof("running on %s", im.Environment().Name)
+	log.Infof("InstanceMetadata: running on %s", im.Environment().Name)
 
 	return &core{
 		InstanceMetadata: im,
diff --git a/pkg/util/instancemetadata/instancemetadata.go b/pkg/util/instancemetadata/instancemetadata.go
index 1cd15a726..8cf125d33 100644
--- a/pkg/util/instancemetadata/instancemetadata.go
+++ b/pkg/util/instancemetadata/instancemetadata.go
@@ -5,8 +5,10 @@ package instancemetadata
 
 import (
 	"context"
+	"os"
 
 	"github.com/Azure/go-autorest/autorest/azure"
+	"github.com/sirupsen/logrus"
 )
 
 type InstanceMetadata interface {
@@ -51,10 +53,17 @@ func (im *instanceMetadata) Environment() *azure.Environment {
 	return im.environment
 }
 
-func New(ctx context.Context, isLocalDevelopmentMode bool) (InstanceMetadata, error) {
+func New(ctx context.Context, log *logrus.Entry, isLocalDevelopmentMode bool) (InstanceMetadata, error) {
 	if isLocalDevelopmentMode {
+		log.Info("creating development InstanceMetadata")
 		return NewDev(true)
 	}
 
-	return newProd(ctx)
+	if os.Getenv("AZURE_EV2") != "" {
+		log.Info("creating InstanceMetadata from Environment")
+		return newProdFromEnv(ctx)
+	} else {
+		log.Info("creating InstanceMetadata from Azure Instance Metadata Service (AIMS)")
+		return newProd(ctx)
+	}
 }
diff --git a/pkg/util/instancemetadata/prodfromenv.go b/pkg/util/instancemetadata/prodfromenv.go
new file mode 100644
index 000000000..393c586a1
--- /dev/null
+++ b/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
+}


Test patch:
diff --git a/pkg/util/instancemetadata/prodfromenv_test.go b/pkg/util/instancemetadata/prodfromenv_test.go
new file mode 100644
index 000000000..cb6119d41
--- /dev/null
+++ b/pkg/util/instancemetadata/prodfromenv_test.go
@@ -0,0 +1,104 @@
+package instancemetadata
+
+// Copyright (c) Microsoft Corporation.
+// Licensed under the Apache License 2.0.
+
+import (
+	"os"
+	"reflect"
+	"testing"
+
+	"github.com/Azure/go-autorest/autorest/azure"
+	"github.com/google/go-cmp/cmp"
+)
+
+func TestProdEnvPopulateInstanceMetadata(t *testing.T) {
+	hostname, err := os.Hostname()
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	tests := []struct {
+		name                 string
+		environment          map[string]string
+		wantInstanceMetadata instanceMetadata
+		wantErr              string
+	}{
+		{
+			name:    "missing environment variables",
+			wantErr: "environment variable \"AZURE_ENVIRONMENT\" unset",
+		},
+		{
+			name: "valid environment variables",
+			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",
+			},
+			wantInstanceMetadata: instanceMetadata{
+				environment:    &azure.PublicCloud,
+				subscriptionID: "some-sub-guid",
+				tenantID:       "some-tenant-guid",
+				location:       "some-region",
+				resourceGroup:  "my-resourceGroup",
+				hostname:       hostname,
+			},
+		},
+		{
+			name: "valid environment variables, but invalid Azure environment name",
+			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",
+			},
+			wantErr: "autorest/azure: There is no cloud environment matching the name \"THISENVDOESNOTEXIST\"",
+		},
+		{
+			name: "valid environment variables 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",
+			},
+			wantInstanceMetadata: 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 {
+		t.Run(test.name, func(t *testing.T) {
+			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 err != nil && err.Error() != test.wantErr ||
+				err == nil && test.wantErr != "" {
+				t.Error(err)
+			}
+			if !reflect.DeepEqual(p.instanceMetadata, test.wantInstanceMetadata) {
+				opts := cmp.AllowUnexported(instanceMetadata{})
+				t.Error(cmp.Diff(p.instanceMetadata, test.wantInstanceMetadata, opts))
+			}
+		})
+	}
+}