internal: Allow status on HTTPRoutes to be set #3456

stevesloka · 2021-03-08T17:49:53Z

Update the status package to process GatewayAPI HTTPRoutes.
Update the dag.GatewayAPIProcessor to set status.

Signed-off-by: Steve Sloka [email protected]

codecov · 2021-03-08T18:30:14Z

Codecov Report

Merging #3456 (5e8ab4c) into main (74ac550) will decrease coverage by 0.17%.
The diff coverage is 56.19%.

❗ Current head 5e8ab4c differs from pull request most recent head 5ad0395. Consider uploading reports for the commit 5ad0395 to get more accurate results

@@            Coverage Diff             @@
##             main    #3456      +/-   ##
==========================================
- Coverage   75.45%   75.28%   -0.18%     
==========================================
  Files          98       99       +1     
  Lines        6740     6818      +78     
==========================================
+ Hits         5086     5133      +47     
- Misses       1534     1569      +35     
+ Partials      120      116       -4

Impacted Files	Coverage Δ
internal/k8s/status.go	`10.25% <ø> (ø)`
internal/status/httproutestatus.go	`34.54% <34.54%> (ø)`
internal/status/cache.go	`70.58% <46.66%> (-18.31%)`	⬇️
internal/status/proxystatus.go	`82.45% <86.66%> (+1.50%)`	⬆️
internal/dag/builder.go	`100.00% <100.00%> (ø)`
internal/dag/gatewayapi_processor.go	`82.72% <100.00%> (+17.04%)`	⬆️
internal/k8s/kind.go	`52.38% <100.00%> (+2.38%)`	⬆️
internal/k8s/log.go	`63.04% <0.00%> (-6.53%)`	⬇️

internal/status/cache.go

skriss · 2021-03-18T14:41:31Z

has a conflict to resolve now.

stevesloka · 2021-03-23T15:47:28Z

All rebased @skriss, there are probably more spots to set status now that more features have been added in the last 2 weeks. I can look to add them, but won't change the overall code base.

skriss · 2021-03-23T16:08:31Z

👍 I'll try to get through this this afternoon, sorry for the delay

stevesloka · 2021-03-23T17:38:54Z

sorry for the delay

No worries! 🌮

sunjayBhatia

Overall looks good and straightforward to me! Just some comments on some specifics but generally LGTM

internal/k8s/kind.go

sunjayBhatia · 2021-03-23T18:00:03Z

