Repository: knative/serving
Pull request #10146 (2020-11-18 23:34:54)

Validate the deserialized rollout
This should prevent bad things from happening.

Raises test coverage by some extent as well.

/assign @tcnghia 

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

This pull-request has been approved by: *<a href="https://github.com/knative/serving/pull/10146#" 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/reconciler/route/OWNERS](https://github.com/knative/serving/blob/master/pkg/reconciler/route/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":[]} -->
knative-prow-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/serving/pull/10146#pullrequestreview-533990179" title="LGTM">tcnghia</a>*, *<a href="https://github.com/knative/serving/pull/10146#" 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/reconciler/route/OWNERS](https://github.com/knative/serving/blob/master/pkg/reconciler/route/OWNERS)~~ [tcnghia,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/reconciler/route/reconcile_resources.go b/pkg/reconciler/route/reconcile_resources.go
index 4e232ad20..059158e6d 100644
--- a/pkg/reconciler/route/reconcile_resources.go
+++ b/pkg/reconciler/route/reconcile_resources.go
@@ -222,5 +222,9 @@ func deserializeRollout(ctx context.Context, ro string) *traffic.Rollout {
 			zap.Error(err))
 		return nil
 	}
+	if !r.Validate() {
+		logging.FromContext(ctx).Warnw("Deserializing Rollout is invalid: " + ro)
+		return nil
+	}
 	return r
 }
diff --git a/pkg/reconciler/route/traffic/rollout.go b/pkg/reconciler/route/traffic/rollout.go
index c3f3297db..dd393a300 100644
--- a/pkg/reconciler/route/traffic/rollout.go
+++ b/pkg/reconciler/route/traffic/rollout.go
@@ -80,6 +80,27 @@ type RevisionRollout struct {
 	Percent int `json:"percent"`
 }
 
+// Validate validates current rollout for inconsistencies.
+// This is expected to be invoked after annotation deserialization.
+// If it returns false — the deserialized object should be discarded.
+func (cur *Rollout) Validate() bool {
+	for _, c := range cur.Configurations {
+		// Cannot be over 100% in our system.
+		if c.Percent > 100 {
+			return false
+		}
+		// If total % values in the revision do not add up — discard.
+		tot := 0
+		for _, r := range c.Revisions {
+			tot += r.Percent
+		}
+		if tot != c.Percent {
+			return false
+		}
+	}
+	return true
+}
+
 // Step merges this rollout object with the previous state and
 // returns a new Rollout object representing the merged state.
 // At the end of the call the returned object will contain the


