Merge pull request #9338 from killianmuldoon/pr-extensionconfig-reconcile-speed

🌱 Improve ClusterClass watch for ExtensionConfigs

Author: k8s-ci-robot

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
+				}
 			}
 		}
 	}

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,71 @@ 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{
+			{NamespacedName: types.NamespacedName{Namespace: onePatchClusterClass.Namespace, Name: onePatchClusterClass.Name}},
+			{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{
+			{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)
+		}
+	})
+}

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 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
+}

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)
+			}
+		})
+	}
+}