Skip to content

Commit

Permalink
refactor: Update routing export mode annotations (#69)
Browse files Browse the repository at this point in the history 
- Replace single "routing.opendatahub.io/export-mode" annotation with
separate annotations for each mode (e.g.,
"routing.opendatahub.io/export-mode-external")
- Modify test suite to reflect new annotation structure


Closes https://issues.redhat.com/browse/RHOAIENG-12080

---------

Co-authored-by: Bartosz Majsak <[email protected]>
  • Loading branch information
@cam-garrison @bartoszmajsak
cam-garrison and bartoszmajsak authored Sep 4, 2024
1 parent d84c4c6 commit 2f7f715
Show file tree
Hide file tree
Showing 5 changed files with 135 additions and 81 deletions.
83 changes: 29 additions & 54 deletions controllers/routingctrl/controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import (

. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
"github.com/opendatahub-io/odh-platform/pkg/metadata"
"github.com/opendatahub-io/odh-platform/pkg/metadata/annotations"
"github.com/opendatahub-io/odh-platform/test"
. "github.com/opendatahub-io/odh-platform/test/matchers"
Expand Down Expand Up @@ -88,8 +87,8 @@ var _ = Describe("Platform routing setup for the component", test.EnvTest(), fun
It("should have external routing resources created", func(ctx context.Context) {
// given
// required annotation for watched custom resource:
// routing.opendatahub.io/export-mode: "external"
component, createErr := createComponentRequiringPlatformRouting(ctx, "exported-test-component", "external", appNs.Name)
// routing.opendatahub.io/export-mode-external: "true"
component, createErr := createComponentRequiringPlatformRouting(ctx, "exported-test-component", appNs.Name, annotations.ExternalMode())
Expect(createErr).ToNot(HaveOccurred())
toRemove = append(toRemove, component)

Expand All @@ -105,8 +104,8 @@ var _ = Describe("Platform routing setup for the component", test.EnvTest(), fun
It("should have new hosts propagated back to watched resource", func(ctx context.Context) {
// given
// required annotation for watched custom resource:
// routing.opendatahub.io/export-mode: "external"
component, createErr := createComponentRequiringPlatformRouting(ctx, "exported-test-component", "external", appNs.Name)
// routing.opendatahub.io/export-mode-external: "true"
component, createErr := createComponentRequiringPlatformRouting(ctx, "exported-test-component", appNs.Name, annotations.ExternalMode())
Expect(createErr).ToNot(HaveOccurred())
toRemove = append(toRemove, component)

Expand Down Expand Up @@ -148,8 +147,8 @@ var _ = Describe("Platform routing setup for the component", test.EnvTest(), fun
It("should have routing resources for out-of-mesh access created", func(ctx context.Context) {
// given
// required annotation for watched custom resource:
// routing.opendatahub.io/export-mode: "public"
component, createErr := createComponentRequiringPlatformRouting(ctx, "public-test-component", "public", appNs.Name)
// routing.opendatahub.io/export-mode-public: "true"
component, createErr := createComponentRequiringPlatformRouting(ctx, "public-test-component", appNs.Name, annotations.PublicMode())
Expect(createErr).ToNot(HaveOccurred())
toRemove = append(toRemove, component)

Expand All @@ -167,8 +166,8 @@ var _ = Describe("Platform routing setup for the component", test.EnvTest(), fun
It("should have new hosts propagated back to watched resource by the controller", func(ctx context.Context) {
// given
// required annotation for watched custom resource:
// routing.opendatahub.io/export-mode: "public"
component, createErr := createComponentRequiringPlatformRouting(ctx, "public-test-component", "public", appNs.Name)
// routing.opendatahub.io/export-mode-public: "true"
component, createErr := createComponentRequiringPlatformRouting(ctx, "public-test-component", appNs.Name, annotations.PublicMode())
Expect(createErr).ToNot(HaveOccurred())
toRemove = append(toRemove, component)

Expand Down Expand Up @@ -214,8 +213,10 @@ var _ = Describe("Platform routing setup for the component", test.EnvTest(), fun
It("should have both external and cluster-local resources created", func(ctx context.Context) {
// given
// required annotation for watched custom resource:
// routing.opendatahub.io/export-mode: "public;external"
component, createErr := createComponentRequiringPlatformRouting(ctx, "public-and-external-test-component", "public;external", appNs.Name)
// routing.opendatahub.io/export-mode-external: "true"
// routing.opendatahub.io/export-mode-public: "true"
component, createErr := createComponentRequiringPlatformRouting(ctx, "public-and-external-test-component",
appNs.Name, annotations.ExternalMode(), annotations.PublicMode())
Expect(createErr).ToNot(HaveOccurred())
toRemove = append(toRemove, component)

Expand Down Expand Up @@ -270,9 +271,10 @@ var _ = Describe("Platform routing setup for the component", test.EnvTest(), fun
It("should remove the routing resources when both public;external", func(ctx context.Context) {
// given
// required annotation for watched custom resource:
// routing.opendatahub.io/export-mode: "public;external"
// routing.opendatahub.io/export-mode-external: "true"
// routing.opendatahub.io/export-mode-public: "true"
component, createErr := createComponentRequiringPlatformRouting(ctx, "public-and-external-test-component",
"public;external", appNs.Name)
appNs.Name, annotations.ExternalMode(), annotations.PublicMode())
Expect(createErr).ToNot(HaveOccurred())
toRemove = append(toRemove, component)

Expand Down Expand Up @@ -311,9 +313,10 @@ var _ = Describe("Platform routing setup for the component", test.EnvTest(), fun
It("should remove all created routing resources", func(ctx context.Context) {
// given
// required annotation for watched custom resource:
// routing.opendatahub.io/export-mode: "public;external"
// routing.opendatahub.io/export-mode-external: "true"
// routing.opendatahub.io/export-mode-public: "true"
component, createErr := createComponentRequiringPlatformRouting(ctx, "public-and-external-remove-annotation",
"public;external", appNs.Name)
appNs.Name, annotations.ExternalMode(), annotations.PublicMode())
Expect(createErr).ToNot(HaveOccurred())

// required labels for the exported service:
Expand All @@ -327,8 +330,8 @@ var _ = Describe("Platform routing setup for the component", test.EnvTest(), fun
publicResourcesShouldExist(ctx, svc)

// when
By("removing the export annotation", func() {
setExportMode(ctx, component, remove)
By("removing the export annotations", func() {
setExportModes(ctx, component, removeExternal, removePublic)
})

// then
Expand All @@ -348,9 +351,10 @@ var _ = Describe("Platform routing setup for the component", test.EnvTest(), fun
It("should remove all routing resources from removed", func(ctx context.Context) {
// given
// required annotation for watched custom resource:
// routing.opendatahub.io/export-mode: "public;external"
// routing.opendatahub.io/export-mode-external: "true"
// routing.opendatahub.io/export-mode-public: "true"
component, createErr := createComponentRequiringPlatformRouting(ctx, "public-and-external-change-annotation",
"public;external", appNs.Name)
appNs.Name, annotations.ExternalMode(), annotations.PublicMode())
Expect(createErr).ToNot(HaveOccurred())

// required labels for the exported service:
Expand All @@ -364,7 +368,7 @@ var _ = Describe("Platform routing setup for the component", test.EnvTest(), fun
publicResourcesShouldExist(ctx, svc)

By("removing external from the export modes", func() {
setExportMode(ctx, component, "public")
setExportModes(ctx, component, enablePublic, disableExternal)
})

// then
Expand Down Expand Up @@ -402,9 +406,10 @@ var _ = Describe("Platform routing setup for the component", test.EnvTest(), fun
It("should remove all routing resources when the unsupported mode is used", func(ctx context.Context) {
// given
// required annotation for watched custom resource:
// routing.opendatahub.io/export-mode: "public;external"
// routing.opendatahub.io/export-mode-external: "true"
// routing.opendatahub.io/export-mode-public: "true"
component, createErr := createComponentRequiringPlatformRouting(ctx, "public-and-external-changed-to-non-existing",
"public;external", appNs.Name)
appNs.Name, annotations.ExternalMode(), annotations.PublicMode())
Expect(createErr).ToNot(HaveOccurred())

// required labels for the exported service:
Expand All @@ -417,8 +422,8 @@ var _ = Describe("Platform routing setup for the component", test.EnvTest(), fun
externalResourcesShouldExist(ctx, svc)
publicResourcesShouldExist(ctx, svc)

By("removing external from the export modes", func() {
setExportMode(ctx, component, "not-supported-mode")
By("setting a non-supported mode", func() {
setExportModes(ctx, component, enableNonSupportedMode, removePublic, removeExternal)
})

// then
Expand All @@ -435,36 +440,6 @@ var _ = Describe("Platform routing setup for the component", test.EnvTest(), fun

})

type exportMode string

var remove = exportMode("--blank--")

func (m exportMode) ApplyToMeta(obj metav1.Object) {
annos := obj.GetAnnotations()
key := annotations.RoutingExportMode("").Key()

switch m {
default:
annos[key] = string(m)
case remove:
delete(annos, key)
}

obj.SetAnnotations(annos)
}

func setExportMode(ctx context.Context, component *unstructured.Unstructured, mode exportMode) {
errGetComponent := envTest.Client.Get(ctx, client.ObjectKey{
Namespace: component.GetNamespace(),
Name: component.GetName(),
}, component)
Expect(errGetComponent).ToNot(HaveOccurred())

metadata.ApplyMetaOptions(component, mode)

Expect(envTest.Client.Update(ctx, component)).To(Succeed())
}

func externalResourcesShouldExist(ctx context.Context, svc *corev1.Service) {
Eventually(routeExistsFor(svc)).
WithContext(ctx).
Expand Down
56 changes: 53 additions & 3 deletions controllers/routingctrl/fixtures_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"github.com/opendatahub-io/odh-platform/pkg/metadata/labels"
"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"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
Expand Down Expand Up @@ -46,12 +47,61 @@ func addRoutingRequirementsToSvc(ctx context.Context, exportedSvc *corev1.Servic
Expect(errExportSvc).ToNot(HaveOccurred())
}

func createComponentRequiringPlatformRouting(ctx context.Context, componentName, mode, appNs string) (*unstructured.Unstructured, error) {
// createComponentRequiringPlatformRouting creates a new component with the specified routing modes.
func createComponentRequiringPlatformRouting(ctx context.Context, componentName, appNs string, modes ...annotations.RoutingExportMode) (*unstructured.Unstructured, error) {
component, errCreate := test.CreateUnstructured(componentResource(componentName, appNs))
Expect(errCreate).ToNot(HaveOccurred())

// set component's "routing.opendatahub.io/export-mode" annotation to the specified mode.
metadata.ApplyMetaOptions(component, annotations.RoutingExportMode(mode))
for _, mode := range modes {
metadata.ApplyMetaOptions(component, mode)
}

return component, envTest.Client.Create(ctx, component)
}

type exportModeAction struct {
mode string
value string
}

var (
enablePublic = exportModeAction{mode: annotations.PublicMode().Key(), value: "true"}
disableExternal = exportModeAction{mode: annotations.ExternalMode().Key(), value: "false"}
removeExternal = exportModeAction{mode: annotations.ExternalMode().Key(), value: ""}
removePublic = exportModeAction{mode: annotations.PublicMode().Key(), value: ""}
enableNonSupportedMode = exportModeAction{
mode: annotations.RoutingExportModePrefix + "notsupported",
value: "true",
}
)

func (m exportModeAction) ApplyToMeta(obj metav1.Object) {
annos := obj.GetAnnotations()
if annos == nil {
annos = make(map[string]string)
}

key := m.mode

if m.value == "" {
delete(annos, key)
} else {
annos[key] = m.value
}

obj.SetAnnotations(annos)
}

func setExportModes(ctx context.Context, component *unstructured.Unstructured, actions ...exportModeAction) {
errGetComponent := envTest.Client.Get(ctx, client.ObjectKey{
Namespace: component.GetNamespace(),
Name: component.GetName(),
}, component)
Expect(errGetComponent).ToNot(HaveOccurred())

for _, action := range actions {
metadata.ApplyMetaOptions(component, action)
}

Expect(envTest.Client.Update(ctx, component)).To(Succeed())
}
31 changes: 16 additions & 15 deletions controllers/routingctrl/reconcile_resources.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"context"
"errors"
"fmt"
"strings"

"github.com/opendatahub-io/odh-platform/pkg/cluster"
"github.com/opendatahub-io/odh-platform/pkg/config"
Expand Down Expand Up @@ -124,24 +123,26 @@ func (r *Controller) ensureResourceHasFinalizer(ctx context.Context, target *uns
return nil
}

// extractExportModes retrieves the enabled export modes from the target's annotations.
func (r *Controller) extractExportModes(target *unstructured.Unstructured) []routing.RouteType {
exportModes, exportModeFound := target.GetAnnotations()[annotations.RoutingExportMode("").Key()]
if !exportModeFound {
targetAnnotations := target.GetAnnotations()
if targetAnnotations == nil {
return nil
}

exportModesSplit := strings.Split(exportModes, ";")
validRouteTypes := make([]routing.RouteType, 0, len(exportModesSplit))

for _, exportMode := range exportModesSplit {
routeType := routing.RouteType(strings.TrimSpace(exportMode))
if routing.IsValidRouteType(routeType) {
validRouteTypes = append(validRouteTypes, routeType)
} else {
r.log.Info("Invalid route type found",
"invalidRouteType", routeType,
"resourceName", target.GetName(),
"resourceNamespace", target.GetNamespace())
validRouteTypes := make([]routing.RouteType, 0)

for key, value := range targetAnnotations {
if value == "true" {
routeType, valid := routing.IsValidRouteType(key)
if valid {
validRouteTypes = append(validRouteTypes, routeType)
} else {
r.log.Info("Invalid route type found",
"invalidRouteType", routeType,
"resourceName", target.GetName(),
"resourceNamespace", target.GetNamespace())
}
}
}

Expand Down
27 changes: 21 additions & 6 deletions pkg/metadata/annotations/types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,10 @@ import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)

const (
RoutingExportModePrefix = "routing.opendatahub.io/export-mode-"
)

type Annotation interface {
metadata.Option
metadata.KeyValue
Expand Down Expand Up @@ -44,21 +48,32 @@ func (a AuthorizationGroup) Value() string {
return string(a)
}

// RoutingExportMode defines the export mode for the routing capability. It can be
// either "public" or "external" or both, delimited by ";".
// It is intended to be defined on the component's Custom Resource.
type RoutingExportMode string
// RoutingExportMode defines an individual export mode for the routing capability.
// Each mode (currently: "public" or "external") is represented by a separate annotation.
// The annotation key is formed by prefixing the mode with "routing.opendatahub.io/export-mode-", value is boolean.
type RoutingExportMode struct {
mode string
value string
}

func ExternalMode() RoutingExportMode {
return RoutingExportMode{mode: "external", value: "true"}
}

func PublicMode() RoutingExportMode {
return RoutingExportMode{mode: "public", value: "true"}
}

func (r RoutingExportMode) ApplyToMeta(obj metav1.Object) {
addAnnotation(r, obj)
}

func (r RoutingExportMode) Key() string {
return "routing.opendatahub.io/export-mode"
return RoutingExportModePrefix + r.mode
}

func (r RoutingExportMode) Value() string {
return string(r)
return r.value
}

// RoutingAddressesPublic exposes the public addresses set by Platform's routing capability.
Expand Down
19 changes: 16 additions & 3 deletions pkg/routing/types.go
View file
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
package routing

import (
"slices"
"strings"

"github.com/opendatahub-io/odh-platform/pkg/metadata/annotations"
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
)
Expand All @@ -18,8 +19,20 @@ func AllRouteTypes() []RouteType {
return []RouteType{PublicRoute, ExternalRoute}
}

func IsValidRouteType(routeType RouteType) bool {
return slices.Contains(AllRouteTypes(), routeType)
func IsValidRouteType(annotationKey string) (RouteType, bool) {
if !strings.HasPrefix(annotationKey, annotations.RoutingExportModePrefix) {
return "", false
}

routeType := RouteType(strings.TrimPrefix(annotationKey, annotations.RoutingExportModePrefix))

for _, validType := range AllRouteTypes() {
if routeType == validType {
return routeType, true
}
}

return routeType, false
}

func UnusedRouteTypes(exportModes []RouteType) []RouteType {
Expand Down

0 comments on commit 2f7f715

Please sign in to comment.