Repository: knative/pkg
Pull request #1271 (2020-05-01 01:24:09)

Make updating metrics config and metrics exporter atomic
Avoid using an out-of-date metrics config with a newly created exporter when config-observability is updated. See #1263 for description of potential race condition issues.

This change also works around the potential deadlock with using k8s.io/client-go/kubernetes library within the metrics package described in #807.

The tl;dr of the deadlock cycle is the kubeclient will try and record a latency metric using knative/pkg metrics.record while knative/pkg UpdateExporter will try and use kubeclient to get a Secret for new exporters.

knative-prow-robot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/1271#" title="Author self-approved">anniefu</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **mdemirhan**
You can assign the PR to them by writing `/assign @mdemirhan` in a comment when ready.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative%2Fpkg).

<details open>
Needs approval from an approver in each of these files:

- **[metrics/OWNERS](https://github.com/knative/pkg/blob/master/metrics/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["mdemirhan"]} -->
anniefu: /assign @yanweiguo 
/assing @evankanderson 
knative-metrics-robot: The following is the coverage report on the affected files.
Say `/test pull-knative-pkg-go-coverage` to re-run this coverage report

File | Old Coverage | New Coverage | Delta
---- |:------------:|:------------:|:-----:
[metrics/config.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_pkg/1271/pull-knative-pkg-go-coverage/1256031907280326657/artifacts/line-cov.html#file0) | 94.4% | 92.9% | -1.6
[metrics/exporter.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_pkg/1271/pull-knative-pkg-go-coverage/1256031907280326657/artifacts/line-cov.html#file1) | 87.8% | 88.5% | 0.6
[metrics/opencensus_exporter.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_pkg/1271/pull-knative-pkg-go-coverage/1256031907280326657/artifacts/line-cov.html#file2) | 55.6% | 65.5% | 10.0
[metrics/stackdriver_exporter.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_pkg/1271/pull-knative-pkg-go-coverage/1256031907280326657/artifacts/line-cov.html#file3) | 70.4% | 80.2% | 9.9


Patch:
diff --git a/go.mod b/go.mod
index 20fba60b7..219e83140 100644
--- a/go.mod
+++ b/go.mod
@@ -65,7 +65,7 @@ require (
 	k8s.io/kube-openapi v0.0.0-20190816220812-743ec37842bf
 	k8s.io/test-infra v0.0.0-20191212060232-70b0b49fe247
 	k8s.io/utils v0.0.0-20190907131718-3d4f5b7dea0b // indirect
-	knative.dev/test-infra v0.0.0-20200429211942-f4c4853375cf
+	knative.dev/test-infra v0.0.0-20200430225942-f7c1fafc1cde
 )
 
 replace (
diff --git a/go.sum b/go.sum
index dc743314b..fd82e8c49 100644
--- a/go.sum
+++ b/go.sum
@@ -906,8 +906,8 @@ k8s.io/utils v0.0.0-20190801114015-581e00157fb1/go.mod h1:sZAwmy6armz5eXlNoLmJcl
 k8s.io/utils v0.0.0-20190907131718-3d4f5b7dea0b h1:eMM0sTvh3KBVGwJfuNcU86P38TJhlVMAICbFPDG3t0M=
 k8s.io/utils v0.0.0-20190907131718-3d4f5b7dea0b/go.mod h1:sZAwmy6armz5eXlNoLmJcl4F1QuKu7sr+mFQ0byX7Ew=
 knative.dev/pkg v0.0.0-20191101194912-56c2594e4f11/go.mod h1:pgODObA1dTyhNoFxPZTTjNWfx6F0aKsKzn+vaT9XO/Q=
-knative.dev/test-infra v0.0.0-20200429211942-f4c4853375cf h1:rNWg3NiXNLjZC9C1EJf2qKA+mRnrWMLW1KONsEusLYg=
-knative.dev/test-infra v0.0.0-20200429211942-f4c4853375cf/go.mod h1:xcdUkMJrLlBswIZqL5zCuBFOC22WIPMQoVX1L35i0vQ=
+knative.dev/test-infra v0.0.0-20200430225942-f7c1fafc1cde h1:QSzxFsf21WXNhODvh0jRKbFR+c5UI7WFjiISy/sMOLg=
+knative.dev/test-infra v0.0.0-20200430225942-f7c1fafc1cde/go.mod h1:xcdUkMJrLlBswIZqL5zCuBFOC22WIPMQoVX1L35i0vQ=
 modernc.org/cc v1.0.0/go.mod h1:1Sk4//wdnYJiUIxnW8ddKpaOJCF37yAdqYnkxUpaYxw=
 modernc.org/golex v1.0.0/go.mod h1:b/QX9oBD/LhixY6NDh+IdGv17hgB+51fET1i2kPSmvk=
 modernc.org/mathutil v1.0.0/go.mod h1:wU0vUrJsVWBZ4P6e7xtFJEhFSNsfRLJ8H458uRjg03k=
diff --git a/metrics/config.go b/metrics/config.go
index 4403ddb3d..71161baa1 100644
--- a/metrics/config.go
+++ b/metrics/config.go
@@ -29,6 +29,7 @@ import (
 
 	"go.opencensus.io/stats"
 	"go.uber.org/zap"
+	corev1 "k8s.io/api/core/v1"
 	"knative.dev/pkg/metrics/metricskey"
 )
 
@@ -88,9 +89,8 @@ type metricsConfig struct {
 	// writing the metrics to the stats.RecordWithOptions interface.
 	recorder func(context.Context, []stats.Measurement, ...stats.Options) error
 
-	// secretFetcher provides access for fetching Kubernetes Secrets from an
-	// informer cache.
-	secretFetcher SecretFetcher
+	// secret contains credentials for an exporter to use for authentication.
+	secret *corev1.Secret
 
 	// ---- OpenCensus specific below ----
 	// collectorAddress is the address of the collector, if not `localhost:55678`
@@ -162,10 +162,6 @@ func (mc *metricsConfig) record(ctx context.Context, mss []stats.Measurement, ro
 func createMetricsConfig(ops ExporterOptions, logger *zap.SugaredLogger) (*metricsConfig, error) {
 	var mc metricsConfig
 
-	// We don't check if this is `nil` right now, because this is a transition step.
-	// Eventually, this should be a startup check.
-	mc.secretFetcher = ops.Secrets
-
 	if ops.Domain == "" {
 		return nil, errors.New("metrics domain cannot be empty")
 	}
@@ -205,6 +201,13 @@ func createMetricsConfig(ops ExporterOptions, logger *zap.SugaredLogger) (*metri
 			if mc.requireSecure, err = strconv.ParseBool(isSecure); err != nil {
 				return nil, fmt.Errorf("invalid %s value %q", CollectorSecureKey, isSecure)
 			}
+
+			if mc.requireSecure {
+				mc.secret, err = getOpenCensusSecret(ops.Component, ops.Secrets)
+				if err != nil {
+					return nil, err
+				}
+			}
 		}
 	}
 
@@ -265,6 +268,15 @@ func createMetricsConfig(ops ExporterOptions, logger *zap.SugaredLogger) (*metri
 				return stats.RecordWithOptions(ctx, append(ros, stats.WithMeasurements(mss...))...)
 			}
 		}
+
+		if scc.UseSecret {
+			secret, err := getStackdriverSecret(ops.Secrets)
+			if err != nil {
+				return nil, err
+			}
+
+			mc.secret = secret
+		}
 	}
 
 	// If reporting period is specified, use the value from the configuration.
diff --git a/metrics/exporter.go b/metrics/exporter.go
index f3f0006e4..b8f4fd12f 100644
--- a/metrics/exporter.go
+++ b/metrics/exporter.go
@@ -121,6 +121,7 @@ func UpdateExporterFromConfigMapWithOpts(opts ExporterOptions, logger *zap.Sugar
 			Component:      opts.Component,
 			ConfigMap:      configMap.Data,
 			PrometheusPort: opts.PrometheusPort,
+			Secrets:        opts.Secrets,
 		}, logger)
 	}, nil
 }
@@ -130,6 +131,7 @@ func UpdateExporterFromConfigMapWithOpts(opts ExporterOptions, logger *zap.Sugar
 // to prevent a race condition between reading the current configuration
 // and updating the current exporter.
 func UpdateExporter(ops ExporterOptions, logger *zap.SugaredLogger) error {
+	// TODO(https://github.com/knative/pkg/issues/1273): check if ops.secrets is `nil` after new metrics plan lands
 	newConfig, err := createMetricsConfig(ops, logger)
 	if err != nil {
 		if getCurMetricsConfig() == nil {
@@ -141,28 +143,33 @@ func UpdateExporter(ops ExporterOptions, logger *zap.SugaredLogger) error {
 		return err
 	}
 
+	// Updating the metrics config and the metrics exporters needs to be atomic to
+	// avoid using an outdated metrics config with new exporters.
+	metricsMux.Lock()
+	defer metricsMux.Unlock()
+
 	if isNewExporterRequired(newConfig) {
 		logger.Info("Flushing the existing exporter before setting up the new exporter.")
-		FlushExporter()
+		flushGivenExporter(curMetricsExporter)
 		e, err := newMetricsExporter(newConfig, logger)
 		if err != nil {
 			logger.Errorf("Failed to update a new metrics exporter based on metric config %v. error: %v", newConfig, err)
 			return err
 		}
-		existingConfig := getCurMetricsConfig()
-		setCurMetricsExporter(e)
+		existingConfig := curMetricsConfig
+		curMetricsExporter = e
 		logger.Infof("Successfully updated the metrics exporter; old config: %v; new config %v", existingConfig, newConfig)
 	}
 
-	setCurMetricsConfig(newConfig)
+	setCurMetricsConfigUnlocked(newConfig)
 	return nil
 }
 
 // isNewExporterRequired compares the non-nil newConfig against curMetricsConfig. When backend changes,
 // or stackdriver project ID changes for stackdriver backend, we need to update the metrics exporter.
-// This function is not implicitly thread-safe.
+// This function must be called with the metricsMux reader (or writer) locked.
 func isNewExporterRequired(newConfig *metricsConfig) bool {
-	cc := getCurMetricsConfig()
+	cc := curMetricsConfig
 	if cc == nil || newConfig.backendDestination != cc.backendDestination {
 		return true
 	}
@@ -177,15 +184,14 @@ func isNewExporterRequired(newConfig *metricsConfig) bool {
 }
 
 // newMetricsExporter gets a metrics exporter based on the config.
-// This function is not implicitly thread-safe.
+// This function must be called with the metricsMux reader (or writer) locked.
 func newMetricsExporter(config *metricsConfig, logger *zap.SugaredLogger) (view.Exporter, error) {
-	ce := getCurMetricsExporter()
 	// If there is a Prometheus Exporter server running, stop it.
 	resetCurPromSrv()
 
 	// TODO(https://github.com/knative/pkg/issues/866): Move Stackdriver and Promethus
 	// operations before stopping to an interface.
-	if se, ok := ce.(stoppable); ok {
+	if se, ok := curMetricsExporter.(stoppable); ok {
 		se.StopMetricsExporter()
 	}
 
@@ -230,6 +236,10 @@ func getCurMetricsConfig() *metricsConfig {
 func setCurMetricsConfig(c *metricsConfig) {
 	metricsMux.Lock()
 	defer metricsMux.Unlock()
+	setCurMetricsConfigUnlocked(c)
+}
+
+func setCurMetricsConfigUnlocked(c *metricsConfig) {
 	if c != nil {
 		view.SetReportingPeriod(c.reportingPeriod)
 	} else {
@@ -244,6 +254,10 @@ func setCurMetricsConfig(c *metricsConfig) {
 // Return value indicates whether the exporter is flushable or not.
 func FlushExporter() bool {
 	e := getCurMetricsExporter()
+	return flushGivenExporter(e)
+}
+
+func flushGivenExporter(e view.Exporter) bool {
 	if e == nil {
 		return false
 	}
diff --git a/metrics/opencensus_exporter.go b/metrics/opencensus_exporter.go
index 5e3924646..dd80c855d 100644
--- a/metrics/opencensus_exporter.go
+++ b/metrics/opencensus_exporter.go
@@ -24,6 +24,7 @@ import (
 	"go.opencensus.io/stats/view"
 	"go.uber.org/zap"
 	"google.golang.org/grpc/credentials"
+	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
 )
 
@@ -33,49 +34,53 @@ func newOpenCensusExporter(config *metricsConfig, logger *zap.SugaredLogger) (vi
 		opts = append(opts, ocagent.WithAddress(config.collectorAddress))
 	}
 	if config.requireSecure {
-		opts = append(opts, ocagent.WithTLSCredentials(credentialFetcher(config.component, config.secretFetcher, logger)))
+		opts = append(opts, ocagent.WithTLSCredentials(getCredentials(config.component, config.secret, logger)))
 	} else {
 		opts = append(opts, ocagent.WithInsecure())
 	}
 	e, err := ocagent.NewExporter(opts...)
 	if err != nil {
-		logger.Errorw("Failed to create the OpenCensus exporter.", zap.Error(err))
+		logger.Errorw("failed to create the OpenCensus exporter.", zap.Error(err))
 		return nil, err
 	}
-	logger.Infof("Created OpenCensus exporter with config: %+v.", *config)
+	logger.Infof("created OpenCensus exporter with config: %+v.", *config)
 	view.RegisterExporter(e)
 	return e, nil
 }
 
-// credentialFetcher attempts to locate a secret containing TLS credentials
+// getOpenCensusSecret attempts to locate a secret containing TLS credentials
 // for communicating with the OpenCensus Agent. To do this, it first looks
 // for a secret named "<component>-opencensus", then for a generic
 // "opencensus" secret.
-func credentialFetcher(component string, lister SecretFetcher, logger *zap.SugaredLogger) credentials.TransportCredentials {
+func getOpenCensusSecret(component string, lister SecretFetcher) (*corev1.Secret, error) {
 	if lister == nil {
-		logger.Errorf("No secret lister provided for component %q; cannot use requireSecure=true", component)
+		return nil, fmt.Errorf("no secret lister provided for component %q; cannot use requireSecure=true", component)
+	}
+	secret, err := lister(component + "-opencensus")
+	if errors.IsNotFound(err) {
+		secret, err = lister("opencensus")
+	}
+	if err != nil {
+		return nil, fmt.Errorf("unable to fetch opencensus secret for %q, cannot use requireSecure=true: %+v", component, err)
+	}
+
+	return secret, nil
+}
+
+// getCredentials attempts to create a certificate containing TLS credentials
+// for communicating with the OpenCensus Agent.
+func getCredentials(component string, secret *corev1.Secret, logger *zap.SugaredLogger) credentials.TransportCredentials {
+	if secret == nil {
+		logger.Errorf("no secret provided for component %q; cannot use requireSecure=true", component)
 		return nil
 	}
 	return credentials.NewTLS(&tls.Config{
 		GetClientCertificate: func(*tls.CertificateRequestInfo) (*tls.Certificate, error) {
-			// We ignore the CertificateRequestInfo for now, and hand back a single fixed certificate.
-			// TODO(evankanderson): maybe do something SPIFFE-ier?
-			cert, err := certificateFetcher(component+"-opencensus", lister)
-			if errors.IsNotFound(err) {
-				cert, err = certificateFetcher("opencensus", lister)
-			}
+			cert, err := tls.X509KeyPair(secret.Data["client-cert.pem"], secret.Data["client-key.pem"])
 			if err != nil {
-				return nil, fmt.Errorf("Unable to fetch opencensus secret for %q, cannot use requireSecure=true: %+v", component, err)
+				return nil, err
 			}
-			return &cert, err
+			return &cert, nil
 		},
 	})
 }
-
-func certificateFetcher(secretName string, lister SecretFetcher) (tls.Certificate, error) {
-	secret, err := lister(secretName)
-	if err != nil {
-		return tls.Certificate{}, err
-	}
-	return tls.X509KeyPair(secret.Data["client-cert.pem"], secret.Data["client-key.pem"])
-}
diff --git a/metrics/stackdriver_exporter.go b/metrics/stackdriver_exporter.go
index e87a3e37f..87bb722a2 100644
--- a/metrics/stackdriver_exporter.go
+++ b/metrics/stackdriver_exporter.go
@@ -115,7 +115,7 @@ func newOpencensusSDExporter(o stackdriver.Options) (view.Exporter, error) {
 func newStackdriverExporter(config *metricsConfig, logger *zap.SugaredLogger) (view.Exporter, error) {
 	gm := getMergedGCPMetadata(config)
 	mpf := getMetricPrefixFunc(config.stackdriverMetricTypePrefix, config.stackdriverCustomMetricTypePrefix)
-	co, err := getStackdriverExporterClientOptions(&config.stackdriverClientConfig)
+	co, err := getStackdriverExporterClientOptions(config)
 	if err != nil {
 		logger.Warnw("Issue configuring Stackdriver exporter client options, no additional client options will be used: ", zap.Error(err))
 	}
@@ -140,21 +140,21 @@ func newStackdriverExporter(config *metricsConfig, logger *zap.SugaredLogger) (v
 
 // getStackdriverExporterClientOptions creates client options for the opencensus Stackdriver exporter from the given stackdriverClientConfig.
 // On error, an empty array of client options is returned.
-func getStackdriverExporterClientOptions(sdconfig *StackdriverClientConfig) ([]option.ClientOption, error) {
+func getStackdriverExporterClientOptions(config *metricsConfig) ([]option.ClientOption, error) {
 	var co []option.ClientOption
-	if sdconfig.UseSecret && useStackdriverSecretEnabled {
-		secret, err := getStackdriverSecret(sdconfig)
-		if err != nil {
-			return co, err
+
+	// SetStackdriverSecretLocation must have been called by calling package for this to work.
+	if config.stackdriverClientConfig.UseSecret {
+		if config.secret == nil {
+			return co, fmt.Errorf("No secret provided for component %q; cannot use stackdriver-use-secret=true", config.component)
 		}
 
-		if opt, err := convertSecretToExporterOption(secret); err == nil {
+		if opt, err := convertSecretToExporterOption(config.secret); err == nil {
 			co = append(co, opt)
 		} else {
 			return co, err
 		}
 	}
-
 	return co, nil
 }
 
@@ -215,19 +215,31 @@ func getMetricPrefixFunc(metricTypePrefix, customMetricTypePrefix string) func(n
 }
 
 // getStackdriverSecret returns the Kubernetes Secret specified in the given config.
+// SetStackdriverSecretLocation must have been called by calling package for this to work.
 // TODO(anniefu): Update exporter if Secret changes (https://github.com/knative/pkg/issues/842)
-func getStackdriverSecret(sdconfig *StackdriverClientConfig) (*corev1.Secret, error) {
-	if err := ensureKubeclient(); err != nil {
-		return nil, err
-	}
-
+func getStackdriverSecret(secretFetcher SecretFetcher) (*corev1.Secret, error) {
 	stackdriverMtx.RLock()
 	defer stackdriverMtx.RUnlock()
 
-	sec, secErr := kubeclient.CoreV1().Secrets(secretNamespace).Get(secretName, metav1.GetOptions{})
+	if !useStackdriverSecretEnabled {
+		return nil, nil
+	}
+
+	var secErr error
+	var sec *corev1.Secret
+	if secretFetcher != nil {
+		sec, secErr = secretFetcher(fmt.Sprintf("%s/%s", secretNamespace, secretName))
+	} else {
+		// This else-block can be removed once UpdateExporterFromConfigMap is fully deprecated in favor of ConfigMapWatcher
+		if err := ensureKubeclient(); err != nil {
+			return nil, err
+		}
+
+		sec, secErr = kubeclient.CoreV1().Secrets(secretNamespace).Get(secretName, metav1.GetOptions{})
+	}
 
 	if secErr != nil {
-		return nil, fmt.Errorf("Error getting Secret [%s] in namespace [%s]: %w", secretName, secretNamespace, secErr)
+		return nil, fmt.Errorf("error getting Secret [%v] in namespace [%v]: %v", secretName, secretNamespace, secErr)
 	}
 
 	return sec, nil
diff --git a/vendor/modules.txt b/vendor/modules.txt
index 61b7731a1..75d2c8c39 100644
--- a/vendor/modules.txt
+++ b/vendor/modules.txt
@@ -868,7 +868,7 @@ k8s.io/utils/buffer
 k8s.io/utils/integer
 k8s.io/utils/pointer
 k8s.io/utils/trace
-# knative.dev/test-infra v0.0.0-20200429211942-f4c4853375cf
+# knative.dev/test-infra v0.0.0-20200430225942-f7c1fafc1cde
 knative.dev/test-infra/scripts
 # sigs.k8s.io/yaml v1.1.0
 sigs.k8s.io/yaml


Test patch:
diff --git a/metrics/config_test.go b/metrics/config_test.go
index 914f9ed52..1fd0d18dd 100644
--- a/metrics/config_test.go
+++ b/metrics/config_test.go
@@ -32,6 +32,7 @@ import (
 	"knative.dev/pkg/metrics/metricstest"
 
 	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
 // TODO UTs should move to eventing and serving, as appropriate.
@@ -201,6 +202,12 @@ var (
 				},
 				Domain:    servingDomain,
 				Component: testComponent,
+				Secrets: fakeSecretList(corev1.Secret{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      StackdriverSecretNameDefault,
+						Namespace: StackdriverSecretNamespaceDefault,
+					},
+				}).Get,
 			},
 			expectedConfig: metricsConfig{
 				domain:                            servingDomain,
@@ -216,6 +223,12 @@ var (
 					ClusterName: "cluster",
 					UseSecret:   true,
 				},
+				secret: &corev1.Secret{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      StackdriverSecretNameDefault,
+						Namespace: StackdriverSecretNamespaceDefault,
+					},
+				},
 			},
 			expectedNewExporter: true,
 		}, {
@@ -253,6 +266,15 @@ var (
 				},
 				Domain:    servingDomain,
 				Component: testComponent,
+				Secrets: fakeSecretList(corev1.Secret{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "opencensus",
+					},
+					Data: map[string][]byte{
+						"client-cert.pem": {},
+						"client-key.pem":  {},
+					},
+				}).Get,
 			},
 			expectedConfig: metricsConfig{
 				domain:             servingDomain,
@@ -260,6 +282,15 @@ var (
 				backendDestination: OpenCensus,
 				collectorAddress:   "external-svc:55678",
 				requireSecure:      true,
+				secret: &corev1.Secret{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "opencensus",
+					},
+					Data: map[string][]byte{
+						"client-cert.pem": {},
+						"client-key.pem":  {},
+					},
+				},
 			},
 			expectedNewExporter: true,
 		}, {
@@ -514,6 +545,10 @@ var (
 		}}
 )
 
+func successTestsInit() {
+	SetStackdriverSecretLocation(StackdriverSecretNameDefault, StackdriverSecretNamespaceDefault)
+}
+
 func TestGetMetricsConfig(t *testing.T) {
 	for _, test := range errorTests {
 		t.Run(test.name, func(t *testing.T) {
@@ -525,6 +560,7 @@ func TestGetMetricsConfig(t *testing.T) {
 		})
 	}
 
+	successTestsInit()
 	for _, test := range successTests {
 		t.Run(test.name, func(t *testing.T) {
 			defer ClearAll()
@@ -558,6 +594,7 @@ func TestGetMetricsConfig_fromEnv(t *testing.T) {
 
 func TestIsNewExporterRequiredFromNilConfig(t *testing.T) {
 	setCurMetricsConfig(nil)
+	successTestsInit()
 	for _, test := range successTests {
 		t.Run(test.name, func(t *testing.T) {
 			defer ClearAll()
@@ -685,6 +722,7 @@ func TestIsNewExporterRequired(t *testing.T) {
 func TestUpdateExporter(t *testing.T) {
 	setCurMetricsConfig(nil)
 	oldConfig := getCurMetricsConfig()
+	successTestsInit()
 	for _, test := range successTests[1:] {
 		t.Run(test.name, func(t *testing.T) {
 			defer ClearAll()
@@ -715,6 +753,7 @@ func TestUpdateExporter(t *testing.T) {
 func TestUpdateExporterFromConfigMapWithOpts(t *testing.T) {
 	setCurMetricsConfig(nil)
 	oldConfig := getCurMetricsConfig()
+	successTestsInit()
 	for _, test := range successTests[1:] {
 		t.Run(test.name, func(t *testing.T) {
 			defer ClearAll()
@@ -722,6 +761,7 @@ func TestUpdateExporterFromConfigMapWithOpts(t *testing.T) {
 				Component:      test.ops.Component,
 				Domain:         test.ops.Domain,
 				PrometheusPort: test.ops.PrometheusPort,
+				Secrets:        test.ops.Secrets,
 			}
 			updateFunc, err := UpdateExporterFromConfigMapWithOpts(opts, TestLogger(t))
 			if err != nil {
diff --git a/metrics/opencensus_exporter_test.go b/metrics/opencensus_exporter_test.go
index c21380fe6..40ba726cd 100644
--- a/metrics/opencensus_exporter_test.go
+++ b/metrics/opencensus_exporter_test.go
@@ -65,7 +65,7 @@ func TestOpenCensusConfig(t *testing.T) {
 			domain:             "secure",
 			component:          "test",
 			backendDestination: OpenCensus,
-			secretFetcher: fakeSecretList(corev1.Secret{
+			secret: &corev1.Secret{
 				ObjectMeta: metav1.ObjectMeta{
 					Name: "test-opencensus",
 				},
@@ -73,7 +73,7 @@ func TestOpenCensusConfig(t *testing.T) {
 					"client-cert.pem": cert,
 					"client-key.pem":  key,
 				},
-			}).Get,
+			},
 			requireSecure: true,
 		},
 		tls: &tls.Config{},
@@ -140,6 +140,10 @@ func fakeSecretList(s ...corev1.Secret) *fakeSecrets {
 
 func (f *fakeSecrets) Get(name string) (*corev1.Secret, error) {
 	for _, s := range f.secrets {
+		if fmt.Sprintf("%s/%s", s.Namespace, s.Name) == name {
+			return &s, nil
+		}
+
 		if s.Name == name {
 			return &s, nil
 		}
diff --git a/metrics/stackdriver_exporter_test.go b/metrics/stackdriver_exporter_test.go
index 6f4663d03..e88901045 100644
--- a/metrics/stackdriver_exporter_test.go
+++ b/metrics/stackdriver_exporter_test.go
@@ -24,6 +24,8 @@ import (
 	"contrib.go.opencensus.io/exporter/stackdriver"
 	"go.opencensus.io/metric/metricdata"
 	"go.opencensus.io/stats/view"
+	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	. "knative.dev/pkg/logging/testing"
 	"knative.dev/pkg/metrics/metricskey"
 )
@@ -471,48 +473,45 @@ func assertStringsEqual(t *testing.T, description string, expected string, actua
 }
 
 func TestSetStackdriverSecretLocation(t *testing.T) {
+	// Prevent pollution from other tests
+	useStackdriverSecretEnabled = false
 	// Reset global state after test
 	defer func() {
 		secretName = StackdriverSecretNameDefault
 		secretNamespace = StackdriverSecretNamespaceDefault
+		useStackdriverSecretEnabled = false
 	}()
 
-	sdConfig := &StackdriverClientConfig{
-		ProjectID:   "project",
-		GCPLocation: "us-west2",
-		ClusterName: "cluster",
-		UseSecret:   false,
+	const testName, testNamespace = "test-name", "test-namespace"
+	secretFetcher := func(name string) (*corev1.Secret, error) {
+		return &corev1.Secret{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      testName,
+				Namespace: testNamespace,
+			},
+		}, nil
 	}
 
 	// Sanity checks
 	assertStringsEqual(t, "DefaultSecretName", secretName, StackdriverSecretNameDefault)
 	assertStringsEqual(t, "DefaultSecretNamespace", secretNamespace, StackdriverSecretNamespaceDefault)
-	if _, err := getStackdriverExporterClientOptions(sdConfig); err != nil {
-		t.Errorf("Got unexpected error when creating exporter client options: [%v]", err)
+	sec, err := getStackdriverSecret(secretFetcher)
+	if err != nil {
+		t.Errorf("Got unexpected error when getting secret: %v", err)
 	}
-
-	// Check configuration's UseSecret value is ignored until the consuming package calls SetStackdriverSecretLocation
-	// If an attempt to read a Secret was made, there should be an error because there's no valid in-cluster kubeclient.
-	sdConfig.UseSecret = true
-	if _, e1 := getStackdriverExporterClientOptions(sdConfig); e1 != nil {
-		t.Errorf("Got unexpected error when creating exporter client options: [%v]", e1)
+	if sec != nil {
+		t.Errorf("Stackdriver secret should not be fetched unless SetStackdriverSecretLocation has been called")
 	}
 
-	testName, testNamespace := "test-name", "test-namespace"
-	// SetStackdriverSecretLocation has been called & config's UseSecret value is set
-	// There should be an attempt to read the Secret, and an error because there's no valid in-cluster kubeclient.
-	SetStackdriverSecretLocation("test-name", "test-namespace")
-	if _, e1 := getStackdriverExporterClientOptions(sdConfig); e1 == nil {
-		t.Errorf("Expected a known error when getting exporter options with Secrets enabled (cannot create valid kubeclient in tests). Did the function run as expected?")
+	// Once SetStackdriverSecretLocation has been called, attempts to get the secret should complete.
+	SetStackdriverSecretLocation(testName, testNamespace)
+	sec, err = getStackdriverSecret(secretFetcher)
+	if err != nil {
+		t.Errorf("Got unexpected error when getting secret: %v", err)
+	}
+	if sec == nil {
+		t.Error("expected secret to be non-nil if there is no error and SetStackdriverSecretLocation has been called")
 	}
 	assertStringsEqual(t, "secretName", secretName, testName)
 	assertStringsEqual(t, "secretNamespace", secretNamespace, testNamespace)
-
-	randomName, randomNamespace := "random-name", "random-namespace"
-	SetStackdriverSecretLocation(randomName, randomNamespace)
-	if _, e1 := getStackdriverExporterClientOptions(sdConfig); e1 == nil {
-		t.Errorf("Expected a known error when getting exporter options with Secrets enabled (cannot create valid kubeclient in tests). Did the function run as expected?")
-	}
-	assertStringsEqual(t, "secretName", secretName, randomName)
-	assertStringsEqual(t, "secretNamespace", secretNamespace, randomNamespace)
 }
diff --git a/vendor/knative.dev/test-infra/scripts/library.sh b/vendor/knative.dev/test-infra/scripts/library.sh
index a8aa72e73..b90f4e1d7 100644
--- a/vendor/knative.dev/test-infra/scripts/library.sh
+++ b/vendor/knative.dev/test-infra/scripts/library.sh
@@ -483,16 +483,22 @@ function run_go_tool() {
   [[ $1 =~ ^[\./].* ]] && action=install
   # Avoid running `go get` from root dir of the repository, as it can change go.sum and go.mod files.
   # See discussions in https://github.com/golang/go/issues/27643.
-  if [[ ${action} == "get" && $(pwd) == "${REPO_ROOT_DIR}" ]]; then
-    local temp_dir="$(mktemp -d)"
-    local install_failed=0
-    # Swallow the output as we are returning the stdout in the end.
-    pushd "${temp_dir}" > /dev/null 2>&1
-    GOFLAGS="" go ${action} $1 || install_failed=1
-    popd > /dev/null 2>&1
-    (( install_failed )) && return ${install_failed}
-  else
-    GOFLAGS="" go ${action} $1
+  if [[ -z "$(which ${tool})" ]]; then
+    local action=get
+    [[ $1 =~ ^[\./].* ]] && action=install
+    # Avoid running `go get` from root dir of the repository, as it can change go.sum and go.mod files.
+    # See discussions in https://github.com/golang/go/issues/27643.
+    if [[ ${action} == "get" && $(pwd) == "${REPO_ROOT_DIR}" ]]; then
+      local temp_dir="$(mktemp -d)"
+      local install_failed=0
+      # Swallow the output as we are returning the stdout in the end.
+      pushd "${temp_dir}" > /dev/null 2>&1
+      GOFLAGS="" go ${action} $1 || install_failed=1
+      popd > /dev/null 2>&1
+      (( install_failed )) && return ${install_failed}
+    else
+      GOFLAGS="" go ${action} $1
+    fi
   fi
   shift 2
   ${tool} "$@"