refactor: how handlers are added to server (#154)

Co-authored-by: Jimmi Dyson <[email protected]>

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"
@@ -112,43 +113,52 @@ 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(),
+		kubernetesimagerepository.NewPatch(),
+		etcd.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),
-
+		kubernetesimagerepository.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(),
+		kubernetesimagerepository.NewMetaPatch(),
+		etcd.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...),
+	}
 
-		kubernetesimagerepository.NewVariable(),
-		kubernetesimagerepository.NewPatch(kubernetesimagerepository.VariableName),
+	var allHandlers []handlers.Named
+	allHandlers = append(allHandlers, lifeCycleHandlers...)
+	allHandlers = append(allHandlers, patchHandlers...)
+	allHandlers = append(allHandlers, variableHandlers...)
+	allHandlers = append(allHandlers, metaHandlers...)
 
-		etcd.NewVariable(),
-		etcd.NewPatch(etcd.VariableName),
+	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(),
-			kubernetesimagerepository.NewPatch(
-				clusterconfig.VariableName,
-				kubernetesimagerepository.VariableName,
-			),
-			etcd.NewPatch(clusterconfig.VariableName, etcd.VariableName),
-		),
-	)
 	if err := mgr.Add(runtimeWebhookServer); err != nil {
 		setupLog.Error(err, "unable to add runtime webhook server runnable to controller manager")
 		os.Exit(1)

pkg/handlers/clusterconfig/variables.go

@@ -10,19 +10,17 @@ import (
 	runtimehooksv1 "sigs.k8s.io/cluster-api/exp/runtime/hooks/api/v1alpha1"
 
 	"github.com/d2iq-labs/capi-runtime-extensions/api/v1alpha1"
-	"github.com/d2iq-labs/capi-runtime-extensions/common/pkg/capi/clustertopology/handlers"
+	commonhandlers "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/pkg/handlers"
 )
 
 var (
-	_ handlers.Named             = &clusterConfigVariableHandler{}
+	_ commonhandlers.Named       = &clusterConfigVariableHandler{}
 	_ mutation.DiscoverVariables = &clusterConfigVariableHandler{}
 )
 
 const (
-	// VariableName is http proxy external patch variable name.
-	VariableName = "clusterConfig"
-
 	// HandlerNameVariable is the name of the variable handler.
 	HandlerNameVariable = "ClusterConfigVars"
 )
@@ -43,7 +41,7 @@ func (h *clusterConfigVariableHandler) DiscoverVariables(
 	resp *runtimehooksv1.DiscoverVariablesResponse,
 ) {
 	resp.Variables = append(resp.Variables, clusterv1.ClusterClassVariable{
-		Name:     VariableName,
+		Name:     handlers.MetaVariableName,
 		Required: false,
 		Schema:   v1alpha1.ClusterConfigSpec{}.VariableSchema(),
 	})

pkg/handlers/clusterconfig/variables_test.go

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

pkg/handlers/cni/calico/handler.go

@@ -24,14 +24,19 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 
 	"github.com/d2iq-labs/capi-runtime-extensions/api/v1alpha1"
-	"github.com/d2iq-labs/capi-runtime-extensions/common/pkg/capi/clustertopology/handlers"
+	commonhandlers "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/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"
 	"github.com/d2iq-labs/capi-runtime-extensions/pkg/handlers/cni"
 )
 
+const (
+	variableName = "cni"
+)
+
 type CalicoCNIConfig struct {
 	defaultsNamespace string
 
@@ -72,23 +77,21 @@ type CalicoCNI struct {
 }
 
 var (
-	_ handlers.Named                         = &CalicoCNI{}
+	_ commonhandlers.Named                   = &CalicoCNI{}
 	_ lifecycle.AfterControlPlaneInitialized = &CalicoCNI{}
 
 	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: handlers.MetaVariableName,
+		variablePath: []string{variableName},
 	}
 }
 

pkg/handlers/etcd/inject.go

@@ -18,11 +18,12 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	"github.com/d2iq-labs/capi-runtime-extensions/api/v1alpha1"
-	"github.com/d2iq-labs/capi-runtime-extensions/common/pkg/capi/clustertopology/handlers"
+	commonhandlers "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/capi/clustertopology/patches"
 	"github.com/d2iq-labs/capi-runtime-extensions/common/pkg/capi/clustertopology/patches/selectors"
 	"github.com/d2iq-labs/capi-runtime-extensions/common/pkg/capi/clustertopology/variables"
+	"github.com/d2iq-labs/capi-runtime-extensions/pkg/handlers"
 )
 
 const (
@@ -37,11 +38,19 @@ type etcdPatchHandler struct {
 }
 
 var (
-	_ handlers.Named           = &etcdPatchHandler{}
+	_ commonhandlers.Named     = &etcdPatchHandler{}
 	_ mutation.GeneratePatches = &etcdPatchHandler{}
 )
 
-func NewPatch(
+func NewPatch() *etcdPatchHandler {
+	return newEtcdPatchHandler(variableName)
+}
+
+func NewMetaPatch() *etcdPatchHandler {
+	return newEtcdPatchHandler(handlers.MetaVariableName, variableName)
+}
+
+func newEtcdPatchHandler(
 	variableName string,
 	variableFieldPath ...string,
 ) *etcdPatchHandler {

pkg/handlers/etcd/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: "etcd imageRepository and imageTag set",
 			Vars: []runtimehooksv1.Variable{
 				capitest.VariableWithValue(
-					VariableName,
+					variableName,
 					v1alpha1.Etcd{
 						Image: &v1alpha1.Image{
 							Repository: "my-registry.io/my-org/my-repo",
@@ -55,7 +55,7 @@ func TestGeneratePatches(t *testing.T) {
 			Name: "etcd imageRepository set",
 			Vars: []runtimehooksv1.Variable{
 				capitest.VariableWithValue(
-					VariableName,
+					variableName,
 					v1alpha1.Etcd{
 						Image: &v1alpha1.Image{
 							Repository: "my-registry.io/my-org/my-repo",
@@ -83,7 +83,7 @@ func TestGeneratePatches(t *testing.T) {
 			Name: "etcd imageTag set",
 			Vars: []runtimehooksv1.Variable{
 				capitest.VariableWithValue(
-					VariableName,
+					variableName,
 					v1alpha1.Etcd{
 						Image: &v1alpha1.Image{
 							Tag: "v3.5.99_custom.0",

pkg/handlers/etcd/variables.go

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

pkg/handlers/etcd/variables_test.go

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

pkg/handlers/extraapiservercertsans/inject.go

@@ -18,11 +18,12 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	"github.com/d2iq-labs/capi-runtime-extensions/api/v1alpha1"
-	"github.com/d2iq-labs/capi-runtime-extensions/common/pkg/capi/clustertopology/handlers"
+	commonhandlers "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/capi/clustertopology/patches"
 	"github.com/d2iq-labs/capi-runtime-extensions/common/pkg/capi/clustertopology/patches/selectors"
 	"github.com/d2iq-labs/capi-runtime-extensions/common/pkg/capi/clustertopology/variables"
+	"github.com/d2iq-labs/capi-runtime-extensions/pkg/handlers"
 )
 
 const (
@@ -37,11 +38,19 @@ type extraAPIServerCertSANsPatchHandler struct {
 }
 
 var (
-	_ handlers.Named           = &extraAPIServerCertSANsPatchHandler{}
+	_ commonhandlers.Named     = &extraAPIServerCertSANsPatchHandler{}
 	_ mutation.GeneratePatches = &extraAPIServerCertSANsPatchHandler{}
 )
 
-func NewPatch(
+func NewPatch() *extraAPIServerCertSANsPatchHandler {
+	return newExtraAPIServerCertSANsPatchHandler(variableName)
+}
+
+func NewMetaPatch() *extraAPIServerCertSANsPatchHandler {
+	return newExtraAPIServerCertSANsPatchHandler(handlers.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{