From f05c502b77339fde265f075e1b89319db056ae94 Mon Sep 17 00:00:00 2001 From: Bartosz Majsak Date: Thu, 29 Aug 2024 15:36:36 +0200 Subject: [PATCH] chore: simplifies resource loading (#68) Route resources loader has many unused parameters being part of the interface definition. This change streamlines the abstraction to only rely on a route data structure and type based on which particular resources are loaded. Data structure holding routing information has been renamed to RoutingData and new constructor func has been introduced to encapsulate logic of creating service-related fields. --- .../routingctrl/reconcile_resources.go | 16 ++----- pkg/metadata/labels/funcs.go | 7 ++- pkg/routing/routing.go | 6 +-- pkg/routing/routing_test.go | 45 ++++++++++++------- pkg/spi/types.go | 22 ++++++--- 5 files changed, 55 insertions(+), 41 deletions(-) diff --git a/controllers/routingctrl/reconcile_resources.go b/controllers/routingctrl/reconcile_resources.go index 31286c0..0b5df53 100644 --- a/controllers/routingctrl/reconcile_resources.go +++ b/controllers/routingctrl/reconcile_resources.go @@ -15,7 +15,6 @@ import ( "github.com/opendatahub-io/odh-platform/pkg/unstruct" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" - "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" ) @@ -69,22 +68,13 @@ func (r *Controller) createRoutingResources(ctx context.Context, target *unstruc func (r *Controller) exportService(ctx context.Context, target *unstructured.Unstructured, exportedSvc *corev1.Service, domain string) error { exportModes := r.extractExportModes(target) - templateData := spi.RoutingTemplateData{ - PlatformRoutingConfiguration: r.config, - PublicServiceName: exportedSvc.GetName() + "-" + exportedSvc.GetNamespace(), - ServiceName: exportedSvc.GetName(), - ServiceNamespace: exportedSvc.GetNamespace(), - ServiceTargetPort: exportedSvc.Spec.Ports[0].TargetPort.String(), - Domain: domain, - } + templateData := spi.NewRoutingData(r.config, exportedSvc, domain) // To establish ownership for watched component ownershipLabels := append(labels.AsOwner(target), labels.AppManagedBy("odh-routing-controller")) - targetKey := client.ObjectKeyFromObject(target) - for _, exportMode := range exportModes { - resources, err := r.templateLoader.Load(ctx, exportMode, targetKey, templateData) + resources, err := r.templateLoader.Load(templateData, exportMode) if err != nil { return fmt.Errorf("could not load templates for type %s: %w", exportMode, err) } @@ -98,7 +88,7 @@ func (r *Controller) exportService(ctx context.Context, target *unstructured.Uns return r.propagateHostsToWatchedCR(target, templateData) } -func (r *Controller) propagateHostsToWatchedCR(target *unstructured.Unstructured, data spi.RoutingTemplateData) error { +func (r *Controller) propagateHostsToWatchedCR(target *unstructured.Unstructured, data *spi.RoutingData) error { exportModes := r.extractExportModes(target) // Remove all existing routing addresses diff --git a/pkg/metadata/labels/funcs.go b/pkg/metadata/labels/funcs.go index 1451c95..411d7bf 100644 --- a/pkg/metadata/labels/funcs.go +++ b/pkg/metadata/labels/funcs.go @@ -38,7 +38,12 @@ func StandardLabelsFrom(source metav1.Object) []metadata.Option { // AppendStandardLabelsFrom appends standard labels found in source object but only // when they are not already present in the target object. func AppendStandardLabelsFrom(source metav1.Object) *LabelAppender { - return &LabelAppender{labels: standardLabelsFrom(source)} + return AppendLabels(standardLabelsFrom(source)...) +} + +// AppendLabels appends provided labels to the target object but only when they are not already present. +func AppendLabels(labels ...Label) *LabelAppender { + return &LabelAppender{labels: labels} } // MatchingLabels returns a client.MatchingLabels selector for the provided labels. diff --git a/pkg/routing/routing.go b/pkg/routing/routing.go index f7d4674..748aee1 100644 --- a/pkg/routing/routing.go +++ b/pkg/routing/routing.go @@ -2,7 +2,6 @@ package routing import ( "bytes" - "context" _ "embed" // needed for go:embed directive "fmt" "strings" @@ -11,7 +10,6 @@ import ( "github.com/opendatahub-io/odh-platform/pkg/schema" "github.com/opendatahub-io/odh-platform/pkg/spi" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" - "k8s.io/apimachinery/pkg/types" ) //go:embed template/routing_public.yaml @@ -27,7 +25,7 @@ func NewStaticTemplateLoader() spi.RoutingTemplateLoader { return &staticTemplateLoader{} } -func (s *staticTemplateLoader) Load(_ context.Context, routeType spi.RouteType, key types.NamespacedName, data spi.RoutingTemplateData) ([]*unstructured.Unstructured, error) { +func (s *staticTemplateLoader) Load(data *spi.RoutingData, routeType spi.RouteType) ([]*unstructured.Unstructured, error) { var resources []*unstructured.Unstructured var templateContent []byte @@ -60,7 +58,7 @@ func (s *staticTemplateLoader) Load(_ context.Context, routeType spi.RouteType, return resources, nil } -func (s *staticTemplateLoader) resolveTemplate(tmpl []byte, data spi.RoutingTemplateData) ([]byte, error) { +func (s *staticTemplateLoader) resolveTemplate(tmpl []byte, data *spi.RoutingData) ([]byte, error) { engine, err := template.New("routing").Parse(string(tmpl)) if err != nil { return []byte{}, fmt.Errorf("could not create template engine: %w", err) diff --git a/pkg/routing/routing_test.go b/pkg/routing/routing_test.go index 8031746..608d844 100644 --- a/pkg/routing/routing_test.go +++ b/pkg/routing/routing_test.go @@ -1,40 +1,53 @@ package routing_test import ( - "context" - . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" "github.com/opendatahub-io/odh-platform/pkg/routing" "github.com/opendatahub-io/odh-platform/pkg/spi" "github.com/opendatahub-io/odh-platform/test" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" - "k8s.io/apimachinery/pkg/types" + "k8s.io/utils/ptr" ) var _ = Describe("Resource functions", test.Unit(), func() { Context("Template Loader", func() { - data := spi.RoutingTemplateData{ - PlatformRoutingConfiguration: spi.PlatformRoutingConfiguration{ - GatewayNamespace: "opendatahub", - IngressSelectorLabel: "istio", - IngressSelectorValue: "rhoai-gateway", - IngressService: "rhoai-router-ingress", - }, - PublicServiceName: "registry-office", - ServiceName: "registry", - ServiceNamespace: "office", - Domain: "app-crc.testing", + config := spi.PlatformRoutingConfiguration{ + GatewayNamespace: "opendatahub", + IngressSelectorLabel: "istio", + IngressSelectorValue: "rhoai-gateway", + IngressService: "rhoai-router-ingress", } + data := spi.NewRoutingData(config, + &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "registry", + Namespace: "office", + }, + Spec: corev1.ServiceSpec{ + Ports: []corev1.ServicePort{ + { + Name: "http-api", + Port: 80, + AppProtocol: ptr.To("http"), + }, + }, + }, + }, + "app-crc.testing", + ) + It("should load public resources", func() { // given // data^ // when - res, err := routing.NewStaticTemplateLoader().Load(context.Background(), spi.PublicRoute, types.NamespacedName{}, data) + res, err := routing.NewStaticTemplateLoader().Load(data, spi.PublicRoute) // then Expect(err).ShouldNot(HaveOccurred()) @@ -46,7 +59,7 @@ var _ = Describe("Resource functions", test.Unit(), func() { // data^ // when - res, err := routing.NewStaticTemplateLoader().Load(context.Background(), spi.ExternalRoute, types.NamespacedName{}, data) + res, err := routing.NewStaticTemplateLoader().Load(data, spi.ExternalRoute) // then Expect(err).ShouldNot(HaveOccurred()) diff --git a/pkg/spi/types.go b/pkg/spi/types.go index b9b1b50..182c337 100644 --- a/pkg/spi/types.go +++ b/pkg/spi/types.go @@ -9,6 +9,7 @@ import ( authorinov1beta2 "github.com/kuadrant/authorino/api/v1beta2" "github.com/opendatahub-io/odh-platform/pkg/platform" + corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/types" ) @@ -130,8 +131,7 @@ type PlatformRoutingConfiguration struct { GatewayNamespace string } -// TODO(mvp) revise the stuct name - is it only for templates? -type RoutingTemplateData struct { +type RoutingData struct { PlatformRoutingConfiguration PublicServiceName string // [service-name]-[service-namespace] @@ -143,10 +143,18 @@ type RoutingTemplateData struct { Domain string } -// RoutingTemplateLoader provides a way to differentiate the Route template used based on -// - RouteType -// - Namespace / Resource name -// - Loader source +func NewRoutingData(config PlatformRoutingConfiguration, svc *corev1.Service, domain string) *RoutingData { + return &RoutingData{ + PlatformRoutingConfiguration: config, + PublicServiceName: svc.GetName() + "-" + svc.GetNamespace(), + ServiceName: svc.GetName(), + ServiceNamespace: svc.GetNamespace(), + ServiceTargetPort: svc.Spec.Ports[0].TargetPort.String(), + Domain: domain, + } +} + +// RoutingTemplateLoader provides a way to differentiate the Route resource templates used based on its types. type RoutingTemplateLoader interface { - Load(ctx context.Context, routeType RouteType, key types.NamespacedName, data RoutingTemplateData) ([]*unstructured.Unstructured, error) + Load(data *RoutingData, routeType RouteType) ([]*unstructured.Unstructured, error) }