refactor: webhook migration runtime swap

[tnsimon]

Reason for Change:
Wire up the new webhook controller runtime for #1981

Requirements

• added unit tests and e2e tests (if applicable).

Issue Fixed:

Notes for Reviewers:

[kaito-pr-agent]

Title

(Describe updated until commit f3580d7)

refactor: migrate webhook infrastructure from Knative to controller-runtime

---

Description

• Migrate webhook infrastructure from Knative to controller-runtime.

• Implement local FieldError shim to replace knative.dev/pkg/apis.

• Add Secret-informer-based certificate loader for dynamic TLS.

• Update Workspace and RAGEngine webhooks to use new validator interface.

---

Changes walkthrough 📝

	Relevant files
Enhancement	6 files main.go Migrate webhook server setup to controller-runtime              +142/-22 main.go Migrate webhook server setup to controller-runtime              +121/-17 fielderror.go Implement FieldError shim for validation                                  +284/-12 webhooks.go Migrate Workspace webhooks to controller-runtime                  +90/-37  webhooks.go Migrate RAGEngine webhooks to controller-runtime                  +46/-21  loader.go Implement Secret-informer-based certificate loader              +92/-0
Tests	2 files fielderror_test.go Add unit tests for FieldError shim                                              +174/-0  loader_test.go Add unit tests for certificate loader                                        +244/-0
Additional files	16 files inference_config_validation.go +1/-1      inferenceset_validation.go +2/-1      ragengine_validation.go +1/-1      tuning_config_validation.go +1/-1      workspace_validation.go +1/-1      inference_config_validation.go +1/-1      ragengine_validation.go +1/-1      tuning_config_validation.go +1/-1      workspace_validation.go +1/-1      workspace_validation_test.go +1/-1      webhooks.yaml +2/-0      webhooks.yaml +27/-3    go.mod +3/-21    go.sum +10/-531 preset_tuning.go +1/-1      webhooks_test.go +58/-63

---

Need help?

Type /help how to ... in the comments thread for any questions about PR-Agent usage.

Check out the documentation for more information.

[kaito-pr-agent]

PR Reviewer Guide 🔍

