Repository: knative/serving
Pull request #9808 (2020-10-13 21:31:19)

Some MakeIngress cleanups
- thread `traffic.Config` throughout, it helps reduce params in some cases and we'll need those for rollout
- improve test coverage
- better usage of constants

For #9766

/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/9808#" 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/9808?src=pr&el=h1) Report
> Merging [#9808](https://codecov.io/gh/knative/serving/pull/9808?src=pr&el=desc) into [master](https://codecov.io/gh/knative/serving/commit/d6bcb330adfdb1ac97f17159130b5dc65f130835?el=desc) will **increase** coverage by `0.04%`.
> The diff coverage is `100.00%`.

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

```diff
@@            Coverage Diff             @@
##           master    #9808      +/-   ##
==========================================
+ Coverage   87.73%   87.78%   +0.04%     
==========================================
  Files         181      181              
  Lines        8580     8578       -2     
==========================================
+ Hits         7528     7530       +2     
+ Misses        803      801       -2     
+ Partials      249      247       -2     
```


| [Impacted Files](https://codecov.io/gh/knative/serving/pull/9808?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/reconciler/route/resources/ingress.go](https://codecov.io/gh/knative/serving/pull/9808/diff?src=pr&el=tree#diff-cGtnL3JlY29uY2lsZXIvcm91dGUvcmVzb3VyY2VzL2luZ3Jlc3MuZ28=) | `100.00% <100.00%> (+1.58%)` | :arrow_up: |
| [pkg/reconciler/revision/reconcile\_resources.go](https://codecov.io/gh/knative/serving/pull/9808/diff?src=pr&el=tree#diff-cGtnL3JlY29uY2lsZXIvcmV2aXNpb24vcmVjb25jaWxlX3Jlc291cmNlcy5nbw==) | `80.72% <0.00%> (-2.41%)` | :arrow_down: |
| [pkg/activator/net/revision\_backends.go](https://codecov.io/gh/knative/serving/pull/9808/diff?src=pr&el=tree#diff-cGtnL2FjdGl2YXRvci9uZXQvcmV2aXNpb25fYmFja2VuZHMuZ28=) | `91.40% <0.00%> (ø)` | |
| [pkg/autoscaler/statforwarder/forwarder.go](https://codecov.io/gh/knative/serving/pull/9808/diff?src=pr&el=tree#diff-cGtnL2F1dG9zY2FsZXIvc3RhdGZvcndhcmRlci9mb3J3YXJkZXIuZ28=) | `82.25% <0.00%> (+1.07%)` | :arrow_up: |
| [pkg/reconciler/autoscaling/kpa/scaler.go](https://codecov.io/gh/knative/serving/pull/9808/diff?src=pr&el=tree#diff-cGtnL3JlY29uY2lsZXIvYXV0b3NjYWxpbmcva3BhL3NjYWxlci5nbw==) | `90.00% <0.00%> (+1.42%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/knative/serving/pull/9808?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/9808?src=pr&el=footer). Last update [d6bcb33...027dc44](https://codecov.io/gh/knative/serving/pull/9808?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/9808#pullrequestreview-507847200" title="LGTM">tcnghia</a>*, *<a href="https://github.com/knative/serving/pull/9808#" 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/resources/ingress.go b/pkg/reconciler/route/resources/ingress.go
index 87f7ee8d0..161c6521d 100644
--- a/pkg/reconciler/route/resources/ingress.go
+++ b/pkg/reconciler/route/resources/ingress.go
@@ -32,7 +32,7 @@ import (
 	ingress "knative.dev/networking/pkg/ingress"
 	"knative.dev/pkg/kmeta"
 	"knative.dev/serving/pkg/activator"
-	apiConfig "knative.dev/serving/pkg/apis/config"
+	apicfg "knative.dev/serving/pkg/apis/config"
 	"knative.dev/serving/pkg/apis/serving"
 	servingv1 "knative.dev/serving/pkg/apis/serving/v1"
 	"knative.dev/serving/pkg/reconciler/route/config"
@@ -61,7 +61,7 @@ func MakeIngress(
 	ingressClass string,
 	acmeChallenges ...netv1alpha1.HTTP01Challenge,
 ) (*netv1alpha1.Ingress, error) {
-	spec, err := MakeIngressSpec(ctx, r, tls, tc.Targets, tc.Visibility, acmeChallenges...)
+	spec, err := makeIngressSpec(ctx, r, tls, tc, acmeChallenges...)
 	if err != nil {
 		return nil, err
 	}
@@ -84,19 +84,18 @@ func MakeIngress(
 	}, nil
 }
 
-// MakeIngressSpec creates a new IngressSpec
-func MakeIngressSpec(
+// makeIngressSpec builds a new IngressSpec from inputs.
+func makeIngressSpec(
 	ctx context.Context,
 	r *servingv1.Route,
 	tls []netv1alpha1.IngressTLS,
-	targets map[string]traffic.RevisionTargets,
-	visibility map[string]netv1alpha1.IngressVisibility,
+	tc *traffic.Config,
 	acmeChallenges ...netv1alpha1.HTTP01Challenge,
 ) (netv1alpha1.IngressSpec, error) {
 	// Domain should have been specified in route status
 	// before calling this func.
-	names := make([]string, 0, len(targets))
-	for name := range targets {
+	names := make([]string, 0, len(tc.Targets))
+	for name := range tc.Targets {
 		names = append(names, name)
 	}
 	// Sort the names to give things a deterministic ordering.
@@ -110,7 +109,7 @@ func MakeIngressSpec(
 	for _, name := range names {
 		visibilities := []netv1alpha1.IngressVisibility{netv1alpha1.IngressVisibilityClusterLocal}
 		// If this is a public target (or not being marked as cluster-local), we also make public rule.
-		if v, ok := visibility[name]; !ok || v == netv1alpha1.IngressVisibilityExternalIP {
+		if v, ok := tc.Visibility[name]; !ok || v == netv1alpha1.IngressVisibilityExternalIP {
 			visibilities = append(visibilities, netv1alpha1.IngressVisibilityExternalIP)
 		}
 		for _, visibility := range visibilities {
@@ -118,8 +117,8 @@ func MakeIngressSpec(
 			if err != nil {
 				return netv1alpha1.IngressSpec{}, err
 			}
-			rule := makeIngressRule(ctx, domains, r.Namespace, visibility, targets[name])
-			if featuresConfig.TagHeaderBasedRouting == apiConfig.Enabled {
+			rule := makeIngressRule(ctx, domains, r.Namespace, visibility, tc.Targets[name])
+			if featuresConfig.TagHeaderBasedRouting == apicfg.Enabled {
 				if rule.HTTP.Paths[0].AppendHeaders == nil {
 					rule.HTTP.Paths[0].AppendHeaders = make(map[string]string)
 				}
@@ -134,7 +133,7 @@ func MakeIngressSpec(
 					// If a request has one of the `names`(tag name) except the default path,
 					// the request will be routed via one of the ingress paths, corresponding to the tag name.
 					rule.HTTP.Paths = append(
-						makeTagBasedRoutingIngressPaths(ctx, r.Namespace, targets, names), rule.HTTP.Paths...)
+						makeTagBasedRoutingIngressPaths(ctx, r.Namespace, tc, names), rule.HTTP.Paths...)
 				} else {
 					// If a request is routed by a tag-attached hostname instead of the tag header,
 					// the request may not have the tag header "Knative-Serving-Tag",
@@ -230,12 +229,12 @@ func makeIngressRule(ctx context.Context,
 }
 
 func makeTagBasedRoutingIngressPaths(
-	ctx context.Context, ns string, targets map[string]traffic.RevisionTargets, names []string) []netv1alpha1.HTTPIngressPath {
+	ctx context.Context, ns string, tc *traffic.Config, names []string) []netv1alpha1.HTTPIngressPath {
 	paths := make([]netv1alpha1.HTTPIngressPath, 0, len(names))
 
 	for _, name := range names {
 		if name != traffic.DefaultTarget {
-			path := makeBaseIngressPath(ctx, ns, targets[name])
+			path := makeBaseIngressPath(ctx, ns, tc.Targets[name])
 			path.Headers = map[string]netv1alpha1.HeaderMatch{network.TagHeaderName: {Exact: name}}
 			paths = append(paths, *path)
 		}
@@ -277,6 +276,11 @@ func makeBaseIngressPath(
 	}
 }
 
+// We want to set the ingress timeout to a really long timeout to
+// helps with issues like
+// https://github.com/knative/serving/issues/7350#issuecomment-669278261.
+const longTimeout = 48 * time.Hour
+
 // Before https://github.com/knative/networking/pull/64 we used a
 // default value in the KIngress timeout settings. However, that
 // does not work well with gRPC streaming timeout, so we stop the
@@ -289,15 +293,10 @@ func makeBaseIngressPath(
 // implement this `no timeout` behavior we will specify a high
 // timeout value in Route controller in the mean time.
 func ingressTimeout(ctx context.Context) time.Duration {
-	// We want to set the ingress timeout to a really long timeout to
-	// helps with issues like
-	// https://github.com/knative/serving/issues/7350#issuecomment-669278261
-	longTimeout := time.Hour * 48
-
-	// However, if the MaxRevisionTimeout is longer, we should still honor that.
-	if apiConfig.FromContext(ctx) != nil && apiConfig.FromContext(ctx).Defaults != nil {
-		maxRevisionTimeout := time.Duration(
-			apiConfig.FromContext(ctx).Defaults.MaxRevisionTimeoutSeconds) * time.Second
+	// However, if the MaxRevisionTimeout is longer than `longTimeout`,
+	// we should still honor that.
+	if cfg := apicfg.FromContext(ctx); cfg != nil && cfg.Defaults != nil {
+		maxRevisionTimeout := time.Duration(cfg.Defaults.MaxRevisionTimeoutSeconds) * time.Second
 		if maxRevisionTimeout > longTimeout {
 			return maxRevisionTimeout
 		}


Test patch:
diff --git a/pkg/reconciler/route/resources/ingress_test.go b/pkg/reconciler/route/resources/ingress_test.go
index 1c0022079..b6909a6a2 100644
--- a/pkg/reconciler/route/resources/ingress_test.go
+++ b/pkg/reconciler/route/resources/ingress_test.go
@@ -20,13 +20,13 @@ import (
 	"context"
 	"strings"
 	"testing"
-	"time"
+
+	"github.com/google/go-cmp/cmp"
 
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/intstr"
 
-	"github.com/google/go-cmp/cmp"
 	network "knative.dev/networking/pkg"
 	"knative.dev/networking/pkg/apis/networking"
 	netv1alpha1 "knative.dev/networking/pkg/apis/networking/v1alpha1"
@@ -35,7 +35,7 @@ import (
 	pkgnet "knative.dev/pkg/network"
 	"knative.dev/pkg/ptr"
 	"knative.dev/pkg/system"
-	apiConfig "knative.dev/serving/pkg/apis/config"
+	apicfg "knative.dev/serving/pkg/apis/config"
 	"knative.dev/serving/pkg/apis/serving"
 	v1 "knative.dev/serving/pkg/apis/serving/v1"
 	"knative.dev/serving/pkg/reconciler/route/config"
@@ -90,7 +90,7 @@ func TestMakeIngressCorrectMetadata(t *testing.T) {
 	}
 }
 
-func TestIngress_NoKubectlAnnotation(t *testing.T) {
+func TestIngressNoKubectlAnnotation(t *testing.T) {
 	targets := map[string]traffic.RevisionTargets{}
 	r := Route(ns, testRouteName, WithRouteAnnotation(map[string]string{
 		networking.IngressClassAnnotationKey: testIngressClass,
@@ -105,7 +105,7 @@ func TestIngress_NoKubectlAnnotation(t *testing.T) {
 	}
 }
 
-func TestMakeIngressSpec_CorrectRules(t *testing.T) {
+func TestMakeIngressSpecCorrectRules(t *testing.T) {
 	targets := map[string]traffic.RevisionTargets{
 		traffic.DefaultTarget: {{
 			TrafficTarget: v1.TrafficTarget{
@@ -149,7 +149,7 @@ func TestMakeIngressSpec_CorrectRules(t *testing.T) {
 						"Knative-Serving-Namespace": ns,
 					},
 				}},
-				Timeout: &metav1.Duration{Duration: 48 * time.Hour},
+				Timeout: &metav1.Duration{Duration: longTimeout},
 			}},
 		},
 		Visibility: netv1alpha1.IngressVisibilityClusterLocal,
@@ -171,7 +171,7 @@ func TestMakeIngressSpec_CorrectRules(t *testing.T) {
 						"Knative-Serving-Namespace": ns,
 					},
 				}},
-				Timeout: &metav1.Duration{Duration: 48 * time.Hour},
+				Timeout: &metav1.Duration{Duration: longTimeout},
 			}},
 		},
 		Visibility: netv1alpha1.IngressVisibilityExternalIP,
@@ -195,7 +195,7 @@ func TestMakeIngressSpec_CorrectRules(t *testing.T) {
 						"Knative-Serving-Namespace": ns,
 					},
 				}},
-				Timeout: &metav1.Duration{Duration: 48 * time.Hour},
+				Timeout: &metav1.Duration{Duration: longTimeout},
 			}},
 		},
 		Visibility: netv1alpha1.IngressVisibilityClusterLocal,
@@ -217,13 +217,13 @@ func TestMakeIngressSpec_CorrectRules(t *testing.T) {
 						"Knative-Serving-Namespace": ns,
 					},
 				}},
-				Timeout: &metav1.Duration{Duration: 48 * time.Hour},
+				Timeout: &metav1.Duration{Duration: longTimeout},
 			}},
 		},
 		Visibility: netv1alpha1.IngressVisibilityExternalIP,
 	}}
 
-	ci, err := MakeIngressSpec(testContext(), r, nil, targets, nil /* visibility */)
+	ci, err := makeIngressSpec(testContext(), r, nil, &traffic.Config{Targets: targets})
 	if err != nil {
 		t.Error("Unexpected error", err)
 	}
@@ -233,7 +233,7 @@ func TestMakeIngressSpec_CorrectRules(t *testing.T) {
 	}
 }
 
-func TestMakeIngressSpec_CorrectRuleVisibility(t *testing.T) {
+func TestMakeIngressSpecCorrectRuleVisibility(t *testing.T) {
 	cases := []struct {
 		name               string
 		route              *v1.Route
@@ -299,7 +299,10 @@ func TestMakeIngressSpec_CorrectRuleVisibility(t *testing.T) {
 	}}
 	for _, c := range cases {
 		t.Run(c.name, func(t *testing.T) {
-			ci, err := MakeIngressSpec(testContext(), c.route, nil, c.targets, c.serviceVisibility)
+			ci, err := makeIngressSpec(testContext(), c.route, nil, &traffic.Config{
+				Targets:    c.targets,
+				Visibility: c.serviceVisibility,
+			})
 			if err != nil {
 				t.Error("Unexpected error", err)
 			}
@@ -316,7 +319,7 @@ func TestMakeIngressSpec_CorrectRuleVisibility(t *testing.T) {
 	}
 }
 
-func TestMakeIngressSpec_CorrectRulesWithTagBasedRouting(t *testing.T) {
+func TestMakeIngressSpecCorrectRulesWithTagBasedRouting(t *testing.T) {
 	targets := map[string]traffic.RevisionTargets{
 		traffic.DefaultTarget: {{
 			TrafficTarget: v1.TrafficTarget{
@@ -365,7 +368,7 @@ func TestMakeIngressSpec_CorrectRulesWithTagBasedRouting(t *testing.T) {
 						"Knative-Serving-Namespace": ns,
 					},
 				}},
-				Timeout: &metav1.Duration{Duration: 48 * time.Hour},
+				Timeout: &metav1.Duration{Duration: longTimeout},
 			}, {
 				AppendHeaders: map[string]string{
 					network.DefaultRouteHeaderName: "true",
@@ -382,7 +385,7 @@ func TestMakeIngressSpec_CorrectRulesWithTagBasedRouting(t *testing.T) {
 						"Knative-Serving-Namespace": ns,
 					},
 				}},
-				Timeout: &metav1.Duration{Duration: 48 * time.Hour},
+				Timeout: &metav1.Duration{Duration: longTimeout},
 			}},
 		},
 		Visibility: netv1alpha1.IngressVisibilityClusterLocal,
@@ -409,7 +412,7 @@ func TestMakeIngressSpec_CorrectRulesWithTagBasedRouting(t *testing.T) {
 						"Knative-Serving-Namespace": ns,
 					},
 				}},
-				Timeout: &metav1.Duration{Duration: 48 * time.Hour},
+				Timeout: &metav1.Duration{Duration: longTimeout},
 			}, {
 				AppendHeaders: map[string]string{
 					network.DefaultRouteHeaderName: "true",
@@ -426,7 +429,7 @@ func TestMakeIngressSpec_CorrectRulesWithTagBasedRouting(t *testing.T) {
 						"Knative-Serving-Namespace": ns,
 					},
 				}},
-				Timeout: &metav1.Duration{Duration: 48 * time.Hour},
+				Timeout: &metav1.Duration{Duration: longTimeout},
 			}},
 		},
 		Visibility: netv1alpha1.IngressVisibilityExternalIP,
@@ -453,7 +456,7 @@ func TestMakeIngressSpec_CorrectRulesWithTagBasedRouting(t *testing.T) {
 						"Knative-Serving-Namespace": ns,
 					},
 				}},
-				Timeout: &metav1.Duration{Duration: 48 * time.Hour},
+				Timeout: &metav1.Duration{Duration: longTimeout},
 			}},
 		},
 		Visibility: netv1alpha1.IngressVisibilityClusterLocal,
@@ -478,16 +481,16 @@ func TestMakeIngressSpec_CorrectRulesWithTagBasedRouting(t *testing.T) {
 						"Knative-Serving-Namespace": ns,
 					},
 				}},
-				Timeout: &metav1.Duration{Duration: 48 * time.Hour},
+				Timeout: &metav1.Duration{Duration: longTimeout},
 			}},
 		},
 		Visibility: netv1alpha1.IngressVisibilityExternalIP,
 	}}
 
 	ctx := testContext()
