Fetch metadata from project attributes

This implementation enables getting the metadata from project attributes
as well.

BUG=b/208651902
TEST=presubmit and tested on VM
RELEASE_NOTE=None

Change-Id: Ib0d7db982db95ba0e0d86636600a7dc64a96f5b7
diff --git a/pkg/configfetcher/configfetcher.go b/pkg/configfetcher/configfetcher.go
index e26e77e..6c8fc71 100644
--- a/pkg/configfetcher/configfetcher.go
+++ b/pkg/configfetcher/configfetcher.go
@@ -15,6 +15,7 @@
 package configfetcher
 
 import (
+	"context"
 	"encoding/json"
 	"fmt"
 	"strconv"
@@ -49,10 +50,28 @@
 	keyGoogleLoggingEnabled    = "google-logging-enabled"
 	keyGoogleMonitoringEnabled = "google-monitoring-enabled"
 
+	// Note that there are 2 types of metadata entries: endpoint and
+	// directory. Endpoint maps to a single value, while directory contains
+	// a list of key value pairs.
+	// URL path for metadata directory. Note that the trailing slash
+	// indicates that the entry is a directory instead of an endpoint.
+	// Without it the request will just be forwared by the metadata
+	// server with a 301 status code.
+	// The leading slash is required since these entries are relative to the
+	// metadata server URL of the form http://.../...
+	instanceCustomDataDirectory = "/instance/attributes/"
+	projectCustomDataDirectory  = "/project/attributes/"
+
 	// The sleep time when watching user config failed.
 	retryInterval = 5 * time.Second
 )
 
+type mtd struct {
+	metadata map[string]string
+	etag     string
+	err      error
+}
+
 // PollUserConfig watches the update of the InstanceConfig specified by user in GCE
 // metadata. It repeatedly sends request to wait for update of userConfig. When an update
 // is returned, PollUserConfig compares it to the cached userConfig and outputs to the
@@ -60,13 +79,14 @@
 func PollUserConfig(out chan *protos.InstanceConfig, gceMetadataURL string) {
 	var userConfig, lastUserConfig *protos.InstanceConfig
 	var err error
-	etag := ""
+	var instanceEtag, projectEtag string
 	lastUserConfig = nil
 	for {
-		userConfig, etag, err = getUserConfig(etag, gceMetadataURL)
+		userConfig, instanceEtag, projectEtag, err = getUserConfig(instanceEtag, projectEtag, gceMetadataURL)
 		if err != nil {
 			time.Sleep(retryInterval)
-			etag = ""
+			instanceEtag = ""
+			projectEtag = ""
 			continue // Retry forever.
 		}
 		// Check whether userConfig is updated.
@@ -82,30 +102,90 @@
 	}
 }
 
