Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RHOAIENG-17634: chore(odh-nbc/tests): implement diff minimization for k8s objects diffs #503

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

RHOAIENG-17634: chore(odh-nbc/tests): implement diff minimization for k8s objects diffs #503

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a8d721b
RHOAIENG-17634: ref(odh-nbc/tests): create Gomega custom matcher for …
jiridanek Dec 17, 2024
6c9317c
RHOAIENG-17634: chore(odh-nbc/tests): implement diff minimization for…
jiridanek Jan 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
173 changes: 173 additions & 0 deletions components/odh-notebook-controller/controllers/matchers_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,173 @@
package controllers

import (
"fmt"
"log"
"reflect"
"testing"

"github.com/google/go-cmp/cmp"
"github.com/onsi/gomega/types"

routev1 "github.com/openshift/api/route/v1"
"k8s.io/apimachinery/pkg/runtime"
)

// See https://onsi.github.io/gomega/#adding-your-own-matchers

// BeMatchingK8sResource is a custom Gomega matcher that compares using `comparator` function and reports differences using
// [cmp.Diff]. It attempts to minimize the diff to only include those entries that cause `comparator` to fail.
//
// Use this to replace assertions such as
// > Expect(CompareNotebookRoutes(*route, expectedRoute)).Should(BeTrueBecause(cmp.Diff(*route, expectedRoute)))
// with
// > Expect(*route).To(BeMatchingK8sResource(expectedRoute, CompareNotebookRoutes))
//
// NOTE: The diff minimization functionality is best-effort. It is designed to never under-approximate, but over-approximation is possible.
// NOTE2: Using [gcustom.MakeMatcher] is not possible because it does not conveniently allow running [cmp.Diff] at the time of failure message generation.
func BeMatchingK8sResource[T any, PT interface {
deepCopyer[T]
*T
}](expected T, comparator func(T, T) bool) types.GomegaMatcher {
return &beMatchingK8sResource[T, PT]{
expected: expected,
comparator: comparator,
}
}

type beMatchingK8sResource[T any, PT interface {
*T
deepCopyer[T]
}] struct {
expected T
comparator func(r1 T, r2 T) bool
}

var _ types.GomegaMatcher = &beMatchingK8sResource[routev1.Route, *routev1.Route]{}

func (m *beMatchingK8sResource[T, PT]) Match(actual interface{}) (success bool, err error) {
actualT, ok := actual.(T)
if !ok {
return false, fmt.Errorf("BeMatchingK8sResource matcher expects two objects of the same type")
}

return m.comparator(m.expected, actualT), nil
}

func (m *beMatchingK8sResource[T, PT]) FailureMessage(actual interface{}) (message string) {
//m.expected
diff := m.computeMinimizedDiff(actual.(T))
return fmt.Sprintf("Expected\n\t%#v\nto compare identical to\n\t%#v\nbut it differs in\n%s", actual, m.expected, diff)
}

func (m *beMatchingK8sResource[T, PT]) NegatedFailureMessage(actual interface{}) (message string) {
diff := m.computeMinimizedDiff(actual.(T))
return fmt.Sprintf("Expected\n\t%#v\nto not compare identical to\n\t%#v\nit differs in\n%s", actual, m.expected, diff)
}

// diffReporter is the basis of a custom [cmp.Reporter] that records differences detected during comparison.
// See example_reporter_test.go in the go-cmp package where this comes from.
type diffReporter struct {
path cmp.Path

// handler that allows us to work with the currently stored [path]
// called whenever we are visiting leaf values that are unequal
OnDiff func(path cmp.Path)
}

func (r *diffReporter) PushStep(ps cmp.PathStep) {
// NOTE (from docs): The PathStep itself is only valid until the step is popped.
r.path = append(r.path, ps)
}
func (r *diffReporter) Report(rs cmp.Result) {
if rs.Equal() {
return
}
fmt.Println("Report", r.path, "Endreport")
r.OnDiff(r.path)
}
func (r *diffReporter) PopStep() {
r.path = r.path[:len(r.path)-1]
}

// deepCopyer reflects the deep copy methods that Kubernetes types all have.
// This is used to create deep copies of the compared structs for the purposes of
// isolating what specific differences cause them to not be equal.
type deepCopyer[T any] interface {
DeepCopyInto(out *T)
DeepCopy() *T
DeepCopyObject() runtime.Object
}

