Skip to content

Commit

Permalink
internal/status: Update HTTPRoute status to only allow a single Type (#…
Browse files Browse the repository at this point in the history 
…3600)

internal/status: Update HTTPRoute status to only allow a single Type

Updates the HTTPRoute status processing to only allow a single "Type" to
be specified at any given time. If there are duplicate conditions for the same
type, then the messages are appended together.

Fixes #3534

Signed-off-by: Steve Sloka <slokas@vmware.com>
  • Loading branch information
@stevesloka
stevesloka committed Apr 22, 2021
1 parent 6884df9 commit 3e59de8
Show file tree
Hide file tree
Showing 4 changed files with 95 additions and 32 deletions.
2 changes: 1 addition & 1 deletion internal/dag/gatewayapi_processor.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -328,7 +328,7 @@ func (p *GatewayAPIProcessor) computeHTTPRoute(route *gatewayapi_v1alpha1.HTTPRo
// TODO: Refactor EnsureService to take an int32 so conversion to intstr is not needed.
service, err := p.dag.EnsureService(meta, intstr.FromInt(int(*forward.Port)), p.source)
if err != nil {
routeAccessor.AddCondition(status.ConditionResolvedRefs, metav1.ConditionFalse, status.ReasonDegraded, fmt.Sprintf("Service %q does not exist in namespace %q", meta.Name, meta.Namespace))
routeAccessor.AddCondition(status.ConditionResolvedRefs, metav1.ConditionFalse, status.ReasonDegraded, fmt.Sprintf("Service %q does not exist", meta.Name))
continue
}

Expand Down
92 changes: 74 additions & 18 deletions internal/dag/status_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2503,19 +2503,22 @@ func TestGatewayAPIDAGStatus(t *testing.T) {

var gotConditions []metav1.Condition
for _, u := range updates {
gotConditions = append(gotConditions, u.Conditions...)
for _, cond := range u.Conditions {
gotConditions = append(gotConditions, cond)
}
}

ops := []cmp.Option{
cmpopts.IgnoreFields(metav1.Condition{}, "LastTransitionTime"),
cmpopts.SortSlices(func(i, j int) bool {
return tc.want[i].Message < tc.want[j].Message
cmpopts.SortSlices(func(i, j metav1.Condition) bool {
return i.Message < j.Message
}),
}

if diff := cmp.Diff(tc.want, gotConditions, ops...); diff != "" {
t.Fatalf("expected: %v, got %v", tc.want, diff)
}

})
}

Expand Down Expand Up @@ -2666,15 +2669,15 @@ func TestGatewayAPIDAGStatus(t *testing.T) {
},
}},
want: []metav1.Condition{{
Type: string(status.ConditionNotImplemented),
Status: contour_api_v1.ConditionTrue,
Reason: string(status.ReasonPathMatchType),
Message: "HTTPRoute.Spec.Rules.PathMatch: Only Prefix match type and Exact match type are supported.",
}, {
Type: string(gatewayapi_v1alpha1.ConditionRouteAdmitted),
Status: contour_api_v1.ConditionFalse,
Reason: string(status.ReasonErrorsExist),
Message: "Errors found, check other Conditions for details.",
}, {
Type: string(status.ConditionNotImplemented),
Status: contour_api_v1.ConditionTrue,
Reason: string(status.ReasonPathMatchType),
Message: "HTTPRoute.Spec.Rules.PathMatch: Only Prefix match type and Exact match type are supported.",
}},
})

