Skip to content

Commit

Permalink
Validate nested reconcilers
Browse files Browse the repository at this point in the history 
* Nested validation is configured via the context with
  reconcilers#WithNestedValidation

* (Sub)Reconciler tests always do nested validation

* Moves the Validator interface from testing into reconcilers

* Sequence now implements the Validator interface. Its Validate is not
  called from its SetupWithManager to avoid duplicate validations.

fixes reconcilerio#583

Signed-off-by: Max Brauer <[email protected]>
  • Loading branch information
@mamachanko
mamachanko committed Feb 7, 2025
1 parent d0e059e commit 3d33d35
Show file tree
Hide file tree
Showing 20 changed files with 668 additions and 75 deletions.
12 changes: 9 additions & 3 deletions reconcilers/advice.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,7 @@ import (
"sigs.k8s.io/controller-runtime/pkg/client"
)

var (
_ SubReconciler[client.Object] = (*Advice[client.Object])(nil)
)
var _ SubReconciler[client.Object] = (*Advice[client.Object])(nil)

// Advice is a sub reconciler for advising the lifecycle of another sub reconciler in an aspect
// oriented programming style.
Expand Down Expand Up @@ -127,6 +125,14 @@ func (r *Advice[T]) Validate(ctx context.Context) error {
return fmt.Errorf("Advice %q must implement at least one of Before, Around or After", r.Name)
}

if hasNestedValidation(ctx) {
if v, ok := r.Reconciler.(Validator); ok {
if err := v.Validate(ctx); err != nil {
return fmt.Errorf("Advice %q must have a valid Reconciler: %s", r.Name, err)
}
}
}

return nil
}