func (m *beMatchingK8sResource[T, PT]) computeMinimizedDiff(actual T) (message string) {
debug := false

accumulator := PT(&actual).DeepCopy()
reporter := diffReporter{OnDiff: func(diffPath cmp.Path) {
fmt.Println(diffPath)
clone := PT(&actual).DeepCopy()

// reflect.ValueOf(&v).Elem() is used to get a modifiable value
current := reflect.ValueOf(clone).Elem()
pointer := reflect.ValueOf(accumulator).Elem()

// the root of the path is operation-less, skip path[0]
for i := 1; i < len(diffPath); i++ {
switch step := diffPath[i].(type) {
case cmp.StructField:
current = current.Field(step.Index())
pointer = pointer.Field(step.Index())
case cmp.SliceIndex:
current = current.Index(step.Key())
pointer = pointer.Index(step.Key())
case cmp.MapIndex:
current = current.MapIndex(step.Key())
pointer = pointer.MapIndex(step.Key())
case cmp.Indirect:
current = reflect.Indirect(current)
pointer = reflect.Indirect(pointer)
case cmp.TypeAssertion:
// this is imo a noop, we don't care about checking type assertions
case cmp.Transform:
panic("unhandled situation: we can't go deeper and have to end the traversal here")
default:
panic("unreachable")
}
}
vx, vy := diffPath.Last().Values()
current.Set(vx)

if debug {
log.Printf("%#v:\n\t-: %+v\n\t+: %+v\n", diffPath, vx, vy)
}

// Check that our comparator function cares about this field
if !m.comparator(actual, *clone) {
// If yes, store that in the accumulator object
pointer.Set(vx)
}
}}
cmp.Equal(m.expected, actual, cmp.Reporter(&reporter))

return cmp.Diff(actual, *accumulator)
}

// TODO: make it assert something
// todo: don't use actual production cmp funcs
func TestDiffingRoute(t *testing.T) {
someRoute := routev1.Route{
Spec: routev1.RouteSpec{
Host: "",
Path: "",
},
}
someOtherRoute := routev1.Route{
Spec: routev1.RouteSpec{
Host: "xxxx",
Path: "yy",
},
}
cmp := BeMatchingK8sResource(someRoute, CompareNotebookRoutes)
cmp.Match(someOtherRoute)

Check failure on line 171 in components/odh-notebook-controller/controllers/matchers_test.go

View workflow job for this annotation

GitHub Actions / golangci-lint (components/odh-notebook-controller) 

Error return value of `cmp.Match` is not checked (errcheck)
cmp.FailureMessage(someOtherRoute)
}
47 changes: 19 additions & 28 deletions components/odh-notebook-controller/controllers/notebook_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ import (
"time"

"github.com/go-logr/logr"
"github.com/google/go-cmp/cmp"
. "github.com/onsi/ginkgo"
. "github.com/onsi/gomega"

Expand Down Expand Up @@ -100,7 +99,7 @@ var _ = Describe("The Openshift Notebook controller", func() {
key := types.NamespacedName{Name: Name, Namespace: Namespace}
return cli.Get(ctx, key, route)
}, duration, interval).Should(Succeed())
Expect(CompareNotebookRoutes(*route, expectedRoute)).Should(BeTrueBecause(cmp.Diff(*route, expectedRoute)))
Expect(*route).To(BeMatchingK8sResource(expectedRoute, CompareNotebookRoutes))
})

It("Should reconcile the Route when modified", func() {
Expand All @@ -118,7 +117,7 @@ var _ = Describe("The Openshift Notebook controller", func() {
}
return route.Spec.To.Name, nil
}, duration, interval).Should(Equal(Name))
Expect(CompareNotebookRoutes(*route, expectedRoute)).Should(BeTrueBecause(cmp.Diff(*route, expectedRoute)))
Expect(*route).To(BeMatchingK8sResource(expectedRoute, CompareNotebookRoutes))
})

It("Should recreate the Route when deleted", func() {
Expand All @@ -131,7 +130,7 @@ var _ = Describe("The Openshift Notebook controller", func() {
key := types.NamespacedName{Name: Name, Namespace: Namespace}
return cli.Get(ctx, key, route)
}, duration, interval).Should(Succeed())
Expect(CompareNotebookRoutes(*route, expectedRoute)).Should(BeTrueBecause(cmp.Diff(*route, expectedRoute)))
Expect(*route).To(BeMatchingK8sResource(expectedRoute, CompareNotebookRoutes))
})

