From 4dd68fa9ae01261eb948894f8250d17d3eaf6561 Mon Sep 17 00:00:00 2001 From: Scot Wells Date: Thu, 26 Feb 2026 18:28:27 -0600 Subject: [PATCH] fix(notes): resolve subject API version dynamically instead of hardcoding Notes webhook was failing when creating notes on Domain resources with error "no matches for kind Domain in version networking.datumapis.com/v1alpha1" because the webhook assumed all resources use v1alpha1, but Domain uses v1alpha. Now the webhook discovers the correct API version automatically using the cluster's REST mapper. Co-Authored-By: Claude Opus 4.5 --- .../controller-manager/controllermanager.go | 5 ++++ .../notes/v1alpha1/clusternote_webhook.go | 27 +++++++++++------- .../v1alpha1/clusternote_webhook_test.go | 7 +++-- .../webhooks/notes/v1alpha1/note_webhook.go | 27 +++++++++++------- .../notes/v1alpha1/note_webhook_test.go | 28 +++++++++++++++++-- 5 files changed, 70 insertions(+), 24 deletions(-) diff --git a/cmd/milo/controller-manager/controllermanager.go b/cmd/milo/controller-manager/controllermanager.go index 7de15bb9..87bf5e30 100644 --- a/cmd/milo/controller-manager/controllermanager.go +++ b/cmd/milo/controller-manager/controllermanager.go @@ -466,6 +466,11 @@ func Run(ctx context.Context, c *config.CompletedConfig, opts *Options) error { Metrics: metricsserver.Options{ BindAddress: "0", }, + // Use the same REST mapper as the controller context to share the + // cached API discovery across the webhook manager and controllers. + MapperProvider: func(c *restclient.Config, httpClient *http.Client) (meta.RESTMapper, error) { + return controllerContext.RESTMapper, nil + }, WebhookServer: webhook.NewServer(webhook.Options{ Port: opts.ControllerRuntimeWebhookPort, CertDir: opts.SecureServing.ServerCert.CertDirectory, diff --git a/internal/webhooks/notes/v1alpha1/clusternote_webhook.go b/internal/webhooks/notes/v1alpha1/clusternote_webhook.go index dbf5643b..5559a8d5 100644 --- a/internal/webhooks/notes/v1alpha1/clusternote_webhook.go +++ b/internal/webhooks/notes/v1alpha1/clusternote_webhook.go @@ -8,6 +8,7 @@ import ( iamv1alpha1 "go.miloapis.com/milo/pkg/apis/iam/v1alpha1" notesv1alpha1 "go.miloapis.com/milo/pkg/apis/notes/v1alpha1" "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/api/meta" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" @@ -27,8 +28,9 @@ func SetupClusterNoteWebhooksWithManager(mgr ctrl.Manager) error { return ctrl.NewWebhookManagedBy(mgr). For(¬esv1alpha1.ClusterNote{}). WithDefaulter(&ClusterNoteMutator{ - Client: mgr.GetClient(), - Scheme: mgr.GetScheme(), + Client: mgr.GetClient(), + Scheme: mgr.GetScheme(), + RESTMapper: mgr.GetRESTMapper(), }). WithValidator(&ClusterNoteValidator{ Client: mgr.GetClient(), @@ -39,8 +41,9 @@ func SetupClusterNoteWebhooksWithManager(mgr ctrl.Manager) error { // +kubebuilder:webhook:path=/mutate-notes-miloapis-com-v1alpha1-clusternote,mutating=true,failurePolicy=fail,sideEffects=None,groups=notes.miloapis.com,resources=clusternotes,verbs=create,versions=v1alpha1,name=mclusternote.notes.miloapis.com,admissionReviewVersions={v1,v1beta1},serviceName=milo-controller-manager,servicePort=9443,serviceNamespace=milo-system type ClusterNoteMutator struct { - Client client.Client - Scheme *runtime.Scheme + Client client.Client + Scheme *runtime.Scheme + RESTMapper meta.RESTMapper } var _ admission.CustomDefaulter = &ClusterNoteMutator{} @@ -83,15 +86,19 @@ func (m *ClusterNoteMutator) setSubjectOwnerReference(ctx context.Context, clust return nil // Subject is namespaced, can't set owner reference on cluster-scoped resource } - // Get the subject resource - gvk := schema.GroupVersionKind{ - Group: clusterNote.Spec.SubjectRef.APIGroup, - Version: "v1alpha1", // Assuming v1alpha1, this could be made more flexible - Kind: clusterNote.Spec.SubjectRef.Kind, + // Resolve the GVK using REST mapper to discover the correct API version + groupKind := schema.GroupKind{ + Group: clusterNote.Spec.SubjectRef.APIGroup, + Kind: clusterNote.Spec.SubjectRef.Kind, + } + + mapping, err := m.RESTMapper.RESTMapping(groupKind) + if err != nil { + return fmt.Errorf("failed to get REST mapping for %s: %w", groupKind, err) } subject := &unstructured.Unstructured{} - subject.SetGroupVersionKind(gvk) + subject.SetGroupVersionKind(mapping.GroupVersionKind) if err := m.Client.Get(ctx, types.NamespacedName{ Name: clusterNote.Spec.SubjectRef.Name, diff --git a/internal/webhooks/notes/v1alpha1/clusternote_webhook_test.go b/internal/webhooks/notes/v1alpha1/clusternote_webhook_test.go index 685e875c..e3a4b0af 100644 --- a/internal/webhooks/notes/v1alpha1/clusternote_webhook_test.go +++ b/internal/webhooks/notes/v1alpha1/clusternote_webhook_test.go @@ -16,6 +16,8 @@ import ( "sigs.k8s.io/controller-runtime/pkg/webhook/admission" ) +// Note: newTestRESTMapper is defined in note_webhook_test.go and shared across tests in this package + func TestClusterNoteMutator_Default(t *testing.T) { tests := map[string]struct { clusterNote *notesv1alpha1.ClusterNote @@ -124,8 +126,9 @@ func TestClusterNoteMutator_Default(t *testing.T) { fakeClient := fake.NewClientBuilder().WithScheme(testScheme).WithObjects(objects...).Build() mutator := &ClusterNoteMutator{ - Client: fakeClient, - Scheme: testScheme, + Client: fakeClient, + Scheme: testScheme, + RESTMapper: newTestRESTMapper(), } // Create admission request context diff --git a/internal/webhooks/notes/v1alpha1/note_webhook.go b/internal/webhooks/notes/v1alpha1/note_webhook.go index d74a82e8..298294fd 100644 --- a/internal/webhooks/notes/v1alpha1/note_webhook.go +++ b/internal/webhooks/notes/v1alpha1/note_webhook.go @@ -8,6 +8,7 @@ import ( iamv1alpha1 "go.miloapis.com/milo/pkg/apis/iam/v1alpha1" notesv1alpha1 "go.miloapis.com/milo/pkg/apis/notes/v1alpha1" "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/api/meta" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" @@ -27,8 +28,9 @@ func SetupNoteWebhooksWithManager(mgr ctrl.Manager) error { return ctrl.NewWebhookManagedBy(mgr). For(¬esv1alpha1.Note{}). WithDefaulter(&NoteMutator{ - Client: mgr.GetClient(), - Scheme: mgr.GetScheme(), + Client: mgr.GetClient(), + Scheme: mgr.GetScheme(), + RESTMapper: mgr.GetRESTMapper(), }). WithValidator(&NoteValidator{ Client: mgr.GetClient(), @@ -39,8 +41,9 @@ func SetupNoteWebhooksWithManager(mgr ctrl.Manager) error { // +kubebuilder:webhook:path=/mutate-notes-miloapis-com-v1alpha1-note,mutating=true,failurePolicy=fail,sideEffects=None,groups=notes.miloapis.com,resources=notes,verbs=create,versions=v1alpha1,name=mnote.notes.miloapis.com,admissionReviewVersions={v1,v1beta1},serviceName=milo-controller-manager,servicePort=9443,serviceNamespace=milo-system type NoteMutator struct { - Client client.Client - Scheme *runtime.Scheme + Client client.Client + Scheme *runtime.Scheme + RESTMapper meta.RESTMapper } var _ admission.CustomDefaulter = &NoteMutator{} @@ -83,15 +86,19 @@ func (m *NoteMutator) setSubjectOwnerReference(ctx context.Context, note *notesv return nil } - // Get the subject resource - gvk := schema.GroupVersionKind{ - Group: note.Spec.SubjectRef.APIGroup, - Version: "v1alpha1", // Assuming v1alpha1, this could be made more flexible - Kind: note.Spec.SubjectRef.Kind, + // Resolve the GVK using REST mapper to discover the correct API version + groupKind := schema.GroupKind{ + Group: note.Spec.SubjectRef.APIGroup, + Kind: note.Spec.SubjectRef.Kind, + } + + mapping, err := m.RESTMapper.RESTMapping(groupKind) + if err != nil { + return fmt.Errorf("failed to get REST mapping for %s: %w", groupKind, err) } subject := &unstructured.Unstructured{} - subject.SetGroupVersionKind(gvk) + subject.SetGroupVersionKind(mapping.GroupVersionKind) if err := m.Client.Get(ctx, types.NamespacedName{ Name: note.Spec.SubjectRef.Name, diff --git a/internal/webhooks/notes/v1alpha1/note_webhook_test.go b/internal/webhooks/notes/v1alpha1/note_webhook_test.go index d11b8c96..684b010a 100644 --- a/internal/webhooks/notes/v1alpha1/note_webhook_test.go +++ b/internal/webhooks/notes/v1alpha1/note_webhook_test.go @@ -8,9 +8,11 @@ import ( "github.com/stretchr/testify/require" iamv1alpha1 "go.miloapis.com/milo/pkg/apis/iam/v1alpha1" notesv1alpha1 "go.miloapis.com/milo/pkg/apis/notes/v1alpha1" + "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" utilruntime "k8s.io/apimachinery/pkg/util/runtime" "sigs.k8s.io/controller-runtime/pkg/client" @@ -25,6 +27,27 @@ func init() { utilruntime.Must(notesv1alpha1.AddToScheme(testScheme)) } +// newTestRESTMapper creates a RESTMapper for testing that knows about common test resources +func newTestRESTMapper() meta.RESTMapper { + mapper := meta.NewDefaultRESTMapper([]schema.GroupVersion{ + {Group: "networking.datumapis.com", Version: "v1alpha1"}, + {Group: "resourcemanager.miloapis.com", Version: "v1alpha1"}, + }) + // Register Domain as a namespaced resource + mapper.Add(schema.GroupVersionKind{ + Group: "networking.datumapis.com", + Version: "v1alpha1", + Kind: "Domain", + }, meta.RESTScopeNamespace) + // Register Organization as a cluster-scoped resource + mapper.Add(schema.GroupVersionKind{ + Group: "resourcemanager.miloapis.com", + Version: "v1alpha1", + Kind: "Organization", + }, meta.RESTScopeRoot) + return mapper +} + func TestNoteMutator_Default(t *testing.T) { tests := map[string]struct { note *notesv1alpha1.Note @@ -137,8 +160,9 @@ func TestNoteMutator_Default(t *testing.T) { fakeClient := fake.NewClientBuilder().WithScheme(testScheme).WithObjects(objects...).Build() mutator := &NoteMutator{ - Client: fakeClient, - Scheme: testScheme, + Client: fakeClient, + Scheme: testScheme, + RESTMapper: newTestRESTMapper(), } // Create admission request context