Repository: knative/pkg
Pull request #1592 (2020-08-06 21:23:55)

filter "control-plane" labeled namespaces out of our webhooks
fixes: #1590 

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

This pull-request has been approved by: *<a href="https://github.com/knative/pkg/pull/1592#" title="Author self-approved">n3wscott</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **dprotaso**
You can assign the PR to them by writing `/assign @dprotaso` 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:

- **[webhook/OWNERS](https://github.com/knative/pkg/blob/master/webhook/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":["dprotaso"]} -->
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
---- |:------------:|:------------:|:-----:
[webhook/resourcesemantics/defaulting/defaulting.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_pkg/1592/pull-knative-pkg-go-coverage/1291485280280252416/artifacts/line-cov.html#file0) | 87.4% | 86.7% | -0.7
[webhook/resourcesemantics/validation/reconcile_config.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative_pkg/1592/pull-knative-pkg-go-coverage/1291485280280252416/artifacts/line-cov.html#file1) | 91.7% | 89.6% | -2.1

knative-prow-robot: @n3wscott: The following test **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-knative-pkg-unit-tests | 409484249fb373a965696c889edace467c337ab6 | [link](https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative_pkg/1592/pull-knative-pkg-unit-tests/1291485280280252417) | `/test pull-knative-pkg-unit-tests`

[Full PR test history](https://gubernator.knative.dev/pr/knative_pkg/1592). [Your PR dashboard](https://gubernator.knative.dev/pr/n3wscott).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
knative-prow-robot: @n3wscott: The following test **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-knative-pkg-unit-tests | 9b2791127f77d0fb5fc9eae6ed7c49769ce7dc5d | [link](https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative_pkg/1592/pull-knative-pkg-unit-tests/1291488550994317315) | `/test pull-knative-pkg-unit-tests`

[Full PR test history](https://gubernator.knative.dev/pr/knative_pkg/1592). [Your PR dashboard](https://gubernator.knative.dev/pr/n3wscott).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->

Patch:
diff --git a/configmap/informed_watcher.go b/configmap/informed_watcher.go
index 3231e7d28..16447b634 100644
--- a/configmap/informed_watcher.go
+++ b/configmap/informed_watcher.go
@@ -33,13 +33,6 @@ import (
 	"k8s.io/client-go/tools/cache"
 )
 
-// NewDefaultWatcher creates a new default configmap.Watcher instance.
-//
-// Deprecated: Use NewInformedWatcher
-func NewDefaultWatcher(kc kubernetes.Interface, namespace string) *InformedWatcher {
-	return NewInformedWatcher(kc, namespace)
-}
-
 // NewInformedWatcherFromFactory watches a Kubernetes namespace for ConfigMap changes.
 func NewInformedWatcherFromFactory(sif informers.SharedInformerFactory, namespace string) *InformedWatcher {
 	return &InformedWatcher{
diff --git a/controller/controller.go b/controller/controller.go
index e32db29bd..121db8567 100644
--- a/controller/controller.go
+++ b/controller/controller.go
@@ -262,7 +262,9 @@ func (c *Impl) EnqueueAfter(obj interface{}, after time.Duration) {
 // and enqueues that key in the slow lane.
 func (c *Impl) EnqueueSlowKey(key types.NamespacedName) {
 	c.workQueue.SlowLane().Add(key)
-	c.logger.Debugf("Adding to the slow queue %s (depth(total/slow): %d/%d)", safeKey(key), c.workQueue.Len(), c.workQueue.SlowLane().Len())
+	c.logger.With(zap.Object(logkey.Key, logging.NamespacedName(key))).
+		Debug("Adding to the slow queue %s (depth(total/slow): %d/%d)",
+			safeKey(key), c.workQueue.Len(), c.workQueue.SlowLane().Len())
 }
 
 // EnqueueSlow extracts namesspeced name from the object and enqueues it on the slow
@@ -388,7 +390,8 @@ func (c *Impl) EnqueueNamespaceOf(obj interface{}) {
 // EnqueueKey takes a namespace/name string and puts it onto the work queue.
 func (c *Impl) EnqueueKey(key types.NamespacedName) {
 	c.workQueue.Add(key)
-	c.logger.Debugf("Adding to queue %s (depth: %d)", safeKey(key), c.workQueue.Len())
+	c.logger.With(zap.Object(logkey.Key, logging.NamespacedName(key))).
+		Debugf("Adding to queue %s (depth: %d)", safeKey(key), c.workQueue.Len())
 }
 
 // MaybeEnqueueBucketKey takes a Bucket and namespace/name string and puts it onto
@@ -403,7 +406,8 @@ func (c *Impl) MaybeEnqueueBucketKey(bkt reconciler.Bucket, key types.Namespaced
 // the work queue after given delay.
 func (c *Impl) EnqueueKeyAfter(key types.NamespacedName, delay time.Duration) {
 	c.workQueue.AddAfter(key, delay)
-	c.logger.Debugf("Adding to queue %s (delay: %v, depth: %d)", safeKey(key), delay, c.workQueue.Len())
+	c.logger.With(zap.Object(logkey.Key, logging.NamespacedName(key))).
+		Debugf("Adding to queue %s (delay: %v, depth: %d)", safeKey(key), delay, c.workQueue.Len())
 }
 
 // RunContext starts the controller's worker threads, the number of which is threadiness.
diff --git a/metrics/opencensus_exporter.go b/metrics/opencensus_exporter.go
index 651d3ce66..d94b53e69 100644
--- a/metrics/opencensus_exporter.go
+++ b/metrics/opencensus_exporter.go
@@ -52,11 +52,15 @@ func newOpenCensusExporter(config *metricsConfig, logger *zap.SugaredLogger) (vi
 	}
 	logger.Infow("Created OpenCensus exporter with config:", zap.Any("config", *config))
 	view.RegisterExporter(e)
-	return e, getFactory(opts), nil
+	return e, getFactory(e, opts), nil
 }
 
-func getFactory(stored []ocagent.ExporterOption) ResourceExporterFactory {
+func getFactory(defaultExporter view.Exporter, stored []ocagent.ExporterOption) ResourceExporterFactory {
 	return func(r *resource.Resource) (view.Exporter, error) {
+		if r == nil || (r.Type == "" && len(r.Labels) == 0) {
+			// Don't create duplicate exporters for the default exporter.
+			return defaultExporter, nil
+		}
 		opts := append(stored, ocagent.WithResourceDetector(
 			func(context.Context) (*resource.Resource, error) {
 				return r, nil
diff --git a/metrics/resource_view.go b/metrics/resource_view.go
index 2fcf24ef9..7d69ae94e 100644
--- a/metrics/resource_view.go
+++ b/metrics/resource_view.go
@@ -177,6 +177,21 @@ func flushResourceExporters() {
 	}
 }
 
+// ClearMetersForTest clears the internal set of metrics being exported,
+// including cleaning up background threads.
+func ClearMetersForTest() {
+	allMeters.lock.Lock()
+	defer allMeters.lock.Unlock()
+
+	for k, meter := range allMeters.meters {
+		if k == "" {
+			continue
+		}
+		meter.m.Stop()
+		delete(allMeters.meters, k)
+	}
+}
+
 func meterExporterForResource(r *resource.Resource) *meterExporter {
 	key := resourceToKey(r)
 	mE := allMeters.meters[key]
diff --git a/metrics/stackdriver_exporter.go b/metrics/stackdriver_exporter.go
index 44ae68b68..befb95dba 100644
--- a/metrics/stackdriver_exporter.go
+++ b/metrics/stackdriver_exporter.go
@@ -21,6 +21,7 @@ import (
 	"fmt"
 	"path"
 	"sync"
+	"time"
 
 	sd "contrib.go.opencensus.io/exporter/stackdriver"
 	"go.opencensus.io/resource"
@@ -49,6 +50,8 @@ const (
 	StackdriverSecretNameDefault = "stackdriver-service-account-key"
 	// secretDataFieldKey is the name of the k8s Secret field that contains the Secret's key.
 	secretDataFieldKey = "key.json"
+	// stackdriverApiTimeout is the timeout value of Stackdriver API service side.
+	stackdriverApiTimeout = 12 * time.Second
 )
 
 var (
@@ -186,6 +189,7 @@ func newStackdriverExporter(config *metricsConfig, logger *zap.SugaredLogger) (v
 		GetMetricPrefix:         mpf,
 		ReportingInterval:       config.reportingPeriod,
 		DefaultMonitoringLabels: &sd.Labels{},
+		Timeout:                 stackdriverApiTimeout,
 	})
 	if err != nil {
 		logger.Errorw("Failed to create the Stackdriver exporter: ", zap.Error(err))
diff --git a/webhook/resourcesemantics/defaulting/defaulting.go b/webhook/resourcesemantics/defaulting/defaulting.go
index 80d1dcc9a..faebc5f6e 100644
--- a/webhook/resourcesemantics/defaulting/defaulting.go
+++ b/webhook/resourcesemantics/defaulting/defaulting.go
@@ -186,6 +186,11 @@ func (ac *reconciler) reconcileMutatingWebhook(ctx context.Context, caCert []byt
 			MatchExpressions: []metav1.LabelSelectorRequirement{{
 				Key:      "webhooks.knative.dev/exclude",
 				Operator: metav1.LabelSelectorOpDoesNotExist,
+			}, {
+				// "control-plane" is added to support Azure's AKS, otherwise the controllers fight.
+				// See knative/pkg#1590 for details.
+				Key:      "control-plane",
+				Operator: metav1.LabelSelectorOpDoesNotExist,
 			}},
 		}
 		webhook.Webhooks[i].ClientConfig.CABundle = caCert
diff --git a/webhook/resourcesemantics/validation/reconcile_config.go b/webhook/resourcesemantics/validation/reconcile_config.go
index 1537adc78..e598ec5fd 100644
--- a/webhook/resourcesemantics/validation/reconcile_config.go
+++ b/webhook/resourcesemantics/validation/reconcile_config.go
@@ -149,6 +149,11 @@ func (ac *reconciler) reconcileValidatingWebhook(ctx context.Context, caCert []b
 			MatchExpressions: []metav1.LabelSelectorRequirement{{
 				Key:      "webhooks.knative.dev/exclude",
 				Operator: metav1.LabelSelectorOpDoesNotExist,
+			}, {
+				// "control-plane" is added to support Azure's AKS, otherwise the controllers fight.
+				// See knative/pkg#1590 for details.
+				Key:      "control-plane",
+				Operator: metav1.LabelSelectorOpDoesNotExist,
 			}},
 		}
 		webhook.Webhooks[i].ClientConfig.CABundle = caCert


Test patch:
diff --git a/metrics/resource_view_test.go b/metrics/resource_view_test.go
index 72d09817e..e579b16a3 100644
--- a/metrics/resource_view_test.go
+++ b/metrics/resource_view_test.go
@@ -29,7 +29,6 @@ import (
 	"strings"
 	"sync"
 	"testing"
-	"time"
 
 	sd "contrib.go.opencensus.io/exporter/stackdriver"
 	ocmetrics "github.com/census-instrumentation/opencensus-proto/gen-go/agent/metrics/v1"
@@ -326,7 +325,7 @@ testComponent_testing_value{project="p1",revision="r2"} 1
 	}, {
 		name: "OpenCensus",
 		init: func() error {
-			if err := ocFake.start(); err != nil {
+			if err := ocFake.start(len(resources) + 1); err != nil {
 				return err
 			}
 			t.Logf("Created exporter at %s", ocFake.address)
@@ -339,10 +338,7 @@ testComponent_testing_value{project="p1",revision="r2"} 1
 			// [new duration] in the future.
 			view.Unregister(globalCounter)
 			UnregisterResourceView(gaugeView, resourceCounter)
-			FlushExporter()
 
-			time.Sleep(10 * time.Millisecond)
-			ocFake.srv.Stop() // Force close connections
 			records := []metricExtract{}
 			for record := range ocFake.published {
 				for _, m := range record.Metrics {
@@ -399,6 +395,7 @@ testComponent_testing_value{project="p1",revision="r2"} 1
 
 	for _, c := range harnesses {
 		t.Run(c.name, func(t *testing.T) {
+			ClearMetersForTest()
 			sdFake.t = t
 			if err := c.init(); err != nil {
 				t.Fatalf("unable to init: %+v", err)
@@ -599,11 +596,12 @@ func TestStackDriverExports(t *testing.T) {
 type openCensusFake struct {
 	address   string
 	srv       *grpc.Server
+	exports   sync.WaitGroup
 	wg        sync.WaitGroup
 	published chan ocmetrics.ExportMetricsServiceRequest
 }
 
-func (oc *openCensusFake) start() error {
+func (oc *openCensusFake) start(expectedStreams int) error {
 	ln, err := net.Listen("tcp", oc.address)
 	if err != nil {
 		return err
@@ -619,13 +617,21 @@ func (oc *openCensusFake) start() error {
 		oc.wg.Wait()
 		close(oc.published)
 	}()
+	oc.exports.Add(expectedStreams)
+	go oc.stop()
 	return nil
 }
 
+func (oc *openCensusFake) stop() {
+	oc.exports.Wait()
+	oc.srv.Stop()
+}
+
 func (oc *openCensusFake) Export(stream ocmetrics.MetricsService_ExportServer) error {
 	var streamResource *ocresource.Resource
 	oc.wg.Add(1)
 	defer oc.wg.Done()
+	metricSeen := false
 	for {
 		in, err := stream.Recv()
 		if err == io.EOF {
@@ -643,6 +649,10 @@ func (oc *openCensusFake) Export(stream ocmetrics.MetricsService_ExportServer) e
 				in.Resource = streamResource
 			}
 			oc.published <- *in
+			if !metricSeen {
+				oc.exports.Done()
+				metricSeen = true
+			}
 		}
 	}
 }
diff --git a/webhook/resourcesemantics/defaulting/table_test.go b/webhook/resourcesemantics/defaulting/table_test.go
index 37c1bedea..384ebc596 100644
--- a/webhook/resourcesemantics/defaulting/table_test.go
+++ b/webhook/resourcesemantics/defaulting/table_test.go
@@ -66,6 +66,9 @@ func TestReconcile(t *testing.T) {
 		MatchExpressions: []metav1.LabelSelectorRequirement{{
 			Key:      "webhooks.knative.dev/exclude",
 			Operator: metav1.LabelSelectorOpDoesNotExist,
+		}, {
+			Key:      "control-plane",
+			Operator: metav1.LabelSelectorOpDoesNotExist,
 		}},
 	}
 
diff --git a/webhook/resourcesemantics/validation/reconcile_config_test.go b/webhook/resourcesemantics/validation/reconcile_config_test.go
index bb803208d..505011232 100644
--- a/webhook/resourcesemantics/validation/reconcile_config_test.go
+++ b/webhook/resourcesemantics/validation/reconcile_config_test.go
@@ -66,6 +66,9 @@ func TestReconcile(t *testing.T) {
 		MatchExpressions: []metav1.LabelSelectorRequirement{{
 			Key:      "webhooks.knative.dev/exclude",
 			Operator: metav1.LabelSelectorOpDoesNotExist,
+		}, {
+			Key:      "control-plane",
+			Operator: metav1.LabelSelectorOpDoesNotExist,
 		}},
 	}