internal/dag/gatewayapi_processor.go


 	// Validate TLS Configuration
 	if route.Spec.TLS != nil {
-		p.Error("NOT IMPLEMENTED: The 'RouteTLSConfig' is not yet implemented.")
+		routeAccessor.ConditionFor(status.NotImplemented, "NotImplemented", "HTTPRoute.Spec.TLS: Not yet implemented.")


Maybe worth calling out somewhere that we are adding additional conditions with details about why a Route is invalid, I forget if the original design doc mentioned this specifically? Or if it was assumed the reason/description on the "Admitted" reason would have all this info?

Another point for not using status as message passing is that it could be relatively unwieldy it seems, maybe keying off of reasons is good enough, but if you need more detail than that you would want some structured description (JSON probably?)

Echoing @sunjayBhatia's q on the conditions, "Admitted" is the only type that Gateway API documents, so should we only use that? I'm not sure, I guess it depends on who is consuming the info.

So, it's okay for implementations to add their own Conditions, but they should roll up into the Admitted condition. I think that Steve has handled that here in another way, but I think it would be best to be clear about what we're doing, and make sure that the status gets rolled up into the Admitted condition at the end (we can use the commit function to do that).

I think that ConditionFor is not a good name for this one, as it's really doing an AddCondition to the proxy update.

If we only use Admitted then we can only set a single status on the resource? I was under the impression that since it's a set, we'd want to list out all the issues with a route and not just a single.

internal/dag/status_test.go

sunjayBhatia · 2021-03-23T18:06:02Z

internal/dag/gatewayapi_processor.go

@@ -194,44 +197,44 @@ func (p *GatewayAPIProcessor) computeHTTPRoute(route *gatewayapi_v1alpha1.HTTPRo
 			case gatewayapi_v1alpha1.PathMatchPrefix:
 				pathPrefixes = append(pathPrefixes, stringOrDefault(match.Path.Value, "/"))
 			default:
-				p.Error("NOT IMPLEMENTED: Only PathMatchPrefix is currently implemented.")
+				routeAccessor.ConditionFor(status.Invalid, "PathMatchType", "HTTPRoute.Spec.Rules.PathMatch: Only Prefix match type is supported.")


Do we want to pull the reasons into constants as well or since these are temporary we're ok with leaving them as is?

Same logic as the conditions I guess, constraining what could be passed in with a type

In general, I agree that we should make a set of defined Reasons for any condition we're using. As you say, because this is temporary though, we can probably not worry for now. Depends on how we update ConditionFor.

sunjayBhatia · 2021-03-23T18:06:29Z

internal/dag/gatewayapi_processor.go

 		for _, forward := range rule.ForwardTo {
 			// Verify the service is valid
 			if forward.ServiceName == nil {
-				p.Error("ServiceName must be specified and is currently only type implemented!")
+				routeAccessor.ConditionFor(status.Invalid, "ForwardTo", "Spec.Rules.ForwardTo.ServiceName must be specified.")


This reason seems like it would be good to have as a constant?

internal/status/httproutestatus.go

skriss

overall looks good, handful of comments

skriss · 2021-03-24T15:29:48Z

internal/dag/gatewayapi_processor.go


 	// Validate TLS Configuration
 	if route.Spec.TLS != nil {
-		p.Error("NOT IMPLEMENTED: The 'RouteTLSConfig' is not yet implemented.")
+		routeAccessor.ConditionFor(status.NotImplemented, "NotImplemented", "HTTPRoute.Spec.TLS: Not yet implemented.")


Echoing @sunjayBhatia's q on the conditions, "Admitted" is the only type that Gateway API documents, so should we only use that? I'm not sure, I guess it depends on who is consuming the info.

skriss · 2021-03-24T15:43:34Z

internal/dag/gatewayapi_processor.go

+	// Determine if any errors exist in conditions and set the "Admitted"
+	// condition accordingly.
+	switch len(routeAccessor.Conditions) {
+	case 0:
+		routeAccessor.ConditionFor(status.AdmittedConditionValid, "Valid", "Valid HTTPRoute")
+	default:
+		routeAccessor.ConditionFor(status.AdmittedConditionInvalid, "ErrorsExist", "Errors Found, check other Conditions for details.")
+	}


Ah, I see now that you're setting the Admitted condition here.

Yeah, this is the rollup I was talking about. I think it's definitely worth some comments/explanations that this is what we are doing, as it's hard to see the flow as it stands. I also think that we need to try to use the upstream Reasons wherever we can, as if we don't we will diverge from the standards. We can, of course, push updates to the upstream reasons if we need to.

skriss · 2021-03-24T15:45:52Z

internal/status/cache.go

+// AdmittedConditionValid is the valid ConditionType for Admitted.
+const AdmittedConditionValid ConditionType = "AdmittedValid"
+
+// AdmittedConditionInValid is the invalid ConditionType for Admitted.
+const AdmittedConditionInvalid ConditionType = "AdmittedInvalid"


this was slightly confusing because these aren't actually condition types; they correspond to the different states ("True" or "False" for the Admitted condition type). Thinking about suggestions here.

internal/status/httproutestatus.go

skriss · 2021-03-24T15:50:50Z

internal/status/httproutestatus.go

+}
+
+// ConditionFor returns a v1.Condition for a given ConditionType.
+func (pu *HTTPRouteUpdate) ConditionFor(cond ConditionType, reason string, message string) metav1.Condition {


WDYT about having this function also take a status as an arg, so instead of having the "AdmittedValid" and "AdmittedInvalid" psuedo-condition types, you can just explicitly pass "Admitted", "True" or "Admitted", "False" from the call sites?

Yes, the pattern I used for DetailedCondition doesn't work here. That pattern was that you call ConditionFor to get a DetailedCondition, that has AddError and AddWarning methods on it. In this case, the upstream Condition has none of that, so a better pattern would probably be to just have an AddCondition or UpsertCondition method that would upsert a Condition to the object, taking type, status, reason, and message, and then using the holding HTTPRouteUpdate's Generation to calculate the ObservedGeneration for you.

So, my suggestion for this signature is this:

Suggested change

func (pu *HTTPRouteUpdate) ConditionFor(cond ConditionType, reason string, message string) metav1.Condition {

func (pu *HTTPRouteUpdate) AddCondition(cond ConditionType, status bool, reason string, message string) metav1.Condition {

Obviously, this would mean changes at the calling site as well.

I'm sorry that I led us down the wrong path with DetailedCondition.

internal/status/cache.go

youngnick

This is looking pretty good, although I have two broad things I think need fixing:

It's vital that we get the ObservedGeneration behavior right.
If we're going to generate extra Conditions per GatewayRef, we have to document what they are going to be, and provide a type with constants for the type and reason fields, and document what they're for when we use them, just like the upstream gateway-apis code does.

I think that having extra conditions is great, as long as we make sure we're doing the Condition contract as expected by the upstream code.

internal/status/cache.go

internal/status/httproutestatus.go

youngnick · 2021-03-26T00:42:31Z

internal/status/httproutestatus.go

+}
+
+// ConditionFor returns a v1.Condition for a given ConditionType.
+func (pu *HTTPRouteUpdate) ConditionFor(cond ConditionType, reason string, message string) metav1.Condition {


Yes, the pattern I used for DetailedCondition doesn't work here. That pattern was that you call ConditionFor to get a DetailedCondition, that has AddError and AddWarning methods on it. In this case, the upstream Condition has none of that, so a better pattern would probably be to just have an AddCondition or UpsertCondition method that would upsert a Condition to the object, taking type, status, reason, and message, and then using the holding HTTPRouteUpdate's Generation to calculate the ObservedGeneration for you.

So, my suggestion for this signature is this:

Suggested change

func (pu *HTTPRouteUpdate) ConditionFor(cond ConditionType, reason string, message string) metav1.Condition {

func (pu *HTTPRouteUpdate) AddCondition(cond ConditionType, status bool, reason string, message string) metav1.Condition {

Obviously, this would mean changes at the calling site as well.

I'm sorry that I led us down the wrong path with DetailedCondition.

youngnick · 2021-03-26T00:47:33Z

internal/status/httproutestatus.go

+type HTTPRouteUpdate struct {
+	Fullname           types.NamespacedName
+	Conditions         []metav1.Condition
+	ExistingConditions []metav1.Condition
+	GatewayRef         types.NamespacedName
+}


Because of how the Condition contract requires us to update the ObservedGeneration each time we update the Condition, the HTTPRouteUpdate has to include a Generation field, like the ProxyUpdate does. This should be populated from the object when the update is created with the HTTPRouteAccessor() call, and then copied into the ObservedGeneration of the condition when the condition is generated with ConditionFor.

I'll add some other comments to cover this.

youngnick · 2021-03-26T00:49:12Z

internal/status/httproutestatus.go

+		newDc.Type = "Admitted"
+	}
+	newDc.Message = message
+	newDc.LastTransitionTime = metav1.NewTime(time.Now())


newDC.ObservedGeneration should be updated around here as well.

youngnick · 2021-03-26T00:50:55Z

internal/status/httproutestatus_test.go

+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+func TestHTTPRouteConditionFor(t *testing.T) {


The test is also going to need to test the ObservedGeneration bits, I think.

youngnick · 2021-03-26T01:02:09Z

internal/dag/gatewayapi_processor.go


 	// Validate TLS Configuration
 	if route.Spec.TLS != nil {
-		p.Error("NOT IMPLEMENTED: The 'RouteTLSConfig' is not yet implemented.")
+		routeAccessor.ConditionFor(status.NotImplemented, "NotImplemented", "HTTPRoute.Spec.TLS: Not yet implemented.")


So, it's okay for implementations to add their own Conditions, but they should roll up into the Admitted condition. I think that Steve has handled that here in another way, but I think it would be best to be clear about what we're doing, and make sure that the status gets rolled up into the Admitted condition at the end (we can use the commit function to do that).

I think that ConditionFor is not a good name for this one, as it's really doing an AddCondition to the proxy update.

youngnick · 2021-03-26T01:03:43Z

internal/dag/gatewayapi_processor.go

@@ -194,44 +197,44 @@ func (p *GatewayAPIProcessor) computeHTTPRoute(route *gatewayapi_v1alpha1.HTTPRo
 			case gatewayapi_v1alpha1.PathMatchPrefix:
 				pathPrefixes = append(pathPrefixes, stringOrDefault(match.Path.Value, "/"))
 			default:
-				p.Error("NOT IMPLEMENTED: Only PathMatchPrefix is currently implemented.")
+				routeAccessor.ConditionFor(status.Invalid, "PathMatchType", "HTTPRoute.Spec.Rules.PathMatch: Only Prefix match type is supported.")


In general, I agree that we should make a set of defined Reasons for any condition we're using. As you say, because this is temporary though, we can probably not worry for now. Depends on how we update ConditionFor.

youngnick · 2021-03-26T01:05:36Z

internal/dag/gatewayapi_processor.go

+	// Determine if any errors exist in conditions and set the "Admitted"
+	// condition accordingly.
+	switch len(routeAccessor.Conditions) {
+	case 0:
+		routeAccessor.ConditionFor(status.AdmittedConditionValid, "Valid", "Valid HTTPRoute")
+	default:
+		routeAccessor.ConditionFor(status.AdmittedConditionInvalid, "ErrorsExist", "Errors Found, check other Conditions for details.")
+	}


Yeah, this is the rollup I was talking about. I think it's definitely worth some comments/explanations that this is what we are doing, as it's hard to see the flow as it stands. I also think that we need to try to use the upstream Reasons wherever we can, as if we don't we will diverge from the standards. We can, of course, push updates to the upstream reasons if we need to.

internal/dag/status_test.go

stevesloka · 2021-03-29T20:07:34Z

I think I've addressed everyone's comments, can you all take a look again? Thanks!

internal/dag/gatewayapi_processor.go

internal/status/cache.go

internal/status/httproutestatus.go

skriss · 2021-03-29T21:55:27Z

internal/status/httproutestatus.go

+	for _, cond := range routeUpdate.Conditions {
+
+		// Look through the newly calculated conditions, if they already exist
+		// on the object or if our obervation is stale, then leave them be,
+		// otherwise add them to the list to be written back to the API.
+		for _, existingCond := range routeUpdate.ExistingConditions {
+			if (cond.Status == existingCond.Status &&
+				cond.Reason == existingCond.Reason &&
+				cond.Message == existingCond.Message &&
+				cond.Type == existingCond.Type) ||
+				existingCond.ObservedGeneration > cond.ObservedGeneration {
+
+				cond.ObservedGeneration = existingCond.ObservedGeneration
+				cond.LastTransitionTime = existingCond.LastTransitionTime
+			}
+		}
+
+		conditionsToWrite = append(conditionsToWrite, cond)


OK, I took a look at the HTTPProxy equivalent here, and I think there are a couple things:

at the top of the loop, we should always set cond.ObservedGeneration = routeUpdate.Generation and cond.LastTransitionTime = routeUpdate.LastTransitionTime

the inner loop should be looking for an existing Condition on the HTTPRoute that's got the same Type, but a newer observed generation. If we find one, that means cond is stale, so we should keep the existing one instead. If we don't find a newer existing one, then we should use cond.

So I think this should look more like (I wrote this in GitHub so it's definitely not 100% right):

for _, cond := range routeUpdate.Conditions { // set the Condition's observed generation based on // the generation of the HTTPRoute we looked at. cond.ObservedGeneration = routeUpdate.Generation cond.LastTransitionTime = routeUpdate.TransitionTime // is there a newer Condition on the HTTPRoute matching // this condition's type? If so, our observation is stale, // so don't write it, keep the newer one instead. var newerConditionExists bool for _, existingCond := routeUpdate.ExistingConditions { if existingCond.Type != cond.Type { continue } if existingCond.ObservedGeneration > cond.ObservedGeneration { conditionsToWrite = append(conditionsToWrite, existingCond) newerConditionExists = true break } } // if we didn't find a newer version of the Condition on the // HTTPRoute, then write the one we computed. if !newerConditionExists { conditionsToWrite = append(conditionsToWrite, cond) } }

That's what I was looking for in my earlier comment, thanks for making it clearer @skriss!

youngnick

This mostly LGTM, I think that Steve's suggestion is great.

internal/status/cache.go

youngnick · 2021-03-29T22:12:02Z

internal/status/httproutestatus.go

+	for _, cond := range routeUpdate.Conditions {
+
+		// Look through the newly calculated conditions, if they already exist
+		// on the object or if our obervation is stale, then leave them be,
+		// otherwise add them to the list to be written back to the API.
+		for _, existingCond := range routeUpdate.ExistingConditions {
+			if (cond.Status == existingCond.Status &&
+				cond.Reason == existingCond.Reason &&
+				cond.Message == existingCond.Message &&
+				cond.Type == existingCond.Type) ||
+				existingCond.ObservedGeneration > cond.ObservedGeneration {
+
+				cond.ObservedGeneration = existingCond.ObservedGeneration
+				cond.LastTransitionTime = existingCond.LastTransitionTime
+			}
+		}
+
+		conditionsToWrite = append(conditionsToWrite, cond)


That's what I was looking for in my earlier comment, thanks for making it clearer @skriss!

internal/status/proxystatus_test.go

youngnick · 2021-03-29T23:02:19Z

internal/status/httproutestatus.go

+}
+
+// AddCondition returns a metav1.Condition for a given ConditionType.
+func (routeUpdate *HTTPRouteUpdate) AddCondition(cond gatewayapi_v1alpha1.RouteConditionType, status metav1.ConditionStatus, reason RouteReasonType, message string) metav1.Condition {


This method really should do some checking by cond to see if there's an existing one, since conditions are supposed to be a List that's keyed of the Type. Not a blocker on this one, maybe a TODO though?

Ohh you can't have multiple conditions of the same type? What if you had two different missing services in a ForwardTo? Or on two different paths in the same HTTPRoute?

You'll have to concatenate the details into the Message, but yeah, the Conditions is a ListTypeMap, meaning it's a map that's implemented as a list.

I wonder what happens if you do add multiple instances of the same Type, since the current code can definitely do that.

Right now you get multiple conditions since I thought you wouldn't want to concat but you'd want to have the details.

But like it seems with this API, my thought of how it should work is completely opposite. I'll have to come up with some concat thing to fix this since I guess I'm not following the spec in the current impl.

Here's an example of that: https://github.com/projectcontour/contour/pull/3456/files#diff-72b53646a46f97f3db8eabaf70aa271e571f0a3e51c73a1ba593abd85d53bfebR2761-R2776

Is this OK to merge as-is for 1.14? The current code can definitely potentially add multiple conditions of the same type, and i'm not sure what the Kube behavior is in that scenario - does it accept all of them? IMHO, given Gateway support is all alpha/WIP, I think we're probably OK as long as it doesn't cause Contour to crash or anything like that.

added #3534 to track this

youngnick

LGTM

sunjayBhatia · 2021-03-30T14:48:58Z

internal/dag/status_test.go

+					Rules: []gatewayapi_v1alpha1.HTTPRouteRule{{
+						Matches: []gatewayapi_v1alpha1.HTTPRouteMatch{{
+							Path: gatewayapi_v1alpha1.HTTPPathMatch{
+								Type:  "Exact", // <---- exact type not yet supported


We'll have to be careful around the order of merging this PR and #3478

sunjayBhatia · 2021-03-30T14:58:36Z

internal/status/httproutestatus_test.go

+	gatewayapi_v1alpha1 "sigs.k8s.io/gateway-api/apis/v1alpha1"
+)
+
+func TestHTTPRouteAddCondition(t *testing.T) {


Worth adding more testing around conditions not being updated if the existing conditions are more recent than the attempted update?

I think we definitely should, though personally I'd be OK merging this PR as-is and adding more testing in subsequent PRs, since this area is still WIP and this is all alpha anyway.

skriss

mostly LGTM, couple comments on true vs. false conditions

internal/dag/gatewayapi_processor.go

skriss · 2021-03-30T15:01:25Z

internal/status/httproutestatus.go

+}
+
+// AddCondition returns a metav1.Condition for a given ConditionType.
+func (routeUpdate *HTTPRouteUpdate) AddCondition(cond gatewayapi_v1alpha1.RouteConditionType, status metav1.ConditionStatus, reason RouteReasonType, message string) metav1.Condition {


I wonder what happens if you do add multiple instances of the same Type, since the current code can definitely do that.

- Update the status package to process GatewayAPI HTTPRoutes. - Update the dag.GatewayAPIProcessor to set status. Fixes projectcontour#3436 Signed-off-by: Steve Sloka <[email protected]>

Signed-off-by: Steve Sloka <[email protected]>

skriss

I think this is probably OK to merge now, had a couple comments/questions on open threads though.

skriss · 2021-03-30T20:43:51Z

internal/status/httproutestatus.go

+}
+
+// AddCondition returns a metav1.Condition for a given ConditionType.
+func (routeUpdate *HTTPRouteUpdate) AddCondition(cond gatewayapi_v1alpha1.RouteConditionType, status metav1.ConditionStatus, reason RouteReasonType, message string) metav1.Condition {


Is this OK to merge as-is for 1.14? The current code can definitely potentially add multiple conditions of the same type, and i'm not sure what the Kube behavior is in that scenario - does it accept all of them? IMHO, given Gateway support is all alpha/WIP, I think we're probably OK as long as it doesn't cause Contour to crash or anything like that.

skriss · 2021-03-30T20:45:48Z

internal/status/httproutestatus_test.go

+	gatewayapi_v1alpha1 "sigs.k8s.io/gateway-api/apis/v1alpha1"
+)
+
+func TestHTTPRouteAddCondition(t *testing.T) {


I think we definitely should, though personally I'd be OK merging this PR as-is and adding more testing in subsequent PRs, since this area is still WIP and this is all alpha anyway.

stevesloka · 2021-03-31T14:50:14Z

Is this OK to merge as-is for 1.14?

I think it's fine to merge and gives us a set of status information which is helpful. I agree that there's lots of room for improvement. =)

sunjayBhatia · 2021-03-31T14:57:12Z

Is this OK to merge as-is for 1.14?

I think it's fine to merge and gives us a set of status information which is helpful. I agree that there's lots of room for improvement. =)

Could you add issues and/or TODOs for the bits that have open questions/discussion threads so we don't lose them?

stevesloka added this to the 1.14.0 milestone Mar 8, 2021

stevesloka requested a review from a team as a code owner March 8, 2021 17:49

stevesloka requested review from danehans, skriss, youngnick and sunjayBhatia and removed request for a team March 8, 2021 17:49

stevesloka force-pushed the httpRouteStatus branch 2 times, most recently from 865c6a2 to 3c46077 Compare March 8, 2021 18:26

skriss reviewed Mar 10, 2021

View reviewed changes

internal/status/cache.go Show resolved Hide resolved

stevesloka force-pushed the httpRouteStatus branch from 3c46077 to dcda7ea Compare March 16, 2021 16:56

stevesloka force-pushed the httpRouteStatus branch from dcda7ea to 5e53531 Compare March 23, 2021 15:46

stevesloka requested a review from skriss March 23, 2021 15:47

stevesloka force-pushed the httpRouteStatus branch 2 times, most recently from b0de520 to dd113d6 Compare March 23, 2021 16:48

sunjayBhatia reviewed Mar 23, 2021

View reviewed changes

skriss reviewed Mar 24, 2021

View reviewed changes

youngnick suggested changes Mar 26, 2021

View reviewed changes

stevesloka force-pushed the httpRouteStatus branch 3 times, most recently from 625dc1d to 5c943c6 Compare March 29, 2021 19:54

stevesloka requested review from skriss, youngnick and sunjayBhatia March 29, 2021 20:07

skriss reviewed Mar 29, 2021

View reviewed changes

projectcontour deleted a comment from stevesloka Mar 29, 2021

youngnick reviewed Mar 29, 2021

View reviewed changes

youngnick approved these changes Mar 30, 2021

View reviewed changes

sunjayBhatia reviewed Mar 30, 2021

View reviewed changes

skriss reviewed Mar 30, 2021

View reviewed changes

stevesloka and others added 8 commits March 30, 2021 16:25

internal: Allow status on HTTPRoutes to be set

30b52bf

- Update the status package to process GatewayAPI HTTPRoutes. - Update the dag.GatewayAPIProcessor to set status. Fixes projectcontour#3436 Signed-off-by: Steve Sloka <[email protected]>

rebase + some review comments

b3cd128

Signed-off-by: Steve Sloka <[email protected]>

review comments

a206a53

Signed-off-by: Steve Sloka <[email protected]>

add check for observation

7f19de8

Signed-off-by: Steve Sloka <[email protected]>

review comments

f29c3c4

Signed-off-by: Steve Sloka <[email protected]>

more review comments

2075433

Signed-off-by: Steve Sloka <[email protected]>

shorten ReasonRouteDegraded-->ReasonDegraded

0a1d740

Signed-off-by: Steve Sloka <[email protected]>

move degrated to status false

5ad0395

Signed-off-by: Steve Sloka <[email protected]>

stevesloka force-pushed the httpRouteStatus branch from b0e3e2d to 5ad0395 Compare March 30, 2021 20:25

skriss approved these changes Mar 30, 2021

View reviewed changes

sunjayBhatia mentioned this pull request Mar 31, 2021

Gateway API: Ensure *Route status updates respect observed generation status update order #3533

Closed

skriss mentioned this pull request Mar 31, 2021

Gateway API: HTTPRoutes should have at most one Condition of a given type #3534

Closed

sunjayBhatia merged commit 5a49a27 into projectcontour:main Mar 31, 2021

	func (pu *HTTPRouteUpdate) ConditionFor(cond ConditionType, reason string, message string) metav1.Condition {
	func (pu *HTTPRouteUpdate) AddCondition(cond ConditionType, status bool, reason string, message string) metav1.Condition {

internal: Allow status on HTTPRoutes to be set #3456

internal: Allow status on HTTPRoutes to be set #3456

Conversation

stevesloka commented Mar 8, 2021

codecov bot commented Mar 8, 2021 • edited Loading

Codecov Report

skriss commented Mar 18, 2021

stevesloka commented Mar 23, 2021

skriss commented Mar 23, 2021

stevesloka commented Mar 23, 2021

sunjayBhatia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skriss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youngnick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevesloka commented Mar 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youngnick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youngnick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skriss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skriss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevesloka commented Mar 31, 2021

sunjayBhatia commented Mar 31, 2021

codecov bot commented Mar 8, 2021 •

edited

Loading