Repository: knative/serving
Pull request #10130 (2020-11-17 22:41:15)

Make code to reconcile ingresses pretty
We can move the construction of rollout in the default case to the ingress and simplify how the code
in the reconciler resources is.

/assign @tcnghia mattmoor

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

This pull-request has been approved by: *<a href="https://github.com/knative/serving/pull/10130#" 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":[]} -->
codecov[bot]: # [Codecov](https://codecov.io/gh/knative/serving/pull/10130?src=pr&el=h1) Report
> Merging [#10130](https://codecov.io/gh/knative/serving/pull/10130?src=pr&el=desc) (f37ff02) into [master](https://codecov.io/gh/knative/serving/commit/3b6b40aec07654c1d2d411397122ae279a4ccfe4?el=desc) (3b6b40a) will **decrease** coverage by `0.05%`.
> The diff coverage is `60.00%`.

[![Impacted file tree graph](https://codecov.io/gh/knative/serving/pull/10130/graphs/tree.svg?width=650&height=150&src=pr&token=MPpDfAj4Ui)](https://codecov.io/gh/knative/serving/pull/10130?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #10130      +/-   ##
==========================================
- Coverage   87.97%   87.92%   -0.06%     
==========================================
  Files         184      184              
  Lines        8585     8590       +5     
==========================================
  Hits         7553     7553              
- Misses        789      792       +3     
- Partials      243      245       +2     
```


| [Impacted Files](https://codecov.io/gh/knative/serving/pull/10130?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/reconciler/route/reconcile\_resources.go](https://codecov.io/gh/knative/serving/pull/10130/diff?src=pr&el=tree#diff-cGtnL3JlY29uY2lsZXIvcm91dGUvcmVjb25jaWxlX3Jlc291cmNlcy5nbw==) | `79.38% <60.00%> (+0.95%)` | :arrow_up: |
| [pkg/reconciler/route/resources/ingress.go](https://codecov.io/gh/knative/serving/pull/10130/diff?src=pr&el=tree#diff-cGtnL3JlY29uY2lsZXIvcm91dGUvcmVzb3VyY2VzL2luZ3Jlc3MuZ28=) | `96.82% <60.00%> (-3.18%)` | :arrow_down: |
| [pkg/reconciler/configuration/configuration.go](https://codecov.io/gh/knative/serving/pull/10130/diff?src=pr&el=tree#diff-cGtnL3JlY29uY2lsZXIvY29uZmlndXJhdGlvbi9jb25maWd1cmF0aW9uLmdv) | `85.93% <0.00%> (-2.35%)` | :arrow_down: |
| [pkg/activator/net/revision\_backends.go](https://codecov.io/gh/knative/serving/pull/10130/diff?src=pr&el=tree#diff-cGtnL2FjdGl2YXRvci9uZXQvcmV2aXNpb25fYmFja2VuZHMuZ28=) | `90.49% <0.00%> (-0.91%)` | :arrow_down: |
| [pkg/autoscaler/statforwarder/forwarder.go](https://codecov.io/gh/knative/serving/pull/10130/diff?src=pr&el=tree#diff-cGtnL2F1dG9zY2FsZXIvc3RhdGZvcndhcmRlci9mb3J3YXJkZXIuZ28=) | `82.44% <0.00%> (+1.06%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/knative/serving/pull/10130?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/knative/serving/pull/10130?src=pr&el=footer). Last update [3b6b40a...f37ff02](https://codecov.io/gh/knative/serving/pull/10130?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

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

This pull-request has been approved by: *<a href="https://github.com/knative/serving/pull/10130#pullrequestreview-532884753" title="LGTM">tcnghia</a>*, *<a href="https://github.com/knative/serving/pull/10130#" 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":[]} -->
tcnghia: /hold
Please update commit message / PR description to be shorter
vagababov: done

/unhold

Patch:
diff --git a/pkg/reconciler/route/reconcile_resources.go b/pkg/reconciler/route/reconcile_resources.go
index da5b2afec..4e232ad20 100644
--- a/pkg/reconciler/route/reconcile_resources.go
+++ b/pkg/reconciler/route/reconcile_resources.go
@@ -21,7 +21,6 @@ import (
 	"encoding/json"
 	"fmt"
 
-	"github.com/davecgh/go-spew/spew"
 	"go.uber.org/zap"
 	"golang.org/x/sync/errgroup"
 	corev1 "k8s.io/api/core/v1"
@@ -33,7 +32,6 @@ import (
 	"knative.dev/networking/pkg/apis/networking"
 	netv1alpha1 "knative.dev/networking/pkg/apis/networking/v1alpha1"
 	"knative.dev/pkg/controller"
-	"knative.dev/pkg/kmeta"
 	"knative.dev/pkg/logging"
 	v1 "knative.dev/serving/pkg/apis/serving/v1"
 	"knative.dev/serving/pkg/reconciler/route/resources"
@@ -49,19 +47,12 @@ func (c *Reconciler) reconcileIngress(
 ) (*netv1alpha1.Ingress, error) {
 	recorder := controller.GetEventRecorder(ctx)
 
-	desired, err := resources.MakeIngress(ctx, r, tc, tls, ingressClass, acmeChallenges...)
-	if err != nil {
-		return nil, err
-	}
-	// Get the current rollout state as described by the traffic.
-	curRO := tc.BuildRollout()
-
 	ingress, err := c.ingressLister.Ingresses(r.Namespace).Get(names.Ingress(r))
 	if apierrs.IsNotFound(err) {
-		// If there is no exisiting Ingress, then current rollout is _the_ rollout.
-		desired.Annotations = kmeta.UnionMaps(desired.Annotations, map[string]string{
-			networking.RolloutAnnotationKey: serializeRollout(ctx, curRO),
-		})
+		desired, err := resources.MakeIngress(ctx, r, tc, tls, ingressClass, acmeChallenges...)
+		if err != nil {
+			return nil, err
+		}
 		ingress, err = c.netclient.NetworkingV1alpha1().Ingresses(desired.Namespace).Create(ctx, desired, metav1.CreateOptions{})
 		if err != nil {
 			recorder.Eventf(r, corev1.EventTypeWarning, "CreationFailed", "Failed to create Ingress: %v", err)
@@ -74,11 +65,23 @@ func (c *Reconciler) reconcileIngress(
 		return nil, err
 	} else {
 		// Ingress exists. We need to compute the rollout spec diff.
-		prevRO := deserializeRollout(ctx, ingress.Annotations[networking.RolloutAnnotationKey])
+		// Get the current rollout state as described by the traffic.
+		curRO := tc.BuildRollout()
+
+		// Get the previous rollout state from the annotation.
+		// If it's corrupt, inexistent, or otherwise incorrect,
+		// the prevRO will be just nil rollout.
+		prevRO := deserializeRollout(ctx,
+			ingress.Annotations[networking.RolloutAnnotationKey])
+
+		// And recompute the rollout state.
 		effectiveRO := curRO.Step(prevRO)
-		// Update the annotation.
-		desired.Annotations[networking.RolloutAnnotationKey] = serializeRollout(ctx, effectiveRO)
-		// TODO(vagababov): apply the Rollout to the ingress spec here.
+		desired, err := resources.MakeIngressWithRollout(ctx, r, tc, effectiveRO,
+			tls, ingressClass, acmeChallenges...)
+		if err != nil {
+			return nil, err
+		}
+
 		if !equality.Semantic.DeepEqual(ingress.Spec, desired.Spec) ||
 			!equality.Semantic.DeepEqual(ingress.Annotations, desired.Annotations) ||
 			!equality.Semantic.DeepEqual(ingress.Labels, desired.Labels) {
@@ -206,17 +209,6 @@ func (c *Reconciler) updatePlaceholderServices(ctx context.Context, route *v1.Ro
 	return eg.Wait()
 }
 
-func serializeRollout(ctx context.Context, r *traffic.Rollout) string {
-	sr, err := json.Marshal(r)
-	if err != nil {
-		// This must never happen in the normal course of things.
-		logging.FromContext(ctx).Warnw("Error serializing Rollout: "+spew.Sprint(r),
-			zap.Error(err))
-		return ""
-	}
-	return string(sr)
-}
-
 func deserializeRollout(ctx context.Context, ro string) *traffic.Rollout {
 	if ro == "" {
 		return nil
diff --git a/pkg/reconciler/route/resources/ingress.go b/pkg/reconciler/route/resources/ingress.go
index 129d66e9e..cae5804c7 100644
--- a/pkg/reconciler/route/resources/ingress.go
+++ b/pkg/reconciler/route/resources/ingress.go
@@ -18,8 +18,11 @@ package resources
 
 import (
 	"context"
+	"encoding/json"
 	"sort"
 
+	"github.com/davecgh/go-spew/spew"
+	"go.uber.org/zap"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/intstr"
@@ -30,6 +33,7 @@ import (
 	netv1alpha1 "knative.dev/networking/pkg/apis/networking/v1alpha1"
 	ingress "knative.dev/networking/pkg/ingress"
 	"knative.dev/pkg/kmeta"
+	"knative.dev/pkg/logging"
 	"knative.dev/serving/pkg/activator"
 	apicfg "knative.dev/serving/pkg/apis/config"
 	"knative.dev/serving/pkg/apis/serving"
@@ -59,6 +63,23 @@ func MakeIngress(
 	tls []netv1alpha1.IngressTLS,
 	ingressClass string,
 	acmeChallenges ...netv1alpha1.HTTP01Challenge,
+) (*netv1alpha1.Ingress, error) {
+	return MakeIngressWithRollout(
+		// If no rollout is specified, we just build the default one.
+		ctx, r, tc, tc.BuildRollout(), tls, ingressClass, acmeChallenges...)
+}
+
+// MakeIngressWithRollout builds a KIngress object from the given parameters.
+// When building the ingress the builder will take into the account
+// the desired rollout state to split the traffic.
+func MakeIngressWithRollout(
+	ctx context.Context,
+	r *servingv1.Route,
+	tc *traffic.Config,
+	ro *traffic.Rollout,
+	tls []netv1alpha1.IngressTLS,
+	ingressClass string,
+	acmeChallenges ...netv1alpha1.HTTP01Challenge,
 ) (*netv1alpha1.Ingress, error) {
 	spec, err := makeIngressSpec(ctx, r, tls, tc, acmeChallenges...)
 	if err != nil {
@@ -74,6 +95,7 @@ func MakeIngress(
 			}),
 			Annotations: kmeta.FilterMap(kmeta.UnionMaps(map[string]string{
 				networking.IngressClassAnnotationKey: ingressClass,
+				networking.RolloutAnnotationKey:      serializeRollout(ctx, ro),
 			}, r.GetAnnotations()), func(key string) bool {
 				return key == corev1.LastAppliedConfigAnnotation
 			}),
@@ -83,6 +105,17 @@ func MakeIngress(
 	}, nil
 }
 
+func serializeRollout(ctx context.Context, r *traffic.Rollout) string {
+	sr, err := json.Marshal(r)
+	if err != nil {
+		// This must never happen in the normal course of things.
+		logging.FromContext(ctx).Warnw("Error serializing Rollout: "+spew.Sprint(r),
+			zap.Error(err))
+		return ""
+	}
+	return string(sr)
+}
+
 // makeIngressSpec builds a new IngressSpec from inputs.
 func makeIngressSpec(
 	ctx context.Context,


Test patch:
diff --git a/pkg/reconciler/route/resources/ingress_test.go b/pkg/reconciler/route/resources/ingress_test.go
index c3e5753d5..ac1339e87 100644
--- a/pkg/reconciler/route/resources/ingress_test.go
+++ b/pkg/reconciler/route/resources/ingress_test.go
@@ -48,6 +48,8 @@ import (
 const (
 	ns = "test-ns"
 
+	emptyRollout = "{}"
+
 	testRouteName       = "test-route"
 	testAnnotationValue = "test-annotation-value"
 	testIngressClass    = "test-ingress"
@@ -78,6 +80,7 @@ func TestMakeIngressCorrectMetadata(t *testing.T) {
 		Annotations: map[string]string{
 			// Make sure to get passdownIngressClass instead of ingressClass
 			networking.IngressClassAnnotationKey: passdownIngressClass,
+			networking.RolloutAnnotationKey:      emptyRollout,
 			"test-annotation":                    "bar",
 		},
 		OwnerReferences: []metav1.OwnerReference{*kmeta.NewControllerRef(r)},
@@ -99,6 +102,15 @@ func TestMakeIngressWithRollout(t *testing.T) {
 	)
 	cfg := &traffic.Config{
 		Targets: map[string]traffic.RevisionTargets{
+			"tagged": {{
+				TrafficTarget: v1.TrafficTarget{
+					ConfigurationName: "thor",
+					LatestRevision:    ptr.Bool(true),
+					Percent:           ptr.Int64(100),
+					RevisionName:      "thor-02020",
+				},
+				ServiceName: "hammer",
+			}},
 			traffic.DefaultTarget: {{
 				TrafficTarget: v1.TrafficTarget{
 					ConfigurationName: "valhalla",
@@ -129,6 +141,7 @@ func TestMakeIngressWithRollout(t *testing.T) {
 		Annotations: map[string]string{
 			// Make sure to get passdownIngressClass instead of ingressClass
 			networking.IngressClassAnnotationKey: passdownIngressClass,
+			networking.RolloutAnnotationKey:      `{"configurations":[{"configurationName":"valhalla","percent":100,"revisions":[{"revisionName":"valhalla-01982","percent":100}]},{"configurationName":"thor","tag":"tagged","percent":100,"revisions":[{"revisionName":"thor-02020","percent":100}]}]}`,
 			"test-annotation":                    "bar",
 		},
 		OwnerReferences: []metav1.OwnerReference{*kmeta.NewControllerRef(r)},
@@ -919,6 +932,7 @@ func TestMakeIngressWithTLS(t *testing.T) {
 			Namespace: ns,
 			Annotations: map[string]string{
 				networking.IngressClassAnnotationKey: ingressClass,
+				networking.RolloutAnnotationKey:      emptyRollout,
 			},
 			Labels: map[string]string{
 				serving.RouteLabelKey:          "test-route",
diff --git a/pkg/reconciler/route/table_test.go b/pkg/reconciler/route/table_test.go
index 5c3e986fc..0afc597e1 100644
--- a/pkg/reconciler/route/table_test.go
+++ b/pkg/reconciler/route/table_test.go
@@ -18,6 +18,7 @@ package route
 
 import (
 	"context"
+	"encoding/json"
 	"fmt"
 	"testing"
 	"time"
@@ -2454,13 +2455,7 @@ func ingressWithClass(r *v1.Route, tc *traffic.Config, class string, io ...Ingre
 }
 
 func baseIngressWithClass(r *v1.Route, tc *traffic.Config, class string, io ...IngressOption) *netv1alpha1.Ingress {
-	ingress, _ := resources.MakeIngress(getContext(), r, tc, nil, class)
-
-	// By default attach current rollout.
-	ro := tc.BuildRollout()
-	ingress.Annotations = kmeta.UnionMaps(ingress.Annotations, map[string]string{
-		networking.RolloutAnnotationKey: serializeRollout(context.Background(), ro),
-	})
+	ingress, _ := resources.MakeIngress(getContext(), r, tc, nil /*tls*/, class)
 
 	for _, opt := range io {
 		opt(ingress)
@@ -2476,12 +2471,6 @@ func ingressWithTLS(r *v1.Route, tc *traffic.Config, tls []netv1alpha1.IngressTL
 func baseIngressWithTLS(r *v1.Route, tc *traffic.Config, tls []netv1alpha1.IngressTLS, challenges []netv1alpha1.HTTP01Challenge, io ...IngressOption) *netv1alpha1.Ingress {
 	ingress, _ := resources.MakeIngress(getContext(), r, tc, tls, TestIngressClass, challenges...)
 
-	// By default attach current rollout.
-	ro := tc.BuildRollout()
-	ingress.Annotations = kmeta.UnionMaps(ingress.Annotations, map[string]string{
-		networking.RolloutAnnotationKey: serializeRollout(context.Background(), ro),
-	})
-
 	for _, opt := range io {
 		opt(ingress)
 	}
@@ -2623,7 +2612,9 @@ func simpleRollout(cfg string, revs []traffic.RevisionRollout) IngressOption {
 				Revisions:         revs,
 			}},
 		}
-		ro := serializeRollout(context.Background(), r)
-		i.Annotations[networking.RolloutAnnotationKey] = ro
+		i.Annotations[networking.RolloutAnnotationKey] = func() string {
+			d, _ := json.Marshal(r)
+			return string(d)
+		}()
 	}
 }