It("Should delete the Openshift Route", func() {
Expand Down Expand Up @@ -490,15 +489,14 @@ var _ = Describe("The Openshift Notebook controller", func() {
key := types.NamespacedName{Name: Name + "-ctrl-np", Namespace: Namespace}
return cli.Get(ctx, key, notebookNetworkPolicy)
}, duration, interval).Should(Succeed())
Expect(CompareNotebookNetworkPolicies(*notebookNetworkPolicy, expectedNotebookNetworkPolicy)).Should(BeTrueBecause(cmp.Diff(*notebookNetworkPolicy, expectedNotebookNetworkPolicy)))
Expect(*notebookNetworkPolicy).To(BeMatchingK8sResource(expectedNotebookNetworkPolicy, CompareNotebookNetworkPolicies))

By("By checking that the controller has created Network policy to allow all requests on OAuth port")
Eventually(func() error {
key := types.NamespacedName{Name: Name + "-oauth-np", Namespace: Namespace}
return cli.Get(ctx, key, notebookOAuthNetworkPolicy)
}, duration, interval).Should(Succeed())
Expect(CompareNotebookNetworkPolicies(*notebookOAuthNetworkPolicy, expectedNotebookOAuthNetworkPolicy)).
To(BeTrueBecause(cmp.Diff(*notebookOAuthNetworkPolicy, expectedNotebookOAuthNetworkPolicy)))
Expect(*notebookOAuthNetworkPolicy).To(BeMatchingK8sResource(expectedNotebookOAuthNetworkPolicy, CompareNotebookNetworkPolicies))
})

It("Should reconcile the Network policies when modified", func() {
Expand All @@ -516,8 +514,7 @@ var _ = Describe("The Openshift Notebook controller", func() {
}
return string(notebookNetworkPolicy.Spec.PolicyTypes[0]), nil
}, duration, interval).Should(Equal("Ingress"))
Expect(CompareNotebookNetworkPolicies(*notebookNetworkPolicy, expectedNotebookNetworkPolicy)).Should(
BeTrueBecause(cmp.Diff(*notebookNetworkPolicy, expectedNotebookNetworkPolicy)))
Expect(*notebookNetworkPolicy).To(BeMatchingK8sResource(expectedNotebookNetworkPolicy, CompareNotebookNetworkPolicies))
})

It("Should recreate the Network Policy when deleted", func() {
Expand All @@ -530,8 +527,7 @@ var _ = Describe("The Openshift Notebook controller", func() {
key := types.NamespacedName{Name: Name + "-oauth-np", Namespace: Namespace}
return cli.Get(ctx, key, notebookOAuthNetworkPolicy)
}, duration, interval).Should(Succeed())
Expect(CompareNotebookNetworkPolicies(*notebookOAuthNetworkPolicy, expectedNotebookOAuthNetworkPolicy)).Should(
BeTrueBecause(cmp.Diff(*notebookOAuthNetworkPolicy, expectedNotebookOAuthNetworkPolicy)))
Expect(*notebookOAuthNetworkPolicy).To(BeMatchingK8sResource(expectedNotebookOAuthNetworkPolicy, CompareNotebookNetworkPolicies))
})

It("Should delete the Network Policies", func() {
Expand Down Expand Up @@ -664,20 +660,17 @@ var _ = Describe("The Openshift Notebook controller", func() {
time.Sleep(interval)

By("By checking that the webhook has injected the sidecar container")
Expect(CompareNotebooks(*notebook, expectedNotebook)).Should(BeTrueBecause(cmp.Diff(*notebook, expectedNotebook)))
Expect(*notebook).To(BeMatchingK8sResource(expectedNotebook, CompareNotebooks))
})

It("Should remove the reconciliation lock annotation", func() {
By("By checking that the annotation lock annotation is not present")
delete(expectedNotebook.Annotations, culler.STOP_ANNOTATION)
Eventually(func() bool {
Eventually(func() (nbv1.Notebook, error) {
key := types.NamespacedName{Name: Name, Namespace: Namespace}
err := cli.Get(ctx, key, notebook)
if err != nil {
return false
}
return CompareNotebooks(*notebook, expectedNotebook)
}, duration, interval).Should(BeTrueBecause(cmp.Diff(*notebook, expectedNotebook)))
return *notebook, err
}, duration, interval).Should(BeMatchingK8sResource(expectedNotebook, CompareNotebooks))
})

It("Should reconcile the Notebook when modified", func() {
Expand All @@ -693,7 +686,7 @@ var _ = Describe("The Openshift Notebook controller", func() {
key := types.NamespacedName{Name: Name, Namespace: Namespace}
return cli.Get(ctx, key, notebook)
}, duration, interval).Should(Succeed())
Expect(CompareNotebooks(*notebook, expectedNotebook)).Should(BeTrueBecause(cmp.Diff(*notebook, expectedNotebook)))
Expect(*notebook).To(BeMatchingK8sResource(expectedNotebook, CompareNotebooks))
})

serviceAccount := &corev1.ServiceAccount{}
Expand All @@ -705,8 +698,7 @@ var _ = Describe("The Openshift Notebook controller", func() {
key := types.NamespacedName{Name: Name, Namespace: Namespace}
return cli.Get(ctx, key, serviceAccount)
}, duration, interval).Should(Succeed())
Expect(CompareNotebookServiceAccounts(*serviceAccount, expectedServiceAccount)).Should(
BeTrueBecause(cmp.Diff(*serviceAccount, expectedServiceAccount)))
Expect(*serviceAccount).To(BeMatchingK8sResource(expectedServiceAccount, CompareNotebookServiceAccounts))
})

