Validate AdmissionWebhookAdapter (#597)

Defines and calls Validate() on AdmissionWebhookAdapter when building
the webhook. Calls validate with nested validation inside
AdmissionWebhookTestCase.

AdmissionWebhookAdapter#Build is now deprecated in favor of
AdmissionWebhookAdapter#BuildWithContext which accepts a context and
returns an error when validation fails. The previous Build will use a
TODO context and panic on validation errors.

AdmissionWebhookTestSuite#Run, AdmissionWebhookTests#Run, and
AdmissionWebhookTestCase#Run are deprecated in favor of RunWithContext.
The existing Run method delegates to RunWithContext, but requires users
to manually configure nested validation (if desired).

Signed-off-by: Scott Andrews <scott@andrews.me>

Author: scothis

README.md

@@ -1169,6 +1169,8 @@ Backwards support may be removed in a future release, users are encouraged to mi
 - status `InitializeConditions()` is deprecated in favor of `InitializeConditions(context.Context)`.
 - `ConditionSet#Manage` is deprecated in favor of `ConditionSet#ManageWithContext`.
 - `HaltSubReconcilers` is deprecated in favor of `ErrHaltSubReconcilers`.
+- `AdmissionWebhookAdapter#Build` is deprecated in favor of `AdmissionWebhookAdapter#BuildWithContext`.
+- `AdmissionWebhookTestSuite#Run`, `AdmissionWebhookTests#Run`, and `AdmissionWebhookTestCase#Run` are deprecated in favor of `#RunWithContext`.
 
 ## Community
 

reconcilers/webhook.go

@@ -33,6 +33,7 @@ import (
 	"k8s.io/apimachinery/pkg/types"
 	"reconciler.io/runtime/internal"
 	rtime "reconciler.io/runtime/time"
+	"reconciler.io/runtime/validation"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	crlog "sigs.k8s.io/controller-runtime/pkg/log"
 	"sigs.k8s.io/controller-runtime/pkg/webhook"
@@ -115,30 +116,72 @@ func (r *AdmissionWebhookAdapter[T]) init() {
 	})
 }
 
+func (r *AdmissionWebhookAdapter[T]) Validate(ctx context.Context) error {
+	r.init()
+
+	// validate Reconciler value
+	if r.Reconciler == nil {
+		return fmt.Errorf("AdmissionWebhookAdapter %q must define Reconciler", r.Name)
+	}
+	if validation.IsRecursive(ctx) {
+		if v, ok := r.Reconciler.(validation.Validator); ok {
+			if err := v.Validate(ctx); err != nil {
+				return fmt.Errorf("AdmissionWebhookAdapter %q must have a valid Reconciler: %w", r.Name, err)
+			}
+		}
+	}
+
+	return nil
+}
+
+// Deprecated use BuildWithContext
 func (r *AdmissionWebhookAdapter[T]) Build() *admission.Webhook {
+	webhook, err := r.BuildWithContext(context.TODO())
+	if err != nil {
+		panic(err)
+	}
+	return webhook
+}
+
+func (r *AdmissionWebhookAdapter[T]) BuildWithContext(ctx context.Context) (*admission.Webhook, error) {
 	r.init()
+
+	if err := r.Validate(r.withContext(ctx)); err != nil {
+		return nil, err
+	}
+
 	return &admission.Webhook{
 		Handler: r,
 		WithContextFunc: func(ctx context.Context, req *http.Request) context.Context {
+			ctx = r.withContext(ctx)
+
 			log := crlog.FromContext(ctx).
-				WithName("controller-runtime.webhook.webhooks").
-				WithName(r.Name).
 				WithValues(
 					"webhook", req.URL.Path,
 				)
 			ctx = logr.NewContext(ctx, log)
 
-			ctx = WithStash(ctx)
-
-			ctx = StashConfig(ctx, r.Config)
-			ctx = StashOriginalConfig(ctx, r.Config)
-			ctx = StashResourceType(ctx, r.Type)
-			ctx = StashOriginalResourceType(ctx, r.Type)
 			ctx = StashHTTPRequest(ctx, req)
 
 			return ctx
 		},
-	}
+	}, nil
+}
+
+func (r *AdmissionWebhookAdapter[T]) withContext(ctx context.Context) context.Context {
+	log := crlog.FromContext(ctx).
+		WithName("controller-runtime.webhook.webhooks").
+		WithName(r.Name)
+	ctx = logr.NewContext(ctx, log)
+
+	ctx = WithStash(ctx)
+
+	ctx = StashConfig(ctx, r.Config)
+	ctx = StashOriginalConfig(ctx, r.Config)
+	ctx = StashResourceType(ctx, r.Type)
+	ctx = StashOriginalResourceType(ctx, r.Type)
+
+	return ctx
 }
 
 // Handle implements admission.Handler

reconcilers/webhook_test.go