Expand Down Expand Up @@ -2713,15 +2716,15 @@ func TestGatewayAPIDAGStatus(t *testing.T) {
},
}},
want: []metav1.Condition{{
Type: string(status.ConditionNotImplemented),
Status: contour_api_v1.ConditionTrue,
Reason: string(status.ReasonHeaderMatchType),
Message: "HTTPRoute.Spec.Rules.HeaderMatch: Only Exact match type is supported.",
}, {
Type: string(gatewayapi_v1alpha1.ConditionRouteAdmitted),
Status: contour_api_v1.ConditionFalse,
Reason: string(status.ReasonErrorsExist),
Message: "Errors found, check other Conditions for details.",
}, {
Type: string(status.ConditionNotImplemented),
Status: contour_api_v1.ConditionTrue,
Reason: string(status.ReasonHeaderMatchType),
Message: "HTTPRoute.Spec.Rules.HeaderMatch: Only Exact match type is supported.",
}},
})

Expand Down Expand Up @@ -2818,6 +2821,59 @@ func TestGatewayAPIDAGStatus(t *testing.T) {
}},
})

run(t, "spec.rules.forwardTo.serviceName invalid on two matches", testcase{
objs: []interface{}{
gateway,
&gatewayapi_v1alpha1.HTTPRoute{
ObjectMeta: metav1.ObjectMeta{
Name: "basic",
Namespace: "default",
Labels: map[string]string{
"app": "contour",
},
},
Spec: gatewayapi_v1alpha1.HTTPRouteSpec{
Hostnames: []gatewayapi_v1alpha1.Hostname{
"test.projectcontour.io",
},
Rules: []gatewayapi_v1alpha1.HTTPRouteRule{{
Matches: []gatewayapi_v1alpha1.HTTPRouteMatch{{
Path: gatewayapi_v1alpha1.HTTPPathMatch{
Type: "Prefix",
Value: "/",
},
}},
ForwardTo: []gatewayapi_v1alpha1.HTTPRouteForwardTo{{
ServiceName: pointer.StringPtr("invalid-one"),
Port: gatewayPort(8080),
}},
}, {
Matches: []gatewayapi_v1alpha1.HTTPRouteMatch{{
Path: gatewayapi_v1alpha1.HTTPPathMatch{
Type: "Prefix",
Value: "/blog",
},
}},
ForwardTo: []gatewayapi_v1alpha1.HTTPRouteForwardTo{{
ServiceName: pointer.StringPtr("invalid-two"),
Port: gatewayPort(8080),
}},
}},
},
}},
want: []metav1.Condition{{
Type: string(status.ConditionResolvedRefs),
Status: contour_api_v1.ConditionFalse,
Reason: string(status.ReasonDegraded),
Message: "Service \"invalid-one\" does not exist, Service \"invalid-two\" does not exist",
}, {
Type: "Admitted",
Status: contour_api_v1.ConditionFalse,
Reason: "ErrorsExist",
Message: "Errors found, check other Conditions for details.",
}},
})

run(t, "spec.rules.forwardTo.servicePort not specified", testcase{
objs: []interface{}{
gateway,
Expand Down Expand Up @@ -3051,15 +3107,15 @@ func TestGatewayAPIDAGStatus(t *testing.T) {
},
}},
want: []metav1.Condition{{
Type: string(status.ConditionNotImplemented),
Status: contour_api_v1.ConditionTrue,
Reason: string(status.ReasonHTTPRouteFilterType),
Message: "HTTPRoute.Spec.Rules.Filters: Only RequestHeaderModifier type is supported.",
}, {
Type: string(gatewayapi_v1alpha1.ConditionRouteAdmitted),
Status: contour_api_v1.ConditionFalse,
Reason: string(status.ReasonErrorsExist),
Message: "Errors found, check other Conditions for details.",
}, {
Type: string(status.ConditionNotImplemented),
Status: contour_api_v1.ConditionTrue,
Reason: string(status.ReasonHTTPRouteFilterType),
Message: "HTTPRoute.Spec.Rules.Filters: Only RequestHeaderModifier type is supported.",
}},
})

Expand Down
26 changes: 19 additions & 7 deletions internal/status/httproutestatus.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,15 +38,20 @@ const ReasonErrorsExist RouteReasonType = "ErrorsExist"

