diff --git a/CHANGELOG.md b/CHANGELOG.md index 4510b22a40..15b6b9a7ed 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -182,6 +182,13 @@ Adding a new version? You'll need three changes: - Admin and proxy listens in the deploy manifests now use the same parameters as the default upstream kong.conf. [#3165](https://github.com/Kong/kubernetes-ingress-controller/pull/3165) +- Fix the behavior of filtering hostnames in `HTTPRoute` when listeners + of parent gateways specified hostname. + If an `HTTPRoute` does not specify hostnames, and one of its parent listeners + has not specified hostname, the `HTTPRoute` matches any hostname. + If an `HTTPRoute` specifies hostnames, and no intersecting hostnames + could be found in its parent listners, it is not accepted. + [#3180](https://github.com/Kong/kubernetes-ingress-controller/pull/3180) ## [2.7.0] diff --git a/internal/controllers/gateway/gateway_utils.go b/internal/controllers/gateway/gateway_utils.go index 715901fa5c..2c0e1ddff0 100644 --- a/internal/controllers/gateway/gateway_utils.go +++ b/internal/controllers/gateway/gateway_utils.go @@ -153,6 +153,17 @@ func listSecretNamesReferredByGateway(gateway *gatewayv1beta1.Gateway) map[types return nsNames } +// extractListenerSpecFromGateway returns the spec of the listener with the given name. +// returns nil if the listener with given name is not found. +func extractListenerSpecFromGateway(gateway *gatewayv1beta1.Gateway, listenerName gatewayv1beta1.SectionName) *gatewayv1beta1.Listener { + for i, l := range gateway.Spec.Listeners { + if l.Name == listenerName { + return &gateway.Spec.Listeners[i] + } + } + return nil +} + // ListenerTracker holds Gateway Listeners and their statuses, and provides methods to update statuses upon // reconciliation. type ListenerTracker struct { diff --git a/internal/controllers/gateway/httproute_controller.go b/internal/controllers/gateway/httproute_controller.go index 222adc1d47..c6386a915a 100644 --- a/internal/controllers/gateway/httproute_controller.go +++ b/internal/controllers/gateway/httproute_controller.go @@ -318,8 +318,23 @@ func (r *HTTPRouteReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( // perform operations on the kong store only if the route is in accepted status if isRouteAccepted(gateways) { - // remove all the hostnames that don't match with at least one Listener's Hostname - filteredHTTPRoute := filterHostnames(gateways, httproute.DeepCopy()) + // if there is no matched hosts in listeners for the httproute, the httproute should not be accepted + // and have an "Accepted" condition with status false. + // https://gateway-api.sigs.k8s.io/references/spec/#gateway.networking.k8s.io/v1beta1.HTTPRoute + filteredHTTPRoute, err := filterHostnames(gateways, httproute.DeepCopy()) + if err != nil { + debug(log, httproute, "not accepting a route: no matching hostnames found after filtering") + _, err := r.ensureParentsAcceptedCondition( + ctx, + httproute, gateways, + metav1.ConditionFalse, + gatewayv1beta1.RouteReasonNoMatchingListenerHostname, + err.Error(), + ) + if err != nil { + return ctrl.Result{}, err + } + } // if the gateways are ready, and the HTTPRoute is destined for them, ensure that // the object is pushed to the dataplane. @@ -585,3 +600,103 @@ func (r *HTTPRouteReconciler) getHTTPRouteRuleReason(ctx context.Context, httpRo } return gatewayv1beta1.RouteReasonResolvedRefs, nil } + +// ensureParentsAcceptedCondition sets the "Accepted" condition of HTTPRoute status. +// returns a non-nil error if updating status failed, +// and returns true in the first return value if status changed. +func (r *HTTPRouteReconciler) ensureParentsAcceptedCondition( + ctx context.Context, + httproute *gatewayv1beta1.HTTPRoute, + gateways []supportedGatewayWithCondition, + conditionStatus metav1.ConditionStatus, + conditionReason gatewayv1beta1.RouteConditionReason, + conditionMessage string, +) (bool, error) { + // map the existing parentStatues to avoid duplications + parentStatuses := make(map[string]*gatewayv1beta1.RouteParentStatus) + for _, existingParent := range httproute.Status.Parents { + namespace := httproute.Namespace + if existingParent.ParentRef.Namespace != nil { + namespace = string(*existingParent.ParentRef.Namespace) + } + existingParentCopy := existingParent + var sectionName string + if existingParent.ParentRef.SectionName != nil { + sectionName = string(*existingParent.ParentRef.SectionName) + } + parentStatuses[fmt.Sprintf("%s/%s/%s", namespace, existingParent.ParentRef.Name, sectionName)] = &existingParentCopy + } + + statusChanged := false + for _, g := range gateways { + gateway := g.gateway + parentRefKey := fmt.Sprintf("%s/%s/%s", gateway.Namespace, gateway.Name, g.listenerName) + parentStatus, ok := parentStatuses[parentRefKey] + if ok { + // update existing parent in status. + changed := updateAcceptedConditionInRouteParentStatus(parentStatus, conditionStatus, conditionReason, conditionMessage, httproute.Generation) + statusChanged = statusChanged || changed + } else { + // add a new parent if the parent is not found in status. + newParentStatus := &gatewayv1beta1.RouteParentStatus{ + ParentRef: gatewayv1beta1.ParentReference{ + Namespace: (*gatewayv1beta1.Namespace)(pointer.String(gateway.Namespace)), + Name: gatewayv1beta1.ObjectName(gateway.Name), + SectionName: (*gatewayv1beta1.SectionName)(pointer.String(g.listenerName)), + // TODO: set port after gateway port matching implemented: https://github.com/Kong/kubernetes-ingress-controller/issues/3016 + }, + Conditions: []metav1.Condition{ + { + Type: string(gatewayv1beta1.RouteConditionAccepted), + Status: conditionStatus, + ObservedGeneration: httproute.Generation, + LastTransitionTime: metav1.Now(), + Reason: string(conditionReason), + Message: conditionMessage, + }, + }, + } + httproute.Status.Parents = append(httproute.Status.Parents, *newParentStatus) + parentStatuses[parentRefKey] = newParentStatus + statusChanged = true + } + } + + // update status if needed. + if statusChanged { + if err := r.Status().Update(ctx, httproute); err != nil { + return false, err + } + return true, nil + } + // no need to update if no status is changed. + return false, nil +} + +// updateAcceptedConditionInRouteParentStatus updates conditions with type "Accepted" in parentStatus. +// returns true if the parentStatus was modified. +func updateAcceptedConditionInRouteParentStatus( + parentStatus *gatewayv1beta1.RouteParentStatus, + conditionStatus metav1.ConditionStatus, + conditionReason gatewayv1beta1.RouteConditionReason, + conditionMessage string, + generation int64, +) bool { + changed := false + for i, condition := range parentStatus.Conditions { + if condition.Type == string(gatewayv1beta1.RouteConditionAccepted) { + if condition.Status != conditionStatus || condition.Reason != string(conditionReason) || condition.Message != conditionMessage { + parentStatus.Conditions[i] = metav1.Condition{ + Type: string(gatewayv1beta1.RouteConditionAccepted), + Status: conditionStatus, + ObservedGeneration: generation, + LastTransitionTime: metav1.Now(), + Reason: string(conditionReason), + Message: conditionMessage, + } + changed = true + } + } + } + return changed +} diff --git a/internal/controllers/gateway/route_utils.go b/internal/controllers/gateway/route_utils.go index f5cb1237f3..00ba555385 100644 --- a/internal/controllers/gateway/route_utils.go +++ b/internal/controllers/gateway/route_utils.go @@ -27,6 +27,8 @@ const ( unsupportedGW = "no supported Gateway found for route" ) +var ErrNoMatchingListenerHostname = fmt.Errorf("no matching hostnames in listener") + // supportedGatewayWithCondition is a struct that wraps a gateway and some further info // such as the condition Status condition Accepted of the gateway and the listenerName. type supportedGatewayWithCondition struct { @@ -458,22 +460,28 @@ func listenerHostnameIntersectWithRouteHostnames[H types.HostnameT, L types.List return false } +// isListenerHostnameEffective returns true if the listener can specify an effective +// hostname to match hostnames in requests. +// It basically checks if the listener is using any these protocols: HTTP, HTTPS, or TLS. +func isListenerHostnameEffective(listener gatewayv1beta1.Listener) bool { + return listener.Protocol == gatewayv1beta1.HTTPProtocolType || + listener.Protocol == gatewayv1beta1.HTTPSProtocolType || + listener.Protocol == gatewayv1beta1.TLSProtocolType +} + // filterHostnames accepts a HTTPRoute and returns a version of the same object with only a subset of the // hostnames, the ones matching with the listeners' hostname. -func filterHostnames(gateways []supportedGatewayWithCondition, httpRoute *gatewayv1beta1.HTTPRoute) *gatewayv1beta1.HTTPRoute { +// it returns an error if the intersection of hostname match in httproute and listeners is empty. +func filterHostnames(gateways []supportedGatewayWithCondition, httpRoute *gatewayv1beta1.HTTPRoute) (*gatewayv1beta1.HTTPRoute, error) { filteredHostnames := make([]gatewayv1beta1.Hostname, 0) - - // if no hostnames are specified in the route spec, get all the hostnames from - // the gateway + // if no hostnames are specified in the route spec, we use the UNION of all hostnames in supported gateways. + // if any of supported listener has not specified hostname, the hostnames of HTTPRoute remains empty + // to match **ANY** hostname. if len(httpRoute.Spec.Hostnames) == 0 { - for _, gateway := range gateways { - for _, listener := range gateway.gateway.Spec.Listeners { - if listenerName := gateway.listenerName; listenerName == "" || listenerName == string(listener.Name) { - if listener.Hostname != nil { - filteredHostnames = append(filteredHostnames, (*listener.Hostname)) - } - } - } + var matchAnyHost bool + filteredHostnames, matchAnyHost = getUnionOfGatewayHostnames(gateways) + if matchAnyHost { + return httpRoute, nil } } else { for _, hostname := range httpRoute.Spec.Hostnames { @@ -481,10 +489,47 @@ func filterHostnames(gateways []supportedGatewayWithCondition, httpRoute *gatewa filteredHostnames = append(filteredHostnames, hostnameMatching) } } + if len(filteredHostnames) == 0 { + return httpRoute, ErrNoMatchingListenerHostname + } } httpRoute.Spec.Hostnames = filteredHostnames - return httpRoute + return httpRoute, nil +} + +// getUnionOfGatewayHostnames returns UNION of hostnames specified in supported gateways. +// the second return value is true if any hostname could be matched in at least one listener +// in supported gateways and listeners, so the `HTTPRoute` could match any hostname. +func getUnionOfGatewayHostnames(gateways []supportedGatewayWithCondition) ([]gatewayv1beta1.Hostname, bool) { + hostnames := make([]gatewayv1beta1.Hostname, 0) + for _, gateway := range gateways { + if gateway.listenerName != "" { + if listener := extractListenerSpecFromGateway( + gateway.gateway, + gatewayv1beta1.SectionName(gateway.listenerName), + ); listener != nil { + // return true if the listener has not specified hostname to match any hostname. + if listener.Hostname == nil { + return nil, true + } + hostnames = append(hostnames, *listener.Hostname) + } + } else { + for _, listener := range gateway.gateway.Spec.Listeners { + // here we consider ALL listeners that are able to configure a hostname if no listener attached. + // may be changed if there is a conclusion on the upstream discussion about it: + // https://github.com/kubernetes-sigs/gateway-api/discussions/1563 + if isListenerHostnameEffective(listener) { + if listener.Hostname == nil { + return nil, true + } + hostnames = append(hostnames, *listener.Hostname) + } + } + } + } + return hostnames, false } // getMinimumHostnameIntersection returns the minimum intersecting hostname, in the sense that: @@ -548,6 +593,5 @@ func isHTTPReferenceGranted(grantSpec gatewayv1alpha2.ReferenceGrantSpec, backen } } } - return false } diff --git a/internal/controllers/gateway/route_utils_test.go b/internal/controllers/gateway/route_utils_test.go index aa5fa265ac..7262f4abde 100644 --- a/internal/controllers/gateway/route_utils_test.go +++ b/internal/controllers/gateway/route_utils_test.go @@ -48,6 +48,9 @@ func TestFilterHostnames(t *testing.T) { Name: "listener-3", Hostname: util.StringToGatewayAPIHostnamePtr("*.anotherwildcard.io"), }, + { + Name: "listener-4", + }, }, }, } @@ -57,6 +60,8 @@ func TestFilterHostnames(t *testing.T) { gateways []supportedGatewayWithCondition httpRoute *gatewayv1beta1.HTTPRoute expectedHTTPRoute *gatewayv1beta1.HTTPRoute + hasError bool + errString string }{ { name: "listener 1 - specific", @@ -160,12 +165,31 @@ func TestFilterHostnames(t *testing.T) { }, }, { - name: "no match", + name: "no listner specified - no hostname", gateways: []supportedGatewayWithCondition{ { gateway: commonGateway, }, }, + httpRoute: &gatewayv1beta1.HTTPRoute{ + Spec: gatewayv1beta1.HTTPRouteSpec{ + Hostnames: []gatewayv1beta1.Hostname{}, + }, + }, + expectedHTTPRoute: &gatewayv1beta1.HTTPRoute{ + Spec: gatewayv1beta1.HTTPRouteSpec{ + Hostnames: []gatewayv1beta1.Hostname{}, + }, + }, + }, + { + name: "listener 1 - no match", + gateways: []supportedGatewayWithCondition{ + { + gateway: commonGateway, + listenerName: "listner-1", + }, + }, httpRoute: &gatewayv1beta1.HTTPRoute{ Spec: gatewayv1beta1.HTTPRouteSpec{ Hostnames: []gatewayv1beta1.Hostname{ @@ -179,12 +203,20 @@ func TestFilterHostnames(t *testing.T) { Hostnames: []gatewayv1beta1.Hostname{}, }, }, + hasError: true, + errString: "no matching hostnames in listener", }, } for _, tc := range testCases { - filteredHTTPRoute := filterHostnames(tc.gateways, tc.httpRoute) - assert.Equal(t, tc.expectedHTTPRoute.Spec, filteredHTTPRoute.Spec, tc.name) + filteredHTTPRoute, err := filterHostnames(tc.gateways, tc.httpRoute) + if tc.hasError { + assert.Error(t, err) + assert.Contains(t, err.Error(), tc.errString) + } else { + assert.NoError(t, err) + assert.Equal(t, tc.expectedHTTPRoute.Spec, filteredHTTPRoute.Spec, tc.name) + } } } diff --git a/test/integration/httproute_test.go b/test/integration/httproute_test.go index d5237889dd..32447c6f42 100644 --- a/test/integration/httproute_test.go +++ b/test/integration/httproute_test.go @@ -23,6 +23,7 @@ import ( "github.com/kong/kubernetes-ingress-controller/v2/internal/annotations" gatewaypkg "github.com/kong/kubernetes-ingress-controller/v2/internal/controllers/gateway" "github.com/kong/kubernetes-ingress-controller/v2/internal/util" + "github.com/kong/kubernetes-ingress-controller/v2/internal/util/builder" "github.com/kong/kubernetes-ingress-controller/v2/internal/versions" kongv1 "github.com/kong/kubernetes-ingress-controller/v2/pkg/apis/configuration/v1" "github.com/kong/kubernetes-ingress-controller/v2/pkg/clientset" @@ -494,3 +495,149 @@ func TestHTTPRouteMultipleServices(t *testing.T) { t.Log("verifying that misconfigured service rules are _not_ routed") eventuallyGETPath(t, "test-http-route-multiple-services-broken", http.StatusNotFound, "", emptyHeaderSet) } + +func TestHTTPRouteFilterHosts(t *testing.T) { + ns, cleaner := setup(t) + defer func() { + if t.Failed() { + output, err := cleaner.DumpDiagnostics(ctx, t.Name()) + t.Logf("%s failed, dumped diagnostics to %s", t.Name(), output) + assert.NoError(t, err) + } + assert.NoError(t, cleaner.Cleanup(ctx)) + }() + + listenerHostname := gatewayv1beta1.Hostname("test.specific.io") + + t.Log("getting a gateway client") + gatewayClient, err := gatewayclient.NewForConfig(env.Cluster().Config()) + require.NoError(t, err) + + t.Log("deploying a new gatewayClass") + gatewayClassName := uuid.NewString() + gwc, err := DeployGatewayClass(ctx, gatewayClient, gatewayClassName) + require.NoError(t, err) + cleaner.Add(gwc) + + t.Log("deploying a new gateway with specified hostname") + gatewayName := uuid.NewString() + gateway, err := DeployGateway(ctx, gatewayClient, ns.Name, gatewayClassName, func(gw *gatewayv1beta1.Gateway) { + gw.Name = gatewayName + for i := range gw.Spec.Listeners { + gw.Spec.Listeners[i].Hostname = &listenerHostname + } + }) + require.NoError(t, err) + cleaner.Add(gateway) + + t.Log("deploying a minimal HTTP container deployment to test Ingress routes") + container := generators.NewContainer("httpbin", test.HTTPBinImage, 80) + deployment := generators.NewDeploymentForContainer(container) + deployment, err = env.Cluster().Client().AppsV1().Deployments(ns.Name).Create(ctx, deployment, metav1.CreateOptions{}) + require.NoError(t, err) + + t.Logf("exposing deployment %s via service", deployment.Name) + service := generators.NewServiceForDeployment(deployment, corev1.ServiceTypeLoadBalancer) + _, err = env.Cluster().Client().CoreV1().Services(ns.Name).Create(ctx, service, metav1.CreateOptions{}) + require.NoError(t, err) + + t.Logf("creating an httproute with a same hostname and another unmatched hostname") + httpRoute := &gatewayv1beta1.HTTPRoute{ + ObjectMeta: metav1.ObjectMeta{ + Name: uuid.NewString(), + Annotations: map[string]string{ + annotations.AnnotationPrefix + annotations.StripPathKey: "true", + annotations.AnnotationPrefix + annotations.PluginsKey: "correlation", + }, + }, + Spec: gatewayv1beta1.HTTPRouteSpec{ + CommonRouteSpec: gatewayv1beta1.CommonRouteSpec{ + ParentRefs: []gatewayv1beta1.ParentReference{{ + Name: gatewayv1beta1.ObjectName(gateway.Name), + }}, + }, + Hostnames: []gatewayv1beta1.Hostname{ + gatewayv1beta1.Hostname("test.specific.io"), + gatewayv1beta1.Hostname("another.specific.io"), + }, + Rules: []gatewayv1beta1.HTTPRouteRule{{ + Matches: []gatewayv1beta1.HTTPRouteMatch{ + builder.NewHTTPRouteMatch().WithPathPrefix("/test-http-route-filter-hosts").Build(), + }, + BackendRefs: []gatewayv1beta1.HTTPBackendRef{ + builder.NewHTTPBackendRef(service.Name).WithPort(80).Build(), + }, + }}, + }, + } + _, err = gatewayClient.GatewayV1beta1().HTTPRoutes(ns.Name).Create(ctx, httpRoute, metav1.CreateOptions{}) + require.NoError(t, err) + cleaner.Add(httpRoute) + + // testGetByHost tries to get the test path with specified host in request, + // and returns true if 200 returned. + testGetByHost := func(t *testing.T, host string) bool { + req, err := http.NewRequest("GET", proxyURL.String()+"/test-http-route-filter-hosts", nil) + require.NoError(t, err) + req.Host = host + resp, err := httpc.Do(req) + if err != nil { + return false + } + defer resp.Body.Close() + return resp.StatusCode == http.StatusOK + } + + t.Logf("test host matched hostname in listeners") + require.Eventually(t, func() bool { + return testGetByHost(t, "test.specific.io") + }, ingressWait, waitTick) + t.Logf("test host matched in httproute, but not in listeners") + require.False(t, testGetByHost(t, "another.specific.io")) + + t.Logf("update hostnames in httproute to wildcard") + httpRoute, err = gatewayClient.GatewayV1beta1().HTTPRoutes(ns.Name).Get(ctx, httpRoute.Name, metav1.GetOptions{}) + require.NoError(t, err) + httpRoute.Spec.Hostnames = []gatewayv1beta1.Hostname{ + gatewayv1beta1.Hostname("*.specific.io"), + } + _, err = gatewayClient.GatewayV1beta1().HTTPRoutes(ns.Name).Update(ctx, httpRoute, metav1.UpdateOptions{}) + require.NoError(t, err) + t.Logf("test host matched hostname in listeners") + require.Eventually(t, func() bool { + return testGetByHost(t, "test.specific.io") + }, ingressWait, waitTick) + t.Logf("test host matched in httproute, but not in listeners") + require.False(t, testGetByHost(t, "another2.specific.io")) + + t.Logf("update hostname in httproute to an unmatched host") + httpRoute, err = gatewayClient.GatewayV1beta1().HTTPRoutes(ns.Name).Get(ctx, httpRoute.Name, metav1.GetOptions{}) + require.NoError(t, err) + httpRoute.Spec.Hostnames = []gatewayv1beta1.Hostname{ + gatewayv1beta1.Hostname("another.specific.io"), + } + _, err = gatewayClient.GatewayV1beta1().HTTPRoutes(ns.Name).Update(ctx, httpRoute, metav1.UpdateOptions{}) + require.NoError(t, err) + t.Logf("status of httproute should contain an 'Accepted' condition with 'False' status") + require.Eventuallyf(t, func() bool { + currentHTTPRoute, err := gatewayClient.GatewayV1beta1().HTTPRoutes(ns.Name).Get(ctx, httpRoute.Name, metav1.GetOptions{}) + require.NoError(t, err) + for _, parent := range currentHTTPRoute.Status.Parents { + for _, condition := range parent.Conditions { + if condition.Type == string(gatewayv1beta1.RouteReasonAccepted) && condition.Status == metav1.ConditionFalse { + return true + } + } + } + return false + }, ingressWait, waitTick, + func() string { + currentHTTPRoute, err := gatewayClient.GatewayV1beta1().HTTPRoutes(ns.Name).Get(ctx, httpRoute.Name, metav1.GetOptions{}) + if err != nil { + return err.Error() + } + return fmt.Sprintf("current status of HTTPRoute %s/%s:%v", httpRoute.Namespace, httpRoute.Name, currentHTTPRoute.Status) + }()) + t.Logf("test host matched in httproute, but not in listeners") + require.False(t, testGetByHost(t, "another.specific.io")) +}