diff --git a/CHANGELOG.md b/CHANGELOG.md index b0c18a03ae..f12c2a1a5d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -128,8 +128,12 @@ Adding a new version? You'll need three changes: - using filters in backendRefs of rules [#5312](https://github.com/Kong/kubernetes-ingress-controller/pull/5312) - Added functionality to the `KongUpstreamPolicy` controller to properly set and - enforce `KongUpstreamPolicy` status. + enforce `KongUpstreamPolicy` status. + The controller will set an ancestor status in `KongUpstreamPolicy` status for + each of its ancestors (i.e. `Service` or `KongServiceFacade`) with the `Accepted` + condition. [#5185](https://github.com/Kong/kubernetes-ingress-controller/pull/5185) + [#5428](https://github.com/Kong/kubernetes-ingress-controller/pull/5428) - New CRD `KongVault` to reperesent a custom Kong vault for storing senstive data used in plugin configurations. Now users can create a `KongVault` to create a custom Kong vault and reference the values in configurations of diff --git a/internal/controllers/configuration/kongupstreampolicy_controller.go b/internal/controllers/configuration/kongupstreampolicy_controller.go index 523d105c2f..8aca948ffd 100644 --- a/internal/controllers/configuration/kongupstreampolicy_controller.go +++ b/internal/controllers/configuration/kongupstreampolicy_controller.go @@ -22,6 +22,7 @@ import ( "github.com/kong/kubernetes-ingress-controller/v3/internal/gatewayapi" "github.com/kong/kubernetes-ingress-controller/v3/internal/util" kongv1beta1 "github.com/kong/kubernetes-ingress-controller/v3/pkg/apis/configuration/v1beta1" + incubatorv1alpha1 "github.com/kong/kubernetes-ingress-controller/v3/pkg/apis/incubator/v1alpha1" ) // ----------------------------------------------------------------------------- @@ -36,6 +37,10 @@ type KongUpstreamPolicyReconciler struct { Scheme *runtime.Scheme DataplaneClient controllers.DataPlane CacheSyncTimeout time.Duration + + // KongServiceFacadeEnabled determines whether the controller should populate the KongUpstreamPolicy's ancestor + // status for KongServiceFacades. + KongServiceFacadeEnabled bool } // SetupWithManager sets up the controller with the Manager. @@ -51,28 +56,67 @@ func (r *KongUpstreamPolicyReconciler) SetupWithManager(mgr ctrl.Manager) error return err } - if err := mgr.GetCache().IndexField(context.Background(), &corev1.Service{}, upstreamPolicyIndexKey, indexServicesOnUpstreamPolicyAnnotation); err != nil { - return err - } - - if err := mgr.GetCache().IndexField(context.Background(), &gatewayapi.HTTPRoute{}, routeBackendRefServiceNameIndexKey, indexRoutesOnBackendRefServiceName); err != nil { + if err := r.setupIndices(mgr); err != nil { return err } if err := c.Watch( source.Kind(mgr.GetCache(), &corev1.Service{}), - handler.EnqueueRequestsFromMapFunc(r.getUpstreamPolicyForService), - predicate.NewPredicateFuncs(doesServiceUseUpstreamPolicy), + handler.EnqueueRequestsFromMapFunc(r.getUpstreamPolicyForObject), + predicate.NewPredicateFuncs(doesObjectReferUpstreamPolicy), ); err != nil { return err } + if r.KongServiceFacadeEnabled { + if err := c.Watch( + source.Kind(mgr.GetCache(), &incubatorv1alpha1.KongServiceFacade{}), + handler.EnqueueRequestsFromMapFunc(r.getUpstreamPolicyForObject), + predicate.NewPredicateFuncs(doesObjectReferUpstreamPolicy), + ); err != nil { + return err + } + } + return c.Watch( source.Kind(mgr.GetCache(), &kongv1beta1.KongUpstreamPolicy{}), &handler.EnqueueRequestForObject{}, ) } +func (r *KongUpstreamPolicyReconciler) setupIndices(mgr ctrl.Manager) error { + if err := mgr.GetCache().IndexField( + context.Background(), + &corev1.Service{}, + upstreamPolicyIndexKey, + indexServicesOnUpstreamPolicyAnnotation, + ); err != nil { + return fmt.Errorf("failed to index services on annotation %s: %w", kongv1beta1.KongUpstreamPolicyAnnotationKey, err) + } + + if err := mgr.GetCache().IndexField( + context.Background(), + &gatewayapi.HTTPRoute{}, + routeBackendRefServiceNameIndexKey, + indexRoutesOnBackendRefServiceName, + ); err != nil { + return fmt.Errorf("failed to index HTTPRoutes on backendReferences: %w", err) + } + + if r.KongServiceFacadeEnabled { + if err := mgr.GetCache().IndexField( + context.Background(), + &incubatorv1alpha1.KongServiceFacade{}, + upstreamPolicyIndexKey, + indexServiceFacadesOnUpstreamPolicyAnnotation, + ); err != nil { + return fmt.Errorf("failed to index KongServiceFacades on annotation %s: %w", kongv1beta1.KongUpstreamPolicyAnnotationKey, err) + } + } + + return nil +} + // ----------------------------------------------------------------------------- // KongUpstreamPolicy Controller - Indexers // ----------------------------------------------------------------------------- @@ -103,45 +147,57 @@ func indexRoutesOnBackendRefServiceName(o client.Object) []string { return []string{} } - indexes := []string{} + var indexes []string for _, rule := range httpRoute.Spec.Rules { for _, br := range rule.BackendRefs { serviceRef := backendRefToServiceRef(httpRoute.Namespace, br.BackendRef) if serviceRef == "" { continue } - indexes = append(indexes, serviceRef) + indexes = append(indexes, string(serviceRef)) } } return indexes } +// indexServiceFacadesOnUpstreamPolicyAnnotation indexes the KongServiceFacades on the annotation konghq.com/upstream-policy. +func indexServiceFacadesOnUpstreamPolicyAnnotation(o client.Object) []string { + service, ok := o.(*incubatorv1alpha1.KongServiceFacade) + if !ok { + return []string{} + } + if service.Annotations != nil { + if policy, ok := service.Annotations[kongv1beta1.KongUpstreamPolicyAnnotationKey]; ok { + return []string{policy} + } + } + return []string{} +} + // ----------------------------------------------------------------------------- // KongUpstreamPolicy Controller - Watch Predicates // ----------------------------------------------------------------------------- -// getUpstreamPolicyForService enqueue a new reconcile request for the KongUpstreamPolicy -// referenced by a service. -func (r *KongUpstreamPolicyReconciler) getUpstreamPolicyForService(ctx context.Context, obj client.Object) []reconcile.Request { - service, ok := obj.(*corev1.Service) - if !ok { - return nil - } - if service.Annotations == nil { +// getUpstreamPolicyForObject enqueues a new reconcile request for the KongUpstreamPolicy referenced by an object. +func (r *KongUpstreamPolicyReconciler) getUpstreamPolicyForObject(ctx context.Context, obj client.Object) []reconcile.Request { + annotations := obj.GetAnnotations() + if annotations == nil { return nil } - policyName, ok := service.Annotations[kongv1beta1.KongUpstreamPolicyAnnotationKey] + policyName, ok := annotations[kongv1beta1.KongUpstreamPolicyAnnotationKey] if !ok { return nil } kongUpstreamPolicy := &kongv1beta1.KongUpstreamPolicy{} if err := r.Get(ctx, k8stypes.NamespacedName{ - Namespace: service.Namespace, + Namespace: obj.GetNamespace(), Name: policyName, }, kongUpstreamPolicy); err != nil { if !apierrors.IsNotFound(err) { - r.Log.Error(err, "Failed to retrieve KongUpstreamPolicy in watch predicates", "KongUpstreamPolicy", fmt.Sprintf("%s/%s", service.Namespace, policyName)) + r.Log.Error(err, "Failed to retrieve KongUpstreamPolicy in watch predicates", + "KongUpstreamPolicy", fmt.Sprintf("%s/%s", obj.GetNamespace(), policyName), + ) } return []reconcile.Request{} } @@ -149,23 +205,20 @@ func (r *KongUpstreamPolicyReconciler) getUpstreamPolicyForService(ctx context.C return []reconcile.Request{ { NamespacedName: k8stypes.NamespacedName{ - Namespace: service.Namespace, + Namespace: obj.GetNamespace(), Name: policyName, }, }, } } -// doesServiceUseUpstreamPolicy filters out all the services not referencing KongUpstreamPolicies. -func doesServiceUseUpstreamPolicy(obj client.Object) bool { - service, ok := obj.(*corev1.Service) - if !ok { - return false - } - if service.Annotations == nil { +// doesObjectReferUpstreamPolicy filters out all the objects not referencing KongUpstreamPolicies. +func doesObjectReferUpstreamPolicy(obj client.Object) bool { + annotations := obj.GetAnnotations() + if annotations == nil { return false } - _, ok = service.Annotations[kongv1beta1.KongUpstreamPolicyAnnotationKey] + _, ok := annotations[kongv1beta1.KongUpstreamPolicyAnnotationKey] return ok } @@ -177,6 +230,7 @@ func doesServiceUseUpstreamPolicy(obj client.Object) bool { // +kubebuilder:rbac:groups=configuration.konghq.com,resources=kongupstreampolicies/status,verbs=get;update;patch // +kubebuilder:rbac:groups=gateway.networking.k8s.io,resources=httproutes,verbs=get;list;watch // +kubebuilder:rbac:groups="",resources=services,verbs=get;list;watch +// +kubebuilder:rbac:groups=incubator.ingress-controller.konghq.com,resources=kongservicefacades,verbs=get;list;watch // Reconcile processes the watched objects. func (r *KongUpstreamPolicyReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { diff --git a/internal/controllers/configuration/kongupstreampolicy_utils.go b/internal/controllers/configuration/kongupstreampolicy_utils.go index 9728d76b43..6d54e34972 100644 --- a/internal/controllers/configuration/kongupstreampolicy_utils.go +++ b/internal/controllers/configuration/kongupstreampolicy_utils.go @@ -16,63 +16,68 @@ import ( gatewaycontroller "github.com/kong/kubernetes-ingress-controller/v3/internal/controllers/gateway" "github.com/kong/kubernetes-ingress-controller/v3/internal/gatewayapi" kongv1beta1 "github.com/kong/kubernetes-ingress-controller/v3/pkg/apis/configuration/v1beta1" + incubatorv1alpha1 "github.com/kong/kubernetes-ingress-controller/v3/pkg/apis/incubator/v1alpha1" ) const maxNAncestors = 16 -type serviceStatus struct { - service corev1.Service +// upstreamPolicyAncestorKind represents kind of KongUpstreamPolicy ancestor (Service or KongServiceFacade). +type upstreamPolicyAncestorKind string + +const ( + upstreamPolicyAncestorKindService upstreamPolicyAncestorKind = "Service" + upstreamPolicyAncestorKindKongServiceFacade upstreamPolicyAncestorKind = "KongServiceFacade" +) + +// ancestorStatus represents the status of an ancestor (Service or KongServiceFacade). +// A collection of all ancestors' statuses is used to build the KongUpstreamPolicy status. +type ancestorStatus struct { + namespacedName k8stypes.NamespacedName + ancestorKind upstreamPolicyAncestorKind acceptedCondition metav1.Condition + creationTimestamp metav1.Time } -// ----------------------------------------------------------------------------- -// KongUpstreamPolicy Controller - Reconciler Helpers -// ----------------------------------------------------------------------------- +// serviceKey is used as a key for indexing Services by "namespace/name". +type serviceKey string + +// servicesSet is a set of serviceKeys. +type servicesSet map[serviceKey]struct{} // enforceKongUpstreamPolicyStatus gets a list of services (ancestors) along with their desired status and enforce them // in the KongUpstreamPolicy status. -func (r *KongUpstreamPolicyReconciler) enforceKongUpstreamPolicyStatus(ctx context.Context, oldPolicy *kongv1beta1.KongUpstreamPolicy) (bool, error) { - // get all the services that reference this UpstreamPolicy - services := &corev1.ServiceList{} - err := r.List(ctx, services, - client.InNamespace(oldPolicy.Namespace), - client.MatchingFields{ - upstreamPolicyIndexKey: oldPolicy.Name, - }, - ) +func (r *KongUpstreamPolicyReconciler) enforceKongUpstreamPolicyStatus( + ctx context.Context, + oldPolicy *kongv1beta1.KongUpstreamPolicy, +) (bool, error) { + policyNN := k8stypes.NamespacedName{ + Namespace: oldPolicy.Namespace, + Name: oldPolicy.Name, + } + + // Get all objects (Services and KongServiceFacades) that reference this KongUpstreamPolicy. + services, err := r.getServicesReferencingUpstreamPolicy(ctx, policyNN) if err != nil { return false, err } - - // build the desired KongUpstreamPolicy status - servicesStatus, err := r.buildServicesStatus(ctx, k8stypes.NamespacedName{ - Namespace: oldPolicy.Namespace, - Name: oldPolicy.Name, - }, services.Items) + serviceFacades, err := r.maybeGetServiceFacadesReferencingUpstreamPolicy(ctx, policyNN) if err != nil { return false, err } - newPolicyStatus := gatewayapi.PolicyStatus{} - if len(servicesStatus) > 0 { - newPolicyStatus.Ancestors = make([]gatewayapi.PolicyAncestorStatus, 0, len(servicesStatus)) + // Build the status for each ancestor. + ancestorsStatus, err := r.buildAncestorsStatus(ctx, services, serviceFacades) + if err != nil { + return false, err } - for _, ss := range servicesStatus { - newPolicyStatus.Ancestors = append(newPolicyStatus.Ancestors, - gatewayapi.PolicyAncestorStatus{ - AncestorRef: gatewayapi.ParentReference{ - Group: lo.ToPtr(gatewayapi.Group("core")), - Kind: lo.ToPtr(gatewayapi.Kind("Service")), - Namespace: lo.ToPtr(gatewayapi.Namespace(ss.service.Namespace)), - Name: gatewayapi.ObjectName(ss.service.Name), - }, - ControllerName: gatewaycontroller.GetControllerName(), - Conditions: []metav1.Condition{ - ss.acceptedCondition, - }, - }, - ) + + // Build the desired KongUpstreamPolicy status. + newPolicyStatus, err := r.buildPolicyStatus(policyNN, ancestorsStatus) + if err != nil { + return false, err } + + // If the status is not updated, we don't need to patch the KongUpstreamPolicy. if isStatusUpdated := isPolicyStatusUpdated(oldPolicy.Status, newPolicyStatus); !isStatusUpdated { newPolicy := oldPolicy.DeepCopy() newPolicy.Status = newPolicyStatus @@ -81,73 +86,132 @@ func (r *KongUpstreamPolicyReconciler) enforceKongUpstreamPolicyStatus(ctx conte return false, nil } -// indexedServiceStatus is a serviceStatus with an index associated to enable preserving the order of the services. -type indexedServiceStatus struct { - index int - data serviceStatus +func (r *KongUpstreamPolicyReconciler) getServicesReferencingUpstreamPolicy( + ctx context.Context, + upstreamPolicyNN k8stypes.NamespacedName, +) ([]corev1.Service, error) { + services := &corev1.ServiceList{} + err := r.List(ctx, services, + client.InNamespace(upstreamPolicyNN.Namespace), + client.MatchingFields{ + upstreamPolicyIndexKey: upstreamPolicyNN.Name, + }, + ) + if err != nil { + return nil, fmt.Errorf("failed listing Services: %w", err) + } + return services.Items, nil } -// buildServicesStatus creates a list of services with their conditions associated. -func (r *KongUpstreamPolicyReconciler) buildServicesStatus(ctx context.Context, upstreamPolicyNN k8stypes.NamespacedName, services []corev1.Service) ([]serviceStatus, error) { - // sort the services by creationTimestamp - sort.Slice(services, func(i, j int) bool { - return services[i].CreationTimestamp.Before(&services[j].CreationTimestamp) - }) +// maybeGetServiceFacadesReferencingUpstreamPolicy returns a list of KongServiceFacades that reference the given KongUpstreamPolicy. +// Skips the lookup if KongServiceFacade is not enabled. +func (r *KongUpstreamPolicyReconciler) maybeGetServiceFacadesReferencingUpstreamPolicy( + ctx context.Context, + upstreamPolicyNN k8stypes.NamespacedName, +) ([]incubatorv1alpha1.KongServiceFacade, error) { + if !r.KongServiceFacadeEnabled { + // KongServiceFacade is not enabled, so we don't need to check for it. + return nil, nil + } + serviceFacades := &incubatorv1alpha1.KongServiceFacadeList{} + err := r.List(ctx, serviceFacades, + client.InNamespace(upstreamPolicyNN.Namespace), + client.MatchingFields{ + upstreamPolicyIndexKey: upstreamPolicyNN.Name, + }, + ) + if err != nil { + return nil, fmt.Errorf("failed listing KongServiceFacades: %w", err) + } + return serviceFacades.Items, nil +} - // prepare a service mapping to be used in subsequent operations - mappedServices := make(map[string]indexedServiceStatus) - for i, service := range services { - if i < maxNAncestors { - acceptedCondition := metav1.Condition{ - Type: string(gatewayapi.PolicyConditionAccepted), - Status: metav1.ConditionTrue, - Reason: string(gatewayapi.PolicyReasonAccepted), - LastTransitionTime: metav1.Now(), - } - mappedServices[buildServiceReference(service.Namespace, service.Name)] = indexedServiceStatus{ - index: i, - data: serviceStatus{ - service: service, - acceptedCondition: acceptedCondition, - }, - } - } else { - r.Log.Info(fmt.Sprintf("kongUpstreamPolicy %s/%s status has already %d ancestors, cannot set service %s/%s as an ancestor in the status", - upstreamPolicyNN.Namespace, - upstreamPolicyNN.Name, - maxNAncestors, - service.Namespace, - service.Name)) +// buildAncestorsStatus creates a list of services with their conditions associated. +func (r *KongUpstreamPolicyReconciler) buildAncestorsStatus( + ctx context.Context, + services []corev1.Service, + serviceFacades []incubatorv1alpha1.KongServiceFacade, +) ([]ancestorStatus, error) { + // Check if any Services have conflicts. We do not verify conflicts for KongServiceFacades as there's + // no scenario in which they would have one. + conflictedServices, err := r.getConflictedServices(ctx, services) + if err != nil { + return nil, err + } + + // Prepare conditions. + acceptedCondition := metav1.Condition{ + Type: string(gatewayapi.PolicyConditionAccepted), + Status: metav1.ConditionTrue, + Reason: string(gatewayapi.PolicyReasonAccepted), + LastTransitionTime: metav1.Now(), + } + conflictedCondition := metav1.Condition{ + Type: string(gatewayapi.PolicyConditionAccepted), + Status: metav1.ConditionFalse, + Reason: string(gatewayapi.PolicyReasonConflicted), + LastTransitionTime: metav1.Now(), + } + + // Build the status for each ancestor (Services and KongServiceFacades). + ancestorsStatus := make([]ancestorStatus, 0, len(services)+len(serviceFacades)) + for _, service := range services { + condition := acceptedCondition + if _, isConflicted := conflictedServices[buildServiceReference(service.Namespace, service.Name)]; isConflicted { + // If the service is conflicted, we will use the conflictedCondition. + condition = conflictedCondition } + ancestorsStatus = append(ancestorsStatus, ancestorStatus{ + namespacedName: k8stypes.NamespacedName{ + Namespace: service.Namespace, + Name: service.Name, + }, + ancestorKind: upstreamPolicyAncestorKindService, + acceptedCondition: condition, + }) } + for _, serviceFacade := range serviceFacades { + ancestorsStatus = append(ancestorsStatus, ancestorStatus{ + namespacedName: k8stypes.NamespacedName{ + Namespace: serviceFacade.Namespace, + Name: serviceFacade.Name, + }, + ancestorKind: upstreamPolicyAncestorKindKongServiceFacade, + acceptedCondition: acceptedCondition, + }) + } + + return ancestorsStatus, nil +} - for serviceKey, serviceStatus := range mappedServices { +// getConflictedServices returns a set of services that have conflicts. +func (r *KongUpstreamPolicyReconciler) getConflictedServices(ctx context.Context, services []corev1.Service) (servicesSet, error) { + // Prepare a mapping for efficient lookups if a Service uses this KongUpstreamPolicy. + upstreamPolicyServices := make(servicesSet) + for _, service := range services { + upstreamPolicyServices[buildServiceReference(service.Namespace, service.Name)] = struct{}{} + } + + conflictedServices := make(servicesSet) + for serviceKey := range upstreamPolicyServices { // We fetch all the HTTPRoutes that reference this service. httpRoutes := &gatewayapi.HTTPRouteList{} err := r.List(ctx, httpRoutes, client.MatchingFields{ - routeBackendRefServiceNameIndexKey: serviceKey, + routeBackendRefServiceNameIndexKey: string(serviceKey), }, ) if err != nil { return nil, err } - hasConflict := lo.ContainsBy(httpRoutes.Items, func(httpRoute gatewayapi.HTTPRoute) bool { - return httpRouteHasUpstreamPolicyConflictedBackendRefsWithService(httpRoute, mappedServices, serviceKey) + return httpRouteHasUpstreamPolicyConflictedBackendRefsWithService(httpRoute, upstreamPolicyServices, serviceKey) }) if hasConflict { - serviceStatus.data.acceptedCondition.Status = metav1.ConditionFalse - serviceStatus.data.acceptedCondition.Reason = string(gatewayapi.PolicyReasonConflicted) - mappedServices[serviceKey] = serviceStatus + conflictedServices[serviceKey] = struct{}{} } } - - servicesStatus := make([]serviceStatus, len(mappedServices)) - for _, ms := range mappedServices { - servicesStatus[ms.index] = ms.data - } - return servicesStatus, nil + return conflictedServices, nil } // httpRouteHasUpstreamPolicyConflictedBackendRefsWithService checks if there's any HTTPRoute's rule that uses multiple backendRefs @@ -155,8 +219,8 @@ func (r *KongUpstreamPolicyReconciler) buildServicesStatus(ctx context.Context, // If so, that means that we have a conflict because we cannot apply multiple KongUpstreamPolicy to the same Kong Service. func httpRouteHasUpstreamPolicyConflictedBackendRefsWithService( httpRoute gatewayapi.HTTPRoute, - upstreamPolicyServices map[string]indexedServiceStatus, - serviceKey string, + upstreamPolicyServices servicesSet, + serviceKey serviceKey, ) bool { backendRefsUsedWithThisService := getAllBackendRefsUsedWithService(httpRoute, serviceKey) hasAnyBackendRefNotUsingSameUpstreamPolicy := lo.ContainsBy(backendRefsUsedWithThisService, func(br gatewayapi.HTTPBackendRef) bool { @@ -172,7 +236,7 @@ func httpRouteHasUpstreamPolicyConflictedBackendRefsWithService( } // getAllBackendRefsUsedWithService returns HTTPRoute's backendRefs that use the given service (excluding the given service). -func getAllBackendRefsUsedWithService(httpRoute gatewayapi.HTTPRoute, serviceKey string) []gatewayapi.HTTPBackendRef { +func getAllBackendRefsUsedWithService(httpRoute gatewayapi.HTTPRoute, serviceKey serviceKey) []gatewayapi.HTTPBackendRef { var backendRefs []gatewayapi.HTTPBackendRef for _, rule := range httpRoute.Spec.Rules { // We will look for a backendRef that matches the given service and keep its index if found. @@ -198,9 +262,67 @@ func getAllBackendRefsUsedWithService(httpRoute gatewayapi.HTTPRoute, serviceKey return backendRefs } -// ----------------------------------------------------------------------------- -// KongUpstreamPolicy Controller - Helpers -// ----------------------------------------------------------------------------- +func (r *KongUpstreamPolicyReconciler) buildPolicyStatus( + upstreamPolicyNN k8stypes.NamespacedName, + ancestorsStatus []ancestorStatus, +) (gatewayapi.PolicyStatus, error) { + // Sort the ancestors by creation timestamp and keep only the oldest ones. + sort.Slice(ancestorsStatus, func(i, j int) bool { + return ancestorsStatus[i].creationTimestamp.Before(&ancestorsStatus[j].creationTimestamp) + }) + if len(ancestorsStatus) > maxNAncestors { + r.Log.Info("status has too many ancestors, the newest ones will be ignored", + "KongUpstreamPolicy", upstreamPolicyNN.String(), + "ancestorsCount", len(ancestorsStatus), + "maxAllowedAncestors", maxNAncestors, + ) + ancestorsStatus = ancestorsStatus[:maxNAncestors] + } + + // Populate the KongUpstreamPolicy status with the ancestors' statuses. + policyStatus := gatewayapi.PolicyStatus{} + if len(ancestorsStatus) > 0 { + policyStatus.Ancestors = make([]gatewayapi.PolicyAncestorStatus, 0, len(ancestorsStatus)) + } + for _, ss := range ancestorsStatus { + ancestorRef, err := ancestorRef(ss.namespacedName, ss.ancestorKind) + if err != nil { + return gatewayapi.PolicyStatus{}, fmt.Errorf("failed to build ancestor reference: %w", err) + } + policyStatus.Ancestors = append(policyStatus.Ancestors, + gatewayapi.PolicyAncestorStatus{ + AncestorRef: ancestorRef, + ControllerName: gatewaycontroller.GetControllerName(), + Conditions: []metav1.Condition{ + ss.acceptedCondition, + }, + }, + ) + } + + return policyStatus, nil +} + +func ancestorRef(nn k8stypes.NamespacedName, kind upstreamPolicyAncestorKind) (gatewayapi.ParentReference, error) { + switch kind { + case upstreamPolicyAncestorKindService: + return gatewayapi.ParentReference{ + Group: lo.ToPtr(gatewayapi.Group("core")), + Kind: lo.ToPtr(gatewayapi.Kind("Service")), + Namespace: lo.ToPtr(gatewayapi.Namespace(nn.Namespace)), + Name: gatewayapi.ObjectName(nn.Name), + }, nil + case upstreamPolicyAncestorKindKongServiceFacade: + return gatewayapi.ParentReference{ + Group: lo.ToPtr(gatewayapi.Group(incubatorv1alpha1.GroupVersion.Group)), + Kind: lo.ToPtr(gatewayapi.Kind(incubatorv1alpha1.KongServiceFacadeKind)), + Namespace: lo.ToPtr(gatewayapi.Namespace(nn.Namespace)), + Name: gatewayapi.ObjectName(nn.Name), + }, nil + } + + return gatewayapi.ParentReference{}, fmt.Errorf("unknown ancestor kind %q", kind) +} func isPolicyStatusUpdated(oldStatus, newStatus gatewayapi.PolicyStatus) bool { if len(oldStatus.Ancestors) != len(newStatus.Ancestors) { @@ -232,11 +354,8 @@ func isPolicyStatusUpdated(oldStatus, newStatus gatewayapi.PolicyStatus) bool { return true } -func backendRefToServiceRef(routeNamespace string, br gatewayapi.BackendRef) string { - if br.Group != nil && *br.Group != "" && *br.Group != "core" { - return "" - } - if br.Kind != nil && *br.Kind != "" && *br.Kind != "Service" { +func backendRefToServiceRef(routeNamespace string, br gatewayapi.BackendRef) serviceKey { + if !isSupportedHTTPRouteBackendRef(br) { return "" } namespace := routeNamespace @@ -246,6 +365,13 @@ func backendRefToServiceRef(routeNamespace string, br gatewayapi.BackendRef) str return buildServiceReference(namespace, string(br.Name)) } -func buildServiceReference(namespace, name string) string { - return fmt.Sprintf("%s/%s", namespace, name) +func buildServiceReference(namespace, name string) serviceKey { + return serviceKey(fmt.Sprintf("%s/%s", namespace, name)) +} + +func isSupportedHTTPRouteBackendRef(br gatewayapi.BackendRef) bool { + groupIsCoreOrNil := br.Group == nil || *br.Group == "core" + kindIsServiceOrNil := br.Kind == nil || *br.Kind == "Service" + // We only support core Services. + return groupIsCoreOrNil && kindIsServiceOrNil } diff --git a/internal/controllers/configuration/kongupstreampolicy_utils_test.go b/internal/controllers/configuration/kongupstreampolicy_utils_test.go index 6e7b94367c..771108b39c 100644 --- a/internal/controllers/configuration/kongupstreampolicy_utils_test.go +++ b/internal/controllers/configuration/kongupstreampolicy_utils_test.go @@ -2,9 +2,11 @@ package configuration import ( "context" + "fmt" "testing" "time" + "github.com/go-logr/logr" "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" "github.com/samber/lo" @@ -21,6 +23,7 @@ import ( "github.com/kong/kubernetes-ingress-controller/v3/internal/manager/scheme" "github.com/kong/kubernetes-ingress-controller/v3/internal/util/builder" kongv1beta1 "github.com/kong/kubernetes-ingress-controller/v3/pkg/apis/configuration/v1beta1" + incubatorv1alpha1 "github.com/kong/kubernetes-ingress-controller/v3/pkg/apis/incubator/v1alpha1" ) func TestEnforceKongUpstreamPolicyStatus(t *testing.T) { @@ -437,6 +440,91 @@ func TestEnforceKongUpstreamPolicyStatus(t *testing.T) { }, updated: true, }, + { + name: "service and kong service facade referencing the same policy, accepted", + kongUpstreamPolicy: kongv1beta1.KongUpstreamPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: policyName, + Namespace: testNamespace, + }, + }, + inputObjects: []client.Object{ + &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "svc-1", + Namespace: testNamespace, + Annotations: map[string]string{ + kongv1beta1.KongUpstreamPolicyAnnotationKey: policyName, + }, + CreationTimestamp: metav1.Now(), + }, + }, + &incubatorv1alpha1.KongServiceFacade{ + ObjectMeta: metav1.ObjectMeta{ + Name: "svc-facade-1", + Namespace: testNamespace, + Annotations: map[string]string{ + kongv1beta1.KongUpstreamPolicyAnnotationKey: policyName, + }, + CreationTimestamp: metav1.Time{ + Time: metav1.Now().Add(10 * time.Second), + }, + }, + }, + &gatewayapi.HTTPRoute{ + ObjectMeta: metav1.ObjectMeta{ + Name: "httpRoute", + Namespace: testNamespace, + }, + Spec: gatewayapi.HTTPRouteSpec{ + Rules: []gatewayapi.HTTPRouteRule{ + { + BackendRefs: []gatewayapi.HTTPBackendRef{ + builder.NewHTTPBackendRef("svc-1").Build(), + }, + }, + }, + }, + }, + }, + expectedKongUpstreamPolicyStatus: gatewayapi.PolicyStatus{ + Ancestors: []gatewayapi.PolicyAncestorStatus{ + { + AncestorRef: gatewayapi.ParentReference{ + Group: lo.ToPtr(gatewayapi.Group("core")), + Kind: lo.ToPtr(gatewayapi.Kind("Service")), + Namespace: lo.ToPtr(gatewayapi.Namespace(testNamespace)), + Name: gatewayapi.ObjectName("svc-1"), + }, + ControllerName: gatewaycontroller.GetControllerName(), + Conditions: []metav1.Condition{ + { + Type: string(gatewayapi.PolicyConditionAccepted), + Status: metav1.ConditionTrue, + Reason: string(gatewayapi.PolicyReasonAccepted), + }, + }, + }, + { + AncestorRef: gatewayapi.ParentReference{ + Group: lo.ToPtr(gatewayapi.Group(incubatorv1alpha1.GroupVersion.Group)), + Kind: lo.ToPtr(gatewayapi.Kind(incubatorv1alpha1.KongServiceFacadeKind)), + Namespace: lo.ToPtr(gatewayapi.Namespace(testNamespace)), + Name: gatewayapi.ObjectName("svc-facade-1"), + }, + ControllerName: gatewaycontroller.GetControllerName(), + Conditions: []metav1.Condition{ + { + Type: string(gatewayapi.PolicyConditionAccepted), + Status: metav1.ConditionTrue, + Reason: string(gatewayapi.PolicyReasonAccepted), + }, + }, + }, + }, + }, + updated: true, + }, } for _, tc := range testCases { @@ -450,10 +538,12 @@ func TestEnforceKongUpstreamPolicyStatus(t *testing.T) { WithStatusSubresource(objectsToAdd...). WithIndex(&corev1.Service{}, upstreamPolicyIndexKey, indexServicesOnUpstreamPolicyAnnotation). WithIndex(&gatewayapi.HTTPRoute{}, routeBackendRefServiceNameIndexKey, indexRoutesOnBackendRefServiceName). + WithIndex(&incubatorv1alpha1.KongServiceFacade{}, upstreamPolicyIndexKey, indexServiceFacadesOnUpstreamPolicyAnnotation). Build() reconciler := KongUpstreamPolicyReconciler{ - Client: fakeClient, + Client: fakeClient, + KongServiceFacadeEnabled: true, } updated, err := reconciler.enforceKongUpstreamPolicyStatus(context.TODO(), &tc.kongUpstreamPolicy) @@ -474,14 +564,14 @@ func TestHttpRouteHasUpstreamPolicyConflictedBackendRefsWithService(t *testing.T testCases := []struct { name string httpRoute gatewayapi.HTTPRoute - upstreamPolicyServices map[string]indexedServiceStatus - serviceRef string + upstreamPolicyServices servicesSet + serviceRef serviceKey expected bool }{ { name: "service not referenced by the http route", serviceRef: "default/svc-not-referenced", - upstreamPolicyServices: map[string]indexedServiceStatus{ + upstreamPolicyServices: servicesSet{ "default/svc-not-referenced": {}, }, httpRoute: gatewayapi.HTTPRoute{ @@ -504,7 +594,7 @@ func TestHttpRouteHasUpstreamPolicyConflictedBackendRefsWithService(t *testing.T { name: "service referenced by the http route alone in a rule", serviceRef: "default/svc-1", - upstreamPolicyServices: map[string]indexedServiceStatus{ + upstreamPolicyServices: servicesSet{ "default/svc-1": {}, }, httpRoute: gatewayapi.HTTPRoute{ @@ -527,7 +617,7 @@ func TestHttpRouteHasUpstreamPolicyConflictedBackendRefsWithService(t *testing.T { name: "service referenced by the http route alone in a rule while there's another rule with a service not using the same policy", serviceRef: "default/svc-1", - upstreamPolicyServices: map[string]indexedServiceStatus{ + upstreamPolicyServices: servicesSet{ "default/svc-1": {}, }, httpRoute: gatewayapi.HTTPRoute{ @@ -555,7 +645,7 @@ func TestHttpRouteHasUpstreamPolicyConflictedBackendRefsWithService(t *testing.T { name: "service referenced by the http route in a rule together with another service using the same policy", serviceRef: "default/svc-1", - upstreamPolicyServices: map[string]indexedServiceStatus{ + upstreamPolicyServices: servicesSet{ "default/svc-1": {}, "default/svc-2": {}, }, @@ -580,7 +670,7 @@ func TestHttpRouteHasUpstreamPolicyConflictedBackendRefsWithService(t *testing.T { name: "service referenced by the http route in a rule together with another service not using the same policy", serviceRef: "default/svc-1", - upstreamPolicyServices: map[string]indexedServiceStatus{ + upstreamPolicyServices: servicesSet{ "default/svc-1": {}, }, httpRoute: gatewayapi.HTTPRoute{ @@ -612,3 +702,182 @@ func TestHttpRouteHasUpstreamPolicyConflictedBackendRefsWithService(t *testing.T }) } } + +func TestBuildPolicyStatus(t *testing.T) { + acceptedCondition := metav1.Condition{ + Type: string(gatewayapi.PolicyConditionAccepted), + Status: metav1.ConditionTrue, + Reason: string(gatewayapi.PolicyReasonAccepted), + } + + serviceStatus := func(name string, creationTimestamp time.Time) ancestorStatus { + return ancestorStatus{ + namespacedName: k8stypes.NamespacedName{Namespace: "default", Name: name}, + ancestorKind: upstreamPolicyAncestorKindService, + acceptedCondition: acceptedCondition, + creationTimestamp: metav1.NewTime(creationTimestamp), + } + } + serviceFacadeStatus := func(name string, creationTimestamp time.Time) ancestorStatus { + return ancestorStatus{ + namespacedName: k8stypes.NamespacedName{Namespace: "default", Name: name}, + ancestorKind: upstreamPolicyAncestorKindKongServiceFacade, + acceptedCondition: acceptedCondition, + creationTimestamp: metav1.NewTime(creationTimestamp), + } + } + + serviceExpectedPolicyAncestorStatus := func(name string) gatewayapi.PolicyAncestorStatus { + return gatewayapi.PolicyAncestorStatus{ + AncestorRef: gatewayapi.ParentReference{ + Group: lo.ToPtr(gatewayapi.Group("core")), + Kind: lo.ToPtr(gatewayapi.Kind("Service")), + Namespace: lo.ToPtr(gatewayapi.Namespace("default")), + Name: gatewayapi.ObjectName(name), + }, + ControllerName: gatewaycontroller.GetControllerName(), + Conditions: []metav1.Condition{ + acceptedCondition, + }, + } + } + serviceFacadeExpectedPolicyAncestorStatus := func(name string) gatewayapi.PolicyAncestorStatus { + return gatewayapi.PolicyAncestorStatus{ + AncestorRef: gatewayapi.ParentReference{ + Group: lo.ToPtr(gatewayapi.Group(incubatorv1alpha1.GroupVersion.Group)), + Kind: lo.ToPtr(gatewayapi.Kind(incubatorv1alpha1.KongServiceFacadeKind)), + Namespace: lo.ToPtr(gatewayapi.Namespace("default")), + Name: gatewayapi.ObjectName(name), + }, + ControllerName: gatewaycontroller.GetControllerName(), + Conditions: []metav1.Condition{ + acceptedCondition, + }, + } + } + + now := time.Now() + testCases := []struct { + name string + ancestorsStatuses []ancestorStatus + expected gatewayapi.PolicyStatus + }{ + { + name: "all ordered by creation timestamp (oldest first)", + ancestorsStatuses: []ancestorStatus{ + serviceStatus("svc-1", now.Add(4*time.Second)), + serviceStatus("svc-2", now.Add(3*time.Second)), + serviceFacadeStatus("svc-facade-1", now.Add(2*time.Second)), + serviceFacadeStatus("svc-facade-2", now.Add(1*time.Second)), + }, + expected: gatewayapi.PolicyStatus{ + Ancestors: []gatewayapi.PolicyAncestorStatus{ + serviceFacadeExpectedPolicyAncestorStatus("svc-facade-2"), + serviceFacadeExpectedPolicyAncestorStatus("svc-facade-1"), + serviceExpectedPolicyAncestorStatus("svc-2"), + serviceExpectedPolicyAncestorStatus("svc-1"), + }, + }, + }, + { + name: "more ancestors than allowed - keeps only maxNAncestors oldest ones", + ancestorsStatuses: func() []ancestorStatus { + var ancestors []ancestorStatus + for i := 0; i < maxNAncestors+2; i++ { + ancestors = append(ancestors, serviceStatus(fmt.Sprintf("svc-%d", i), now.Add(time.Duration(i)*time.Second))) + } + return ancestors + }(), + expected: gatewayapi.PolicyStatus{ + Ancestors: func() []gatewayapi.PolicyAncestorStatus { + var ancestors []gatewayapi.PolicyAncestorStatus + for i := 0; i < maxNAncestors; i++ { + ancestors = append(ancestors, serviceExpectedPolicyAncestorStatus(fmt.Sprintf("svc-%d", i))) + } + return ancestors + }(), + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + r := &KongUpstreamPolicyReconciler{Log: logr.Discard()} + policyStatus, err := r.buildPolicyStatus(k8stypes.NamespacedName{Namespace: "default", Name: "test-policy"}, tc.ancestorsStatuses) + require.NoError(t, err) + require.Equal(t, tc.expected, policyStatus) + }) + } +} + +func TestIsSupportedHTTPRouteBackendRef(t *testing.T) { + testCases := []struct { + name string + backendRef gatewayapi.BackendObjectReference + expected bool + }{ + { + name: "core service backend ref", + backendRef: gatewayapi.BackendObjectReference{ + Group: lo.ToPtr(gatewayapi.Group("core")), + Kind: lo.ToPtr(gatewayapi.Kind("Service")), + }, + expected: true, + }, + { + name: "core service with nil group", + backendRef: gatewayapi.BackendObjectReference{ + Kind: lo.ToPtr(gatewayapi.Kind("Service")), + }, + expected: true, + }, + { + name: "core service with nil kind", + backendRef: gatewayapi.BackendObjectReference{ + Group: lo.ToPtr(gatewayapi.Group("core")), + }, + expected: true, + }, + { + name: "core service with nil group and kind", + backendRef: gatewayapi.BackendObjectReference{}, + expected: true, + }, + { + name: "core group unsupported kind", + backendRef: gatewayapi.BackendObjectReference{ + Group: lo.ToPtr(gatewayapi.Group("core")), + Kind: lo.ToPtr(gatewayapi.Kind("UnsupportedKind")), + }, + expected: false, + }, + { + name: "service unsupported group", + backendRef: gatewayapi.BackendObjectReference{ + Group: lo.ToPtr(gatewayapi.Group("unsupported")), + Kind: lo.ToPtr(gatewayapi.Kind("Service")), + }, + expected: false, + }, + { + name: "unsupported group", + backendRef: gatewayapi.BackendObjectReference{ + Group: lo.ToPtr(gatewayapi.Group("unsupported")), + }, + expected: false, + }, + { + name: "unsupported kind", + backendRef: gatewayapi.BackendObjectReference{ + Kind: lo.ToPtr(gatewayapi.Kind("UnsupportedKind")), + }, + expected: false, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + require.Equal(t, tc.expected, isSupportedHTTPRouteBackendRef(gatewayapi.BackendRef{BackendObjectReference: tc.backendRef})) + }) + } +} diff --git a/internal/manager/controllerdef.go b/internal/manager/controllerdef.go index ed201983ea..0eaaadcd6d 100644 --- a/internal/manager/controllerdef.go +++ b/internal/manager/controllerdef.go @@ -264,11 +264,12 @@ func setupControllers( }, }, Controller: &configuration.KongUpstreamPolicyReconciler{ - Client: mgr.GetClient(), - Log: ctrl.LoggerFrom(ctx).WithName("controllers").WithName("KongUpstreamPolicy"), - Scheme: mgr.GetScheme(), - DataplaneClient: dataplaneClient, - CacheSyncTimeout: c.CacheSyncTimeout, + Client: mgr.GetClient(), + Log: ctrl.LoggerFrom(ctx).WithName("controllers").WithName("KongUpstreamPolicy"), + Scheme: mgr.GetScheme(), + DataplaneClient: dataplaneClient, + CacheSyncTimeout: c.CacheSyncTimeout, + KongServiceFacadeEnabled: featureGates.Enabled(featuregates.KongServiceFacade) && c.KongServiceFacadeEnabled, }, }, },