-	config.FromContext(ctx).Features.TagHeaderBasedRouting = apiConfig.Enabled
+	config.FromContext(ctx).Features.TagHeaderBasedRouting = apicfg.Enabled
 
-	ci, err := MakeIngressSpec(ctx, r, nil, targets, nil /* visibility */)
+	ci, err := makeIngressSpec(ctx, r, nil, &traffic.Config{Targets: targets})
 	if err != nil {
 		t.Error("Unexpected error", err)
 	}
@@ -498,7 +501,7 @@ func TestMakeIngressSpec_CorrectRulesWithTagBasedRouting(t *testing.T) {
 }
 
 // One active target.
-func TestMakeIngressRule_Vanilla(t *testing.T) {
+func TestMakeIngressRuleVanilla(t *testing.T) {
 	targets := []traffic.RevisionTarget{{
 		TrafficTarget: v1.TrafficTarget{
 			ConfigurationName: "config",
@@ -529,7 +532,51 @@ func TestMakeIngressRule_Vanilla(t *testing.T) {
 						"Knative-Serving-Namespace": ns,
 					},
 				}},
-				Timeout: &metav1.Duration{Duration: 48 * time.Hour},
+				Timeout: &metav1.Duration{Duration: longTimeout},
+			}},
+		},
+		Visibility: netv1alpha1.IngressVisibilityExternalIP,
+	}
+
+	if !cmp.Equal(expected, rule) {
+		t.Error("Unexpected rule (-want, +got):", cmp.Diff(expected, rule))
+	}
+}
+
+func TestExtraLongTimeout(t *testing.T) {
+	targets := []traffic.RevisionTarget{{
+		TrafficTarget: v1.TrafficTarget{
+			ConfigurationName: "config",
+			RevisionName:      "revision",
+			Percent:           ptr.Int64(100),
+		},
+		ServiceName: "chocolate",
+		Active:      true,
+	}}
+	domains := []string{"a.com", "b.org"}
+	ctx := testContext()
+	apicfg.FromContext(ctx).Defaults.MaxRevisionTimeoutSeconds = int64(longTimeout.Seconds() * 2)
+	rule := makeIngressRule(ctx, domains, ns, netv1alpha1.IngressVisibilityExternalIP, targets)
+	expected := netv1alpha1.IngressRule{
+		Hosts: []string{
+			"a.com",
+			"b.org",
+		},
+		HTTP: &netv1alpha1.HTTPIngressRuleValue{
+			Paths: []netv1alpha1.HTTPIngressPath{{
+				Splits: []netv1alpha1.IngressBackendSplit{{
+					IngressBackend: netv1alpha1.IngressBackend{
+						ServiceNamespace: ns,
+						ServiceName:      "chocolate",
+						ServicePort:      intstr.FromInt(80),
+					},
+					Percent: 100,
+					AppendHeaders: map[string]string{
+						"Knative-Serving-Revision":  "revision",
+						"Knative-Serving-Namespace": ns,
+					},
+				}},
+				Timeout: &metav1.Duration{Duration: 2 * longTimeout},
 			}},
 		},
 		Visibility: netv1alpha1.IngressVisibilityExternalIP,
@@ -541,7 +588,7 @@ func TestMakeIngressRule_Vanilla(t *testing.T) {
 }
 
 // One active target and a target of zero percent.
-func TestMakeIngressRule_ZeroPercentTarget(t *testing.T) {
+func TestMakeIngressRuleZeroPercentTarget(t *testing.T) {
 	targets := []traffic.RevisionTarget{{
 		TrafficTarget: v1.TrafficTarget{
 			ConfigurationName: "config",
@@ -576,7 +623,7 @@ func TestMakeIngressRule_ZeroPercentTarget(t *testing.T) {
 						"Knative-Serving-Namespace": ns,
 					},
 				}},
-				Timeout: &metav1.Duration{Duration: 48 * time.Hour},
+				Timeout: &metav1.Duration{Duration: longTimeout},
 			}},
 		},
 		Visibility: netv1alpha1.IngressVisibilityExternalIP,
@@ -588,7 +635,7 @@ func TestMakeIngressRule_ZeroPercentTarget(t *testing.T) {
 }
 
 // One active target and a target of nil (implied zero) percent.
-func TestMakeIngressRule_NilPercentTarget(t *testing.T) {
+func TestMakeIngressRuleNilPercentTarget(t *testing.T) {
 	targets := []traffic.RevisionTarget{{
 		TrafficTarget: v1.TrafficTarget{
 			ConfigurationName: "config",
@@ -623,7 +670,7 @@ func TestMakeIngressRule_NilPercentTarget(t *testing.T) {
 						"Knative-Serving-Namespace": ns,
 					},
 				}},
-				Timeout: &metav1.Duration{Duration: 48 * time.Hour},
+				Timeout: &metav1.Duration{Duration: longTimeout},
 			}},
 		},
 		Visibility: netv1alpha1.IngressVisibilityExternalIP,
@@ -635,7 +682,7 @@ func TestMakeIngressRule_NilPercentTarget(t *testing.T) {
 }
 
 // Two active targets.
-func TestMakeIngressRule_TwoTargets(t *testing.T) {
+func TestMakeIngressRuleTwoTargets(t *testing.T) {
 	targets := []traffic.RevisionTarget{{
 		TrafficTarget: v1.TrafficTarget{
 			ConfigurationName: "config",
@@ -682,7 +729,7 @@ func TestMakeIngressRule_TwoTargets(t *testing.T) {
 						"Knative-Serving-Revision":  "new-revision",
 					},
 				}},
-				Timeout: &metav1.Duration{Duration: 48 * time.Hour},
+				Timeout: &metav1.Duration{Duration: longTimeout},
 			}},
 		},
 		Visibility: netv1alpha1.IngressVisibilityExternalIP,
@@ -694,7 +741,7 @@ func TestMakeIngressRule_TwoTargets(t *testing.T) {
 }
 
 // Inactive target.
-func TestMakeIngressRule_InactiveTarget(t *testing.T) {
+func TestMakeIngressRuleInactiveTarget(t *testing.T) {
 	targets := []traffic.RevisionTarget{{
 		TrafficTarget: v1.TrafficTarget{
 			ConfigurationName: "config",
@@ -725,7 +772,7 @@ func TestMakeIngressRule_InactiveTarget(t *testing.T) {
 						"Knative-Serving-Namespace": ns,
 					},
 				}},
-				Timeout: &metav1.Duration{Duration: 48 * time.Hour},
+				Timeout: &metav1.Duration{Duration: longTimeout},
 			}},
 		},
 		Visibility: netv1alpha1.IngressVisibilityExternalIP,
@@ -736,7 +783,7 @@ func TestMakeIngressRule_InactiveTarget(t *testing.T) {
 }
 
 // Two inactive targets.
-func TestMakeIngressRule_TwoInactiveTargets(t *testing.T) {
+func TestMakeIngressRuleTwoInactiveTargets(t *testing.T) {
 	targets := []traffic.RevisionTarget{{
 		TrafficTarget: v1.TrafficTarget{
 			ConfigurationName: "config",
@@ -786,7 +833,7 @@ func TestMakeIngressRule_TwoInactiveTargets(t *testing.T) {
 						"Knative-Serving-Namespace": ns,
 					},
 				}},
-				Timeout: &metav1.Duration{Duration: 48 * time.Hour},
+				Timeout: &metav1.Duration{Duration: longTimeout},
 			}},
 		},
 		Visibility: netv1alpha1.IngressVisibilityExternalIP,
@@ -796,7 +843,7 @@ func TestMakeIngressRule_TwoInactiveTargets(t *testing.T) {
 	}
 }
 
-func TestMakeIngressRule_ZeroPercentTargetInactive(t *testing.T) {
+func TestMakeIngressRuleZeroPercentTargetInactive(t *testing.T) {
 	targets := []traffic.RevisionTarget{{
 		TrafficTarget: v1.TrafficTarget{
 			ConfigurationName: "config",
@@ -831,7 +878,7 @@ func TestMakeIngressRule_ZeroPercentTargetInactive(t *testing.T) {
 						"Knative-Serving-Namespace": ns,
 					},
 				}},
-				Timeout: &metav1.Duration{Duration: 48 * time.Hour},
+				Timeout: &metav1.Duration{Duration: longTimeout},
 			}},
 		},
 		Visibility: netv1alpha1.IngressVisibilityExternalIP,
@@ -842,7 +889,7 @@ func TestMakeIngressRule_ZeroPercentTargetInactive(t *testing.T) {
 	}
 }
 
