Skip to content

Commit

Permalink
feat(routing): support changing export modes (maistra#48)
Browse files Browse the repository at this point in the history 
Support changing export modes

## Summary

This PR enhances the routing controller's functionality to ensure that
routing resources are correctly removed when changes occur in the export
annotation of a watched resource, handling the removal and modification
of the `routing.opendatahub.io/export-mode` annotation on watched CR.

Additionally, the PR introduces new test cases to verify this behavior:

1. **Removal of Export Annotation**: Tests that all created routing
resources are removed when the export annotation is deleted from a
component.
2. **Modification of Export Annotation**: Tests that only the routing
resources corresponding to the removed export mode are deleted when the
annotation is updated.

Fixes: https://issues.redhat.com/browse/RHOAIENG-11030

---------

Co-authored-by: Bartosz Majsak <bartosz.majsak@gmail.com>
  • Loading branch information
@cam-garrison @bartoszmajsak
cam-garrison and bartoszmajsak authored Aug 27, 2024
1 parent 9faba49 commit 87baa6c
Show file tree
Hide file tree
Showing 11 changed files with 401 additions and 72 deletions.
14 changes: 9 additions & 5 deletions controllers/routingctrl/controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"github.com/opendatahub-io/odh-platform/pkg/metadata"
"github.com/opendatahub-io/odh-platform/pkg/routing"
"github.com/opendatahub-io/odh-platform/pkg/spi"
"github.com/opendatahub-io/odh-platform/pkg/unstruct"
openshiftroutev1 "github.com/openshift/api/route/v1"
istionetworkingv1beta1 "istio.io/client-go/pkg/apis/networking/v1beta1"
k8serr "k8s.io/apimachinery/pkg/api/errors"
Expand Down Expand Up @@ -62,7 +63,10 @@ func (r *Controller) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
return ctrl.Result{}, nil
}

reconcilers := []platformctrl.SubReconcileFunc{r.reconcileResources}
reconcilers := []platformctrl.SubReconcileFunc{
r.removeUnusedRoutingResources,
r.createRoutingResources,
}

sourceRes := &unstructured.Unstructured{}
sourceRes.SetGroupVersionKind(r.component.ObjectReference.GroupVersionKind)
Expand All @@ -77,12 +81,12 @@ func (r *Controller) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
return ctrl.Result{}, fmt.Errorf("failed getting resource: %w", err)
}

if !sourceRes.GetDeletionTimestamp().IsZero() {
return r.HandleResourceDeletion(ctx, sourceRes)
}

r.log.Info("triggered routing reconcile", "namespace", req.Namespace, "name", req.Name)

if unstruct.IsMarkedForDeletion(sourceRes) {
return ctrl.Result{}, r.handleResourceDeletion(ctx, sourceRes)
}

var errs []error

if controllerutil.AddFinalizer(sourceRes, metadata.Finalizers.Routing) {
Expand Down
230 changes: 208 additions & 22 deletions controllers/routingctrl/controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ 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 All @@ -16,6 +17,7 @@ import (
corev1 "k8s.io/api/core/v1"
k8serr "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/types"
"k8s.io/apimachinery/pkg/util/intstr"
utilrand "k8s.io/apimachinery/pkg/util/rand"
Expand Down Expand Up @@ -297,47 +299,172 @@ var _ = Describe("Platform routing setup for the component", test.EnvTest(), fun
})

// then
Eventually(routeExistsFor(svc)).
WithContext(ctx).
WithTimeout(test.DefaultTimeout).
WithPolling(test.DefaultPolling).
Should(WithTransform(k8serr.IsNotFound, BeTrue()))
externalResourcesShouldNotExist(ctx, svc)

Eventually(ingressVirtualServiceExistsFor(svc)).
WithContext(ctx).
WithTimeout(test.DefaultTimeout).
WithPolling(test.DefaultPolling).
Should(WithTransform(k8serr.IsNotFound, BeTrue()))
publicResourcesShouldNotExist(ctx, svc)

Eventually(publicSvcExistsFor(svc)).
WithContext(ctx).
WithTimeout(test.DefaultTimeout).
WithPolling(test.DefaultPolling).
Should(WithTransform(k8serr.IsNotFound, BeTrue()))
})
})

When("export annotation is removed from previously exported component", func() {

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"
component, createErr := createComponentRequiringPlatformRouting(ctx, "public-and-external-remove-annotation",
"public;external", appNs.Name)
Expect(createErr).ToNot(HaveOccurred())

Eventually(publicVirtualSvcExistsFor(svc)).
// required labels for the exported service:
// routing.opendatahub.io/exported: "true"
// platform.opendatahub.io/owner-name: test-component
// platform.opendatahub.io/owner-kind: Component
addRoutingRequirementsToSvc(ctx, svc, component)

// then
externalResourcesShouldExist(ctx, svc)
publicResourcesShouldExist(ctx, svc)

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

// then
externalResourcesShouldNotExist(ctx, svc)
publicResourcesShouldNotExist(ctx, svc)

Eventually(hasNoAddressAnnotations(component)).
WithContext(ctx).
WithTimeout(test.DefaultTimeout).
WithPolling(test.DefaultPolling).
Should(WithTransform(k8serr.IsNotFound, BeTrue()))
Should(Succeed())
})
})

