update

exp/runtime/topologymutation/walker.go

@@ -31,7 +31,6 @@ import (
 
 	runtimehooksv1 "sigs.k8s.io/cluster-api/exp/runtime/hooks/api/v1alpha1"
 	patchvariables "sigs.k8s.io/cluster-api/internal/controllers/topology/cluster/patches/variables"
-	"sigs.k8s.io/cluster-api/test/extension/api"
 )
 
 // WalkTemplatesOption is some configuration that modifies WalkTemplates behavior.
@@ -81,8 +80,8 @@ func (d PatchFormat) ApplyToWalkTemplates(in *WalkTemplatesOptions) {
 // Also, by using this func it is possible to ignore most of the details of the GeneratePatchesRequest
 // and GeneratePatchesResponse messages format and focus on writing patches/modifying the templates.
 func WalkTemplates(ctx context.Context, decoder runtime.Decoder, req *runtimehooksv1.GeneratePatchesRequest,
-	resp *runtimehooksv1.GeneratePatchesResponse, mutateFunc func(ctx context.Context, obj runtime.Object,
-		builtinVariable patchvariables.Builtins, variables interface{}, holderRef runtimehooksv1.HolderReference) error, opts ...WalkTemplatesOption) {
+	resp *runtimehooksv1.GeneratePatchesResponse, variablesType interface{}, mutateFunc func(ctx context.Context, obj runtime.Object,
+		builtinVariable *patchvariables.Builtins, variables interface{}, holderRef runtimehooksv1.HolderReference) error, opts ...WalkTemplatesOption) {
 	log := ctrl.LoggerFrom(ctx)
 	globalVariables := patchvariables.ToMap(req.Variables)
 
@@ -126,9 +125,8 @@ func WalkTemplates(ctx context.Context, decoder runtime.Decoder, req *runtimehoo
 			fmt.Println(err)
 		}
 
-		// FIXME: must be generic. Try generics?
-		var variablesValue api.Variables
-		if err := json.Unmarshal(variableValuesJSONAll, &variablesValue); err != nil {
+		// FIXME: just using variablesType directly is probably not clean enough
+		if err := json.Unmarshal(variableValuesJSONAll, &variablesType); err != nil {
 			fmt.Println(err)
 		}
 
@@ -170,7 +168,7 @@ func WalkTemplates(ctx context.Context, decoder runtime.Decoder, req *runtimehoo
 		// Calls the mutateFunc.
 		requestItemLog.V(4).Info("Generating patch for template")
 		modified := original.DeepCopyObject()
-		if err := mutateFunc(requestItemCtx, modified, builtinVariableValue, variablesValue, requestItem.HolderReference); err != nil {
+		if err := mutateFunc(requestItemCtx, modified, &builtinVariableValue, variablesType, requestItem.HolderReference); err != nil {
 			resp.Status = runtimehooksv1.ResponseStatusFailure
 			resp.Message = err.Error()
 			return

exp/runtime/topologymutation/walkler_test.go

@@ -51,7 +51,7 @@ func Test_WalkTemplates(t *testing.T) {
 		controlplanev1.GroupVersion,
 		bootstrapv1.GroupVersion,
 	)
-	mutatingFunc := func(ctx context.Context, obj runtime.Object, variables map[string]apiextensionsv1.JSON, holderRef runtimehooksv1.HolderReference) error {
+	mutatingFunc := func(ctx context.Context, obj runtime.Object, builtinVariable *variables.Builtins, variables interface{}, holderRef runtimehooksv1.HolderReference) error {
 		switch obj := obj.(type) {
 		case *controlplanev1.KubeadmControlPlaneTemplate:
 			obj.Annotations = map[string]string{"a": "a"}
@@ -222,7 +222,7 @@ func Test_WalkTemplates(t *testing.T) {
 			response := &runtimehooksv1.GeneratePatchesResponse{}
 			request := &runtimehooksv1.GeneratePatchesRequest{Variables: tt.globalVariables, Items: tt.requestItems}
 
-			WalkTemplates(context.Background(), decoder, request, response, mutatingFunc, tt.options...)
+			WalkTemplates(context.Background(), decoder, request, response, struct{}{}, mutatingFunc, tt.options...)
 
 			g.Expect(response.Status).To(Equal(tt.expectedResponse.Status))
 			g.Expect(response.Message).To(Equal(tt.expectedResponse.Message))

hack/tools/runtime-openapi-gen-2/main.go

@@ -179,7 +179,7 @@ func run(paths, outputFile string) error {
 		}
 	}
 
-	res, err := json.Marshal(variables)
+	res, err := json.MarshalIndent(variables, "", "  ")
 	if err != nil {
 		return err
 	}

test/extension/api/variables.go

@@ -29,10 +29,11 @@ var (
 	//go:embed zz_generated.variables.json
 	variableDefinitionsBytes []byte
 
+	// VariableDefinitions contains the variable definitions of this API.
 	VariableDefinitions []clusterv1.ClusterClassVariable
 )
 
-// Variables
+// Variables defines the schemas of the variables.
 // FIXME: how do we generate the schema types that we then want to return during variable discovery
 // * openapi-gen:
 //   - where do we get the following fields from: default, example, MaxItems++
@@ -43,7 +44,7 @@ var (
 //   - // +kubebuilder:validation:Enum
 //
 // * Let's explore which generator covers which fields and if both cover enough, how we can use them
-// * Result should be to get from those structs to clusterv1.JSONSchemaProps (either statically generated or at least at runtime)
+// * Result should be to get from those structs to clusterv1.JSONSchemaProps (either statically generated or at least at runtime).
 type Variables struct {
 	// LBImageRepository is the image repository of the load balancer.
 	// +kubebuilder:validation:Required

test/extension/api/zz_generated.variables.json

@@ -1 +1,32 @@
-[{"name":"lbImageRepository","required":true,"schema":{"openAPIV3Schema":{"type":"string","default":"kindest"}}},{"name":"imageRepository","required":true,"schema":{"openAPIV3Schema":{"type":"string","default":"registry.k8s.io"}}},{"name":"kubeadmControlPlaneMaxSurge","required":false,"schema":{"openAPIV3Schema":{"type":"string","default":""}}}]
+[
+  {
+    "name": "imageRepository",
+    "required": true,
+    "schema": {
+      "openAPIV3Schema": {
+        "type": "string",
+        "default": "registry.k8s.io"
+      }
+    }
+  },
+  {
+    "name": "kubeadmControlPlaneMaxSurge",
+    "required": false,
+    "schema": {
+      "openAPIV3Schema": {
+        "type": "string",
+        "default": ""
+      }
+    }
+  },
+  {
+    "name": "lbImageRepository",
+    "required": true,
+    "schema": {
+      "openAPIV3Schema": {
+        "type": "string",
+        "default": "kindest"
+      }
+    }
+  }
+]

test/extension/handlers/topologymutation/handler.go

@@ -80,7 +80,7 @@ func (h *ExtensionHandlers) GeneratePatches(ctx context.Context, req *runtimehoo
 	log.Info("GeneratePatches is called")
 
 	// FIXME: validate variable values against variable schemas (just like in the webhook/reconciler today).
-	var userVariableValues []clusterv1.ClusterVariable
+	userVariableValues := []clusterv1.ClusterVariable{}
 	for _, v := range req.Variables {
 		if v.Name == patchvariables.BuiltinsName {
 			continue
@@ -90,6 +90,7 @@ func (h *ExtensionHandlers) GeneratePatches(ctx context.Context, req *runtimehoo
 			Value: v.Value,
 		})
 	}
+	// FIXME: lowerlevel variable (MD) as well
 	errs := variables.ValidateClusterVariables(userVariableValues, api.VariableDefinitions, field.NewPath(""))
 	if len(errs) > 0 {
 		resp.Status = runtimehooksv1.ResponseStatusFailure
@@ -99,20 +100,17 @@ func (h *ExtensionHandlers) GeneratePatches(ctx context.Context, req *runtimehoo
 	// By using WalkTemplates it is possible to implement patches using typed API objects, which makes code
 	// easier to read and less error prone than using unstructured or working with raw json/yaml.
 	// IMPORTANT: by unit testing this func/nested func properly, it is possible to prevent unexpected rollouts when patches are modified.
-	topologymutation.WalkTemplates(ctx, h.decoder, req, resp, func(ctx context.Context, obj runtime.Object, builtinVariable patchvariables.Builtins, variables interface{}, holderRef runtimehooksv1.HolderReference) error {
+	topologymutation.WalkTemplates(ctx, h.decoder, req, resp, &api.Variables{}, func(ctx context.Context, obj runtime.Object, builtinVariable *patchvariables.Builtins, variables interface{}, holderRef runtimehooksv1.HolderReference) error {
 		log := ctrl.LoggerFrom(ctx)
 
-		vars, ok := variables.(api.Variables)
+		vars, ok := variables.(*api.Variables)
 		if !ok {
 			return errors.Errorf("wrong variable type")
 		}
 
 		switch obj := obj.(type) {
 		case *infrav1.DockerClusterTemplate:
-			if err := patchDockerClusterTemplate(ctx, obj, builtinVariable, vars); err != nil {
-				log.Error(err, "error patching DockerClusterTemplate")
-				return errors.Wrap(err, "error patching DockerClusterTemplate")
-			}
+			patchDockerClusterTemplate(ctx, obj, builtinVariable, vars)
 		case *controlplanev1.KubeadmControlPlaneTemplate:
 			err := patchKubeadmControlPlaneTemplate(ctx, obj, builtinVariable, vars)
 			if err != nil {
@@ -145,19 +143,18 @@ func (h *ExtensionHandlers) GeneratePatches(ctx context.Context, req *runtimehoo
 // patchDockerClusterTemplate patches the DockerClusterTemplate.
 // It sets the LoadBalancer.ImageRepository if the lbImageRepository variable is provided.
 // NOTE: this patch is not required for any special reason, it is used for testing the patch machinery itself.
-func patchDockerClusterTemplate(_ context.Context, dockerClusterTemplate *infrav1.DockerClusterTemplate, builtinVariable patchvariables.Builtins, variables api.Variables) error {
+func patchDockerClusterTemplate(_ context.Context, dockerClusterTemplate *infrav1.DockerClusterTemplate, _ *patchvariables.Builtins, variables *api.Variables) {
 	if variables.LBImageRepository != "" {
 		dockerClusterTemplate.Spec.Template.Spec.LoadBalancer.ImageRepository = variables.LBImageRepository
 	}
-	return nil
 }
 
 // patchKubeadmControlPlaneTemplate patches the ControlPlaneTemplate.
 // It sets KubeletExtraArgs["cgroup-driver"] to cgroupfs for Kubernetes < 1.24; this patch is required for tests
 // to work with older kind images.
 // It also sets the RolloutStrategy.RollingUpdate.MaxSurge if the kubeadmControlPlaneMaxSurge is provided.
 // NOTE: RolloutStrategy.RollingUpdate.MaxSurge patch is not required for any special reason, it is used for testing the patch machinery itself.
-func patchKubeadmControlPlaneTemplate(ctx context.Context, kcpTemplate *controlplanev1.KubeadmControlPlaneTemplate, builtinVariable patchvariables.Builtins, variables api.Variables) error {
+func patchKubeadmControlPlaneTemplate(ctx context.Context, kcpTemplate *controlplanev1.KubeadmControlPlaneTemplate, builtinVariable *patchvariables.Builtins, variables *api.Variables) error {
 	log := ctrl.LoggerFrom(ctx)
 
 	// 1) If the Kubernetes version from builtin.controlPlane.version is below 1.24.0 set "cgroup-driver": "cgroupfs" to
@@ -216,7 +213,7 @@ func patchKubeadmControlPlaneTemplate(ctx context.Context, kcpTemplate *controlp
 // patchKubeadmConfigTemplate patches the ControlPlaneTemplate.
 // Only for the templates linked to the default-worker MachineDeployment class, It sets KubeletExtraArgs["cgroup-driver"]
 // to cgroupfs for Kubernetes < 1.24; this patch is required for tests to work with older kind images.
-func patchKubeadmConfigTemplate(ctx context.Context, k *bootstrapv1.KubeadmConfigTemplate, builtinVariable patchvariables.Builtins, variables api.Variables) error {
+func patchKubeadmConfigTemplate(ctx context.Context, k *bootstrapv1.KubeadmConfigTemplate, builtinVariable *patchvariables.Builtins, _ *api.Variables) error {
 	log := ctrl.LoggerFrom(ctx)
 
 	// This is a required variable. Return an error if it's not found.
@@ -259,7 +256,7 @@ func patchKubeadmConfigTemplate(ctx context.Context, k *bootstrapv1.KubeadmConfi
 // patchDockerMachineTemplate patches the DockerMachineTemplate.
 // It sets the CustomImage to an image for the version in use by the controlPlane or by the MachineDeployment
 // the DockerMachineTemplate belongs to. This patch is required to pick up the kind image with the required Kubernetes version.
-func patchDockerMachineTemplate(ctx context.Context, dockerMachineTemplate *infrav1.DockerMachineTemplate, builtinVariable patchvariables.Builtins, variables api.Variables) error {
+func patchDockerMachineTemplate(ctx context.Context, dockerMachineTemplate *infrav1.DockerMachineTemplate, builtinVariable *patchvariables.Builtins, _ *api.Variables) error {
 	log := ctrl.LoggerFrom(ctx)
 
 	// If the DockerMachineTemplate belongs to the ControlPlane, set the images using the ControlPlane version.
@@ -309,9 +306,10 @@ func (h *ExtensionHandlers) ValidateTopology(ctx context.Context, _ *runtimehook
 }
 
 // DiscoverVariables implements the HandlerFunc for the DiscoverVariables hook.
-// # Test via Tilt:
-// ## First terminal: kubectl proxy
-// ## Second terminal: curl -X 'POST' 'http://127.0.0.1:8001/api/v1/namespaces/test-extension-system/services/https:test-extension-webhook-service:443/proxy/hooks.runtime.cluster.x-k8s.io/v1alpha1/discovervariables/discover-variables' -d '{"apiVersion":"hooks.runtime.cluster.x-k8s.io/v1alpha1","kind":"DiscoverVariablesRequest"}' | jq
+// Can be tested via Tilt:
+// First terminal: kubectl proxy
+// Second terminal: curl -X 'POST' 'http://127.0.0.1:8001/api/v1/namespaces/test-extension-system/services/https:test-extension-webhook-service:443/proxy/hooks.runtime.cluster.x-k8s.io/v1alpha1/discovervariables/discover-variables' -d '{"apiVersion":"hooks.runtime.cluster.x-k8s.io/v1alpha1","kind":"DiscoverVariablesRequest"}' | jq
+// Should return the DiscoveryVariablesResponse.
 func (h *ExtensionHandlers) DiscoverVariables(ctx context.Context, _ *runtimehooksv1.DiscoverVariablesRequest, resp *runtimehooksv1.DiscoverVariablesResponse) {
 	log := ctrl.LoggerFrom(ctx)
 	log.Info("DiscoverVariables called")