It("Should recreate the Service Account when deleted", func() {
Expand All @@ -719,8 +711,7 @@ var _ = Describe("The Openshift Notebook controller", func() {
key := types.NamespacedName{Name: Name, Namespace: Namespace}
return cli.Get(ctx, key, serviceAccount)
}, duration, interval).Should(Succeed())
Expect(CompareNotebookServiceAccounts(*serviceAccount, expectedServiceAccount)).Should(
BeTrueBecause(cmp.Diff(*serviceAccount, expectedServiceAccount)))
Expect(*serviceAccount).To(BeMatchingK8sResource(expectedServiceAccount, CompareNotebookServiceAccounts))
})

service := &corev1.Service{}
Expand Down Expand Up @@ -751,7 +742,7 @@ var _ = Describe("The Openshift Notebook controller", func() {
key := types.NamespacedName{Name: Name + "-tls", Namespace: Namespace}
return cli.Get(ctx, key, service)
}, duration, interval).Should(Succeed())
Expect(CompareNotebookServices(*service, expectedService)).Should(BeTrueBecause(cmp.Diff(*service, expectedService)))
Expect(*service).To(BeMatchingK8sResource(expectedService, CompareNotebookServices))
})

It("Should recreate the Service when deleted", func() {
Expand All @@ -764,7 +755,7 @@ var _ = Describe("The Openshift Notebook controller", func() {
key := types.NamespacedName{Name: Name + "-tls", Namespace: Namespace}
return cli.Get(ctx, key, service)
}, duration, interval).Should(Succeed())
Expect(CompareNotebookServices(*service, expectedService)).Should(BeTrueBecause(cmp.Diff(*service, expectedService)))
Expect(*service).To(BeMatchingK8sResource(expectedService, CompareNotebookServices))
})

secret := &corev1.Secret{}
Expand Down Expand Up @@ -827,7 +818,7 @@ var _ = Describe("The Openshift Notebook controller", func() {
key := types.NamespacedName{Name: Name, Namespace: Namespace}
return cli.Get(ctx, key, route)
}, duration, interval).Should(Succeed())
Expect(CompareNotebookRoutes(*route, expectedRoute)).Should(BeTrueBecause(cmp.Diff(*route, expectedRoute)))
Expect(*route).To(BeMatchingK8sResource(expectedRoute, CompareNotebookRoutes))
})

It("Should recreate the Route when deleted", func() {
Expand All @@ -840,7 +831,7 @@ var _ = Describe("The Openshift Notebook controller", func() {
key := types.NamespacedName{Name: Name, Namespace: Namespace}
return cli.Get(ctx, key, route)
}, duration, interval).Should(Succeed())
Expect(CompareNotebookRoutes(*route, expectedRoute)).Should(BeTrueBecause(cmp.Diff(*route, expectedRoute)))
Expect(*route).To(BeMatchingK8sResource(expectedRoute, CompareNotebookRoutes))
})

It("Should reconcile the Route when modified", func() {
Expand All @@ -858,7 +849,7 @@ var _ = Describe("The Openshift Notebook controller", func() {
}
return route.Spec.To.Name, nil
}, duration, interval).Should(Equal(Name + "-tls"))
Expect(CompareNotebookRoutes(*route, expectedRoute)).Should(BeTrueBecause(cmp.Diff(*route, expectedRoute)))
Expect(*route).To(BeMatchingK8sResource(expectedRoute, CompareNotebookRoutes))
})

It("Should delete the OAuth proxy objects", func() {
Expand Down
Loading