Backport of NET-4482: set route condition appropriately when parent r…

…ef includes non-existent section into release/1.2.x (#2555)

* backport of commit 05de2cf

* backport of commit 5f94e4b

* backport of commit ac126a8

* backport of commit 10e32f2

* backport of commit 2c5847e

---------

Co-authored-by: Nathan Coleman <nathan.coleman@hashicorp.com>

.changelog/2420.txt

@@ -0,0 +1,3 @@
+```release-note:bug
+api-gateway: set route condition appropriately when parent ref includes non-existent section name
+```

control-plane/api-gateway/binding/result.go

@@ -34,6 +34,7 @@ var (
 	errRouteNoMatchingListenerHostname      = errors.New("listener cannot bind route with a non-aligned hostname")
 	errRouteInvalidKind                     = errors.New("invalid backend kind")
 	errRouteBackendNotFound                 = errors.New("backend not found")
+	errRouteNoMatchingParent                = errors.New("no matching parent")
 )
 
 // routeValidationResult holds the result of validating a route globally, in other
@@ -128,13 +129,17 @@ type bindResult struct {
 type bindResults []bindResult
 
 // Error constructs a human readable error for bindResults, containing any errors that a route
-// had in binding to a gateway, note that this is only used if a route failed to bind to every
+// had in binding to a gateway. Note that this is only used if a route failed to bind to every
 // listener it attempted to bind to.
 func (b bindResults) Error() string {
 	messages := []string{}
 	for _, result := range b {
 		if result.err != nil {
-			messages = append(messages, fmt.Sprintf("%s: %s", result.section, result.err.Error()))
+			message := result.err.Error()
+			if result.section != "" {
+				message = fmt.Sprintf("%s: %s", result.section, result.err.Error())
+			}
+			messages = append(messages, message)
 		}
 	}
 
@@ -171,13 +176,16 @@ func (b bindResults) Condition() metav1.Condition {
 	// if we only have a single binding error, we can get more specific
 	if len(b) == 1 {
 		for _, result := range b {
-			// if we have a hostname mismatch error, then use the more specific reason
-			if result.err == errRouteNoMatchingListenerHostname {
+			switch result.err {
+			case errRouteNoMatchingListenerHostname:
+				// if we have a hostname mismatch error, then use the more specific reason
 				reason = "NoMatchingListenerHostname"
-			}
-			// or if we have a ref not permitted, then use that
-			if result.err == errRefNotPermitted {
+			case errRefNotPermitted:
+				// or if we have a ref not permitted, then use that
 				reason = "RefNotPermitted"
+			case errRouteNoMatchingParent:
+				// or if the route declares a parent that we can't find
+				reason = "NoMatchingParent"
 			}
 		}
 	}

control-plane/api-gateway/binding/result_test.go

@@ -0,0 +1,67 @@
+package binding
+
+import (
+	"errors"
+	"fmt"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+func TestBindResults_Condition(t *testing.T) {
+	testCases := []struct {
+		Name     string
+		Results  bindResults
+		Expected metav1.Condition
+	}{
+		{
+			Name:     "route successfully bound",
+			Results:  bindResults{{section: "", err: nil}},
+			Expected: metav1.Condition{Type: "Accepted", Status: "True", Reason: "Accepted", Message: "route accepted"},
+		},
+		{
+			Name: "multiple bind results",
+			Results: bindResults{
+				{section: "abc", err: errRouteNoMatchingListenerHostname},
+				{section: "def", err: errRouteNoMatchingParent},
+			},
+			Expected: metav1.Condition{Type: "Accepted", Status: "False", Reason: "NotAllowedByListeners", Message: "abc: listener cannot bind route with a non-aligned hostname; def: no matching parent"},
+		},
+		{
+			Name:     "no matching listener hostname error",
+			Results:  bindResults{{section: "abc", err: errRouteNoMatchingListenerHostname}},
+			Expected: metav1.Condition{Type: "Accepted", Status: "False", Reason: "NoMatchingListenerHostname", Message: "abc: listener cannot bind route with a non-aligned hostname"},
+		},
+		{
+			Name:     "ref not permitted error",
+			Results:  bindResults{{section: "abc", err: errRefNotPermitted}},
+			Expected: metav1.Condition{Type: "Accepted", Status: "False", Reason: "RefNotPermitted", Message: "abc: reference not permitted due to lack of ReferenceGrant"},
+		},
+		{
+			Name:     "no matching parent error",
+			Results:  bindResults{{section: "hello1", err: errRouteNoMatchingParent}},
+			Expected: metav1.Condition{Type: "Accepted", Status: "False", Reason: "NoMatchingParent", Message: "hello1: no matching parent"},
+		},
+		{
+			Name:     "bind result without section name",
+			Results:  bindResults{{section: "", err: errRouteNoMatchingParent}},
+			Expected: metav1.Condition{Type: "Accepted", Status: "False", Reason: "NoMatchingParent", Message: "no matching parent"},
+		},
+		{
+			Name:     "unhandled error type",
+			Results:  bindResults{{section: "abc", err: errors.New("you don't know me")}},
+			Expected: metav1.Condition{Type: "Accepted", Status: "False", Reason: "NotAllowedByListeners", Message: "abc: you don't know me"},
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(fmt.Sprintf("%s_%s", t.Name(), tc.Name), func(t *testing.T) {
+			actual := tc.Results.Condition()
+			assert.Equalf(t, tc.Expected.Type, actual.Type, "expected condition with type %q but got %q", tc.Expected.Type, actual.Type)
+			assert.Equalf(t, tc.Expected.Status, actual.Status, "expected condition with status %q but got %q", tc.Expected.Status, actual.Status)
+			assert.Equalf(t, tc.Expected.Reason, actual.Reason, "expected condition with reason %q but got %q", tc.Expected.Reason, actual.Reason)
+			assert.Equalf(t, tc.Expected.Message, actual.Message, "expected condition with message %q but got %q", tc.Expected.Message, actual.Message)
+		})
+	}
+}

control-plane/api-gateway/binding/route_binding.go

@@ -104,7 +104,22 @@ func (r *Binder) bindRoute(route client.Object, boundCount map[gwv1beta1.Section
 	for _, ref := range filteredParents {
 		var result bindResults
 
-		for _, listener := range listenersFor(&r.config.Gateway, ref.SectionName) {
+		listeners := listenersFor(&r.config.Gateway, ref.SectionName)
+
+		// If there are no matching listeners, then we failed to find the parent
+		if len(listeners) == 0 {
+			var sectionName gwv1beta1.SectionName
+			if ref.SectionName != nil {
+				sectionName = *ref.SectionName
+			}
+
+			result = append(result, bindResult{
+				section: sectionName,
+				err:     errRouteNoMatchingParent,
+			})
+		}
+
+		for _, listener := range listeners {
 			if !routeKindIsAllowedForListener(supportedKindsForProtocol[listener.Protocol], groupKind) {
 				result = append(result, bindResult{
 					section: listener.Name,
@@ -179,9 +194,9 @@ func filterParentRefs(gateway types.NamespacedName, namespace string, refs []gwv
 	return references
 }
 
-// listenersFor returns the listeners corresponding the given section name. If the section
-// name is actually specified, the returned set should just have one listener, if it is
-// unspecified, the all gatweway listeners should be returned.
+// listenersFor returns the listeners corresponding to the given section name. If the section
+// name is actually specified, the returned set will only contain the named listener. If it is
+// unspecified, then all gateway listeners will be returned.
 func listenersFor(gateway *gwv1beta1.Gateway, name *gwv1beta1.SectionName) []gwv1beta1.Listener {
 	listeners := []gwv1beta1.Listener{}
 	for _, listener := range gateway.Spec.Listeners {