@@ -40,6 +40,7 @@ import (
 	"reconciler.io/runtime/reconcilers"
 	rtesting "reconciler.io/runtime/testing"
 	"reconciler.io/runtime/tracker"
+	"reconciler.io/runtime/validation"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
 )
@@ -351,7 +352,7 @@ func TestAdmissionWebhookAdapter(t *testing.T) {
 				AdmissionResponse: response.DieRelease(),
 			},
 			Prepare: func(t *testing.T, ctx context.Context, tc *rtesting.AdmissionWebhookTestCase) (context.Context, error) {
-				key := "test-key"
+				key := reconcilers.StashKey("test-key")
 				value := "test-value"
 				ctx = context.WithValue(ctx, key, value)
 
@@ -375,13 +376,92 @@ func TestAdmissionWebhookAdapter(t *testing.T) {
 				return ctx, nil
 			},
 		},
+		"invalid nested reconciler": {
+			Skip: true,
+			Request: &admission.Request{
+				AdmissionRequest: request.
+					Object(resource.DieReleaseRawExtension()).
+					DieRelease(),
+			},
+			ExpectedResponse: admission.Response{
+				AdmissionResponse: response.DieRelease(),
+			},
+			Metadata: map[string]interface{}{
+				"SubReconciler": func(t *testing.T, c reconcilers.Config) reconcilers.SubReconciler[*resources.TestResource] {
+					return &reconcilers.SyncReconciler[*resources.TestResource]{
+						// Sync: func(ctx context.Context, resource *resources.TestResource) error {
+						// 	return nil
+						// },
+					}
+				},
+			},
+		},
 	}
 
-	wts.Run(t, scheme, func(t *testing.T, wtc *rtesting.AdmissionWebhookTestCase, c reconcilers.Config) *admission.Webhook {
+	wts.RunWithContext(t, scheme, func(t *testing.T, ctx context.Context, wtc *rtesting.AdmissionWebhookTestCase, c reconcilers.Config) (*admission.Webhook, error) {
 		return (&reconcilers.AdmissionWebhookAdapter[*resources.TestResource]{
 			Type:       &resources.TestResource{},
 			Reconciler: wtc.Metadata["SubReconciler"].(func(*testing.T, reconcilers.Config) reconcilers.SubReconciler[*resources.TestResource])(t, c),
 			Config:     c,
-		}).Build()
+		}).BuildWithContext(ctx)
 	})
 }
