diff --git a/apis/gateway/v1beta1/targetgroupconfig_types.go b/apis/gateway/v1beta1/targetgroupconfig_types.go index e689acf9f..717077dc3 100644 --- a/apis/gateway/v1beta1/targetgroupconfig_types.go +++ b/apis/gateway/v1beta1/targetgroupconfig_types.go @@ -129,7 +129,7 @@ const ( ProtocolTCP_UDP Protocol = "TCP_UDP" ) -// +kubebuilder:validation:Enum=http1;http2;grpc +// +kubebuilder:validation:Enum=HTTP1;HTTP2;GRPC type ProtocolVersion string const ( diff --git a/config/crd/gateway/gateway-crds.yaml b/config/crd/gateway/gateway-crds.yaml index 4f9dde5f3..b2f374b4a 100644 --- a/config/crd/gateway/gateway-crds.yaml +++ b/config/crd/gateway/gateway-crds.yaml @@ -491,9 +491,9 @@ spec: description: protocolVersion [HTTP/HTTPS protocol] The protocol version. The possible values are GRPC , HTTP1 and HTTP2 enum: - - http1 - - http2 - - grpc + - HTTP1 + - HTTP2 + - GRPC type: string tags: description: Tags defines list of Tags on target group. @@ -694,9 +694,9 @@ spec: description: protocolVersion [HTTP/HTTPS protocol] The protocol version. The possible values are GRPC , HTTP1 and HTTP2 enum: - - http1 - - http2 - - grpc + - HTTP1 + - HTTP2 + - GRPC type: string tags: description: Tags defines list of Tags on target group. diff --git a/config/crd/gateway/gateway.k8s.aws_targetgroupconfigurations.yaml b/config/crd/gateway/gateway.k8s.aws_targetgroupconfigurations.yaml index b2b21ca7b..bf0d6cc63 100644 --- a/config/crd/gateway/gateway.k8s.aws_targetgroupconfigurations.yaml +++ b/config/crd/gateway/gateway.k8s.aws_targetgroupconfigurations.yaml @@ -186,9 +186,9 @@ spec: description: protocolVersion [HTTP/HTTPS protocol] The protocol version. The possible values are GRPC , HTTP1 and HTTP2 enum: - - http1 - - http2 - - grpc + - HTTP1 + - HTTP2 + - GRPC type: string tags: description: Tags defines list of Tags on target group. @@ -389,9 +389,9 @@ spec: description: protocolVersion [HTTP/HTTPS protocol] The protocol version. The possible values are GRPC , HTTP1 and HTTP2 enum: - - http1 - - http2 - - grpc + - HTTP1 + - HTTP2 + - GRPC type: string tags: description: Tags defines list of Tags on target group. diff --git a/controllers/gateway/gateway_controller.go b/controllers/gateway/gateway_controller.go index 866de8257..630fd8945 100644 --- a/controllers/gateway/gateway_controller.go +++ b/controllers/gateway/gateway_controller.go @@ -196,7 +196,7 @@ func (r *gatewayReconciler) reconcileHelper(ctx context.Context, req reconcile.R mergedLbConfig, err := r.cfgResolver.getLoadBalancerConfigForGateway(ctx, r.k8sClient, gw, gwClass) if err != nil { - statusErr := r.updateGatewayStatusFailure(ctx, gw, gwv1.GatewayReasonInvalid, err) + statusErr := r.updateGatewayStatusFailure(ctx, gw, gwv1.GatewayReasonInvalid, err.Error()) if statusErr != nil { r.logger.Error(statusErr, "Unable to update gateway status on failure to retrieve attached config") } @@ -208,7 +208,7 @@ func (r *gatewayReconciler) reconcileHelper(ctx context.Context, req reconcile.R if err != nil { var loaderErr routeutils.LoaderError if errors.As(err, &loaderErr) { - statusErr := r.updateGatewayStatusFailure(ctx, gw, loaderErr.GetGatewayReason(), loaderErr) + statusErr := r.updateGatewayStatusFailure(ctx, gw, loaderErr.GetGatewayReason(), loaderErr.GetGatewayMessage()) if statusErr != nil { r.logger.Error(statusErr, "Unable to update gateway status on failure to build routes") } @@ -354,10 +354,10 @@ func (r *gatewayReconciler) updateGatewayStatusSuccess(ctx context.Context, lbSt return nil } -func (r *gatewayReconciler) updateGatewayStatusFailure(ctx context.Context, gw *gwv1.Gateway, reason gwv1.GatewayConditionReason, err error) error { +func (r *gatewayReconciler) updateGatewayStatusFailure(ctx context.Context, gw *gwv1.Gateway, reason gwv1.GatewayConditionReason, errMessage string) error { gwOld := gw.DeepCopy() - needPatch := r.gatewayConditionUpdater(gw, string(gwv1.GatewayConditionAccepted), metav1.ConditionFalse, string(reason), err.Error()) + needPatch := r.gatewayConditionUpdater(gw, string(gwv1.GatewayConditionAccepted), metav1.ConditionFalse, string(reason), errMessage) if needPatch { if err := r.k8sClient.Status().Patch(ctx, gw, client.MergeFrom(gwOld)); err != nil { diff --git a/controllers/gateway/route_reconciler_test.go b/controllers/gateway/route_reconciler_test.go index a9c6cc603..83dfe43d3 100644 --- a/controllers/gateway/route_reconciler_test.go +++ b/controllers/gateway/route_reconciler_test.go @@ -12,6 +12,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/log" gwv1 "sigs.k8s.io/gateway-api/apis/v1" "testing" + "time" ) func TestDeferredReconcilerConstructor(t *testing.T) { @@ -421,3 +422,296 @@ func TestEnqueue(t *testing.T) { }) } } + +func Test_updateRouteStatus(t *testing.T) { + tests := []struct { + name string + route client.Object + routeData routeutils.RouteData + validateResult func(t *testing.T, route client.Object) + }{ + { + name: "update HTTPRoute status - condition accepted", + route: &gwv1.HTTPRoute{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-route", + Namespace: "test-namespace", + }, + Spec: gwv1.HTTPRouteSpec{ + Hostnames: []gwv1.Hostname{"example.com"}, + }, + }, + routeData: routeutils.RouteData{ + RouteStatusInfo: routeutils.RouteStatusInfo{ + Accepted: true, + ResolvedRefs: true, + Reason: string(gwv1.RouteConditionAccepted), + Message: "route accepted", + }, + ParentRefGateway: routeutils.ParentRefGateway{ + Name: "test-gateway", + Namespace: "test-namespace", + }, + }, + validateResult: func(t *testing.T, route client.Object) { + httpRoute := route.(*gwv1.HTTPRoute) + if httpRoute.Status.Parents == nil { + assert.Len(t, httpRoute.Status.Parents, 0) + } + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + logger := logr.New(&log.NullLogSink{}) + reconciler := &routeReconcilerImpl{ + logger: logger, + } + err := reconciler.updateRouteStatus(tt.route, tt.routeData) + assert.NoError(t, err) + if tt.validateResult != nil { + tt.validateResult(t, tt.route) + } + }) + } +} + +func Test_setConditionsWithRouteStatusInfo(t *testing.T) { + route := &gwv1.HTTPRoute{ + ObjectMeta: metav1.ObjectMeta{ + Generation: 1, + }, + } + + tests := []struct { + name string + info routeutils.RouteStatusInfo + validateResult func(t *testing.T, conditions []metav1.Condition) + }{ + { + name: "accepted true and resolvedRef true", + info: routeutils.RouteStatusInfo{ + Accepted: true, + ResolvedRefs: true, + Reason: string(gwv1.RouteConditionAccepted), + Message: "route accepted", + }, + validateResult: func(t *testing.T, conditions []metav1.Condition) { + assert.Len(t, conditions, 2) + acceptedCondition := findCondition(conditions, string(gwv1.RouteConditionAccepted)) + assert.NotNil(t, acceptedCondition) + assert.Equal(t, metav1.ConditionTrue, acceptedCondition.Status) + + resolvedRefCondition := findCondition(conditions, string(gwv1.RouteConditionResolvedRefs)) + assert.NotNil(t, resolvedRefCondition) + assert.Equal(t, metav1.ConditionTrue, resolvedRefCondition.Status) + }, + }, + { + name: "accepted false and resolvedRef true", + info: routeutils.RouteStatusInfo{ + Accepted: false, + ResolvedRefs: true, + Reason: string(gwv1.RouteReasonNotAllowedByListeners), + Message: "route not allowed by listeners", + }, + validateResult: func(t *testing.T, conditions []metav1.Condition) { + assert.Len(t, conditions, 2) + acceptedCondition := findCondition(conditions, string(gwv1.RouteConditionAccepted)) + assert.NotNil(t, acceptedCondition) + assert.Equal(t, metav1.ConditionFalse, acceptedCondition.Status) + + resolvedRefCondition := findCondition(conditions, string(gwv1.RouteConditionResolvedRefs)) + assert.NotNil(t, resolvedRefCondition) + assert.Equal(t, metav1.ConditionTrue, resolvedRefCondition.Status) + }, + }, + { + name: "accepted false and resolvedRef false", + info: routeutils.RouteStatusInfo{ + Accepted: false, + ResolvedRefs: false, + Reason: string(gwv1.RouteReasonBackendNotFound), + Message: "backend not found", + }, + validateResult: func(t *testing.T, conditions []metav1.Condition) { + assert.Len(t, conditions, 2) + acceptedCondition := findCondition(conditions, string(gwv1.RouteConditionAccepted)) + assert.NotNil(t, acceptedCondition) + assert.Equal(t, metav1.ConditionFalse, acceptedCondition.Status) + + resolvedRefCondition := findCondition(conditions, string(gwv1.RouteConditionResolvedRefs)) + assert.NotNil(t, resolvedRefCondition) + assert.Equal(t, metav1.ConditionFalse, resolvedRefCondition.Status) + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + logger := logr.New(&log.NullLogSink{}) + reconciler := &routeReconcilerImpl{ + logger: logger, + } + parentStatus := &gwv1.RouteParentStatus{} + reconciler.setConditionsWithRouteStatusInfo(route, parentStatus, tt.info) + if tt.validateResult != nil { + tt.validateResult(t, parentStatus.Conditions) + } + }) + } +} + +func Test_areConditionsEqual(t *testing.T) { + tests := []struct { + name string + oldCon []metav1.Condition + newCon []metav1.Condition + expected bool + }{ + { + name: "same conditions - true", + oldCon: []metav1.Condition{ + { + Type: string(gwv1.RouteConditionAccepted), + Status: metav1.ConditionTrue, + }, + { + Type: string(gwv1.RouteConditionResolvedRefs), + Status: metav1.ConditionTrue, + }, + }, + newCon: []metav1.Condition{ + { + Type: string(gwv1.RouteConditionAccepted), + Status: metav1.ConditionTrue, + }, + { + Type: string(gwv1.RouteConditionResolvedRefs), + Status: metav1.ConditionTrue, + }, + }, + expected: true, + }, + { + name: "different conditions - false", + oldCon: []metav1.Condition{ + { + Type: string(gwv1.RouteConditionAccepted), + Status: metav1.ConditionTrue, + }, + { + Type: string(gwv1.RouteConditionResolvedRefs), + Status: metav1.ConditionTrue, + }, + }, + newCon: []metav1.Condition{ + { + Type: string(gwv1.RouteConditionAccepted), + Status: metav1.ConditionFalse, + }, + { + Type: string(gwv1.RouteConditionResolvedRefs), + Status: metav1.ConditionTrue, + }, + }, + expected: false, + }, + { + name: "different conditions on Reason - false", + oldCon: []metav1.Condition{ + { + Type: string(gwv1.RouteConditionAccepted), + Status: metav1.ConditionTrue, + Reason: "good", + }, + { + Type: string(gwv1.RouteConditionResolvedRefs), + Status: metav1.ConditionTrue, + }, + }, + newCon: []metav1.Condition{ + { + Type: string(gwv1.RouteConditionAccepted), + Status: metav1.ConditionTrue, + Reason: "test-good", + }, + { + Type: string(gwv1.RouteConditionResolvedRefs), + Status: metav1.ConditionTrue, + }, + }, + expected: false, + }, + { + name: "different conditions on LastTransitionTime - true", + oldCon: []metav1.Condition{ + { + Type: string(gwv1.RouteConditionAccepted), + Status: metav1.ConditionTrue, + LastTransitionTime: metav1.NewTime(time.Date(2025, 1, 1, 0, 0, 0, 0, time.UTC)), + }, + { + Type: string(gwv1.RouteConditionResolvedRefs), + Status: metav1.ConditionTrue, + }, + }, + newCon: []metav1.Condition{ + { + Type: string(gwv1.RouteConditionAccepted), + Status: metav1.ConditionTrue, + LastTransitionTime: metav1.NewTime(time.Date(2024, 1, 1, 0, 0, 0, 0, time.UTC)), + }, + { + Type: string(gwv1.RouteConditionResolvedRefs), + Status: metav1.ConditionTrue, + }, + }, + expected: true, + }, + { + name: "different conditions on ObservedGeneration - true", + oldCon: []metav1.Condition{ + { + Type: string(gwv1.RouteConditionAccepted), + Status: metav1.ConditionTrue, + ObservedGeneration: 1, + }, + { + Type: string(gwv1.RouteConditionResolvedRefs), + Status: metav1.ConditionTrue, + }, + }, + newCon: []metav1.Condition{ + { + Type: string(gwv1.RouteConditionAccepted), + Status: metav1.ConditionTrue, + ObservedGeneration: 2, + }, + { + Type: string(gwv1.RouteConditionResolvedRefs), + Status: metav1.ConditionTrue, + }, + }, + expected: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := areConditionsEqual(tt.oldCon, tt.newCon) + assert.Equal(t, tt.expected, result) + }) + } +} + +// helper function +func findCondition(conditions []metav1.Condition, conditionType string) *metav1.Condition { + for _, condition := range conditions { + if condition.Type == conditionType { + return &condition + } + } + return nil +} diff --git a/pkg/gateway/model/utilities.go b/pkg/gateway/model/utilities.go index b717d587d..d081faa58 100644 --- a/pkg/gateway/model/utilities.go +++ b/pkg/gateway/model/utilities.go @@ -3,6 +3,7 @@ package model import ( "sigs.k8s.io/aws-load-balancer-controller/pkg/gateway/routeutils" elbv2model "sigs.k8s.io/aws-load-balancer-controller/pkg/model/elbv2" + v1 "sigs.k8s.io/gateway-api/apis/v1" "sort" "strings" ) @@ -21,7 +22,22 @@ func isIPv6CIDR(cidr string) bool { return strings.Contains(cidr, ":") } +func getHighestPrecedenceHostname(hostnames []v1.Hostname) string { + if len(hostnames) == 0 { + return "" + } + + highestHostname := hostnames[0] + for _, hostname := range hostnames { + if routeutils.GetHostnamePrecedenceOrder(string(hostname), string(highestHostname)) < 0 { + highestHostname = hostname + } + } + return string(highestHostname) +} + func sortRoutesByHostnamePrecedence(routes []routeutils.RouteDescriptor) { + // sort routes based on their highest precedence hostname sort.SliceStable(routes, func(i, j int) bool { hostnameOne := routes[i].GetHostnames() hostnameTwo := routes[j].GetHostnames() @@ -35,7 +51,11 @@ func sortRoutesByHostnamePrecedence(routes []routeutils.RouteDescriptor) { if len(hostnameTwo) == 0 { return true } - precedence := routeutils.GetHostnamePrecedenceOrder(string(hostnameOne[0]), string(hostnameTwo[0])) + + highestPrecedenceHostnameOne := getHighestPrecedenceHostname(hostnameOne) + highestPrecedenceHostnameTwo := getHighestPrecedenceHostname(hostnameTwo) + + precedence := routeutils.GetHostnamePrecedenceOrder(highestPrecedenceHostnameOne, highestPrecedenceHostnameTwo) if precedence != 0 { return precedence < 0 // -1 means higher precedence } diff --git a/pkg/gateway/model/utilities_test.go b/pkg/gateway/model/utilities_test.go index 28b2cd416..f2885ddb6 100644 --- a/pkg/gateway/model/utilities_test.go +++ b/pkg/gateway/model/utilities_test.go @@ -111,6 +111,36 @@ func Test_SortRoutesByHostnamePrecedence(t *testing.T) { &routeutils.MockRoute{Hostnames: []string{}}, }, }, + { + name: "complex mixed hostnames with multiple hostnames in each port", + input: []routeutils.RouteDescriptor{ + &routeutils.MockRoute{Hostnames: []string{"*.example.com", "test.details.example.com"}}, + &routeutils.MockRoute{Hostnames: []string{}}, + &routeutils.MockRoute{Hostnames: []string{"test.example.com"}}, + &routeutils.MockRoute{Hostnames: []string{"another.example.com"}}, + }, + expected: []routeutils.RouteDescriptor{ + &routeutils.MockRoute{Hostnames: []string{"*.example.com", "test.details.example.com"}}, // since test.details.example.com has the highest precedence order here + &routeutils.MockRoute{Hostnames: []string{"another.example.com"}}, + &routeutils.MockRoute{Hostnames: []string{"test.example.com"}}, + &routeutils.MockRoute{Hostnames: []string{}}, + }, + }, + { + name: "shorter hostname with more dots should have higher precedence ", + input: []routeutils.RouteDescriptor{ + &routeutils.MockRoute{Hostnames: []string{"another.example.com"}}, + &routeutils.MockRoute{Hostnames: []string{"a.b.example.com"}}, + &routeutils.MockRoute{Hostnames: []string{}}, + &routeutils.MockRoute{Hostnames: []string{"*.example.com"}}, + }, + expected: []routeutils.RouteDescriptor{ + &routeutils.MockRoute{Hostnames: []string{"a.b.example.com"}}, + &routeutils.MockRoute{Hostnames: []string{"another.example.com"}}, + &routeutils.MockRoute{Hostnames: []string{"*.example.com"}}, + &routeutils.MockRoute{Hostnames: []string{}}, + }, + }, } for _, tt := range tests { diff --git a/pkg/gateway/routeutils/backend.go b/pkg/gateway/routeutils/backend.go index 54f2c44f4..c634bba63 100644 --- a/pkg/gateway/routeutils/backend.go +++ b/pkg/gateway/routeutils/backend.go @@ -11,6 +11,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" gwv1 "sigs.k8s.io/gateway-api/apis/v1" gwbeta1 "sigs.k8s.io/gateway-api/apis/v1beta1" + "strings" ) const ( @@ -32,7 +33,8 @@ func commonBackendLoader(ctx context.Context, k8sClient client.Client, typeSpeci // We only support references of type service. if backendRef.Kind != nil && *backendRef.Kind != "Service" { initialErrorMessage := "Backend Ref must be of kind 'Service'" - return nil, wrapError(errors.Errorf("%s", generateInvalidMessageWithRouteDetails(initialErrorMessage, routeKind, routeIdentifier)), gwv1.GatewayReasonListenersNotValid, gwv1.RouteReasonInvalidKind) + wrappedGatewayErrorMessage := generateInvalidMessageWithRouteDetails(initialErrorMessage, routeKind, routeIdentifier) + return nil, wrapError(errors.Errorf("%s", initialErrorMessage), gwv1.GatewayReasonListenersNotValid, gwv1.RouteReasonInvalidKind, &wrappedGatewayErrorMessage, nil) } if backendRef.Weight != nil && *backendRef.Weight == 0 { @@ -41,7 +43,8 @@ func commonBackendLoader(ctx context.Context, k8sClient client.Client, typeSpeci if backendRef.Port == nil { initialErrorMessage := "Port is required" - return nil, wrapError(errors.Errorf("%s", generateInvalidMessageWithRouteDetails(initialErrorMessage, routeKind, routeIdentifier)), gwv1.GatewayReasonListenersNotValid, gwv1.RouteReasonUnsupportedValue) + wrappedGatewayErrorMessage := generateInvalidMessageWithRouteDetails(initialErrorMessage, routeKind, routeIdentifier) + return nil, wrapError(errors.Errorf("%s", initialErrorMessage), gwv1.GatewayReasonListenersNotValid, gwv1.RouteReasonUnsupportedValue, &wrappedGatewayErrorMessage, nil) } var svcNamespace string @@ -56,7 +59,7 @@ func commonBackendLoader(ctx context.Context, k8sClient client.Client, typeSpeci Name: string(backendRef.Name), } - // Check for reference grant when performing crossname gateway -> route attachment + // Check for reference grant when performing cross namespace gateway -> route attachment if svcNamespace != routeIdentifier.Namespace { allowed, err := referenceGrantCheck(ctx, k8sClient, svcIdentifier, routeIdentifier, routeKind) if err != nil { @@ -68,7 +71,10 @@ func commonBackendLoader(ctx context.Context, k8sClient client.Client, typeSpeci // That way, users can't infer if the route is missing because of a misconfigured service reference // or the sentence grant is not allowing the connection. if !allowed { - return nil, nil + initialErrorMessage := "No explicit ReferenceGrant exits to allow the reference." + wrappedGatewayErrorMessage := generateInvalidMessageWithRouteDetails(initialErrorMessage, routeKind, routeIdentifier) + return nil, wrapError(errors.Errorf("%s", initialErrorMessage), gwv1.GatewayReasonListenersNotValid, gwv1.RouteReasonRefNotPermitted, &wrappedGatewayErrorMessage, nil) + } } @@ -81,7 +87,8 @@ func commonBackendLoader(ctx context.Context, k8sClient client.Client, typeSpeci if convertToNotFoundError == nil { // Svc not found, post an updated status. initialErrorMessage := fmt.Sprintf("Service (%s:%s) not found)", svcIdentifier.Namespace, svcIdentifier.Name) - return nil, wrapError(errors.Errorf("%s", generateInvalidMessageWithRouteDetails(initialErrorMessage, routeKind, routeIdentifier)), gwv1.GatewayReasonListenersNotValid, gwv1.RouteReasonBackendNotFound) + wrappedGatewayErrorMessage := generateInvalidMessageWithRouteDetails(initialErrorMessage, routeKind, routeIdentifier) + return nil, wrapError(errors.Errorf("%s", initialErrorMessage), gwv1.GatewayReasonListenersNotValid, gwv1.RouteReasonBackendNotFound, &wrappedGatewayErrorMessage, nil) } // Otherwise, general error. No need for status update. return nil, errors.Wrap(err, fmt.Sprintf("Unable to fetch svc object %+v", svcIdentifier)) @@ -104,7 +111,33 @@ func commonBackendLoader(ctx context.Context, k8sClient client.Client, typeSpeci if servicePort == nil { initialErrorMessage := fmt.Sprintf("Unable to find service port for port %d", *backendRef.Port) - return nil, wrapError(errors.Errorf("%s", generateInvalidMessageWithRouteDetails(initialErrorMessage, routeKind, routeIdentifier)), gwv1.GatewayReasonListenersNotValid, gwv1.RouteReasonBackendNotFound) + wrappedGatewayErrorMessage := generateInvalidMessageWithRouteDetails(initialErrorMessage, routeKind, routeIdentifier) + return nil, wrapError(errors.Errorf("%s", initialErrorMessage), gwv1.GatewayReasonListenersNotValid, gwv1.RouteReasonBackendNotFound, &wrappedGatewayErrorMessage, nil) + } + + // validate if protocol version is compatible with appProtocol + if tgConfig != nil && servicePort.AppProtocol != nil { + tgProps := tgConfig.GetTargetGroupConfigForRoute(routeIdentifier.Name, routeIdentifier.Namespace, string(routeKind)) + appProtocol := strings.ToLower(*servicePort.AppProtocol) + if tgProps != nil && tgProps.ProtocolVersion != nil { + isCompatible := true + switch *tgProps.ProtocolVersion { + case elbv2gw.ProtocolVersionGRPC: + if appProtocol == "http" { + isCompatible = false + } + case elbv2gw.ProtocolVersionHTTP1, elbv2gw.ProtocolVersionHTTP2: + if appProtocol == "grpc" { + isCompatible = false + } + } + if !isCompatible { + initialErrorMessage := fmt.Sprintf("Service port appProtocol %s is not compatible with target group protocolVersion %s", *servicePort.AppProtocol, *tgProps.ProtocolVersion) + wrappedGatewayErrorMessage := generateInvalidMessageWithRouteDetails(initialErrorMessage, routeKind, routeIdentifier) + + return nil, wrapError(errors.Errorf("%s", initialErrorMessage), gwv1.GatewayReasonListenersNotValid, gwv1.RouteReasonUnsupportedProtocol, &wrappedGatewayErrorMessage, nil) + } + } } // Weight specifies the proportion of requests forwarded to the referenced diff --git a/pkg/gateway/routeutils/backend_test.go b/pkg/gateway/routeutils/backend_test.go index d870819ce..5d6a840cd 100644 --- a/pkg/gateway/routeutils/backend_test.go +++ b/pkg/gateway/routeutils/backend_test.go @@ -186,6 +186,7 @@ func TestCommonBackendLoader(t *testing.T) { }, }, expectNoResult: true, + expectErr: true, storedService: &corev1.Service{ ObjectMeta: metav1.ObjectMeta{ Namespace: namespaceToUse, diff --git a/pkg/gateway/routeutils/listener_attachment_helper.go b/pkg/gateway/routeutils/listener_attachment_helper.go index 7409bb9f5..883ea1c6b 100644 --- a/pkg/gateway/routeutils/listener_attachment_helper.go +++ b/pkg/gateway/routeutils/listener_attachment_helper.go @@ -148,7 +148,7 @@ func (attachmentHelper *listenerAttachmentHelperImpl) hostnameCheck(listener gwv if err != nil { attachmentHelper.logger.Error(err, "listener hostname is not valid", "listener", listener.Name, "hostname", *listener.Hostname) initialErrorMessage := fmt.Sprintf("listener hostname %s is not valid (listener name %s)", listener.Name, *listener.Hostname) - return false, wrapError(errors.Errorf("%s", initialErrorMessage), gwv1.GatewayReasonListenersNotValid, gwv1.RouteReasonUnsupportedValue) + return false, wrapError(errors.Errorf("%s", initialErrorMessage), gwv1.GatewayReasonListenersNotValid, gwv1.RouteReasonUnsupportedValue, nil, nil) } if !isListenerHostnameValid { diff --git a/pkg/gateway/routeutils/loader.go b/pkg/gateway/routeutils/loader.go index a5fcf5b64..b83d136aa 100644 --- a/pkg/gateway/routeutils/loader.go +++ b/pkg/gateway/routeutils/loader.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "github.com/go-logr/logr" + "github.com/pkg/errors" "k8s.io/apimachinery/pkg/util/sets" "sigs.k8s.io/controller-runtime/pkg/client" gwv1 "sigs.k8s.io/gateway-api/apis/v1" @@ -93,9 +94,8 @@ func (l *loaderImpl) LoadRoutesForGateway(ctx context.Context, gw gwv1.Gateway, } // 3. Load the underlying resource(s) for each route that is configured. - loadedRoute, err := l.loadChildResources(ctx, mappedRoutes) + loadedRoute, err := l.loadChildResources(ctx, mappedRoutes, deferredRouteReconciler, gw) if err != nil { - // TODO: add route status update for those with error return nil, err } @@ -111,7 +111,7 @@ func (l *loaderImpl) LoadRoutesForGateway(ctx context.Context, gw gwv1.Gateway, } // loadChildResources responsible for loading all resources that a route descriptor references. -func (l *loaderImpl) loadChildResources(ctx context.Context, preloadedRoutes map[int][]preLoadRouteDescriptor) (map[int32][]RouteDescriptor, error) { +func (l *loaderImpl) loadChildResources(ctx context.Context, preloadedRoutes map[int][]preLoadRouteDescriptor, deferredRouteReconciler RouteReconciler, gw gwv1.Gateway) (map[int32][]RouteDescriptor, error) { // Cache to reduce duplicate route lookups. // Kind -> [NamespacedName:Previously Loaded Descriptor] resourceCache := make(map[string]RouteDescriptor) @@ -132,6 +132,12 @@ func (l *loaderImpl) loadChildResources(ctx context.Context, preloadedRoutes map generatedRoute, err := preloadedRoute.loadAttachedRules(ctx, l.k8sClient) if err != nil { + var loaderErr LoaderError + if errors.As(err, &loaderErr) { + deferredRouteReconciler.Enqueue( + GenerateRouteData(false, false, string(loaderErr.GetRouteReason()), loaderErr.GetRouteMessage(), preloadedRoute.GetRouteNamespacedName(), preloadedRoute.GetRouteKind(), preloadedRoute.GetRouteGeneration(), gw), + ) + } return nil, err } loadedRouteData[int32(port)] = append(loadedRouteData[int32(port)], generatedRoute) diff --git a/pkg/gateway/routeutils/loader_error.go b/pkg/gateway/routeutils/loader_error.go index 86c271301..bbbfb378b 100644 --- a/pkg/gateway/routeutils/loader_error.go +++ b/pkg/gateway/routeutils/loader_error.go @@ -9,20 +9,32 @@ type LoaderError interface { GetRawError() error GetGatewayReason() gwv1.GatewayConditionReason GetRouteReason() gwv1.RouteConditionReason + GetGatewayMessage() string + GetRouteMessage() string } type loaderErrorImpl struct { + underlyingErr error // contains initial error message resolvedGatewayCondition gwv1.GatewayConditionReason resolvedRouteCondition gwv1.RouteConditionReason - underlyingErr error + resolvedGatewayMessage *string + resolvedRouteMessage *string } -func wrapError(underlyingErr error, gatewayCondition gwv1.GatewayConditionReason, routeCondition gwv1.RouteConditionReason) LoaderError { - return &loaderErrorImpl{ +func wrapError(underlyingErr error, gatewayCondition gwv1.GatewayConditionReason, routeCondition gwv1.RouteConditionReason, resolvedGatewayMessage *string, resolvedRouteMessage *string) LoaderError { + e := &loaderErrorImpl{ underlyingErr: underlyingErr, resolvedGatewayCondition: gatewayCondition, resolvedRouteCondition: routeCondition, } + if resolvedGatewayMessage != nil { + e.resolvedGatewayMessage = resolvedGatewayMessage + } + + if resolvedRouteMessage != nil { + e.resolvedRouteMessage = resolvedRouteMessage + } + return e } func (e *loaderErrorImpl) GetRawError() error { @@ -37,6 +49,22 @@ func (e *loaderErrorImpl) GetRouteReason() gwv1.RouteConditionReason { return e.resolvedRouteCondition } +// GetGatewayMessage original error message for gateway status update can be overridden by pass in resolvedGatewayMessage +func (e *loaderErrorImpl) GetGatewayMessage() string { + if e.resolvedGatewayMessage != nil { + return *e.resolvedGatewayMessage + } + return e.underlyingErr.Error() +} + +// GetRouteMessage original error message for route status update can be overridden by pass in resolvedRouteMessage +func (e *loaderErrorImpl) GetRouteMessage() string { + if e.resolvedRouteMessage != nil { + return *e.resolvedRouteMessage + } + return e.underlyingErr.Error() +} + func (e *loaderErrorImpl) Error() string { return e.underlyingErr.Error() } diff --git a/pkg/gateway/routeutils/namespace_selector.go b/pkg/gateway/routeutils/namespace_selector.go index 6cbc22650..c1b289583 100644 --- a/pkg/gateway/routeutils/namespace_selector.go +++ b/pkg/gateway/routeutils/namespace_selector.go @@ -35,7 +35,7 @@ func (n *namespaceSelectorImpl) getNamespacesFromSelector(context context.Contex convertedSelector, err := metav1.LabelSelectorAsSelector(selector) if err != nil { - return nil, wrapError(errors.Wrapf(err, "Unable to parse selector %s", selector), gwv1.GatewayReasonListenersNotValid, gwv1.RouteReasonUnsupportedValue) + return nil, wrapError(errors.Wrapf(err, "Unable to parse selector %s", selector), gwv1.GatewayReasonListenersNotValid, gwv1.RouteReasonUnsupportedValue, nil, nil) } err = n.k8sClient.List(context, &namespaceList, client.MatchingLabelsSelector{Selector: convertedSelector}) diff --git a/pkg/gateway/routeutils/utils.go b/pkg/gateway/routeutils/utils.go index 537173b04..b054feb1f 100644 --- a/pkg/gateway/routeutils/utils.go +++ b/pkg/gateway/routeutils/utils.go @@ -152,6 +152,13 @@ func GetHostnamePrecedenceOrder(hostnameOne, hostnameTwo string) int { } else if isHostnameOneWildcard && !isHostnameTwoWildcard { return 1 } else { + dotsInHostnameOne := strings.Count(hostnameOne, ".") + dotsInHostnameTwo := strings.Count(hostnameTwo, ".") + if dotsInHostnameOne > dotsInHostnameTwo { + return -1 + } else if dotsInHostnameOne < dotsInHostnameTwo { + return 1 + } if len(hostnameOne) > len(hostnameTwo) { return -1 } else if len(hostnameOne) < len(hostnameTwo) { diff --git a/pkg/gateway/routeutils/utils_test.go b/pkg/gateway/routeutils/utils_test.go index c20688be2..be8fe08f1 100644 --- a/pkg/gateway/routeutils/utils_test.go +++ b/pkg/gateway/routeutils/utils_test.go @@ -686,6 +686,20 @@ func TestGetHostnamePrecedenceOrder(t *testing.T) { want: -1, description: "non-empty string should have higher precedence than empty", }, + { + name: "one hostname has more dots", + hostnameOne: "*.example.com", + hostnameTwo: "*.t.exa.com", + want: 1, + description: "hostname with more dots should have higher precedence even if it has less character", + }, + { + name: "two hostnames have same number of dots, one has more characters", + hostnameOne: "*.t.example.com", + hostnameTwo: "*.t.exa.com", + want: -1, + description: "hostname with more characters should have higher precedence order if they have same number of dots", + }, } for _, tt := range tests {