refactor: how handlers are added to server

When adding new handlers I kept forgetting to add it to the meta hander.
Although this commit doesn't really solve it does make it easier to visually see a difference.
It also simplifies adding handlers by not asking for a package variable when calling <patchpackage>.NewPatch.

Author: dkoshkin

cmd/main.go

@@ -24,6 +24,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/manager"
 	metricsserver "sigs.k8s.io/controller-runtime/pkg/metrics/server"
 
+	"github.com/d2iq-labs/capi-runtime-extensions/common/pkg/capi/clustertopology/handlers"
 	"github.com/d2iq-labs/capi-runtime-extensions/common/pkg/capi/clustertopology/handlers/mutation"
 	"github.com/d2iq-labs/capi-runtime-extensions/common/pkg/server"
 	"github.com/d2iq-labs/capi-runtime-extensions/pkg/handlers/auditpolicy"
@@ -111,39 +112,50 @@ func main() {
 		os.Exit(1)
 	}
 
-	runtimeWebhookServer := server.NewServer(
-		runtimeWebhookServerOpts,
-
+	// Handlers for lifecycle hooks.
+	lifeCycleHandlers := []handlers.Named{
 		servicelbgc.New(mgr.GetClient()),
-
-		calico.New(mgr.GetClient(), calicoCNIConfig, clusterconfig.VariableName, "cni"),
-
+	}
+	// Handlers that apply patches to the Cluster object and its objects.
+	// Used by CAPI's GeneratePatches hook.
+	patchHandlers := []handlers.Named{
+		httpproxy.NewPatch(mgr.GetClient()),
+		extraapiservercertsans.NewPatch(),
+		auditpolicy.NewPatch(),
+		kubernetesimageregistry.NewPatch(),
+	}
+	// Handlers used by CAPI's DiscoverVariables hook.
+	// It's ok that this does not match patchHandlers.
+	// Some of those handlers may always get applied and not have a corresponding variable.
+	variableHandlers := []handlers.Named{
 		httpproxy.NewVariable(),
-		httpproxy.NewPatch(mgr.GetClient(), httpproxy.VariableName),
-
 		extraapiservercertsans.NewVariable(),
-		extraapiservercertsans.NewPatch(extraapiservercertsans.VariableName),
-
+		kubernetesimageregistry.NewVariable(),
+	}
+	// This metaPatchHandlers combines all other patch and variable handlers under a single handler.
+	// It allows to specify configuration under a single variable.
+	metaPatchHandlers := []mutation.GeneratePatches{
+		httpproxy.NewMetaPatch(mgr.GetClient()),
+		extraapiservercertsans.NewMetaPatch(),
 		auditpolicy.NewPatch(),
+		kubernetesimageregistry.NewMetaPatch(),
+	}
+	metaHandlers := []handlers.Named{
+		// This Calico handler relies on a variable but does not generate a patch.
+		// Instead it creates other resources in the API.
+		calico.NewMetaHandler(mgr.GetClient(), calicoCNIConfig),
+		clusterconfig.NewVariable(),
+		mutation.NewMetaGeneratePatchesHandler("clusterConfigPatch", metaPatchHandlers...),
+	}
 
-		kubernetesimageregistry.NewVariable(),
-		kubernetesimageregistry.NewPatch(kubernetesimageregistry.VariableName),
+	var allHandlers []handlers.Named
+	allHandlers = append(allHandlers, lifeCycleHandlers...)
+	allHandlers = append(allHandlers, patchHandlers...)
+	allHandlers = append(allHandlers, variableHandlers...)
+	allHandlers = append(allHandlers, metaHandlers...)
+
+	runtimeWebhookServer := server.NewServer(runtimeWebhookServerOpts, allHandlers...)
 
-		clusterconfig.NewVariable(),
-		mutation.NewMetaGeneratePatchesHandler(
-			"clusterConfigPatch",
-			httpproxy.NewPatch(mgr.GetClient(), clusterconfig.VariableName, httpproxy.VariableName),
-			extraapiservercertsans.NewPatch(
-				clusterconfig.VariableName,
-				extraapiservercertsans.VariableName,
-			),
-			auditpolicy.NewPatch(),
-			kubernetesimageregistry.NewPatch(
-				clusterconfig.VariableName,
-				kubernetesimageregistry.VariableName,
-			),
-		),
-	)
 	if err := mgr.Add(runtimeWebhookServer); err != nil {
 		setupLog.Error(err, "unable to add runtime webhook server runnable to controller manager")
 		os.Exit(1)

common/pkg/capi/clustertopology/handlers/mutation/meta.go

@@ -12,6 +12,11 @@ import (
 	"github.com/d2iq-labs/capi-runtime-extensions/common/pkg/capi/clustertopology/handlers"
 )
 
+const (
+	// MetaVariableName is the meta cluster config patch variable name.
+	MetaVariableName = "clusterConfig"
+)
+
 type metaGeneratePatches struct {
 	name            string
 	wrappedHandlers []GeneratePatches

pkg/handlers/clusterconfig/variables.go

@@ -20,9 +20,6 @@ var (
 )
 
 const (
-	// VariableName is http proxy external patch variable name.
-	VariableName = "clusterConfig"
-
 	// HandlerNameVariable is the name of the variable handler.
 	HandlerNameVariable = "ClusterConfigVars"
 )
@@ -43,7 +40,7 @@ func (h *clusterConfigVariableHandler) DiscoverVariables(
 	resp *runtimehooksv1.DiscoverVariablesResponse,
 ) {
 	resp.Variables = append(resp.Variables, clusterv1.ClusterClassVariable{
-		Name:     VariableName,
+		Name:     mutation.MetaVariableName,
 		Required: false,
 		Schema:   v1alpha1.ClusterConfigSpec{}.VariableSchema(),
 	})

pkg/handlers/clusterconfig/variables_test.go

@@ -9,13 +9,14 @@ import (
 	"k8s.io/utils/ptr"
 
 	"github.com/d2iq-labs/capi-runtime-extensions/api/v1alpha1"
+	"github.com/d2iq-labs/capi-runtime-extensions/common/pkg/capi/clustertopology/handlers/mutation"
 	"github.com/d2iq-labs/capi-runtime-extensions/common/pkg/testutils/capitest"
 )
 
 func TestVariableValidation(t *testing.T) {
 	capitest.ValidateDiscoverVariables(
 		t,
-		VariableName,
+		mutation.MetaVariableName,
 		ptr.To(v1alpha1.ClusterConfigSpec{}.VariableSchema()),
 		NewVariable,
 		capitest.VariableTestDef{

pkg/handlers/cni/calico/handler.go

@@ -26,12 +26,17 @@ import (
 	"github.com/d2iq-labs/capi-runtime-extensions/api/v1alpha1"
 	"github.com/d2iq-labs/capi-runtime-extensions/common/pkg/capi/clustertopology/handlers"
 	"github.com/d2iq-labs/capi-runtime-extensions/common/pkg/capi/clustertopology/handlers/lifecycle"
+	"github.com/d2iq-labs/capi-runtime-extensions/common/pkg/capi/clustertopology/handlers/mutation"
 	"github.com/d2iq-labs/capi-runtime-extensions/common/pkg/capi/clustertopology/variables"
 	"github.com/d2iq-labs/capi-runtime-extensions/common/pkg/k8s/client"
 	"github.com/d2iq-labs/capi-runtime-extensions/common/pkg/k8s/parser"
 	"github.com/d2iq-labs/capi-runtime-extensions/pkg/handlers/cni"
 )
 
+const (
+	variableName = "cni"
+)
+
 type CalicoCNIConfig struct {
 	defaultsNamespace string
 
@@ -78,17 +83,15 @@ var (
 	calicoInstallationGK = schema.GroupKind{Group: "operator.tigera.io", Kind: "Installation"}
 )
 
-func New(
+func NewMetaHandler(
 	c ctrlclient.Client,
 	cfg *CalicoCNIConfig,
-	variableName string,
-	variablePath ...string,
 ) *CalicoCNI {
 	return &CalicoCNI{
 		client:       c,
 		config:       cfg,
-		variableName: variableName,
-		variablePath: variablePath,
+		variableName: mutation.MetaVariableName,
+		variablePath: []string{variableName},
 	}
 }
 

pkg/handlers/extraapiservercertsans/inject.go

@@ -41,7 +41,15 @@ var (
 	_ mutation.GeneratePatches = &extraAPIServerCertSANsPatchHandler{}
 )
 
-func NewPatch(
+func NewPatch() *extraAPIServerCertSANsPatchHandler {
+	return newExtraAPIServerCertSANsPatchHandler(variableName)
+}
+
+func NewMetaPatch() *extraAPIServerCertSANsPatchHandler {
+	return newExtraAPIServerCertSANsPatchHandler(mutation.MetaVariableName, variableName)
+}
+
+func newExtraAPIServerCertSANsPatchHandler(
 	variableName string,
 	variableFieldPath ...string,
 ) *extraAPIServerCertSANsPatchHandler {

pkg/handlers/extraapiservercertsans/inject_test.go

@@ -17,15 +17,15 @@ import (
 func TestGeneratePatches(t *testing.T) {
 	capitest.ValidateGeneratePatches(
 		t,
-		func() mutation.GeneratePatches { return NewPatch(VariableName) },
+		func() mutation.GeneratePatches { return NewPatch() },
 		capitest.PatchTestDef{
 			Name: "unset variable",
 		},
 		capitest.PatchTestDef{
 			Name: "extra API server cert SANs set",
 			Vars: []runtimehooksv1.Variable{
 				capitest.VariableWithValue(
-					VariableName,
+					variableName,
 					v1alpha1.ExtraAPIServerCertSANs{"a.b.c.example.com", "d.e.f.example.com"},
 				),
 			},

pkg/handlers/extraapiservercertsans/variables.go

@@ -20,8 +20,8 @@ var (
 )
 
 const (
-	// VariableName is http proxy external patch variable name.
-	VariableName = "extraAPIServerCertSANs"
+	// variableName is http proxy external patch variable name.
+	variableName = "extraAPIServerCertSANs"
 
 	// HandlerNameVariable is the name of the variable handler.
 	HandlerNameVariable = "ExtraAPIServerCertSANsVars"
@@ -43,7 +43,7 @@ func (h *extraAPIServerCertSANsVariableHandler) DiscoverVariables(
 	resp *runtimehooksv1.DiscoverVariablesResponse,
 ) {
 	resp.Variables = append(resp.Variables, clusterv1.ClusterClassVariable{
-		Name:     VariableName,
+		Name:     variableName,
 		Required: false,
 		Schema:   v1alpha1.ExtraAPIServerCertSANs{}.VariableSchema(),
 	})

pkg/handlers/extraapiservercertsans/variables_test.go

@@ -15,7 +15,7 @@ import (
 func TestVariableValidation(t *testing.T) {
 	capitest.ValidateDiscoverVariables(
 		t,
-		VariableName,
+		variableName,
 		ptr.To(v1alpha1.ExtraAPIServerCertSANs{}.VariableSchema()),
 		NewVariable,
 		capitest.VariableTestDef{

pkg/handlers/httpproxy/inject.go

@@ -53,6 +53,18 @@ var (
 
 func NewPatch(
 	cl ctrlclient.Reader,
+) *httpProxyPatchHandler {
+	return newHTTPProxyPatchHandler(cl, variableName)
+}
+
+func NewMetaPatch(
+	cl ctrlclient.Reader,
+) *httpProxyPatchHandler {
+	return newHTTPProxyPatchHandler(cl, mutation.MetaVariableName, variableName)
+}
+
+func newHTTPProxyPatchHandler(
+	cl ctrlclient.Reader,
 	variableName string,
 	variableFieldPath ...string,
 ) *httpProxyPatchHandler {

pkg/handlers/httpproxy/inject_test.go

@@ -22,7 +22,7 @@ func TestGeneratePatches(t *testing.T) {
 		t,
 		func() *httpProxyPatchHandler {
 			fakeClient := fake.NewClientBuilder().Build()
-			return NewPatch(fakeClient, VariableName)
+			return NewPatch(fakeClient)
 		},
 		capitest.PatchTestDef{
 			Name: "unset variable",
@@ -31,7 +31,7 @@ func TestGeneratePatches(t *testing.T) {
 			Name: "http proxy set for KubeadmConfigTemplate default-worker",
 			Vars: []runtimehooksv1.Variable{
 				capitest.VariableWithValue(
-					VariableName,
+					variableName,
 					v1alpha1.HTTPProxy{
 						HTTP:         "http://example.com",
 						HTTPS:        "https://example.com",
@@ -58,7 +58,7 @@ func TestGeneratePatches(t *testing.T) {
 			Name: "http proxy set for KubeadmConfigTemplate generic worker",
 			Vars: []runtimehooksv1.Variable{
 				capitest.VariableWithValue(
-					VariableName,
+					variableName,
 					v1alpha1.HTTPProxy{
 						HTTP:         "http://example.com",
 						HTTPS:        "https://example.com",
@@ -85,7 +85,7 @@ func TestGeneratePatches(t *testing.T) {
 			Name: "http proxy set for KubeadmControlPlaneTemplate",
 			Vars: []runtimehooksv1.Variable{
 				capitest.VariableWithValue(
-					VariableName,
+					variableName,
 					v1alpha1.HTTPProxy{
 						HTTP:         "http://example.com",
 						HTTPS:        "https://example.com",

pkg/handlers/httpproxy/variables.go

@@ -20,8 +20,8 @@ var (
 )
 
 const (
-	// VariableName is http proxy external patch variable name.
-	VariableName = "proxy"
+	// variableName is http proxy external patch variable name.
+	variableName = "proxy"
 
 	// HandlerNameVariable is the name of the variable handler.
 	HandlerNameVariable = "HTTPProxyVars"
@@ -43,7 +43,7 @@ func (h *httpProxyVariableHandler) DiscoverVariables(
 	resp *runtimehooksv1.DiscoverVariablesResponse,
 ) {
 	resp.Variables = append(resp.Variables, clusterv1.ClusterClassVariable{
-		Name:     VariableName,
+		Name:     variableName,
 		Required: false,
 		Schema:   v1alpha1.HTTPProxy{}.VariableSchema(),
 	})

pkg/handlers/httpproxy/variables_test.go

@@ -15,7 +15,7 @@ import (
 func TestVariableValidation(t *testing.T) {
 	capitest.ValidateDiscoverVariables(
 		t,
-		VariableName,
+		variableName,
 		ptr.To(v1alpha1.HTTPProxy{}.VariableSchema()),
 		NewVariable,
 		capitest.VariableTestDef{

pkg/handlers/kubernetesimageregistry/inject.go

@@ -41,7 +41,15 @@ var (
 	_ mutation.GeneratePatches = &imageRegistryPatchHandler{}
 )
 
-func NewPatch(
+func NewPatch() *imageRegistryPatchHandler {
+	return newImageRegistryPatchHandler(variableName)
+}
+
+func NewMetaPatch() *imageRegistryPatchHandler {
+	return newImageRegistryPatchHandler(mutation.MetaVariableName, variableName)
+}
+
+func newImageRegistryPatchHandler(
 	variableName string,
 	variableFieldPath ...string,
 ) *imageRegistryPatchHandler {

pkg/handlers/kubernetesimageregistry/inject_test.go

@@ -17,15 +17,15 @@ import (
 func TestGeneratePatches(t *testing.T) {
 	capitest.ValidateGeneratePatches(
 		t,
-		func() mutation.GeneratePatches { return NewPatch(VariableName) },
+		func() mutation.GeneratePatches { return NewPatch() },
 		capitest.PatchTestDef{
 			Name: "unset variable",
 		},
 		capitest.PatchTestDef{
 			Name: "kubernetesImageRegistry set",
 			Vars: []runtimehooksv1.Variable{
 				capitest.VariableWithValue(
-					VariableName,
+					variableName,
 					v1alpha1.KubernetesImageRegistry("my-registry.io/my-org/my-repo"),
 				),
 			},

pkg/handlers/kubernetesimageregistry/variables.go

@@ -20,8 +20,8 @@ var (
 )
 
 const (
-	// VariableName is http proxy external patch variable name.
-	VariableName = "kubernetesImageRegistry"
+	// variableName is http proxy external patch variable name.
+	variableName = "kubernetesImageRegistry"
 
 	// HandlerNameVariable is the name of the variable handler.
 	HandlerNameVariable = "ImageRegistryVars"
@@ -43,7 +43,7 @@ func (h *imageRegistryVariableHandler) DiscoverVariables(
 	resp *runtimehooksv1.DiscoverVariablesResponse,
 ) {
 	resp.Variables = append(resp.Variables, clusterv1.ClusterClassVariable{
-		Name:     VariableName,
+		Name:     variableName,
 		Required: false,
 		Schema:   v1alpha1.KubernetesImageRegistry("").VariableSchema(),
 	})

pkg/handlers/kubernetesimageregistry/variables_test.go

@@ -15,7 +15,7 @@ import (
 func TestVariableValidation(t *testing.T) {
 	capitest.ValidateDiscoverVariables(
 		t,
-		VariableName,
+		variableName,
 		ptr.To(v1alpha1.KubernetesImageRegistry("").VariableSchema()),
 		NewVariable,
 		capitest.VariableTestDef{