+
+func TestAdmissionWebhookAdapter_Validate(t *testing.T) {
+	tests := []struct {
+		name           string
+		webhook        *reconcilers.AdmissionWebhookAdapter[*resources.TestResource]
+		validateNested bool
+		shouldErr      string
+	}{
+		{
+			name: "valid",
+			webhook: &reconcilers.AdmissionWebhookAdapter[*resources.TestResource]{
+				Reconciler: reconcilers.Sequence[*resources.TestResource]{},
+			},
+		},
+		{
+			name: "missing reconciler",
+			webhook: &reconcilers.AdmissionWebhookAdapter[*resources.TestResource]{
+				Name: "missing reconciler",
+			},
+			shouldErr: `AdmissionWebhookAdapter "missing reconciler" must define Reconciler`,
+		},
+		{
+			name: "valid reconciler",
+			webhook: &reconcilers.AdmissionWebhookAdapter[*resources.TestResource]{
+				Reconciler: &reconcilers.SyncReconciler[*resources.TestResource]{
+					Sync: func(ctx context.Context, resource *resources.TestResource) error {
+						return nil
+					},
+				},
+			},
+			validateNested: true,
+		},
+		{
+			name: "invalid reconciler",
+			webhook: &reconcilers.AdmissionWebhookAdapter[*resources.TestResource]{
+				Reconciler: &reconcilers.SyncReconciler[*resources.TestResource]{
+					// Sync: func(ctx context.Context, resource *resources.TestResource) error {
+					// 	return nil
+					// },
+				},
+			},
+			validateNested: true,
+			shouldErr:      `AdmissionWebhookAdapter "TestResourceAdmissionWebhookAdapter" must have a valid Reconciler: SyncReconciler "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 = validation.WithRecursive(ctx)
+			}
+			err := c.webhook.Validate(ctx)
+			if (err != nil) != (c.shouldErr != "") || (c.shouldErr != "" && c.shouldErr != err.Error()) {
+				t.Errorf("validate() error = %q, shouldErr %q", err, c.shouldErr)
+			}
+		})
+	}
+}

testing/webhook.go

@@ -28,6 +28,7 @@ import (
 	"k8s.io/apimachinery/pkg/runtime"
 	"reconciler.io/runtime/reconcilers"
 	rtime "reconciler.io/runtime/time"
+	"reconciler.io/runtime/validation"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
@@ -122,7 +123,7 @@ type AdmissionWebhookTestCase struct {
 // test case.  Test cases are executed in random order.
 type AdmissionWebhookTests map[string]AdmissionWebhookTestCase
 
-// Run executes the test cases.
+// Deprecated use RunWithContext instead
 func (wt AdmissionWebhookTests) Run(t *testing.T, scheme *runtime.Scheme, factory AdmissionWebhookFactory) {
 	t.Helper()
 	wts := AdmissionWebhookTestSuite{}
@@ -133,12 +134,31 @@ func (wt AdmissionWebhookTests) Run(t *testing.T, scheme *runtime.Scheme, factor
 	wts.Run(t, scheme, factory)
 }
 
+// Run executes the test cases.
+func (wt AdmissionWebhookTests) RunWithContext(t *testing.T, scheme *runtime.Scheme, factory AdmissionWebhookFactoryWithContext) {
+	t.Helper()
+	wts := AdmissionWebhookTestSuite{}
+	for name, wtc := range wt {
+		wtc.Name = name
+		wts = append(wts, wtc)
+	}
+	wts.RunWithContext(t, scheme, factory)
+}
+
 // AdmissionWebhookTestSuite represents a list of webhook test cases. The test cases are
 // executed in order.
 type AdmissionWebhookTestSuite []AdmissionWebhookTestCase
 
-// Run executes the test case.
+// Deprecated use RunWithContext instead
 func (tc *AdmissionWebhookTestCase) Run(t *testing.T, scheme *runtime.Scheme, factory AdmissionWebhookFactory) {
+	tc.RunWithContext(t, scheme, func(t *testing.T, ctx context.Context, wtc *AdmissionWebhookTestCase, c reconcilers.Config) (*admission.Webhook, error) {
+		webhook := factory(t, wtc, c)
+		return webhook, nil
+	})
+}
+
+// Run executes the test case.
+func (tc *AdmissionWebhookTestCase) RunWithContext(t *testing.T, scheme *runtime.Scheme, factory AdmissionWebhookFactoryWithContext) {
 	t.Helper()
 	if tc.Skip {
 		t.SkipNow()
@@ -155,6 +175,7 @@ func (tc *AdmissionWebhookTestCase) Run(t *testing.T, scheme *runtime.Scheme, fa
 		ctx, cancel = context.WithDeadline(ctx, deadline)
 		defer cancel()
 	}
+	ctx = validation.WithRecursive(ctx)
 
 	if tc.Metadata == nil {
 		tc.Metadata = map[string]interface{}{}
@@ -199,7 +220,10 @@ func (tc *AdmissionWebhookTestCase) Run(t *testing.T, scheme *runtime.Scheme, fa
 	}
 
 	c := expectConfig.Config()
-	r := factory(t, tc, c)
+	r, err := factory(t, ctx, tc, c)
+	if err != nil {
+		t.Fatalf("Webhook factory failed: %s", err)
+	}
 
 	// Run the Reconcile we're testing.
 	response := func() admission.Response {
@@ -239,8 +263,16 @@ func (tc *AdmissionWebhookTestCase) Run(t *testing.T, scheme *runtime.Scheme, fa
 	expectConfig.AssertExpectations(t)
 }
 
-// Run executes the webhook test suite.
+// Deprecated use RunWithContext instead
 func (ts AdmissionWebhookTestSuite) Run(t *testing.T, scheme *runtime.Scheme, factory AdmissionWebhookFactory) {
+	ts.RunWithContext(t, scheme, func(t *testing.T, ctx context.Context, wtc *AdmissionWebhookTestCase, c reconcilers.Config) (*admission.Webhook, error) {
+		webhook := factory(t, wtc, c)
+		return webhook, nil
+	})
+}
+
+// Run executes the webhook test suite.
+func (ts AdmissionWebhookTestSuite) RunWithContext(t *testing.T, scheme *runtime.Scheme, factory AdmissionWebhookFactoryWithContext) {
 	t.Helper()
 	focused := AdmissionWebhookTestSuite{}
 	for _, test := range ts {
@@ -256,7 +288,7 @@ func (ts AdmissionWebhookTestSuite) Run(t *testing.T, scheme *runtime.Scheme, fa
 	for _, test := range testsToExecute {
 		t.Run(test.Name, func(t *testing.T) {
 			t.Helper()
-			test.Run(t, scheme, factory)
+			test.RunWithContext(t, scheme, factory)
 		})
 	}
 	if len(focused) > 0 {
@@ -265,3 +297,4 @@ func (ts AdmissionWebhookTestSuite) Run(t *testing.T, scheme *runtime.Scheme, fa
 }
 
 type AdmissionWebhookFactory func(t *testing.T, wtc *AdmissionWebhookTestCase, c reconcilers.Config) *admission.Webhook
+type AdmissionWebhookFactoryWithContext func(t *testing.T, ctx context.Context, wtc *AdmissionWebhookTestCase, c reconcilers.Config) (*admission.Webhook, error)