type HTTPRouteUpdate struct {
FullName types.NamespacedName
Conditions []metav1.Condition
ExistingConditions []metav1.Condition
Conditions map[gatewayapi_v1alpha1.RouteConditionType]metav1.Condition
ExistingConditions map[gatewayapi_v1alpha1.RouteConditionType]metav1.Condition
GatewayRef types.NamespacedName
Generation int64
TransitionTime metav1.Time
}

// 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 {

if c, ok := routeUpdate.Conditions[cond]; ok {
message = fmt.Sprintf("%s, %s", c.Message, message)
}

newDc := metav1.Condition{
Reason: string(reason),
Status: status,
Expand All @@ -55,7 +60,7 @@ func (routeUpdate *HTTPRouteUpdate) AddCondition(cond gatewayapi_v1alpha1.RouteC
LastTransitionTime: metav1.NewTime(time.Now()),
ObservedGeneration: routeUpdate.Generation,
}
routeUpdate.Conditions = append(routeUpdate.Conditions, newDc)
routeUpdate.Conditions[cond] = newDc
return newDc
}

Expand All @@ -66,7 +71,7 @@ func (routeUpdate *HTTPRouteUpdate) AddCondition(cond gatewayapi_v1alpha1.RouteC
func (c *Cache) HTTPRouteAccessor(route *gatewayapi_v1alpha1.HTTPRoute) (*HTTPRouteUpdate, func()) {
pu := &HTTPRouteUpdate{
FullName: k8s.NamespacedNameOf(route),
Conditions: []metav1.Condition{},
Conditions: make(map[gatewayapi_v1alpha1.RouteConditionType]metav1.Condition),
ExistingConditions: c.getGatewayConditions(route.Status.Gateways),
GatewayRef: c.gatewayRef,
Generation: route.Generation,
Expand Down Expand Up @@ -152,12 +157,19 @@ func (routeUpdate *HTTPRouteUpdate) Mutate(obj interface{}) interface{} {
return httpRoute
}

func (c *Cache) getGatewayConditions(gatewayStatus []gatewayapi_v1alpha1.RouteGatewayStatus) []metav1.Condition {
func (c *Cache) getGatewayConditions(gatewayStatus []gatewayapi_v1alpha1.RouteGatewayStatus) map[gatewayapi_v1alpha1.RouteConditionType]metav1.Condition {
for _, gs := range gatewayStatus {
if c.gatewayRef.Name == gs.GatewayRef.Name &&
c.gatewayRef.Namespace == gs.GatewayRef.Namespace {
return gs.Conditions

conditions := make(map[gatewayapi_v1alpha1.RouteConditionType]metav1.Condition)
for _, gsCondition := range gs.Conditions {
if val, ok := conditions[gatewayapi_v1alpha1.RouteConditionType(gsCondition.Type)]; !ok {
conditions[gatewayapi_v1alpha1.RouteConditionType(gsCondition.Type)] = val
}
}
return conditions
}
}
return []metav1.Condition{}
return map[gatewayapi_v1alpha1.RouteConditionType]metav1.Condition{}
}
7 changes: 1 addition & 6 deletions internal/status/httproutestatus_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,12 +38,7 @@ func TestHTTPRouteAddCondition(t *testing.T) {
httpRouteUpdate := HTTPRouteUpdate{
FullName: k8s.NamespacedNameFrom("test/test"),
Generation: testGeneration,
Conditions: []metav1.Condition{{
Type: string(gatewayapi_v1alpha1.ConditionRouteAdmitted),
Status: projectcontour.ConditionTrue,
Reason: "Valid",
Message: "Valid HTTPRoute",
}},
Conditions: make(map[gatewayapi_v1alpha1.RouteConditionType]metav1.Condition),
}

got := httpRouteUpdate.AddCondition(gatewayapi_v1alpha1.ConditionRouteAdmitted, metav1.ConditionTrue, "Valid", "Valid HTTPRoute")
Expand Down

0 comments on commit 3e59de8

Please sign in to comment.