From 5e5f99470ec26ab49fddcfaba3c50edd3f1f54ad Mon Sep 17 00:00:00 2001 From: Shane Utt Date: Thu, 17 Mar 2022 17:14:06 -0400 Subject: [PATCH 1/6] fix: remove duplicate addrs on unmanaged gateways --- internal/controllers/gateway/gateway_controller.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/internal/controllers/gateway/gateway_controller.go b/internal/controllers/gateway/gateway_controller.go index 82286c9f17..db145e4f68 100644 --- a/internal/controllers/gateway/gateway_controller.go +++ b/internal/controllers/gateway/gateway_controller.go @@ -437,11 +437,12 @@ func (r *GatewayReconciler) determineL4ListenersFromService( addresses := make([]gatewayv1alpha2.GatewayAddress, 0, len(svc.Spec.ClusterIPs)) listeners := make([]gatewayv1alpha2.Listener, 0, len(svc.Spec.Ports)) for _, clusterIP := range svc.Spec.ClusterIPs { + addresses = append(addresses, gatewayv1alpha2.GatewayAddress{ + Type: &gatewayIPAddrType, + Value: clusterIP, + }) + for _, port := range svc.Spec.Ports { - addresses = append(addresses, gatewayv1alpha2.GatewayAddress{ - Type: &gatewayIPAddrType, - Value: clusterIP, - }) listeners = append(listeners, gatewayv1alpha2.Listener{ Name: gatewayv1alpha2.SectionName(port.Name), Protocol: gatewayv1alpha2.ProtocolType(port.Protocol), From bd66a62fb72e6e8a73c2b5f335590c714ac0d4c0 Mon Sep 17 00:00:00 2001 From: Shane Utt Date: Thu, 17 Mar 2022 17:14:25 -0400 Subject: [PATCH 2/6] fix: prepend LB addrs in unmanaged GWs --- internal/controllers/gateway/gateway_controller.go | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/internal/controllers/gateway/gateway_controller.go b/internal/controllers/gateway/gateway_controller.go index db145e4f68..c2bc1a76d8 100644 --- a/internal/controllers/gateway/gateway_controller.go +++ b/internal/controllers/gateway/gateway_controller.go @@ -461,19 +461,21 @@ func (r *GatewayReconciler) determineL4ListenersFromService( } // otherwise gather any IPs or Hosts provisioned for the LoadBalancer - // and record them as Gateway Addresses. + // and record them as Gateway Addresses. The LoadBalancer addresses + // are pre-pended to the address list to make them prominent, as they + // are often the most common address used for traffic. for _, ingress := range svc.Status.LoadBalancer.Ingress { if ingress.IP != "" { - addresses = append(addresses, gatewayv1alpha2.GatewayAddress{ + addresses = append([]gatewayv1alpha2.GatewayAddress{{ Type: &gatewayIPAddrType, Value: ingress.IP, - }) + }}, addresses...) } if ingress.Hostname != "" { - addresses = append(addresses, gatewayv1alpha2.GatewayAddress{ + addresses = append([]gatewayv1alpha2.GatewayAddress{{ Type: &gatewayHostAddrType, Value: ingress.Hostname, - }) + }}, addresses...) } } } From f151996ed61b4ae0f895c62d559811ada43522a2 Mon Sep 17 00:00:00 2001 From: Shane Utt Date: Thu, 17 Mar 2022 17:14:54 -0400 Subject: [PATCH 3/6] fix: use ConditionRouteAccepted for HTTPRoute accept --- internal/controllers/gateway/httproute_controller.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/controllers/gateway/httproute_controller.go b/internal/controllers/gateway/httproute_controller.go index fa5f36f83b..94fc7c4b47 100644 --- a/internal/controllers/gateway/httproute_controller.go +++ b/internal/controllers/gateway/httproute_controller.go @@ -366,7 +366,7 @@ func (r *HTTPRouteReconciler) ensureGatewayReferenceStatusAdded(ctx context.Cont }, ControllerName: ControllerName, Conditions: []metav1.Condition{{ - Type: "attached", + Type: string(gatewayv1alpha2.ConditionRouteAccepted), Status: metav1.ConditionTrue, ObservedGeneration: httproute.Generation, LastTransitionTime: metav1.Now(), From 8dc33b0209ca12c02d6f42db8b55f9364af4c880 Mon Sep 17 00:00:00 2001 From: Shane Utt Date: Thu, 17 Mar 2022 17:15:47 -0400 Subject: [PATCH 4/6] fix: wait for dataplane before reporting httproute accepted status --- .../gateway/httproute_controller.go | 44 +++++++++++-------- 1 file changed, 26 insertions(+), 18 deletions(-) diff --git a/internal/controllers/gateway/httproute_controller.go b/internal/controllers/gateway/httproute_controller.go index 94fc7c4b47..d911ea95bd 100644 --- a/internal/controllers/gateway/httproute_controller.go +++ b/internal/controllers/gateway/httproute_controller.go @@ -291,21 +291,6 @@ func (r *HTTPRouteReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( return ctrl.Result{}, err } - // now that we know there are 1 or more supported gateways linked from - // this HTTPRoute, we need to ensure the status is updated accordingly - // before we proceed with any further configurations. - debug(log, httproute, "ensuring status contains Gateway associations") - statusUpdated, err := r.ensureGatewayReferenceStatusAdded(ctx, httproute, gateways...) - if err != nil { - // don't proceed until the statuses can be updated appropriately - return ctrl.Result{}, err - } - if statusUpdated { - // if the status was updated it will trigger a follow-up reconciliation - // so we don't need to do anything further here. - return ctrl.Result{}, nil - } - // the referenced gateway object(s) for the HTTPRoute needs to be ready // before we'll attempt any configurations of it. If it's not we'll // requeue the object and wait until all supported gateways are ready. @@ -317,13 +302,36 @@ func (r *HTTPRouteReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( } } - // finally if all matching has succeeded and the object is not being deleted, - // we can configure it in the data-plane. - debug(log, httproute, "sending httproute information to the data-plane for configuration") + // if the gateways are ready, and the HTTPRoute is destined for them, ensure that + // the object is pushed to the dataplane. if err := r.DataplaneClient.UpdateObject(httproute); err != nil { debug(log, httproute, "failed to update object in data-plane, requeueing") return ctrl.Result{}, err } + if r.DataplaneClient.AreKubernetesObjectReportsEnabled() { + // if the dataplane client has reporting enabled (this is the default and is + // tied in with status updates being enabled in the controller manager) then + // we will wait until the object is reported as successfully configured before + // moving on to status updates. + if !r.DataplaneClient.KubernetesObjectIsConfigured(httproute) { + return ctrl.Result{Requeue: true}, nil + } + } + + // now that the object has been successfully configured for in the dataplane + // we can update the object status to indicate that it's now properly linked + // to the configured Gateways. + debug(log, httproute, "ensuring status contains Gateway associations") + statusUpdated, err := r.ensureGatewayReferenceStatusAdded(ctx, httproute, gateways...) + if err != nil { + // don't proceed until the statuses can be updated appropriately + return ctrl.Result{}, err + } + if statusUpdated { + // if the status was updated it will trigger a follow-up reconciliation + // so we don't need to do anything further here. + return ctrl.Result{}, nil + } // once the data-plane has accepted the HTTPRoute object, we're all set. info(log, httproute, "httproute has been configured on the data-plane") From 5fea81da3013ee4458e2084fc5593ab484a836da Mon Sep 17 00:00:00 2001 From: Shane Utt Date: Thu, 17 Mar 2022 16:02:59 -0400 Subject: [PATCH 5/6] tests: add gw conf test HTTPRouteCrossNamespace --- test/conformance/gateway_conformance_test.go | 33 +++++++++++++++++--- 1 file changed, 29 insertions(+), 4 deletions(-) diff --git a/test/conformance/gateway_conformance_test.go b/test/conformance/gateway_conformance_test.go index 27e3687416..ef0420ea02 100644 --- a/test/conformance/gateway_conformance_test.go +++ b/test/conformance/gateway_conformance_test.go @@ -4,22 +4,27 @@ package conformance import ( + "fmt" "testing" "github.com/google/uuid" "github.com/stretchr/testify/require" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/sets" "sigs.k8s.io/controller-runtime/pkg/client" gatewayv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2" + "sigs.k8s.io/gateway-api/conformance/tests" "sigs.k8s.io/gateway-api/conformance/utils/suite" "github.com/kong/kubernetes-ingress-controller/v2/internal/controllers/gateway" ) var ( - showDebug = true - shouldCleanup = true - conformanceTestsBaseManifests = "https://raw.githubusercontent.com/kubernetes-sigs/gateway-api/master/conformance/base/manifests.yaml" + showDebug = true + shouldCleanup = true + + manifestRepo = "https://raw.githubusercontent.com/kubernetes-sigs/gateway-api/master/" + conformanceTestsBaseManifests = fmt.Sprintf("%s/conformance/base/manifests.yaml", manifestRepo) ) func TestGatewayConformance(t *testing.T) { @@ -50,5 +55,25 @@ func TestGatewayConformance(t *testing.T) { BaseManifests: conformanceTestsBaseManifests, }) cSuite.Setup(t) - // TODO: cSuite.Run(t, tests.ConformanceTests) + + t.Log("configuring gateway conformance tests") + for i := range tests.ConformanceTests { + for j, manifest := range tests.ConformanceTests[i].Manifests { + tests.ConformanceTests[i].Manifests[j] = fmt.Sprintf("%s/conformance/%s", manifestRepo, manifest) + } + } + + t.Log("running gateway conformance tests") + for _, tt := range tests.ConformanceTests { + if enabledGatewayConformanceTests.Has(tt.ShortName) { + t.Run(tt.Description, func(t *testing.T) { tt.Run(t, cSuite) }) + } + } } + +// Today we run only the subset below of all Gateway conformance tests. +// TODO: ensure that this module runs all Gateway conformance tests +// https://github.com/Kong/kubernetes-ingress-controller/issues/2210 +var enabledGatewayConformanceTests = sets.NewString( + "HTTPRouteCrossNamespace", +) From b6bf34df4395ff3b837a2946c3663a668894afb8 Mon Sep 17 00:00:00 2001 From: Shane Utt Date: Thu, 17 Mar 2022 17:32:03 -0400 Subject: [PATCH 6/6] docs: update CHANGELOG.md --- CHANGELOG.md | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3e4ddf648e..6bda39bcf7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -57,6 +57,11 @@ #### Added +- `Gateway` resources which have a `LoadBalancer` address among their list of + addresses will have those addresses listed on the top for convenience, and + so that those addresses are made prominent in the `kubectl get gateways` + short view. + [#2339](https://github.com/Kong/kubernetes-ingress-controller/pull/2339) - The controller manager can now be flagged with a client certificate to use for mTLS authentication with the Kong Admin API. [#1958](https://github.com/Kong/kubernetes-ingress-controller/issues/1958) @@ -74,6 +79,18 @@ #### Fixed +- Status updates for `HTTPRoute` objects no longer mark the resource as + `ConditionRouteAccepted` until the object has been successfully configured + in Kong Gateway at least once, as long as `--update-status` + is enabled (enabled by default). + [#2339](https://github.com/Kong/kubernetes-ingress-controller/pull/2339) +- Status updates for `HTTPRoute` now properly use the `ConditionRouteAccepted` + value for parent `Gateway` conditions when the route becomes configured in + the `Gateway` rather than the previous random `"attached"` string. + [#2339](https://github.com/Kong/kubernetes-ingress-controller/pull/2339) +- Fixed a minor issue where addresses on `Gateway` resources would be + duplicated depending on how many listeners are configured. + [#2339](https://github.com/Kong/kubernetes-ingress-controller/pull/2339) - Unconfigured fields now use their default value according to the Kong proxy instance's reported schema. This addresses an issue where configuration updates would send unnecessary requests to clear a default value.