(Review updated until commit f3580d7)

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 5 🔵🔵🔵🔵🔵

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Cert Rotator Readiness The rotator.AddRotator is configured with IsReady: certReady. The comments indicate certReady may never close if the cert-controller does not trigger a refresh (e.g., existing valid cert). If the rotator or manager relies on this channel closing to signal readiness or complete initialization, the pod may remain in a non-ready state indefinitely. Verify that the rotator's readiness logic does not block the manager's readiness probes or startup sequence given this design choice. if err := rotator.AddRotator(mgr, &rotator.CertRotator{ SecretKey: types.NamespacedName{ Namespace: webhookNamespace, Name: webhookSecretName, }, CertDir: webhookCertDir, CAName: "kaito-workspace-ca", CAOrganization: "kaito", DNSName: fmt.Sprintf("%s.%s.svc", webhookServiceName, webhookNamespace), IsReady: certReady, Webhooks: webhookInfos, }); err != nil { klog.ErrorS(err, "unable to set up cert rotator") exitWithErrorFunc() FieldError Compatibility The FieldError implementation replaces Knative's apis.FieldError. The Error() method flattens and formats errors. Ensure the output string format (message, paths, details) matches Knative's exactly to avoid breaking any external tools or parsers that rely on the specific error string format. The flatten and dedupe logic adds complexity that should be thoroughly tested against existing validation error expectations. // Error implements the error interface. It flattens the receiver and any // accumulated sibling errors into a single newline-separated message in the // same general shape as knative.dev/pkg/apis.FieldError.Error. func (fe *FieldError) Error() string { if fe == nil { return "" } all := fe.flatten() if len(all) == 0 { return "" } // Stable order so test assertions and log output are deterministic. sort.SliceStable(all, func(i, j int) bool { if all[i].Message != all[j].Message { return all[i].Message < all[j].Message } return strings.Join(all[i].Paths, ",") < strings.Join(all[j].Paths, ",") }) parts := make([]string, 0, len(all)) for _, e := range all { parts = append(parts, e.formatLeaf()) } return strings.Join(parts, "\n") } Informer Cache Sync Timeout The code waits for the Secret informer cache to sync using cache.WaitForCacheSync with the shutdown context. If the informer fails to sync (e.g., RBAC issues, network latency) before the shutdown signal, the process exits. Ensure this timeout behavior is acceptable and that RBAC permissions for the Secret are correctly configured in the deployment manifests. if !cache.WaitForCacheSync(ctx.Done(), secretInformer.HasSynced) { klog.ErrorS(fmt.Errorf("timed out waiting for informer cache sync"), "unable to sync webhook cert informer") exitWithErrorFunc() }

[kaito-pr-agent]

PR Code Suggestions ✨

Latest suggestions up to f3580d7
Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Close certReady channel to prevent cert rotator deadlock The certReady channel is created but never closed, which can cause the cert rotator to hang indefinitely. The comments explain that cert-controller's IsReady predicate depends on ensureCertsMounted, which may never write to disk if the Secret already has valid material. Consider closing the channel after the Secret-informer-backed GetCertificate callback successfully loads the first certificate, or remove the IsReady field entirely since the webhook server can start without blocking on cert readiness. cmd/workspace/main.go [237-426] certReady := make(chan struct{}) +close(certReady) // Close immediately since we don't block on cert readiness ... if err := rotator.AddRotator(mgr, &rotator.CertRotator{ ... IsReady: certReady, ... }); err != nil { Suggestion importance[1-10]: 8 __ Why: The certReady channel is passed to rotator.AddRotator as IsReady. If the rotator waits for this channel to close to signal readiness, and it is never closed (as noted in comments regarding cert-controller's ensureCertsMounted), the manager could hang indefinitely. Closing it immediately aligns with the comment "we no longer block webhook registration on it" and prevents a potential startup deadlock.	Medium
	Recursively flatten nested errors in ViaField The comment claims child.errors is "already flattened by Also" but this is not guaranteed. If ViaField is called on an error created with Also, the nested errors may not be flattened. Recursively flatten the child errors to ensure consistency, or document that callers must flatten before calling ViaField. pkg/apis/fielderror.go [190-196] out.errors[i] = FieldError{ Message: child.Message, Paths: prependPaths(pfx, child.Paths), Details: child.Details, - errors: child.errors, // already flattened by Also + errors: flattenErrors(child.errors), // Ensure nested errors are flattened } Suggestion importance[1-10]: 3 __ Why: The comment claims child.errors is "already flattened by Also", which is technically inaccurate as Also merges lists but doesn't recursively flatten nested errors fields. However, since Error() calls flatten() which handles recursion, this is not a functional bug, just a structural inconsistency. The fix is low impact.	Low
General	Test WithBaseline handles nil context safely Add a test case for WithBaseline(nilCtx, original) to ensure it doesn't panic when passed a nil context. The current test only checks GetBaseline(nilCtx), but WithBaseline should also handle nil contexts gracefully. pkg/apis/fielderror_test.go [139-162] func TestBaselineRoundTrip(t *testing.T) { type obj struct{ Name string } original := &obj{Name: "old"} if got := GetBaseline(context.Background()); got != nil { t.Errorf("expected nil baseline on bare context, got %v", got) } ctx := WithBaseline(context.Background(), original) got := GetBaseline(ctx) if got == nil { t.Fatalf("expected non-nil baseline") } if got.(*obj) != original { t.Errorf("baseline round trip mismatch: %v vs %v", got, original) } // nil ctx must not panic. We construct it via a variable so static // analyzers don't flag the literal nil context argument. var nilCtx context.Context if got := GetBaseline(nilCtx); got != nil { t.Errorf("expected nil baseline from nil ctx, got %v", got) } + if got := WithBaseline(nilCtx, original); got == nil { + t.Errorf("expected non-nil context from WithBaseline(nilCtx, original)") + } } Suggestion importance[1-10]: 7 __ Why: Adds valuable test coverage for edge case handling. Improves robustness without fixing a critical bug.	Medium
	Verify baseline context is used in validation The WithinUpdate call is present but verify that the underlying RAGEngine.Validate(ctx) method actually uses apis.GetBaseline(ctx) to access the old object. If it doesn't, the baseline context won't be utilized and update validation may not work as intended. pkg/ragengine/webhooks/webhooks.go [47-57] +func (ragEngineValidator) ValidateUpdate(ctx context.Context, oldObj, newObj runtime.Object) (admission.Warnings, error) { + r, ok := newObj.(*kaitov1beta1.RAGEngine) + if !ok { + return nil, fmt.Errorf("expected v1beta1.RAGEngine, got %T", newObj) + } + ctx = kaitoapis.WithinUpdate(ctx, oldObj) + if fe := r.Validate(ctx); fe != nil { + return nil, fmt.Errorf("RAGEngine validation failed: %w", fe) + } + return nil, nil +} - Suggestion importance[1-10]: 6 __ Why: Verification suggestion with no code change proposed. Valid architectural check but low impact.	Low
	Verify v1alpha1 validation handles baseline context Ensure the v1alpha1 Workspace validator properly handles the baseline context for updates. The validate closure should work correctly since it delegates to ws.Validate(ctx), but verify the v1alpha1 Validate method uses apis.GetBaseline(ctx) for update comparisons. pkg/workspace/webhooks/webhooks.go [88-99] +if err := setupValidator(mgr, &kaitov1alpha1.Workspace{}, &kaitoValidator{ + kind: "Workspace", + validate: func(ctx context.Context, obj runtime.Object) *kaitoapis.FieldError { + ws, ok := obj.(*kaitov1alpha1.Workspace) + if !ok { + return kaitoapis.ErrGeneric(fmt.Sprintf("expected v1alpha1.Workspace, got %T", obj)) + } + return ws.Validate(ctx) + }, +}); err != nil { + return fmt.Errorf("register v1alpha1 Workspace webhook: %w", err) +} - Suggestion importance[1-10]: 6 __ Why: Verification suggestion with no code change proposed. Valid architectural check but low impact.	Low

---

Previous suggestions

Suggestions up to commit ca82959

Category	Suggestion	Impact
Possible issue	Validate webhook namespace environment variable If WebhookNamespace environment variable is not set, webhookNamespace will be empty, resulting in an invalid DNS name like service..svc. Validate that the namespace is not empty before proceeding. cmd/workspace/main.go [336-337] webhookNamespace := os.Getenv(WebhookNamespace) +if webhookNamespace == "" { + klog.ErrorS(nil, "WEBHOOK_NAMESPACE environment variable is not set") + exitWithErrorFunc() +} webhookServiceName := os.Getenv(WebhookServiceName) -// ... -DNSName: fmt.Sprintf("%s.%s.svc", webhookServiceName, webhookNamespace), Suggestion importance[1-10]: 8 __ Why: Validates required environment variables early to prevent invalid DNS names and cryptic errors later in the startup process.	Medium
	Validate old object type before context setup Add type checking for oldObj before passing it to WithinUpdate. If the apiserver dispatches an old object of the wrong type, WithinUpdate may panic or behave unexpectedly. This mirrors the type safety pattern used for newObj. pkg/ragengine/webhooks/webhooks.go [47-52] func (ragEngineValidator) ValidateUpdate(ctx context.Context, oldObj, newObj runtime.Object) (admission.Warnings, error) { r, ok := newObj.(*kaitov1beta1.RAGEngine) if !ok { return nil, fmt.Errorf("expected v1beta1.RAGEngine, got %T", newObj) } + if _, ok := oldObj.(*kaitov1beta1.RAGEngine); !ok { + return nil, fmt.Errorf("expected v1beta1.RAGEngine for old object, got %T", oldObj) + } ctx = kaitoapis.WithinUpdate(ctx, oldObj) Suggestion importance[1-10]: 8 __ Why: The Validate method performs a type assertion base.(*RAGEngine) which will panic if oldObj is not the correct type. Adding the check prevents this panic.	Medium
	Add type check before webhook server cast *The type assertion to webhook.DefaultServer is fragile and will panic if the manager uses a custom webhook server implementation. Add a type check to handle this gracefully and prevent a runtime panic. cmd/workspace/main.go [342-343] -mgr.GetWebhookServer().(*webhook.DefaultServer).Options.CertDir = webhookCertDir -mgr.GetWebhookServer().(*webhook.DefaultServer).Options.Port = p +server, ok := mgr.GetWebhookServer().(*webhook.DefaultServer) +if !ok { + klog.ErrorS(nil, "webhook server is not DefaultServer", "type", fmt.Sprintf("%T", mgr.GetWebhookServer())) + exitWithErrorFunc() +} +server.Options.CertDir = webhookCertDir +server.Options.Port = p Suggestion importance[1-10]: 7 __ Why: Prevents potential runtime panic if the webhook server implementation differs from DefaultServer, improving robustness.	Medium