From c01795f154b558a5f500109553892ca101e7d09a Mon Sep 17 00:00:00 2001 From: Tanzeeb Khalili Date: Wed, 5 Jun 2019 19:20:23 -0400 Subject: [PATCH 1/3] Mark `IngressNotConfigured` on Route when there is no Ingress status Fixes #4072 --- pkg/apis/serving/v1alpha1/route_lifecycle.go | 6 ++++++ pkg/apis/serving/v1alpha1/route_lifecycle_test.go | 10 +++++++++- pkg/reconciler/route/table_test.go | 14 +++++++------- pkg/testing/v1alpha1/route.go | 5 +++++ 4 files changed, 27 insertions(+), 8 deletions(-) diff --git a/pkg/apis/serving/v1alpha1/route_lifecycle.go b/pkg/apis/serving/v1alpha1/route_lifecycle.go index 5df2f212596f..c133767a3013 100644 --- a/pkg/apis/serving/v1alpha1/route_lifecycle.go +++ b/pkg/apis/serving/v1alpha1/route_lifecycle.go @@ -67,6 +67,11 @@ func (rs *RouteStatus) MarkServiceNotOwned(name string) { fmt.Sprintf("There is an existing placeholder Service %q that we do not own.", name)) } +func (rs *RouteStatus) MarkIngressNotConfigured() { + routeCondSet.Manage(rs).MarkUnknown(RouteConditionIngressReady, + "IngressNotConfigured", "Failed to configure route ingress.") +} + func (rs *RouteStatus) MarkTrafficAssigned() { routeCondSet.Manage(rs).MarkTrue(RouteConditionAllTrafficAssigned) } @@ -150,6 +155,7 @@ func (rs *RouteStatus) MarkCertificateNotOwned(name string) { func (rs *RouteStatus) PropagateClusterIngressStatus(cs v1alpha1.IngressStatus) { cc := cs.GetCondition(v1alpha1.IngressConditionReady) if cc == nil { + rs.MarkIngressNotConfigured() return } switch { diff --git a/pkg/apis/serving/v1alpha1/route_lifecycle_test.go b/pkg/apis/serving/v1alpha1/route_lifecycle_test.go index 03cd3c6f47da..916c932649ba 100644 --- a/pkg/apis/serving/v1alpha1/route_lifecycle_test.go +++ b/pkg/apis/serving/v1alpha1/route_lifecycle_test.go @@ -269,7 +269,7 @@ func TestClusterIngressFailureRecovery(t *testing.T) { apitesting.CheckConditionOngoing(r.duck(), RouteConditionIngressReady, t) apitesting.CheckConditionOngoing(r.duck(), RouteConditionReady, t) - // Empty IngressStatus keeps things as-is. + // Empty IngressStatus marks ingress "NotConfigured" r.PropagateClusterIngressStatus(netv1alpha1.IngressStatus{}) apitesting.CheckConditionOngoing(r.duck(), RouteConditionAllTrafficAssigned, t) apitesting.CheckConditionOngoing(r.duck(), RouteConditionIngressReady, t) @@ -378,3 +378,11 @@ func TestRouteNotOwnCertificate(t *testing.T) { apitesting.CheckConditionFailed(r.duck(), RouteConditionCertificateProvisioned, t) } + +func TestIngressNotConfigured(t *testing.T) { + r := &RouteStatus{} + r.InitializeConditions() + r.MarkIngressNotConfigured() + + apitesting.CheckConditionOngoing(r.duck(), RouteConditionIngressReady, t) +} diff --git a/pkg/reconciler/route/table_test.go b/pkg/reconciler/route/table_test.go index f0002de8f1f1..d15a95d26d5f 100644 --- a/pkg/reconciler/route/table_test.go +++ b/pkg/reconciler/route/table_test.go @@ -156,7 +156,7 @@ func TestReconcile(t *testing.T) { WithRouteUID("12-34"), // Populated by reconciliation when all traffic has been assigned. WithURL, WithAddress, WithInitRouteConditions, - MarkTrafficAssigned, WithStatusTraffic(v1alpha1.TrafficTarget{ + MarkTrafficAssigned, MarkIngressNotConfigured, WithStatusTraffic(v1alpha1.TrafficTarget{ TrafficTarget: v1beta1.TrafficTarget{ RevisionName: "config-00001", Percent: 100, @@ -213,7 +213,7 @@ func TestReconcile(t *testing.T) { WithRouteUID("12-34"), WithIngressClass("custom-ingress-class"), // Populated by reconciliation when all traffic has been assigned. WithURL, WithAddress, WithInitRouteConditions, - MarkTrafficAssigned, WithStatusTraffic(v1alpha1.TrafficTarget{ + MarkTrafficAssigned, MarkIngressNotConfigured, WithStatusTraffic(v1alpha1.TrafficTarget{ TrafficTarget: v1beta1.TrafficTarget{ RevisionName: "config-00001", Percent: 100, @@ -274,7 +274,7 @@ func TestReconcile(t *testing.T) { // Populated by reconciliation when all traffic has been assigned. WithLocalDomain, WithAddress, WithInitRouteConditions, WithRouteLabel("serving.knative.dev/visibility", "cluster-local"), - MarkTrafficAssigned, WithStatusTraffic(v1alpha1.TrafficTarget{ + MarkTrafficAssigned, MarkIngressNotConfigured, WithStatusTraffic(v1alpha1.TrafficTarget{ TrafficTarget: v1beta1.TrafficTarget{ RevisionName: "config-00001", Percent: 100, @@ -1273,7 +1273,7 @@ func TestReconcile(t *testing.T) { }, }), WithRouteUID("34-78"), WithURL, WithAddress, WithInitRouteConditions, - MarkTrafficAssigned, WithStatusTraffic( + MarkTrafficAssigned, MarkIngressNotConfigured, WithStatusTraffic( v1alpha1.TrafficTarget{ TrafficTarget: v1beta1.TrafficTarget{ RevisionName: "blue-00001", @@ -1434,7 +1434,7 @@ func TestReconcile(t *testing.T) { }, }), WithRouteUID("1-2"), WithRouteFinalizer, WithURL, WithAddress, WithInitRouteConditions, - MarkTrafficAssigned, WithStatusTraffic( + MarkTrafficAssigned, MarkIngressNotConfigured, WithStatusTraffic( v1alpha1.TrafficTarget{ TrafficTarget: v1beta1.TrafficTarget{ Tag: "gray", @@ -1948,7 +1948,7 @@ func TestReconcile_EnableAutoTLS(t *testing.T) { WithRouteUID("12-34"), // Populated by reconciliation when all traffic has been assigned. WithURL, WithAddress, WithInitRouteConditions, - MarkTrafficAssigned, WithStatusTraffic(v1alpha1.TrafficTarget{ + MarkTrafficAssigned, MarkIngressNotConfigured, WithStatusTraffic(v1alpha1.TrafficTarget{ TrafficTarget: v1beta1.TrafficTarget{ RevisionName: "config-00001", Percent: 100, @@ -2027,7 +2027,7 @@ func TestReconcile_EnableAutoTLS(t *testing.T) { WithRouteUID("12-34"), // Populated by reconciliation when all traffic has been assigned. WithAddress, WithInitRouteConditions, - MarkTrafficAssigned, WithStatusTraffic(v1alpha1.TrafficTarget{ + MarkTrafficAssigned, MarkIngressNotConfigured, WithStatusTraffic(v1alpha1.TrafficTarget{ TrafficTarget: v1beta1.TrafficTarget{ RevisionName: "config-00001", Percent: 100, diff --git a/pkg/testing/v1alpha1/route.go b/pkg/testing/v1alpha1/route.go index bfd06d31c780..c29b247ddfbd 100644 --- a/pkg/testing/v1alpha1/route.go +++ b/pkg/testing/v1alpha1/route.go @@ -177,6 +177,11 @@ func MarkIngressReady(r *v1alpha1.Route) { }) } +// MarkIngressNotConfigured calls the method of the same name on .Status +func MarkIngressNotConfigured(r *v1alpha1.Route) { + r.Status.MarkIngressNotConfigured() +} + // MarkMissingTrafficTarget calls the method of the same name on .Status func MarkMissingTrafficTarget(kind, revision string) RouteOption { return func(r *v1alpha1.Route) { From 37441dd31d4639623e75fe4b63c9da245487fece Mon Sep 17 00:00:00 2001 From: Tanzeeb Khalili Date: Mon, 17 Jun 2019 10:27:18 -0400 Subject: [PATCH 2/3] Update message to be more optimistic --- pkg/apis/serving/v1alpha1/route_lifecycle.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/apis/serving/v1alpha1/route_lifecycle.go b/pkg/apis/serving/v1alpha1/route_lifecycle.go index c133767a3013..4b3763f3dd02 100644 --- a/pkg/apis/serving/v1alpha1/route_lifecycle.go +++ b/pkg/apis/serving/v1alpha1/route_lifecycle.go @@ -69,7 +69,7 @@ func (rs *RouteStatus) MarkServiceNotOwned(name string) { func (rs *RouteStatus) MarkIngressNotConfigured() { routeCondSet.Manage(rs).MarkUnknown(RouteConditionIngressReady, - "IngressNotConfigured", "Failed to configure route ingress.") + "IngressNotConfigured", "Ingress has not yet been reconciled.") } func (rs *RouteStatus) MarkTrafficAssigned() { From caf05838b7746c789b6e7946e76cb1997f4d0917 Mon Sep 17 00:00:00 2001 From: Tanzeeb Khalili Date: Wed, 19 Jun 2019 13:06:50 -0400 Subject: [PATCH 3/3] Add comment for exported function --- pkg/apis/serving/v1alpha1/route_lifecycle.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pkg/apis/serving/v1alpha1/route_lifecycle.go b/pkg/apis/serving/v1alpha1/route_lifecycle.go index 4b3763f3dd02..6b1fa92106a7 100644 --- a/pkg/apis/serving/v1alpha1/route_lifecycle.go +++ b/pkg/apis/serving/v1alpha1/route_lifecycle.go @@ -67,6 +67,8 @@ func (rs *RouteStatus) MarkServiceNotOwned(name string) { fmt.Sprintf("There is an existing placeholder Service %q that we do not own.", name)) } +// MarkIngressNotConfigured changes the IngressReady condition to be unknown to reflect +// that the Ingress does not yet have a Status func (rs *RouteStatus) MarkIngressNotConfigured() { routeCondSet.Manage(rs).MarkUnknown(RouteConditionIngressReady, "IngressNotConfigured", "Ingress has not yet been reconciled.")