-func TestMakeIngressRule_NilPercentTargetInactive(t *testing.T) {
+func TestMakeIngressRuleNilPercentTargetInactive(t *testing.T) {
 	targets := []traffic.RevisionTarget{{
 		TrafficTarget: v1.TrafficTarget{
 			ConfigurationName: "config",
@@ -877,7 +924,7 @@ func TestMakeIngressRule_NilPercentTargetInactive(t *testing.T) {
 						"Knative-Serving-Namespace": ns,
 					},
 				}},
-				Timeout: &metav1.Duration{Duration: 48 * time.Hour},
+				Timeout: &metav1.Duration{Duration: longTimeout},
 			}},
 		},
 		Visibility: netv1alpha1.IngressVisibilityExternalIP,
@@ -1007,7 +1054,7 @@ func TestMakeIngressACMEChallenges(t *testing.T) {
 						"Knative-Serving-Namespace": "test-ns",
 					},
 				}},
-				Timeout: &metav1.Duration{Duration: 48 * time.Hour},
+				Timeout: &metav1.Duration{Duration: longTimeout},
 			}}},
 	}, {
 		Hosts: []string{
@@ -1038,11 +1085,11 @@ func TestMakeIngressACMEChallenges(t *testing.T) {
 						"Knative-Serving-Namespace": "test-ns",
 					},
 				}},
-				Timeout: &metav1.Duration{Duration: 48 * time.Hour},
+				Timeout: &metav1.Duration{Duration: longTimeout},
 			}}},
 	}}
 
-	ci, err := MakeIngressSpec(testContext(), r, nil, targets, nil /* visibility */, acmeChallenge)
+	ci, err := makeIngressSpec(testContext(), r, nil, &traffic.Config{Targets: targets}, acmeChallenge)
 	if err != nil {
 		t.Error("Unexpected error", err)
 	}
@@ -1129,8 +1176,8 @@ func TestMakeIngressFailToGenerateTagHost(t *testing.T) {
 func testContext() context.Context {
 	ctx := context.Background()
 	cfg := testConfig()
-	configDefaults, _ := apiConfig.NewDefaultsConfigFromMap(nil)
-	return config.ToContext(apiConfig.ToContext(ctx, &apiConfig.Config{
+	configDefaults, _ := apicfg.NewDefaultsConfigFromMap(nil)
+	return config.ToContext(apicfg.ToContext(ctx, &apicfg.Config{
 		Defaults: configDefaults,
 	}), cfg)
 }