-// getUserConfig gets the content of the instance custom metadata keys'.
-// Then getUserConfig parses the metadata keys and returns userConfig.
-func getUserConfig(lastEtag string, gceMetadataURL string) (*protos.InstanceConfig, string, error) {
-	rawMetadata, etag, err := FetchMetadata(lastEtag, gceMetadataURL)
+// fetchMetadataTag gets the content of the metadata from the metadata
+// server and parses the raw content. It then returns metadata as a
+// map and etag value.
+func fetchMetadataTag(ctx context.Context, lastEtag string, dataDirectory string, gceMetadataURL string) (map[string]string, string, error) {
+	rawMetadata, etag, err := FetchMetadata(ctx, lastEtag, dataDirectory, gceMetadataURL)
 	if err != nil {
-		glog.Errorf("Failed to fetch metadata from metadata server: %s", err)
 		return nil, "", err
 	}
 
-	metadata, err := parseRawMetadata(rawMetadata)
+	mtd, err := parseRawMetadata(rawMetadata)
 	if err != nil {
-		glog.Errorf("Failed to parse raw metadata: %s", err)
 		return nil, "", err
 	}
-	glog.Infof("Current value of instance custom metadata keys: %v", metadata)
-	userConfig, err := getUserConfigFromMetadata(metadata)
-	if err != nil {
-		glog.Errorf("Failed to resolve userconfig: %s", err)
-		return nil, "", err
-	}
-	return userConfig, etag, nil
+	return mtd, etag, nil
 }
 
-// Converts instance custom metadata from string to key:value pairs.
+// fetchMetadataWithChannel gets the content of the metadata from
+// either instance or project and then writes the result to a channel.
+func fetchMetadataWithChannel(ctx context.Context, lastEtag string, dataDirectory string, gceMetadataURL string, c chan<- mtd) {
+	metadata, etag, err := fetchMetadataTag(ctx, lastEtag, dataDirectory, gceMetadataURL)
+	if err != nil {
+		c <- mtd{nil, "", err}
+		return
+	}
+	c <- mtd{metadata, etag, nil}
+}
+
+// getUserConfig gets the content of the instance and project custom
+// metadata keys. Then getUserConfig parses the metadata keys and
+// returns userConfig.
+func getUserConfig(lastInstanceEtag string, lastProjectEtag string, gceMetadataURL string) (*protos.InstanceConfig, string, string, error) {
+	var instanceMetadata, projectMetadata map[string]string
+	var instanceEtag, projectEtag string
+	var err error
+	instanceChannel := make(chan mtd, 1)
+	projectChannel := make(chan mtd, 1)
+
+	ctx1, cancelCtx1 := context.WithCancel(context.Background())
+	ctx2, cancelCtx2 := context.WithCancel(context.Background())
+
+	go fetchMetadataWithChannel(ctx1, lastInstanceEtag, instanceCustomDataDirectory, gceMetadataURL, instanceChannel)
+	go fetchMetadataWithChannel(ctx2, lastProjectEtag, projectCustomDataDirectory, gceMetadataURL, projectChannel)
+
+	select {
+	case instance := <-instanceChannel:
+		if instance.err != nil {
+			err = instance.err
+			break
+		}
+		cancelCtx2()
+		instanceMetadata = instance.metadata
+		instanceEtag = instance.etag
+		projectMetadata, projectEtag, err = fetchMetadataTag(ctx1, "", projectCustomDataDirectory, gceMetadataURL)
+		if err != nil {
+			break
+		}
+	case project := <-projectChannel:
+		if project.err != nil {
+			err = project.err
+			break
+		}
+		cancelCtx1()
+		projectMetadata = project.metadata
+		projectEtag = project.etag
+		instanceMetadata, instanceEtag, err = fetchMetadataTag(ctx2, "", instanceCustomDataDirectory, gceMetadataURL)
+		if err != nil {
+			break
+		}
+	}
+
+	if err != nil {
+		glog.Errorf("Failed to fetch/parse metadata from metadata server: %s", err)
+		return nil, "", "", err
+	}
+
+	userConfig, err := getUserConfigFromMetadata(instanceMetadata, projectMetadata)
+	if err != nil {
+		glog.Errorf("Failed to resolve userconfig: %s", err)
+		return nil, "", "", err
+	}
+	return userConfig, instanceEtag, projectEtag, nil
+}
+
+// parseRawMetadata converts instance custom metadata from string to key:value pairs.
 func parseRawMetadata(rawMetadata string) (map[string]string, error) {
 	output := make(map[string]string)
 
@@ -129,17 +209,29 @@
 // corresponding key with 'gci-' prefix. For example, 'cos-update-strategy' and
 // 'gci-update-strategy'. The value of 'gci-' key is returned only if the function
 // fail to get the value of 'cos-' key.
-func getCOSGCIConfigSetting(cosKey string, gciKey string, metadata map[string]string) (string, bool) {
-	value, ok := metadata[cosKey]
-	if !ok {
-		value, ok = metadata[gciKey]
+func getCOSGCIConfigSetting(cosKey string, gciKey string, instanceMetadata map[string]string, projectMetadata map[string]string) (string, bool) {
+	value, ok := instanceMetadata[cosKey]
+	if ok {
+		return value, ok
 	}
-	return value, ok
+	value, ok = instanceMetadata[gciKey]
+	if ok {
+		return value, ok
+	}
+	value, ok = projectMetadata[cosKey]
+	if ok {
+		return value, ok
+	}
+	value, ok = projectMetadata[gciKey]
+	if ok {
+		return value, ok
+	}
+	return "", false
 }
 
 // Gets user config based on the metadata keys. An empty InstanceConfig is returned
 // if there is no InstanceConfig-related metadata in the pairs.
-func getUserConfigFromMetadata(metadata map[string]string) (*protos.InstanceConfig, error) {
+func getUserConfigFromMetadata(instanceMetadata map[string]string, projectMetadata map[string]string) (*protos.InstanceConfig, error) {
 	var configStr string
 	var err error
 	var ok, boolean bool
@@ -148,7 +240,7 @@
 
 	// Get legacy instance config from metadata. This is deprecated, but if this is still
 	// being specified, use it and ignore others.
-	if configStr, ok = getCOSGCIConfigSetting("", gciLegacyConfigKey, metadata); ok {
+	if configStr, ok = getCOSGCIConfigSetting("", gciLegacyConfigKey, instanceMetadata, projectMetadata); ok {
 		if configStr != "" {
 			if err := jsonpb.UnmarshalString(configStr, userConfig); err != nil {
 				return nil, fmt.Errorf("failed to unmarshal InstanceConfig %s: %s", configStr, err)
@@ -158,7 +250,7 @@
 	}
 
 	// Get individual config settings. Keys with 'cos-' prefix have the priority.
-	if configStr, ok = getCOSGCIConfigSetting(cosKeyUpdateStrategy, gciKeyUpdateStrategy, metadata); ok {
+	if configStr, ok = getCOSGCIConfigSetting(cosKeyUpdateStrategy, gciKeyUpdateStrategy, instanceMetadata, projectMetadata); ok {
 		if configStr == "update_disabled" {
 			userConfig.UpdateStrategy = proto.String(configStr)
 		} else {
@@ -166,19 +258,19 @@
 		}
 	}
 
-	if configStr, ok = getCOSGCIConfigSetting(cosKeyMetricsEnabled, gciKeyMetricsEnabled, metadata); ok {
+	if configStr, ok = getCOSGCIConfigSetting(cosKeyMetricsEnabled, gciKeyMetricsEnabled, instanceMetadata, projectMetadata); ok {
 		if boolean, err = strconv.ParseBool(configStr); err == nil {
 			userConfig.MetricsEnabled = proto.Bool(boolean)
 		}
 	}
 
-	if configStr, ok := getCOSGCIConfigSetting(keyGoogleLoggingEnabled, "", metadata); ok {
+	if configStr, ok := getCOSGCIConfigSetting(keyGoogleLoggingEnabled, "", instanceMetadata, projectMetadata); ok {
 		if boolean, err = strconv.ParseBool(configStr); err == nil {
 			userConfig.HealthMonitorConfig.LoggingEnabled = proto.Bool(boolean)
 		}
 	}
 
-	if configStr, ok := getCOSGCIConfigSetting(keyGoogleMonitoringEnabled, "", metadata); ok {
+	if configStr, ok := getCOSGCIConfigSetting(keyGoogleMonitoringEnabled, "", instanceMetadata, projectMetadata); ok {
 		if boolean, err = strconv.ParseBool(configStr); err == nil {
 			userConfig.HealthMonitorConfig.MonitoringEnabled = proto.Bool(boolean)
 		}
diff --git a/pkg/configfetcher/configfetcher_test.go b/pkg/configfetcher/configfetcher_test.go
index c8dc5cb..86ba097 100644
--- a/pkg/configfetcher/configfetcher_test.go
+++ b/pkg/configfetcher/configfetcher_test.go
@@ -15,11 +15,11 @@
 package configfetcher
 
 import (
-	"fmt"
 	"net/http"
 	"net/http/httptest"
 	"testing"
 
+	"github.com/golang/glog"
 	"github.com/golang/protobuf/proto"
 	"policy-manager/protos"
 )
@@ -29,14 +29,17 @@
 	tests := []struct {
 		// Name of the test case
 		name string
-		// Key:Value pairs present in metadata.
-		metadata map[string]string
+		// Key:Value pairs present in instance metadata.
+		instanceMetadata map[string]string
+		// Key:Value pairs present in project metadata.
+		projectMetadata map[string]string
 		// Expected InstanceConfig to be returned
 		expectedConfig *protos.InstanceConfig
 	}{
 		{
 			"NoMetadataKeys",
 			map[string]string{},
+			map[string]string{},
 			&protos.InstanceConfig{
 				HealthMonitorConfig: &protos.HealthMonitorConfig{},
 			},
@@ -44,6 +47,7 @@
 		{
 			"UpdateStrategyPresent",
 			map[string]string{gciKeyUpdateStrategy: "update-strategy-1"},
+			map[string]string{},
 			&protos.InstanceConfig{
 				UpdateStrategy:      proto.String(""),
 				HealthMonitorConfig: &protos.HealthMonitorConfig{},
@@ -55,6 +59,9 @@
 				gciKeyUpdateStrategy: "update-strategy-2",
 				gciKeyMetricsEnabled: "true",
 			},
+			map[string]string{
+				gciKeyUpdateStrategy: "update_disabled",
+			},
 			&protos.InstanceConfig{
 				UpdateStrategy:      proto.String(""),
 				MetricsEnabled:      proto.Bool(true),
@@ -67,6 +74,7 @@
 				gciKeyMetricsEnabled:    "no",
 				keyGoogleLoggingEnabled: "yes",
 			},
+			map[string]string{},
 			&protos.InstanceConfig{
 				HealthMonitorConfig: &protos.HealthMonitorConfig{},
 			},
@@ -77,6 +85,7 @@
 				gciLegacyConfigKey:   "{\"update_strategy\":\"update-strategy-3\"}",
 				gciKeyUpdateStrategy: "update-strategy-4",
 			},
+			map[string]string{},
 			&protos.InstanceConfig{
 				UpdateStrategy:      proto.String("update-strategy-3"),
 				HealthMonitorConfig: &protos.HealthMonitorConfig{},
@@ -87,6 +96,9 @@
 			map[string]string{
 				keyGoogleLoggingEnabled: "true",
 			},
+			map[string]string{
+				keyGoogleLoggingEnabled: "false",
+			},
 			&protos.InstanceConfig{
 				HealthMonitorConfig: &protos.HealthMonitorConfig{
 					Enforced:       proto.Bool(true),
@@ -99,6 +111,9 @@
 			map[string]string{
 				keyGoogleMonitoringEnabled: "true",
 			},
+			map[string]string{
+				keyGoogleMonitoringEnabled: "false",
+			},
 			&protos.InstanceConfig{
 				HealthMonitorConfig: &protos.HealthMonitorConfig{
 					Enforced:          proto.Bool(true),
@@ -111,6 +126,9 @@
 			map[string]string{
 				keyGoogleLoggingEnabled: "false",
 			},
+			map[string]string{
+				keyGoogleLoggingEnabled: "true",
+			},
 			&protos.InstanceConfig{
 				HealthMonitorConfig: &protos.HealthMonitorConfig{
 					LoggingEnabled: proto.Bool(false),
@@ -121,7 +139,9 @@
 			"LoggingAndMonitoring",
 			map[string]string{
 				keyGoogleMonitoringEnabled: "true",
-				keyGoogleLoggingEnabled:    "true",
+			},
+			map[string]string{
+				keyGoogleLoggingEnabled: "true",
 			},
 			&protos.InstanceConfig{
 				HealthMonitorConfig: &protos.HealthMonitorConfig{
@@ -135,10 +155,12 @@
 
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
-			actualConfig, _ := getUserConfigFromMetadata(test.metadata)
+			actualConfig, err := getUserConfigFromMetadata(test.instanceMetadata, test.projectMetadata)
+			if err != nil {
+				t.Errorf("got unexpected error: %s", err)
+			}
 			if !proto.Equal(actualConfig, test.expectedConfig) {
-				t.Errorf("FAILED '%s': got %s, expect %s",
-					test.name,
+				t.Errorf("got %s, expect %s",
 					proto.MarshalTextString(actualConfig),
 					proto.MarshalTextString(test.expectedConfig))
 			}
@@ -152,14 +174,17 @@
 	tests := []struct {
 		// Name of the test case
 		name string
-		// The metadata returned by metatdata client
-		metadata string
+		// The instance metadata returned by metatdata client
+		instanceMetadata string
+		// The project metadata returned by metatdata client
+		projectMetadata string
 		// The expected config settings to be returned
 		expectedConfig *protos.InstanceConfig
 	}{
 		{
 			"NoMetadata",
 			"{}",
+			"{}",
 			&protos.InstanceConfig{
 				HealthMonitorConfig: &protos.HealthMonitorConfig{},
 			},
@@ -170,6 +195,9 @@
 				"cos-metrics-enabled": "true",
 				"cos-update-strategy": ""
 			}`,
+			`{
+				"cos-update-strategy": "update_disabled"
+			}`,
 			&protos.InstanceConfig{
 				UpdateStrategy:      proto.String(""),
 				MetricsEnabled:      proto.Bool(true),
@@ -183,6 +211,9 @@
 				"key1":                "value1",
 				"kdy2":                "value2"
 			}`,
+			`{
+				"cos-update-strategy": ""
+			}`,
 			&protos.InstanceConfig{
 				UpdateStrategy:      proto.String("update_disabled"),
 				HealthMonitorConfig: &protos.HealthMonitorConfig{},
@@ -194,6 +225,9 @@
 				"key1": "value1",
 				"kdy2": "value2"
 			}`,
+			`{
+				"key3": "value3"
+			}`,
 			&protos.InstanceConfig{
 				HealthMonitorConfig: &protos.HealthMonitorConfig{},
 			},
@@ -202,10 +236,12 @@
 			"LoggingButNoMonitoring",
 			`{
 				"google-logging-enabled":    "true",
-				"google-monitoring-enabled": "false",
 				"key1":                      "value1",
 				"kdy2":                      "value2"
 			}`,
+			`{
+				"google-monitoring-enabled": "false"
+			}`,
 			&protos.InstanceConfig{
 				HealthMonitorConfig: &protos.HealthMonitorConfig{
 					Enforced:          proto.Bool(true),
@@ -221,6 +257,9 @@
 				"key1":                      "value1",
 				"kdy2":                      "value2"
 			}`,
+			`{
+				"key3": "value3"
+			}`,
 			&protos.InstanceConfig{
 				HealthMonitorConfig: &protos.HealthMonitorConfig{
 					Enforced:          proto.Bool(true),
@@ -231,10 +270,10 @@
 		{
 			"LoggingAndMonitoring",
 			`{
-				"google-logging-enabled":    "true",
-				"google-monitoring-enabled": "true",
-				"key1":                      "value1",
-				"kdy2":                      "value2"
+				"google-logging-enabled":    "true"
+			}`,
+			`{
+				"google-monitoring-enabled": "true"
 			}`,
 			&protos.InstanceConfig{
 				HealthMonitorConfig: &protos.HealthMonitorConfig{
@@ -249,28 +288,25 @@
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
 			// Set up a test server.
-			handler := func(w http.ResponseWriter, r *http.Request) {
-				// Make sure the request is for the directory.
-				if r.URL.Path != instanceCustomDataDirectory {
-					t.Errorf("got URL path %s, want %s",
-						r.URL.Path,
-						instanceCustomDataDirectory)
+			instanceHandler := func(w http.ResponseWriter, r *http.Request) {
+				if _, err := w.Write([]byte(test.instanceMetadata)); err != nil {
+					glog.Errorf("write %q failed: %v", r.URL.Path, err)
 				}
-
-				expectedQuery := "recursive=true"
-				if r.URL.RawQuery != expectedQuery {
-					t.Errorf("got HTTP request query %s, want %s",
-						r.URL.RawQuery,
-						expectedQuery)
-				}
-
-				w.Header().Set("etag", "")
-
-				fmt.Fprint(w, test.metadata)
 			}
-			ts := httptest.NewServer(http.HandlerFunc(handler))
+			projectHandler := func(w http.ResponseWriter, r *http.Request) {
+				if _, err := w.Write([]byte(test.projectMetadata)); err != nil {
+					glog.Errorf("write %q failed: %v", r.URL.Path, err)
+				}
+			}
+			mux := http.NewServeMux()
+			mux.HandleFunc("/instance/attributes/", instanceHandler)
+			mux.HandleFunc("/project/attributes/", projectHandler)
+			ts := httptest.NewServer(mux)
 
-			actualConfig, _, _ := getUserConfig("", ts.URL)
+			actualConfig, _, _, err := getUserConfig("", "", ts.URL)
+			if err != nil {
+				t.Errorf("got unexpected error: %s", err)
+			}
 
 			if !proto.Equal(actualConfig, test.expectedConfig) {
 				t.Errorf("FAILED '%s': got %s, expect %s",
diff --git a/pkg/configfetcher/metadata_retriever.go b/pkg/configfetcher/metadata_retriever.go
index b508afd..649b1f4 100644
--- a/pkg/configfetcher/metadata_retriever.go
+++ b/pkg/configfetcher/metadata_retriever.go
@@ -28,6 +28,7 @@
 // https://cloud.google.com/compute/docs/metadata
 
 import (
+	"context"
 	"fmt"
 	"io/ioutil"
 	"net/http"
@@ -39,18 +40,6 @@
 	// Header to include in every request to the metadata server.
 	requestHeaderKey   = "Metadata-Flavor"
 	requestHeaderValue = "Google"
-
-	// Note that there are 2 types of metadata entries: endpoint and
-	// directory. Endpoint maps to a single value, while directory contains
-	// a list of key value pairs.
-	// URL path for metadata directory. Note that the trailing slash
-	// indicates that the entry is a directory instead of an endpoint.
-	// Without it the request will just be forwared by the metadata
-	// server with a 301 status code.
-	// The leading slash is required since these entries are relative to the
-	// metadata server URL of the form http://.../...
-	instanceCustomDataDirectory = "/instance/attributes/"
-	projectCustomDataDirectory  = "/project/attributes/"
 )
 
 // getEntry performs a HTTP GET request for the metadata entry specified by
@@ -65,7 +54,7 @@
 //	resp: the response body of the HTTP GET request.
 //	etag: "ETag" field of the response head.
 //	err: error.
-func getEntry(entry, metadataURL string) ([]byte, string, error) {
+func getEntry(ctx context.Context, entry, metadataURL string) ([]byte, string, error) {
 	url := metadataURL + entry
 
 	// Construct HTTP client to make the request with the necessary headers.
@@ -73,7 +62,7 @@
 		// Use default policy to stop after 10 consecutive requests.
 		CheckRedirect: nil,
 	}
-	req, err := http.NewRequest("GET", url, nil)
+	req, err := http.NewRequestWithContext(ctx, "GET", url, nil)
 	if err != nil {
 		return nil, "", err
 	}
@@ -101,14 +90,14 @@
 // metadata directory. If the parameter 'lastEtag' is not empty, FetchMetadata
 // won't return until the directory is updated (compared to 'lastEtag').
 // Otherwise, it returns immediately without waiting.
-func FetchMetadata(lastEtag string, gceMetadataURL string) (
+func FetchMetadata(ctx context.Context, lastEtag string, dataDirectory string, gceMetadataURL string) (
 	string, string, error) {
-	path := instanceCustomDataDirectory + "?recursive=true"
+	path := dataDirectory + "?recursive=true"
 	if lastEtag != "" {
 		path += "&wait_for_change=true&last_etag=" + lastEtag
 	}
 
-	resp, etag, err := getEntry(path, gceMetadataURL)
+	resp, etag, err := getEntry(ctx, path, gceMetadataURL)
 	if err != nil {
 		return "", "", err
 	}
diff --git a/pkg/configfetcher/metadata_retriever_test.go b/pkg/configfetcher/metadata_retriever_test.go
index 010a6c9..c43f1f1 100644
--- a/pkg/configfetcher/metadata_retriever_test.go
+++ b/pkg/configfetcher/metadata_retriever_test.go
@@ -15,6 +15,7 @@
 package configfetcher
 
 import (
+	"context"
 	"fmt"
 	"net/http"
 	"net/http/httptest"
@@ -89,8 +90,8 @@
 				fmt.Fprint(w, test.expectResponse)
 			}
 			ts := httptest.NewServer(http.HandlerFunc(handler))
-
-			resp, etag, err := getEntry(test.entry, ts.URL)
+			ctx := context.Background()
+			resp, etag, err := getEntry(ctx, test.entry, ts.URL)
 			if err != nil {
 				if !test.expectErr {
 					t.Errorf("got unexpected error %v", err)
@@ -192,8 +193,8 @@
 				fmt.Fprint(w, test.expectResponse)
 			}
 			ts := httptest.NewServer(http.HandlerFunc(handler))
-
-			resp, etag, err := FetchMetadata(test.lastEtag, ts.URL)
+			ctx := context.Background()
+			resp, etag, err := FetchMetadata(ctx, test.lastEtag, instanceCustomDataDirectory, ts.URL)
 			if err != nil {
 				if !test.expectErr {
 					t.Errorf("got unexpected error %v", err)