Expand Down
43 changes: 39 additions & 4 deletions reconcilers/advice_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -232,10 +232,11 @@ func TestAdvice(t *testing.T) {

func TestAdvice_Validate(t *testing.T) {
tests := []struct {
name string
resource client.Object
reconciler *reconcilers.Advice[*corev1.ConfigMap]
shouldErr string
name string
resource client.Object
reconciler *reconcilers.Advice[*corev1.ConfigMap]
validateNested bool
shouldErr string
}{
{
name: "empty",
Expand Down Expand Up @@ -297,11 +298,45 @@ func TestAdvice_Validate(t *testing.T) {
},
},
},
{
name: "valid reconciler",
resource: &corev1.ConfigMap{},
reconciler: &reconcilers.Advice[*corev1.ConfigMap]{
Reconciler: &reconcilers.SyncReconciler[*corev1.ConfigMap]{
Sync: func(ctx context.Context, resource *corev1.ConfigMap) error {
return nil
},
},
After: func(ctx context.Context, resource *corev1.ConfigMap, result reconcile.Result, err error) (reconcile.Result, error) {
return reconcile.Result{}, nil
},
},
validateNested: true,
},
{
name: "invalid reconciler",
resource: &corev1.ConfigMap{},
reconciler: &reconcilers.Advice[*corev1.ConfigMap]{
Reconciler: &reconcilers.SyncReconciler[*corev1.ConfigMap]{
// Sync: func(ctx context.Context, resource *corev1.ConfigMap) error {
// return nil
// },
},
After: func(ctx context.Context, resource *corev1.ConfigMap, result reconcile.Result, err error) (reconcile.Result, error) {
return reconcile.Result{}, nil
},
},
validateNested: true,
shouldErr: `Advice "" must have a valid Reconciler: SyncReconciler "" must implement Sync or SyncWithResult`,
},
}

for _, c := range tests {
t.Run(c.name, func(t *testing.T) {
ctx := reconcilers.StashResourceType(context.TODO(), c.resource)
if c.validateNested {
ctx = reconcilers.WithNestedValidation(ctx)
}
err := c.reconciler.Validate(ctx)
if (err != nil) != (c.shouldErr != "") || (c.shouldErr != "" && c.shouldErr != err.Error()) {
t.Errorf("validate() error = %q, shouldErr %q", err, c.shouldErr)
Expand Down
19 changes: 12 additions & 7 deletions reconcilers/aggregate.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,7 @@ import (
"reconciler.io/runtime/tracker"
)

var (
_ reconcile.Reconciler = (*AggregateReconciler[client.Object])(nil)
)
var _ reconcile.Reconciler = (*AggregateReconciler[client.Object])(nil)

// AggregateReconciler is a controller-runtime reconciler that reconciles a specific resource. The
// Type resource is fetched for the reconciler
Expand Down Expand Up @@ -198,14 +196,21 @@ func (r *AggregateReconciler[T]) Validate(ctx context.Context) error {
return fmt.Errorf("AggregateReconciler %q must define Request", r.Name)
}

// validate AggregateObjectManager value
if r.AggregateObjectManager == nil {
return fmt.Errorf("AggregateReconciler %q must define AggregateObjectManager", r.Name)
}

// validate Reconciler value
if r.Reconciler == nil && r.DesiredResource == nil {
return fmt.Errorf("AggregateReconciler %q must define Reconciler and/or DesiredResource", r.Name)
}

// validate AggregateObjectManager value
if r.AggregateObjectManager == nil {
return fmt.Errorf("AggregateReconciler %q must define AggregateObjectManager", r.Name)
if hasNestedValidation(ctx) {
if v, ok := r.Reconciler.(Validator); ok {
if err := v.Validate(ctx); err != nil {
return fmt.Errorf("AggregateReconciler %q must have a valid Reconciler: %s", r.Name, err)
}
}
}

return nil
Expand Down
41 changes: 37 additions & 4 deletions reconcilers/aggregate_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -548,10 +548,11 @@ func TestAggregateReconciler_Validate(t *testing.T) {
}

tests := []struct {
name string
reconciler *reconcilers.AggregateReconciler[*resources.TestResource]
shouldErr string
expectedLogs []string
name string
reconciler *reconcilers.AggregateReconciler[*resources.TestResource]
validateNested bool
shouldErr string
expectedLogs []string
}{
{
name: "empty",
Expand Down Expand Up @@ -622,12 +623,44 @@ func TestAggregateReconciler_Validate(t *testing.T) {
},
shouldErr: `AggregateReconciler "AggregateObjectManager missing" must define AggregateObjectManager`,
},
{
name: "valid reconciler",
reconciler: &reconcilers.AggregateReconciler[*resources.TestResource]{
Type: &resources.TestResource{},
Request: req,
Reconciler: &reconcilers.SyncReconciler[*resources.TestResource]{
Sync: func(ctx context.Context, resource *resources.TestResource) error {
return nil
},
},
AggregateObjectManager: &reconcilers.UpdatingObjectManager[*resources.TestResource]{},
},
validateNested: true,
},
{
name: "invalid reconciler",
reconciler: &reconcilers.AggregateReconciler[*resources.TestResource]{
Type: &resources.TestResource{},
Request: req,
Reconciler: &reconcilers.SyncReconciler[*resources.TestResource]{
// Sync: func(ctx context.Context, resource *resources.TestResource) error {
// return nil
// },
},
AggregateObjectManager: &reconcilers.UpdatingObjectManager[*resources.TestResource]{},
},
validateNested: true,
shouldErr: `AggregateReconciler "" must have a valid Reconciler: SyncReconciler "" must implement Sync or SyncWithResult`,
},
}

for _, c := range tests {
t.Run(c.name, func(t *testing.T) {
sink := &bufferedSink{}
ctx := logr.NewContext(context.TODO(), logr.New(sink))
if c.validateNested {
ctx = reconcilers.WithNestedValidation(ctx)
}
err := c.reconciler.Validate(ctx)
if (err != nil) != (c.shouldErr != "") || (c.shouldErr != "" && c.shouldErr != err.Error()) {
t.Errorf("validate() error = %q, shouldErr %q", err, c.shouldErr)
Expand Down
11 changes: 8 additions & 3 deletions reconcilers/cast.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,7 @@ import (
"sigs.k8s.io/controller-runtime/pkg/client"
)

var (
_ SubReconciler[client.Object] = (*CastResource[client.Object, client.Object])(nil)
)
var _ SubReconciler[client.Object] = (*CastResource[client.Object, client.Object])(nil)

// CastResource casts the ResourceReconciler's type by projecting the resource data
// onto a new struct. Casting the reconciled resource is useful to create cross
Expand Down Expand Up @@ -98,6 +96,13 @@ func (r *CastResource[T, CT]) Validate(ctx context.Context) error {
if r.Reconciler == nil {
return fmt.Errorf("CastResource %q must define Reconciler", r.Name)
}
if hasNestedValidation(ctx) {
if v, ok := r.Reconciler.(Validator); ok {
if err := v.Validate(ctx); err != nil {
return fmt.Errorf("CastResource %q must have a valid Reconciler: %s", r.Name, err)
}
}
}

return nil
}
Expand Down
37 changes: 33 additions & 4 deletions reconcilers/cast_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -244,10 +244,11 @@ func TestCastResource(t *testing.T) {

func TestCastResource_Validate(t *testing.T) {
tests := []struct {
name string
resource client.Object
reconciler *reconcilers.CastResource[*corev1.ConfigMap, *corev1.Secret]
shouldErr string
name string
resource client.Object
reconciler *reconcilers.CastResource[*corev1.ConfigMap, *corev1.Secret]
validateNested bool
shouldErr string
}{
{
name: "empty",
Expand Down Expand Up @@ -275,11 +276,39 @@ func TestCastResource_Validate(t *testing.T) {
},
shouldErr: `CastResource "missing reconciler" must define Reconciler`,
},
{
name: "valid reconciler",
resource: &corev1.ConfigMap{},
reconciler: &reconcilers.CastResource[*corev1.ConfigMap, *corev1.Secret]{
Reconciler: &reconcilers.SyncReconciler[*corev1.Secret]{
Sync: func(ctx context.Context, resource *corev1.Secret) error {
return nil
},
},
},
validateNested: true,
},
{
name: "invalid reconciler",
resource: &corev1.ConfigMap{},
reconciler: &reconcilers.CastResource[*corev1.ConfigMap, *corev1.Secret]{
Reconciler: &reconcilers.SyncReconciler[*corev1.Secret]{
// Sync: func(ctx context.Context, resource *corev1.ConfigMap) error {
// return nil
// },
},
},
validateNested: true,
shouldErr: `CastResource "" must have a valid Reconciler: SyncReconciler "" must implement Sync or SyncWithResult`,
},
}

for _, c := range tests {
t.Run(c.name, func(t *testing.T) {
ctx := reconcilers.StashResourceType(context.TODO(), c.resource)
if c.validateNested {
ctx = reconcilers.WithNestedValidation(ctx)
}
err := c.reconciler.Validate(ctx)
if (err != nil) != (c.shouldErr != "") || (c.shouldErr != "" && c.shouldErr != err.Error()) {
t.Errorf("validate() error = %q, shouldErr %q", err, c.shouldErr)
Expand Down
7 changes: 7 additions & 0 deletions reconcilers/config.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,13 @@ func (r *WithConfig[T]) Validate(ctx context.Context) error {
if r.Reconciler == nil {
return fmt.Errorf("WithConfig %q must define Reconciler", r.Name)
}
if hasNestedValidation(ctx) {
if v, ok := r.Reconciler.(Validator); ok {
if err := v.Validate(ctx); err != nil {
return fmt.Errorf("WithConfig %q must have a valid Reconciler: %s", r.Name, err)
}
}
}

return nil
}
Expand Down
43 changes: 39 additions & 4 deletions reconcilers/config_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -338,10 +338,11 @@ func TestWithConfig_Validate(t *testing.T) {
}

tests := []struct {
name string
resource client.Object
reconciler *reconcilers.WithConfig[*corev1.ConfigMap]
shouldErr string
name string
resource client.Object
reconciler *reconcilers.WithConfig[*corev1.ConfigMap]
validateNested bool
shouldErr string
}{
{
name: "empty",
Expand Down Expand Up @@ -379,11 +380,45 @@ func TestWithConfig_Validate(t *testing.T) {
},
shouldErr: `WithConfig "missing reconciler" must define Reconciler`,
},
{
name: "valid reconciler",
resource: &corev1.ConfigMap{},
reconciler: &reconcilers.WithConfig[*corev1.ConfigMap]{
Config: func(ctx context.Context, c reconcilers.Config) (reconcilers.Config, error) {
return config, nil
},
Reconciler: &reconcilers.SyncReconciler[*corev1.ConfigMap]{
Sync: func(ctx context.Context, resource *corev1.ConfigMap) error {
return nil
},
},
},
validateNested: true,
},
{
name: "invalid reconciler",
resource: &corev1.ConfigMap{},
reconciler: &reconcilers.WithConfig[*corev1.ConfigMap]{
Config: func(ctx context.Context, c reconcilers.Config) (reconcilers.Config, error) {
return config, nil
},
Reconciler: &reconcilers.SyncReconciler[*corev1.ConfigMap]{
// Sync: func(ctx context.Context, resource *corev1.ConfigMap) error {
// return nil
// },
},
},
validateNested: true,
shouldErr: `WithConfig "" must have a valid Reconciler: SyncReconciler "" must implement Sync or SyncWithResult`,
},
}

for _, c := range tests {
t.Run(c.name, func(t *testing.T) {
ctx := context.TODO()
if c.validateNested {
ctx = reconcilers.WithNestedValidation(ctx)
}
err := c.reconciler.Validate(ctx)
if (err != nil) != (c.shouldErr != "") || (c.shouldErr != "" && c.shouldErr != err.Error()) {
t.Errorf("validate() error = %q, shouldErr %q", err, c.shouldErr)
Expand Down
11 changes: 8 additions & 3 deletions reconcilers/finalizer.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,7 @@ import (
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
)

var (
_ SubReconciler[client.Object] = (*WithFinalizer[client.Object])(nil)
)
var _ SubReconciler[client.Object] = (*WithFinalizer[client.Object])(nil)

// WithFinalizer ensures the resource being reconciled has the desired finalizer set so that state
// can be cleaned up upon the resource being deleted. The finalizer is added to the resource, if not
Expand Down Expand Up @@ -106,6 +104,13 @@ func (r *WithFinalizer[T]) Validate(ctx context.Context) error {
if r.Reconciler == nil {
return fmt.Errorf("WithFinalizer %q must define Reconciler", r.Name)
}
if hasNestedValidation(ctx) {
if v, ok := r.Reconciler.(Validator); ok {
if err := v.Validate(ctx); err != nil {
return fmt.Errorf("WithFinalizer %q must have a valid Reconciler: %s", r.Name, err)
}
}
}

return nil
}
Expand Down
Loading

0 comments on commit 3d33d35

Please sign in to comment.