When("export annotation is changed on existing exported component", func() {

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"
component, createErr := createComponentRequiringPlatformRouting(ctx, "public-and-external-change-annotation",
"public;external", appNs.Name)
Expect(createErr).ToNot(HaveOccurred())

// required labels for the exported service:
// routing.opendatahub.io/exported: "true"
// platform.opendatahub.io/owner-name: test-component
// platform.opendatahub.io/owner-kind: Component
addRoutingRequirementsToSvc(ctx, svc, component)

// then
externalResourcesShouldExist(ctx, svc)
publicResourcesShouldExist(ctx, svc)

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

// then
externalResourcesShouldNotExist(ctx, svc)
publicResourcesShouldExist(ctx, svc)

Eventually(func(g Gomega, ctx context.Context) error {
updatedComponent := component.DeepCopy()
if errGet := envTest.Get(ctx, client.ObjectKeyFromObject(updatedComponent), updatedComponent); errGet != nil {
return errGet
}

g.Expect(updatedComponent.GetAnnotations()).ToNot(
HaveKey(
annotations.RoutingAddressesExternal("").Key(),
), "public services are not expected to be defined in this mode")

publicAddressAnnotation := annotations.RoutingAddressesPublic(
fmt.Sprintf("%[1]s-%[2]s.%[3]s;%[1]s-%[2]s.%[3]s.svc;%[1]s-%[2]s.%[3]s.svc.cluster.local",
svc.Name, svc.Namespace, routingConfiguration.GatewayNamespace),
)

g.Expect(updatedComponent.GetAnnotations()).To(
HaveKeyWithValue(publicAddressAnnotation.Key(), publicAddressAnnotation.Value()),
)

Eventually(publicGatewayExistsFor(svc)).
return nil
}).
WithContext(ctx).
WithTimeout(test.DefaultTimeout).
WithPolling(test.DefaultPolling).
Should(WithTransform(k8serr.IsNotFound, BeTrue()))
Should(Succeed())
})

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"
component, createErr := createComponentRequiringPlatformRouting(ctx, "public-and-external-changed-to-non-existing",
"public;external", appNs.Name)
Expect(createErr).ToNot(HaveOccurred())

// required labels for the exported service:
// routing.opendatahub.io/exported: "true"
// platform.opendatahub.io/owner-name: test-component
// platform.opendatahub.io/owner-kind: Component
addRoutingRequirementsToSvc(ctx, svc, component)

// then
externalResourcesShouldExist(ctx, svc)
publicResourcesShouldExist(ctx, svc)

Eventually(destinationRuleExistsFor(svc)).
By("removing external from the export modes", func() {
setExportMode(ctx, component, "not-supported-mode")
})

// then
externalResourcesShouldNotExist(ctx, svc)
publicResourcesShouldNotExist(ctx, svc)

Eventually(hasNoAddressAnnotations(component)).
WithContext(ctx).
WithTimeout(test.DefaultTimeout).
WithPolling(test.DefaultPolling).
Should(WithTransform(k8serr.IsNotFound, BeTrue()))

Should(Succeed())
})
})

})

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 Expand Up @@ -378,6 +505,65 @@ func publicResourcesShouldExist(ctx context.Context, svc *corev1.Service) {
Should(Succeed())
}

func externalResourcesShouldNotExist(ctx context.Context, svc *corev1.Service) {
Eventually(routeExistsFor(svc)).
WithContext(ctx).
WithTimeout(test.DefaultTimeout).
WithPolling(test.DefaultPolling).
Should(WithTransform(k8serr.IsNotFound, BeTrue()))

Eventually(ingressVirtualServiceExistsFor(svc)).
WithContext(ctx).
WithTimeout(test.DefaultTimeout).
WithPolling(test.DefaultPolling).
Should(WithTransform(k8serr.IsNotFound, BeTrue()))
}

func publicResourcesShouldNotExist(ctx context.Context, svc *corev1.Service) {
Eventually(publicSvcExistsFor(svc)).
WithContext(ctx).
WithTimeout(test.DefaultTimeout).
WithPolling(test.DefaultPolling).
Should(WithTransform(k8serr.IsNotFound, BeTrue()))

Eventually(publicVirtualSvcExistsFor(svc)).
WithContext(ctx).
WithTimeout(test.DefaultTimeout).
WithPolling(test.DefaultPolling).
Should(WithTransform(k8serr.IsNotFound, BeTrue()))

Eventually(publicGatewayExistsFor(svc)).
WithContext(ctx).
WithTimeout(test.DefaultTimeout).
WithPolling(test.DefaultPolling).
Should(WithTransform(k8serr.IsNotFound, BeTrue()))

Eventually(destinationRuleExistsFor(svc)).
WithContext(ctx).
WithTimeout(test.DefaultTimeout).
WithPolling(test.DefaultPolling).
Should(WithTransform(k8serr.IsNotFound, BeTrue()))
}

func hasNoAddressAnnotations(component *unstructured.Unstructured) func(g Gomega, ctx context.Context) error {
return func(g Gomega, ctx context.Context) error {
updatedComponent := component.DeepCopy()
Expect(envTest.Get(ctx, client.ObjectKeyFromObject(updatedComponent), updatedComponent)).To(Succeed())

g.Expect(updatedComponent.GetAnnotations()).ToNot(
HaveKey(
annotations.RoutingAddressesExternal("").Key(),
), "External services are not expected to be defined in this mode")

g.Expect(updatedComponent.GetAnnotations()).ToNot(
HaveKey(
annotations.RoutingAddressesPublic("").Key(),
), "Public services are not expected to be defined in this mode")

return nil
}
}

func routeExistsFor(exportedSvc *corev1.Service) func(g Gomega, ctx context.Context) error {
return func(g Gomega, ctx context.Context) error {
svcRoute := &openshiftroutev1.Route{}
Expand Down
Loading

0 comments on commit 87baa6c

Please sign in to comment.