Repository: knative/serving
Pull request #9276 (2020-09-02 20:21:59)

Fix a few things in throttler test
- fix logging in throttler itself
- add debugging to catch next time thingy flakes
- add more interesting scenarios and better verify backends updates in case of cc=0

/assign @yanweiguo @julz 

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

This pull-request has been approved by: *<a href="https://github.com/knative/serving/pull/9276#" title="Author self-approved">vagababov</a>*

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

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

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

- ~~[pkg/activator/OWNERS](https://github.com/knative/serving/blob/master/pkg/activator/OWNERS)~~ [vagababov]

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":[]} -->

Patch:
diff --git a/pkg/activator/net/throttler.go b/pkg/activator/net/throttler.go
index 48e159490..5acb46842 100644
--- a/pkg/activator/net/throttler.go
+++ b/pkg/activator/net/throttler.go
@@ -280,7 +280,7 @@ func (rt *revisionThrottler) updateCapacity(backendCount int) {
 			rt.resetTrackers()
 			assigned = assignSlice(rt.podTrackers, ai, ac, rt.containerConcurrency)
 		}
-		rt.logger.Debugf("Trackers %d/%d:  %v", ai, ac, rt.assignedTrackers)
+		rt.logger.Debugf("Trackers %d/%d: assignment: %v", ai, ac, assigned)
 		// The actual write out of the assigned trackers has to be under lock.
 		rt.mux.Lock()
 		defer rt.mux.Unlock()
diff --git a/pkg/apis/serving/v1alpha1/route_validation.go b/pkg/apis/serving/v1alpha1/route_validation.go
index 90206bdad..50a0ae4b8 100644
--- a/pkg/apis/serving/v1alpha1/route_validation.go
+++ b/pkg/apis/serving/v1alpha1/route_validation.go
@@ -19,15 +19,18 @@ package v1alpha1
 import (
 	"context"
 	"fmt"
+	"strconv"
 
 	"k8s.io/apimachinery/pkg/api/equality"
+	"knative.dev/networking/pkg/apis/networking"
 	"knative.dev/pkg/apis"
 	"knative.dev/serving/pkg/apis/serving"
 )
 
 func (r *Route) Validate(ctx context.Context) *apis.FieldError {
-	errs := serving.ValidateObjectMetadata(ctx, r.GetObjectMeta()).ViaField("metadata")
-	errs = errs.Also(r.Spec.Validate(apis.WithinSpec(ctx)).ViaField("spec"))
+	errs := serving.ValidateObjectMetadata(ctx, r.GetObjectMeta()).ViaField("metadata").
+		Also(r.Spec.Validate(apis.WithinSpec(ctx)).ViaField("spec")).
+		Also(validateAnnotations(r.GetAnnotations()))
 
 	if apis.IsInUpdate(ctx) {
 		original := apis.GetBaseline(ctx).(*Route)
@@ -105,3 +108,17 @@ func (rs *RouteSpec) Validate(ctx context.Context) *apis.FieldError {
 	}
 	return errs
 }
+
+// TODO: this should be moved to knative/networking as part of
+// https://github.com/knative/networking/issues/123
+func validateAnnotations(annotations map[string]string) *apis.FieldError {
+	disableAutoTLS := annotations[networking.DisableAutoTLSAnnotationKey]
+
+	if disableAutoTLS != "" {
+		if _, err := strconv.ParseBool(disableAutoTLS); err != nil {
+			return apis.ErrInvalidValue(disableAutoTLS, networking.DisableAutoTLSAnnotationKey)
+		}
+	}
+
+	return nil
+}
diff --git a/pkg/reconciler/route/route.go b/pkg/reconciler/route/route.go
index e7dd4c7ae..d19a8113d 100644
--- a/pkg/reconciler/route/route.go
+++ b/pkg/reconciler/route/route.go
@@ -19,6 +19,7 @@ package route
 import (
 	"context"
 	"sort"
+	"strconv"
 	"strings"
 
 	corev1 "k8s.io/api/core/v1"
@@ -180,10 +181,11 @@ func (c *Reconciler) reconcileIngressResources(ctx context.Context, r *v1.Route,
 
 func (c *Reconciler) tls(ctx context.Context, host string, r *v1.Route, traffic *traffic.Config) ([]netv1alpha1.IngressTLS, []netv1alpha1.HTTP01Challenge, error) {
 	tls := []netv1alpha1.IngressTLS{}
-	if !config.FromContext(ctx).Network.AutoTLS {
+	if !autoTLSEnabled(ctx, r) {
 		r.Status.MarkTLSNotEnabled(v1.AutoTLSNotEnabledMessage)
 		return tls, nil, nil
 	}
+
 	domainToTagMap, err := domains.GetAllDomainsAndTags(ctx, r, getTrafficNames(traffic.Targets), traffic.Visibility)
 	if err != nil {
 		return nil, nil, err
@@ -415,6 +417,25 @@ func setTargetsScheme(rs *v1.RouteStatus, dnsNames []string, scheme string) {
 	}
 }
 
+func autoTLSEnabled(ctx context.Context, r *v1.Route) bool {
+	if !config.FromContext(ctx).Network.AutoTLS {
+		return false
+	}
+
+	logger := logging.FromContext(ctx)
+	annotationValue := r.Annotations[networking.DisableAutoTLSAnnotationKey]
+
+	disabledByAnnotation, err := strconv.ParseBool(annotationValue)
+	if err != nil {
+		// validation should've caught an invalid value here.
+		// if we have one anyways, assume not disabled and log a warning.
+		logger.Warnf("Invalid annotation value for %q. Value: %q",
+			networking.DisableAutoTLSAnnotationKey, annotationValue)
+	}
+
+	return !disabledByAnnotation
+}
+
 func findMatchingWildcardCert(ctx context.Context, domains []string, certs []*netv1alpha1.Certificate) *netv1alpha1.Certificate {
 	for _, cert := range certs {
 		if wildcardCertMatches(ctx, domains, cert) {


Test patch:
diff --git a/pkg/activator/net/throttler_test.go b/pkg/activator/net/throttler_test.go
index 1ea032592..00557e31d 100644
--- a/pkg/activator/net/throttler_test.go
+++ b/pkg/activator/net/throttler_test.go
@@ -25,6 +25,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/davecgh/go-spew/spew"
 	"github.com/google/go-cmp/cmp"
 	"golang.org/x/sync/errgroup"
 	corev1 "k8s.io/api/core/v1"
@@ -333,7 +334,7 @@ func TestThrottlerSuccesses(t *testing.T) {
 		// Double updates exercise additional paths.
 		initUpdates: []revisionDestsUpdate{{
 			Rev:   types.NamespacedName{Namespace: testNamespace, Name: testRevision},
-			Dests: sets.NewString("128.0.0.2:1234"),
+			Dests: sets.NewString("128.0.0.2:1234", "128.0.0.32:1212"),
 		}, {
 			Rev:   types.NamespacedName{Namespace: testNamespace, Name: testRevision},
 			Dests: sets.NewString("128.0.0.1:1234"),
@@ -371,10 +372,21 @@ func TestThrottlerSuccesses(t *testing.T) {
 		revision: revision(types.NamespacedName{Namespace: testNamespace, Name: testRevision}, networking.ProtocolHTTP1, 3),
 		initUpdates: []revisionDestsUpdate{{
 			Rev:   types.NamespacedName{Namespace: testNamespace, Name: testRevision},
-			Dests: sets.NewString("128.0.0.1:1234", "128.0.0.2:1234"),
+			Dests: sets.NewString("128.0.0.1:1234", "128.0.0.2:1234", "128.0.0.2:4236", "128.0.0.2:1233", "128.0.0.2:1230"),
 		}},
 		requests:  3,
 		wantDests: sets.NewString("128.0.0.1:1234"),
+	}, {
+		name: "roundrobin test",
+		revision: revision(types.NamespacedName{Namespace: testNamespace, Name: testRevision},
+			networking.ProtocolHTTP1, 5 /*cc >3*/),
+		initUpdates: []revisionDestsUpdate{{
+			Rev:   types.NamespacedName{Namespace: testNamespace, Name: testRevision},
+			Dests: sets.NewString("128.0.0.1:1234", "128.0.0.2:1234", "211.212.213.214"),
+		}},
+		requests: 3,
+		// All three IP addresses should be used if cc>3.
+		wantDests: sets.NewString("128.0.0.1:1234", "128.0.0.2:1234", "211.212.213.214"),
 	}, {
 		name:     "multiple ClusterIP requests",
 		revision: revisionCC1(types.NamespacedName{Namespace: testNamespace, Name: testRevision}, networking.ProtocolHTTP1),
@@ -421,10 +433,6 @@ func TestThrottlerSuccesses(t *testing.T) {
 				waitInformers()
 			}()
 
-			for _, update := range tc.initUpdates {
-				updateCh <- update
-			}
-
 			publicEp := &corev1.Endpoints{
 				ObjectMeta: metav1.ObjectMeta{
 					Name:      testRevision,
@@ -448,16 +456,26 @@ func TestThrottlerSuccesses(t *testing.T) {
 				t.Fatal("RevisionThrottler can't be found:", err)
 			}
 
+			for _, update := range tc.initUpdates {
+				updateCh <- update
+			}
+
 			// Make sure our informer event has fired.
 			// We send multiple updates in some tests, so make sure the capacity is exact.
 			wantCapacity := 1
 			cc := tc.revision.Spec.ContainerConcurrency
-			if cc != nil && *cc != 0 {
-				dests := tc.initUpdates[len(tc.initUpdates)-1].Dests.Len()
+			dests := tc.initUpdates[len(tc.initUpdates)-1].Dests.Len()
+			if *cc != 0 {
 				wantCapacity = dests * int(*cc)
 			}
 			if err := wait.PollImmediate(10*time.Millisecond, 3*time.Second, func() (bool, error) {
-				return rt.activatorIndex.Load() != -1 && rt.breaker.Capacity() == wantCapacity, nil
+				if *cc != 0 {
+					return rt.activatorIndex.Load() != -1 && rt.breaker.Capacity() == wantCapacity, nil
+				}
+				// If CC=0 then verify number of backends, rather the capacity of breaker.
+				rt.mux.RLock()
+				defer rt.mux.RUnlock()
+				return rt.activatorIndex.Load() != -1 && dests == len(rt.assignedTrackers), nil
 			}); err != nil {
 				t.Fatal("Timed out waiting for the capacity to be updated")
 			}
@@ -481,8 +499,12 @@ func TestThrottlerSuccesses(t *testing.T) {
 				gotDests.Insert(result.dest)
 			}
 
-			if got, want := gotDests, tc.wantDests; !got.Equal(want) {
+			if got, want := gotDests.List(), tc.wantDests.List(); !cmp.Equal(want, got) {
 				t.Errorf("Dests = %v, want: %v, diff: %s", got, want, cmp.Diff(want, got))
+				rt.mux.RLock()
+				defer rt.mux.RUnlock()
+				t.Log("podTrackers:\n", spew.Sdump(rt.podTrackers))
+				t.Log("assignedTrackers:\n", spew.Sdump(rt.assignedTrackers))
 			}
 		})
 	}
diff --git a/pkg/apis/serving/v1alpha1/route_validation_test.go b/pkg/apis/serving/v1alpha1/route_validation_test.go
index e1418f6b2..c09d6e4ae 100644
--- a/pkg/apis/serving/v1alpha1/route_validation_test.go
+++ b/pkg/apis/serving/v1alpha1/route_validation_test.go
@@ -74,6 +74,25 @@ func TestRouteValidation(t *testing.T) {
 			},
 		},
 		want: nil,
+	}, {
+		name: "valid annotation",
+		r: &Route{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: "valid",
+				Annotations: map[string]string{
+					"networking.knative.dev/disableAutoTLS": "true",
+				},
+			},
+			Spec: RouteSpec{
+				Traffic: []TrafficTarget{{
+					TrafficTarget: v1.TrafficTarget{
+						RevisionName: "foo",
+						Percent:      ptr.Int64(100),
+					},
+				}},
+			},
+		},
+		want: nil,
 	}, {
 		name: "invalid traffic entry",
 		r: &Route{
@@ -155,6 +174,27 @@ func TestRouteValidation(t *testing.T) {
 		want: &apis.FieldError{
 			Message: "not a DNS 1035 label: [must be no more than 63 characters]",
 			Paths:   []string{"metadata.name"},
+		}}, {
+		name: "invalid annotation",
+		r: &Route{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: "valid",
+				Annotations: map[string]string{
+					"networking.knative.dev/disableAutoTLS": "foo",
+				},
+			},
+			Spec: RouteSpec{
+				Traffic: []TrafficTarget{{
+					TrafficTarget: v1.TrafficTarget{
+						RevisionName: "foo",
+						Percent:      ptr.Int64(100),
+					},
+				}},
+			},
+		},
+		want: &apis.FieldError{
+			Message: "invalid value: foo",
+			Paths:   []string{"networking.knative.dev/disableAutoTLS"},
 		},
 	}}
 
diff --git a/pkg/reconciler/route/route_test.go b/pkg/reconciler/route/route_test.go
index 00b2d7d5a..2cf00ff3b 100644
--- a/pkg/reconciler/route/route_test.go
+++ b/pkg/reconciler/route/route_test.go
@@ -46,6 +46,7 @@ import (
 	"k8s.io/client-go/tools/record"
 
 	network "knative.dev/networking/pkg"
+	"knative.dev/networking/pkg/apis/networking"
 	"knative.dev/networking/pkg/apis/networking/v1alpha1"
 	"knative.dev/pkg/configmap"
 	"knative.dev/pkg/controller"
@@ -1621,3 +1622,59 @@ func TestRouteDomain(t *testing.T) {
 		})
 	}
 }
+
+func TestAutoTLSEnabled(t *testing.T) {
+	tests := []struct {
+		name                  string
+		configAutoTLSEnabled  bool
+		tlsDisabledAnnotation string
+		wantAutoTLSEnabled    bool
+	}{{
+		name:                 "AutoTLS enabled by config, not disabled by annotation",
+		configAutoTLSEnabled: true,
+		wantAutoTLSEnabled:   true,
+	}, {
+		name:                  "AutoTLS enabled by config, disabled by annotation",
+		configAutoTLSEnabled:  true,
+		tlsDisabledAnnotation: "true",
+		wantAutoTLSEnabled:    false,
+	}, {
+		name:                 "AutoTLS disabled by config, not disabled by annotation",
+		configAutoTLSEnabled: false,
+		wantAutoTLSEnabled:   false,
+	}, {
+		name:                  "AutoTLS disabled by config, disabled by annotation",
+		configAutoTLSEnabled:  false,
+		tlsDisabledAnnotation: "true",
+		wantAutoTLSEnabled:    false,
+	}, {
+		name:                  "AutoTLS enabled by config, invalid annotation",
+		configAutoTLSEnabled:  true,
+		tlsDisabledAnnotation: "foo",
+		wantAutoTLSEnabled:    true,
+	}, {
+		name:                  "AutoTLS disabled by config, invalid annotation",
+		configAutoTLSEnabled:  false,
+		tlsDisabledAnnotation: "foo",
+		wantAutoTLSEnabled:    false,
+	}}
+
+	r := Route("test-ns", "test-route")
+	r.Annotations = map[string]string{}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			ctx := config.ToContext(context.Background(), &config.Config{
+				Network: &network.Config{
+					AutoTLS: test.configAutoTLSEnabled,
+				},
+			})
+
+			r.Annotations[networking.DisableAutoTLSAnnotationKey] = test.tlsDisabledAnnotation
+
+			if got := autoTLSEnabled(ctx, r); got != test.wantAutoTLSEnabled {
+				t.Errorf("autoTLSEnabled = %t, want %t", got, test.wantAutoTLSEnabled)
+			}
+		})
+	}
+}