From a57ade914657e6d7a2740bf728fea1a8e9eec0f5 Mon Sep 17 00:00:00 2001 From: killianmuldoon Date: Tue, 29 Aug 2023 17:09:31 +0100 Subject: [PATCH] Improve ClusterClass reconcile with RuntimeExtensions at scale Signed-off-by: killianmuldoon --- .../clusterclass/clusterclass_controller.go | 14 +++- .../clusterclass_controller_test.go | 73 +++++++++++++++++++ internal/runtime/client/client.go | 23 +++++- internal/runtime/client/client_test.go | 68 +++++++++++++++++ 4 files changed, 174 insertions(+), 4 deletions(-) diff --git a/internal/controllers/clusterclass/clusterclass_controller.go b/internal/controllers/clusterclass/clusterclass_controller.go index d9b86f6fffbc..a2823ae6a9fb 100644 --- a/internal/controllers/clusterclass/clusterclass_controller.go +++ b/internal/controllers/clusterclass/clusterclass_controller.go @@ -398,7 +398,7 @@ func uniqueObjectRefKey(ref *corev1.ObjectReference) string { // of the ExtensionConfig. func (r *Reconciler) extensionConfigToClusterClass(ctx context.Context, o client.Object) []reconcile.Request { res := []ctrl.Request{} - + log := ctrl.LoggerFrom(ctx) ext, ok := o.(*runtimev1.ExtensionConfig) if !ok { panic(fmt.Sprintf("Expected an ExtensionConfig but got a %T", o)) @@ -418,8 +418,16 @@ func (r *Reconciler) extensionConfigToClusterClass(ctx context.Context, o client } for _, patch := range clusterClass.Spec.Patches { if patch.External != nil && patch.External.DiscoverVariablesExtension != nil { - res = append(res, ctrl.Request{NamespacedName: client.ObjectKey{Namespace: clusterClass.Namespace, Name: clusterClass.Name}}) - break + extName, err := runtimeclient.ExtensionNameFromHandlerName(*patch.External.DiscoverVariablesExtension) + if err != nil { + log.Error(err, "failed to reconcile ClusterClass for ExtensionConfig") + continue + } + if extName == ext.Name { + res = append(res, ctrl.Request{NamespacedName: client.ObjectKey{Namespace: clusterClass.Namespace, Name: clusterClass.Name}}) + // Once we've added the ClusterClass once we can break here. + break + } } } } diff --git a/internal/controllers/clusterclass/clusterclass_controller_test.go b/internal/controllers/clusterclass/clusterclass_controller_test.go index 0180bbf799eb..ce3b3d4effc9 100644 --- a/internal/controllers/clusterclass/clusterclass_controller_test.go +++ b/internal/controllers/clusterclass/clusterclass_controller_test.go @@ -28,11 +28,15 @@ import ( "github.com/pkg/errors" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" utilfeature "k8s.io/component-base/featuregate/testing" "k8s.io/utils/pointer" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/reconcile" clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" + runtimev1 "sigs.k8s.io/cluster-api/exp/runtime/api/v1alpha1" runtimecatalog "sigs.k8s.io/cluster-api/exp/runtime/catalog" runtimehooksv1 "sigs.k8s.io/cluster-api/exp/runtime/hooks/api/v1alpha1" "sigs.k8s.io/cluster-api/feature" @@ -545,3 +549,72 @@ func TestReconciler_reconcileVariables(t *testing.T) { }) } } + +func TestReconciler_extensionConfigToClusterClass(t *testing.T) { + firstExtConfig := &runtimev1.ExtensionConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "runtime1", + }, + TypeMeta: metav1.TypeMeta{ + Kind: "ExtensionConfig", + APIVersion: runtimev1.GroupVersion.String(), + }, + Spec: runtimev1.ExtensionConfigSpec{ + NamespaceSelector: &metav1.LabelSelector{}, + }, + } + secondExtConfig := &runtimev1.ExtensionConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "runtime2", + }, + TypeMeta: metav1.TypeMeta{ + Kind: "ExtensionConfig", + APIVersion: runtimev1.GroupVersion.String(), + }, + Spec: runtimev1.ExtensionConfigSpec{ + NamespaceSelector: &metav1.LabelSelector{}, + }, + } + + // These ClusterClasses will be reconciled as they both reference the passed ExtensionConfig `runtime1`. + onePatchClusterClass := builder.ClusterClass(metav1.NamespaceDefault, "cc1"). + WithPatches([]clusterv1.ClusterClassPatch{ + {External: &clusterv1.ExternalPatchDefinition{DiscoverVariablesExtension: pointer.String("discover-variables.runtime1")}}}). + Build() + twoPatchClusterClass := builder.ClusterClass(metav1.NamespaceDefault, "cc2"). + WithPatches([]clusterv1.ClusterClassPatch{ + {External: &clusterv1.ExternalPatchDefinition{DiscoverVariablesExtension: pointer.String("discover-variables.runtime1")}}, + {External: &clusterv1.ExternalPatchDefinition{DiscoverVariablesExtension: pointer.String("discover-variables.runtime2")}}}). + Build() + + // This ClusterClasses will not be reconciled as it does not reference the passed ExtensionConfig `runtime1`. + notReconciledClusterClass := builder.ClusterClass(metav1.NamespaceDefault, "cc3"). + WithPatches([]clusterv1.ClusterClassPatch{ + {External: &clusterv1.ExternalPatchDefinition{DiscoverVariablesExtension: pointer.String("discover-variables.other-runtime-class")}}}). + Build() + + t.Run("test", func(t *testing.T) { + fakeClient := fake.NewClientBuilder().WithObjects(onePatchClusterClass, notReconciledClusterClass, twoPatchClusterClass).Build() + r := &Reconciler{ + Client: fakeClient, + } + + // Expect both onePatchClusterClass and twoPatchClusterClass to trigger a reconcile as both reference ExtensionCopnfig `runtime1`. + firstExtConfigExpected := []reconcile.Request{ + reconcile.Request{NamespacedName: types.NamespacedName{Namespace: onePatchClusterClass.Namespace, Name: onePatchClusterClass.Name}}, + reconcile.Request{NamespacedName: types.NamespacedName{Namespace: twoPatchClusterClass.Namespace, Name: twoPatchClusterClass.Name}}, + } + if got := r.extensionConfigToClusterClass(context.Background(), firstExtConfig); !reflect.DeepEqual(got, firstExtConfigExpected) { + t.Errorf("extensionConfigToClusterClass() = %v, want %v", got, firstExtConfigExpected) + } + + // Expect only twoPatchClusterClass to trigger a reconcile as it's the only class with a reference to ExtensionCopnfig `runtime2`. + secondExtConfigExpected := []reconcile.Request{ + reconcile.Request{NamespacedName: types.NamespacedName{Namespace: twoPatchClusterClass.Namespace, Name: twoPatchClusterClass.Name}}, + } + if got := r.extensionConfigToClusterClass(context.Background(), secondExtConfig); !reflect.DeepEqual(got, secondExtConfigExpected) { + t.Errorf("extensionConfigToClusterClass() = %v, want %v", got, secondExtConfigExpected) + } + + }) +} diff --git a/internal/runtime/client/client.go b/internal/runtime/client/client.go index a6cd941a32c8..c9f643dddd7e 100644 --- a/internal/runtime/client/client.go +++ b/internal/runtime/client/client.go @@ -154,10 +154,14 @@ func (c *client) Discover(ctx context.Context, extensionConfig *runtimev1.Extens modifiedExtensionConfig.Status.Handlers = []runtimev1.ExtensionHandler{} for _, handler := range response.Handlers { + handlerName, err := NameForHandler(handler, extensionConfig) + if err != nil { + return nil, errors.Wrapf(err, "failed to discover extension %q", extensionConfig.Name) + } modifiedExtensionConfig.Status.Handlers = append( modifiedExtensionConfig.Status.Handlers, runtimev1.ExtensionHandler{ - Name: handler.Name + "." + extensionConfig.Name, // Uniquely identifies a handler of an Extension. + Name: handlerName, // Uniquely identifies a handler of an Extension. RequestHook: runtimev1.GroupVersionHook{ APIVersion: handler.RequestHook.APIVersion, Hook: handler.RequestHook.Hook, @@ -655,3 +659,20 @@ func (c *client) matchNamespace(ctx context.Context, selector labels.Selector, n return selector.Matches(labels.Set(ns.GetLabels())), nil } + +// NameForHandler constructs a canonical name for a registered runtime extension handler. +func NameForHandler(handler runtimehooksv1.ExtensionHandler, extensionConfig *runtimev1.ExtensionConfig) (string, error) { + if extensionConfig == nil { + return "", errors.New("extensionConfig was nil") + } + return fmt.Sprint(handler.Name + "." + extensionConfig.Name), nil +} + +// ExtensionNameFromHandlerName extracts the extension name from the canonical name of a registered runtime extension handler. +func ExtensionNameFromHandlerName(registeredHandlerName string) (string, error) { + parts := strings.Split(registeredHandlerName, ".") + if len(parts) != 2 { + return "", errors.Errorf("registered handler name %s was not in the expected format (`HANDLER_NAME.EXTENSION_NAME)", registeredHandlerName) + } + return parts[1], nil +} diff --git a/internal/runtime/client/client_test.go b/internal/runtime/client/client_test.go index 89d04899a63e..347780d935e4 100644 --- a/internal/runtime/client/client_test.go +++ b/internal/runtime/client/client_test.go @@ -1334,3 +1334,71 @@ func newUnstartedTLSServer(handler http.Handler) *httptest.Server { } return srv } + +func TestNameForHandler(t *testing.T) { + tests := []struct { + name string + handler runtimehooksv1.ExtensionHandler + extensionConfig *runtimev1.ExtensionConfig + want string + wantErr bool + }{ + { + name: "return well formatted name", + handler: runtimehooksv1.ExtensionHandler{Name: "discover-variables"}, + extensionConfig: &runtimev1.ExtensionConfig{ObjectMeta: metav1.ObjectMeta{Name: "runtime1"}}, + want: "discover-variables.runtime1", + }, + { + name: "return well formatted name", + handler: runtimehooksv1.ExtensionHandler{Name: "discover-variables"}, + extensionConfig: nil, + wantErr: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := NameForHandler(tt.handler, tt.extensionConfig) + if (err != nil) != tt.wantErr { + t.Errorf("NameForHandler() error = %v, wantErr %v", err, tt.wantErr) + return + } + if got != tt.want { + t.Errorf("NameForHandler() got = %v, want %v", got, tt.want) + } + }) + } +} + +func TestExtensionNameFromHandlerName(t *testing.T) { + tests := []struct { + name string + registeredHandlerName string + want string + wantErr bool + }{ + { + name: "Get name from correctly formatted handler name", + registeredHandlerName: "discover-variables.runtime1", + want: "runtime1", + }, + { + name: "error from incorrectly formatted handler name", + // Two periods make this name badly formed. + registeredHandlerName: "discover-variables.runtime.1", + wantErr: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := ExtensionNameFromHandlerName(tt.registeredHandlerName) + if (err != nil) != tt.wantErr { + t.Errorf("ExtensionNameFromHandlerName() error = %v, wantErr %v", err, tt.wantErr) + return + } + if got != tt.want { + t.Errorf("ExtensionNameFromHandlerName() got = %v, want %v", got, tt.want) + } + }) + } +}