Export reconciler Validate method #580
Conversation
Export the Validate() method for Reconcilers and SubReconcilers. Test cases detect this method and call it before the test is run to fail fast with a meaningful error for ambiguous reconciler definitions. Since the reconciler validation tests can now live in a _test package, they moved to live next to the primary tests for their respective reconciler. Tests previously using invalid reconcilers have been corrected or removed. Signed-off-by: Scott Andrews <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #580 +/- ##
==========================================
- Coverage 58.33% 58.26% -0.08%
==========================================
Files 33 33
Lines 3795 3800 +5
==========================================
Hits 2214 2214
- Misses 1487 1492 +5
Partials 94 94 ☔ View full report in Codecov by Sentry. |
| type Validator interface { | ||
| Validate(ctx context.Context) error | ||
| } | ||
|
|
There was a problem hiding this comment.
[thought]:
I had tested your these changes with my kit, dropping the field ChildObjectManager from one of the reconcilers and expecting an error message. However, it still produced the panic described by #578.
head scratching...
Then I realised that I am working with a higher-order reconciler, i.e. Advice, which delegates to a ChildReconciler. Right now higher-order reconcilers only validate themselves, and not their inner reconcilers. That means the inner validation errors are masked. In fact, we should not even see them at runtime, but panics instead.
Therefore, I think we should export the Validator interface from the reconcilers package instead. That would allow higher-order reconcilers to sniff their delegate reconcilers for Validate and fail fast in case of invalidity.
For example like this with Advice only:
diff --git a/reconcilers/advice.go b/reconcilers/advice.go
index fb083f6..12d223d 100644
--- a/reconcilers/advice.go
+++ b/reconcilers/advice.go
@@ -126,7 +126,11 @@ func (r *Advice[T]) Validate(ctx context.Context) error {
if r.Before == nil && r.Around == nil && r.After == nil {
return fmt.Errorf("Advice %q must implement at least one of Before, Around or After", r.Name)
}
-
+ if v, ok := r.Reconciler.(Validator); ok {
+ if err := v.Validate(ctx); err != nil {
+ return fmt.Errorf("Advice %q has an invalid reconciler: %s", r.Name, err)
+ }
+ }
return nil
}
diff --git a/reconcilers/reconcilers.go b/reconcilers/reconcilers.go
index c3fba65..59b38ae 100644
--- a/reconcilers/reconcilers.go
+++ b/reconcilers/reconcilers.go
@@ -39,6 +39,10 @@ type SubReconciler[Type client.Object] interface {
Reconcile(ctx context.Context, resource Type) (Result, error)
}
+type Validator interface {
+ Validate(ctx context.Context) error
+}
+
var (
// ErrQuiet are not logged or recorded as events within reconciler.io/runtime.
// They are propagated as errors unless otherwise defined.
diff --git a/testing/reconciler.go b/testing/reconciler.go
index e51ad77..0132b85 100644
--- a/testing/reconciler.go
+++ b/testing/reconciler.go
@@ -32,10 +32,6 @@ import (
"sigs.k8s.io/controller-runtime/pkg/reconcile"
)
-type Validator interface {
- Validate(ctx context.Context) error
-}
-
// ReconcilerTestCase holds a single test case of a reconciler test suite.
type ReconcilerTestCase struct {
// Name is a descriptive name for this test suitable as a first argument to t.Run()
@@ -212,7 +208,7 @@ func (tc *ReconcilerTestCase) Run(t *testing.T, scheme *runtime.Scheme, factory
ctx = reconcilers.StashAdditionalConfigs(ctx, configs)
r := factory(t, tc, expectConfig.Config())
- if v, ok := r.(Validator); ok {
+ if v, ok := r.(reconcilers.Validator); ok {
if err := v.Validate(ctx); err != nil {
t.Fatalf("reconciler validation failed: %s", err)
}
diff --git a/testing/subreconciler.go b/testing/subreconciler.go
index ecd1009..abc3dcb 100644
--- a/testing/subreconciler.go
+++ b/testing/subreconciler.go
@@ -228,7 +228,7 @@ func (tc *SubReconcilerTestCase[T]) Run(t *testing.T, scheme *runtime.Scheme, fa
c := expectConfig.Config()
r := factory(t, tc, c)
- if v, ok := r.(Validator); ok {
+ if v, ok := r.(reconcilers.Validator); ok {
if err := v.Validate(ctx); err != nil {
t.Fatalf("subreconciler validation failed: %s", err)
}We would have to extend the same to all higher-order reconcilers
❯ rg "\tReconciler SubReconciler\[T" -l
reconcilers/webhook.go
reconcilers/config.go
reconcilers/advice.go
reconcilers/finalizer.go
reconcilers/aggregate.go
reconcilers/flow.go
reconcilers/resource.go
I'd be happy to follow-up with a PR, if you prefer.
There was a problem hiding this comment.
Good catch regarding nested validation. I'd like to avoid calling into nested validators directory within the validate method because the reconciler name may not be set, which will make it harder to identify where the validation error is actually coming from. For the non-test scenario it would cause the validation to be called n+1 times, where n is depth of the sub reconciler.
Perhaps we can set a value in the context to signal the validation should be recursive and turn it on only for tests. If we want to get fancy with the context, we could also pass a validation error handler via the context and use it to t.Fatal for tests and panic for deployed reconcilers.
There was a problem hiding this comment.
Ok! I like the idea about controlling behaviour via the context and possibly propagating a validation error handler within it. I have recorded #583 and will work on it.
| type Validator interface { | ||
| Validate(ctx context.Context) error | ||
| } | ||
|
|
There was a problem hiding this comment.
Ok! I like the idea about controlling behaviour via the context and possibly propagating a validation error handler within it. I have recorded #583 and will work on it.
Export the Validate() method for Reconcilers and SubReconcilers. Test cases detect this method and call it before the test is run to fail fast with a meaningful error for ambiguous reconciler definitions.
Since the reconciler validation tests can now live in a _test package, they moved to live next to the primary tests for their respective reconciler.
Tests previously using invalid reconcilers have been corrected or removed.
Resolves #578