Test patch:
diff --git a/pkg/reconciler/route/reconcile_resources_test.go b/pkg/reconciler/route/reconcile_resources_test.go
index b227c0722..2ebf847ad 100644
--- a/pkg/reconciler/route/reconcile_resources_test.go
+++ b/pkg/reconciler/route/reconcile_resources_test.go
@@ -18,6 +18,7 @@ package route
 
 import (
 	"context"
+	"encoding/json"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
@@ -25,12 +26,14 @@ import (
 
 	"knative.dev/networking/pkg/apis/networking"
 	netv1alpha1 "knative.dev/networking/pkg/apis/networking/v1alpha1"
+	fakenetworkingclient "knative.dev/networking/pkg/client/injection/client/fake"
 	fakeingressinformer "knative.dev/networking/pkg/client/injection/informers/networking/v1alpha1/ingress/fake"
 	"knative.dev/pkg/ptr"
 	v1 "knative.dev/serving/pkg/apis/serving/v1"
 	fakeservingclient "knative.dev/serving/pkg/client/injection/client/fake"
 	fakerevisioninformer "knative.dev/serving/pkg/client/injection/informers/serving/v1/revision/fake"
 	"knative.dev/serving/pkg/reconciler/route/config"
+	"knative.dev/serving/pkg/reconciler/route/resources"
 	"knative.dev/serving/pkg/reconciler/route/traffic"
 
 	. "knative.dev/serving/pkg/testing/v1"
@@ -86,6 +89,82 @@ func TestReconcileIngressUpdate(t *testing.T) {
 	}
 }
 
+func TestReconcileIngressBadRollout(t *testing.T) {
+	var reconciler *Reconciler
+	ctx, _, _, _, cancel := newTestSetup(t, func(r *Reconciler) {
+		reconciler = r
+	})
+	defer cancel()
+	ctx = updateContext(ctx)
+
+	r := Route("test-ns", "test-route")
+
+	tc, tls := testIngressParams(t, r)
+	ing, err := resources.MakeIngressWithRollout(ctx, r, tc, &traffic.Rollout{
+		Configurations: []traffic.ConfigurationRollout{{
+			ConfigurationName: "configuration",
+			Percent:           202, // <- this is not right!
+		}},
+	}, tls, "foo-ingress")
+	if err != nil {
+		t.Fatal("Error creating ingress:", err)
+	}
+	ingClient := fakenetworkingclient.Get(ctx).NetworkingV1alpha1().Ingresses(ing.Namespace)
+	ingClient.Create(ctx, ing, metav1.CreateOptions{})
+	fakeingressinformer.Get(ctx).Informer().GetIndexer().Add(ing)
+	if _, err := reconciler.reconcileIngress(ctx, r, tc, tls, "foo-ingress"); err != nil {
+		t.Error("Unexpected error:", err)
+	}
+	ing, err = ingClient.Get(ctx, ing.Name, metav1.GetOptions{})
+	if err != nil {
+		t.Fatal("Could not get the ingress:", err)
+	}
+	// We want to ignore the previous one, since it's bogus.
+	want := func() string {
+		d, _ := json.Marshal(tc.BuildRollout())
+		return string(d)
+	}()
+	if got := ing.Annotations[networking.RolloutAnnotationKey]; got != want {
+		t.Errorf("Incorrect Rollout Annotation; diff(-want,+got):\n%s", cmp.Diff(want, got))
+	}
+}
+
+func TestReconcileIngressUnparseableRollout(t *testing.T) {
+	var reconciler *Reconciler
+	ctx, _, _, _, cancel := newTestSetup(t, func(r *Reconciler) {
+		reconciler = r
+	})
+	defer cancel()
+	ctx = updateContext(ctx)
+
+	r := Route("test-ns", "test-route")
+
+	tc, tls := testIngressParams(t, r)
+	ing, err := resources.MakeIngress(ctx, r, tc, tls, "foo-ingress")
+	if err != nil {
+		t.Fatal("Error creating ingress:", err)
+	}
+	ing.Annotations[networking.RolloutAnnotationKey] = `<?xml version="1.0" encoding="utf-8"?><rollout name="from-the-wrong-universe"/>`
+	ingClient := fakenetworkingclient.Get(ctx).NetworkingV1alpha1().Ingresses(ing.Namespace)
+	ingClient.Create(ctx, ing, metav1.CreateOptions{})
+	fakeingressinformer.Get(ctx).Informer().GetIndexer().Add(ing)
+	if _, err := reconciler.reconcileIngress(ctx, r, tc, tls, "foo-ingress"); err != nil {
+		t.Error("Unexpected error:", err)
+	}
+	ing, err = ingClient.Get(ctx, ing.Name, metav1.GetOptions{})
+	if err != nil {
+		t.Fatal("Could not get the ingress:", err)
+	}
+	// We want to ignore the previous one, since it's bogus.
+	want := func() string {
+		d, _ := json.Marshal(tc.BuildRollout())
+		return string(d)
+	}()
+	if got := ing.Annotations[networking.RolloutAnnotationKey]; got != want {
+		t.Errorf("Incorrect Rollout Annotation; diff(-want,+got):\n%s", cmp.Diff(want, got))
+	}
+}
+
 func TestReconcileRevisionTargetDoesNotExist(t *testing.T) {
 	ctx, _, _, _, cancel := newTestSetup(t)
 	defer cancel()
@@ -111,8 +190,10 @@ func testIngressParams(t *testing.T, r *v1.Route, trafficOpts ...func(tc *traffi
 	tc := &traffic.Config{Targets: map[string]traffic.RevisionTargets{
 		traffic.DefaultTarget: {{
 			TrafficTarget: v1.TrafficTarget{
-				RevisionName: "revision",
-				Percent:      ptr.Int64(100),
+				ConfigurationName: "config",
+				RevisionName:      "revision",
+				Percent:           ptr.Int64(100),
+				LatestRevision:    ptr.Bool(true),
 			},
 		}}}}
 
diff --git a/pkg/reconciler/route/traffic/rollout_test.go b/pkg/reconciler/route/traffic/rollout_test.go
index dd73569e8..24044323b 100644
--- a/pkg/reconciler/route/traffic/rollout_test.go
+++ b/pkg/reconciler/route/traffic/rollout_test.go
@@ -604,9 +604,13 @@ func TestStep(t *testing.T) {
 
 	for _, tc := range tests {
 		t.Run(tc.name, func(t *testing.T) {
-			if got, want := tc.cur.Step(tc.prev), tc.want; !cmp.Equal(got, want) {
+			got := tc.cur.Step(tc.prev)
+			if want := tc.want; !cmp.Equal(got, want) {
 				t.Errorf("Wrong rolled rollout, diff(-want,+got):\n%s", cmp.Diff(want, got))
 			}
+			if !got.Validate() {
+				t.Errorf("Step returned an invalid config:\n%#v", got)
+			}
 		})
 	}
 }
@@ -737,3 +741,78 @@ func TestAdjustPercentage(t *testing.T) {
 		})
 	}
 }
+
+func TestValidateFailures(t *testing.T) {
+	tests := []struct {
+		name string
+		r    *Rollout
+	}{{
+		name: "config > 100%",
+		r: &Rollout{
+			Configurations: []ConfigurationRollout{{
+				ConfigurationName: "keith",
+				Percent:           101,
+				Revisions: []RevisionRollout{{
+					RevisionName: "black-on-blue",
+					Percent:      101,
+				}},
+			}},
+		},
+	}, {
+		name: "rev more than config",
+		r: &Rollout{
+			Configurations: []ConfigurationRollout{{
+				ConfigurationName: "keith",
+				Percent:           42,
+				Revisions: []RevisionRollout{{
+					RevisionName: "black-on-blue",
+					Percent:      43,
+				}},
+			}},
+		},
+	}, {
+		name: "revs more than config",
+		r: &Rollout{
+			Configurations: []ConfigurationRollout{{
+				ConfigurationName: "keith",
+				Percent:           42,
+				Revisions: []RevisionRollout{{
+					RevisionName: "black-on-blue",
+					Percent:      41,
+				}, {
+					RevisionName: "smith",
+					Percent:      3,
+				}},
+			}},
+		},
+	}, {
+		name: "2nd config > 100%",
+		r: &Rollout{
+			Configurations: []ConfigurationRollout{{
+				ConfigurationName: "rob",
+				Percent:           10,
+				Revisions: []RevisionRollout{{
+					RevisionName: "roy",
+					Percent:      10,
+				}},
+			}, {
+				ConfigurationName: "keith",
+				Tag:               "richards",
+				Percent:           101,
+				Revisions: []RevisionRollout{{
+					RevisionName: "black-on-blue",
+					Percent:      101,
+				}},
+			}},
+		},
+	}}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			if tc.r.Validate() {
+				t.Errorf("Validate succeeded for\n%#v", tc.r)
+			}
+		})
+	}
+
+}