From 5de6f49cfda7bd4d2dcafc6b45d3c6c473a2d0c4 Mon Sep 17 00:00:00 2001 From: lubronzhan Date: Mon, 6 Nov 2023 19:58:52 -0800 Subject: [PATCH] Fix file that use GetVariable function --- exp/runtime/topologymutation/variables.go | 2 +- .../topologymutation/variables_test.go | 15 +- .../handlers/topologymutation/handler.go | 144 +++++++++--------- 3 files changed, 85 insertions(+), 76 deletions(-) diff --git a/exp/runtime/topologymutation/variables.go b/exp/runtime/topologymutation/variables.go index d5cdbcc8f5cf..8103f66fb7f8 100644 --- a/exp/runtime/topologymutation/variables.go +++ b/exp/runtime/topologymutation/variables.go @@ -27,7 +27,7 @@ import ( patchvariables "sigs.k8s.io/cluster-api/internal/controllers/topology/cluster/patches/variables" ) -// TODO: Add func for validating received variables are of the expected types +// TODO: Add func for validating received variables are of the expected types. // GetVariable get the variable value. func GetVariable(templateVariables map[string]apiextensionsv1.JSON, variableName string) (*apiextensionsv1.JSON, error) { diff --git a/exp/runtime/topologymutation/variables_test.go b/exp/runtime/topologymutation/variables_test.go index c314c01c54bf..b62a715cf833 100644 --- a/exp/runtime/topologymutation/variables_test.go +++ b/exp/runtime/topologymutation/variables_test.go @@ -22,6 +22,7 @@ import ( . "github.com/onsi/gomega" apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" "k8s.io/utils/pointer" + patchvariables "sigs.k8s.io/cluster-api/internal/controllers/topology/cluster/patches/variables" ) @@ -213,7 +214,7 @@ func Test_GetVariableObjectWithNestedType(t *testing.T) { variableName string expectedNotFoundError bool expectedErr bool - object Network + object *Network expectedVariableObject interface{} }{ { @@ -221,7 +222,7 @@ func Test_GetVariableObjectWithNestedType(t *testing.T) { variables: nil, variableName: "invalid[", expectedNotFoundError: false, - object: Network{}, + object: &Network{}, expectedVariableObject: Network{}, expectedErr: true, }, @@ -230,7 +231,7 @@ func Test_GetVariableObjectWithNestedType(t *testing.T) { variables: nil, variableName: "notEsists", expectedNotFoundError: true, - object: Network{}, + object: &Network{}, expectedVariableObject: Network{}, expectedErr: true, }, @@ -242,7 +243,7 @@ func Test_GetVariableObjectWithNestedType(t *testing.T) { }, variableName: "network", expectedNotFoundError: false, - object: Network{}, + object: &Network{}, expectedVariableObject: Network{}, expectedErr: true, }, @@ -255,7 +256,7 @@ func Test_GetVariableObjectWithNestedType(t *testing.T) { variableName: "network", expectedNotFoundError: false, expectedErr: false, - object: Network{}, + object: &Network{}, expectedVariableObject: Network{ Ipv6Primary: pointer.Bool(true), AddressesFromPools: &[]AddressesFromPool{ @@ -268,7 +269,7 @@ func Test_GetVariableObjectWithNestedType(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - err := GetObjectVariableInto(tt.variables, tt.variableName, &tt.object) + err := GetObjectVariableInto(tt.variables, tt.variableName, tt.object) if tt.expectedErr { g.Expect(err).To(HaveOccurred()) } else { @@ -277,7 +278,7 @@ func Test_GetVariableObjectWithNestedType(t *testing.T) { if tt.expectedNotFoundError { g.Expect(patchvariables.IsNotFoundError(err)).To(BeTrue()) } - g.Expect(tt.object).To(Equal(tt.expectedVariableObject)) + g.Expect(*tt.object).To(Equal(tt.expectedVariableObject)) }) } } diff --git a/test/extension/handlers/topologymutation/handler.go b/test/extension/handlers/topologymutation/handler.go index 3807e2eba832..a88eee02cd10 100644 --- a/test/extension/handlers/topologymutation/handler.go +++ b/test/extension/handlers/topologymutation/handler.go @@ -38,6 +38,7 @@ import ( controlplanev1 "sigs.k8s.io/cluster-api/controlplane/kubeadm/api/v1beta1" runtimehooksv1 "sigs.k8s.io/cluster-api/exp/runtime/hooks/api/v1alpha1" "sigs.k8s.io/cluster-api/exp/runtime/topologymutation" + patchvariables "sigs.k8s.io/cluster-api/internal/controllers/topology/cluster/patches/variables" infrav1 "sigs.k8s.io/cluster-api/test/infrastructure/docker/api/v1beta1" infraexpv1 "sigs.k8s.io/cluster-api/test/infrastructure/docker/exp/api/v1beta1" "sigs.k8s.io/cluster-api/test/infrastructure/kind" @@ -129,13 +130,16 @@ func (h *ExtensionHandlers) GeneratePatches(ctx context.Context, req *runtimehoo // It sets the LoadBalancer.ImageRepository if the imageRepository 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, templateVariables map[string]apiextensionsv1.JSON) error { - imageRepo, found, err := topologymutation.GetStringVariable(templateVariables, "imageRepository") + imageRepo, err := topologymutation.GetStringVariable(templateVariables, "imageRepository") if err != nil { + if patchvariables.IsNotFoundError(err) { + return nil + } return errors.Wrap(err, "could not set DockerClusterTemplate loadBalancer imageRepository") } - if found { - dockerClusterTemplate.Spec.Template.Spec.LoadBalancer.ImageRepository = imageRepo - } + + dockerClusterTemplate.Spec.Template.Spec.LoadBalancer.ImageRepository = imageRepo + return nil } @@ -152,15 +156,15 @@ func patchKubeadmControlPlaneTemplate(ctx context.Context, kcpTemplate *controlp // 1) If the Kubernetes version from builtin.controlPlane.version is below 1.24.0 set "cgroup-driver": "cgroupfs" to // - kubeadmConfigSpec.InitConfiguration.NodeRegistration.KubeletExtraArgs // - kubeadmConfigSpec.JoinConfiguration.NodeRegistration.KubeletExtraArgs - cpVersion, found, err := topologymutation.GetStringVariable(templateVariables, "builtin.controlPlane.version") + cpVersion, err := topologymutation.GetStringVariable(templateVariables, "builtin.controlPlane.version") if err != nil { + // This is a required variable. Return an error if it's not found. + // NOTE: this should never happen because it is enforced by the patch engine. + if patchvariables.IsNotFoundError(err) { + return errors.New("could not set cgroup-driver to control plane template kubeletExtraArgs: variable \"builtin.controlPlane.version\" not found") + } return errors.Wrap(err, "could not set cgroup-driver to control plane template kubeletExtraArgs") } - // This is a required variable. Return an error if it's not found. - // NOTE: this should never happen because it is enforced by the patch engine. - if !found { - return errors.New("could not set cgroup-driver to control plane template kubeletExtraArgs: variable \"builtin.controlPlane.version\" not found") - } controlPlaneVersion, err := version.ParseMajorMinorPatchTolerant(cpVersion) if err != nil { @@ -189,22 +193,24 @@ func patchKubeadmControlPlaneTemplate(ctx context.Context, kcpTemplate *controlp // 2) Patch RolloutStrategy RollingUpdate MaxSurge with the value from the Cluster Topology variable. // If this is unset continue as this variable is not required. - kcpControlPlaneMaxSurge, found, err := topologymutation.GetStringVariable(templateVariables, "kubeadmControlPlaneMaxSurge") + kcpControlPlaneMaxSurge, err := topologymutation.GetStringVariable(templateVariables, "kubeadmControlPlaneMaxSurge") if err != nil { + if patchvariables.IsNotFoundError(err) { + return nil + } return errors.Wrap(err, "could not set KubeadmControlPlaneTemplate MaxSurge") } - if found { - // This has to be converted to IntOrString type. - kubeadmControlPlaneMaxSurgeIntOrString := intstrutil.Parse(kcpControlPlaneMaxSurge) - log.Info(fmt.Sprintf("Setting KubeadmControlPlaneMaxSurge to %q", kubeadmControlPlaneMaxSurgeIntOrString.String())) - if kcpTemplate.Spec.Template.Spec.RolloutStrategy == nil { - kcpTemplate.Spec.Template.Spec.RolloutStrategy = &controlplanev1.RolloutStrategy{} - } - if kcpTemplate.Spec.Template.Spec.RolloutStrategy.RollingUpdate == nil { - kcpTemplate.Spec.Template.Spec.RolloutStrategy.RollingUpdate = &controlplanev1.RollingUpdate{} - } - kcpTemplate.Spec.Template.Spec.RolloutStrategy.RollingUpdate.MaxSurge = &kubeadmControlPlaneMaxSurgeIntOrString + + // This has to be converted to IntOrString type. + kubeadmControlPlaneMaxSurgeIntOrString := intstrutil.Parse(kcpControlPlaneMaxSurge) + log.Info(fmt.Sprintf("Setting KubeadmControlPlaneMaxSurge to %q", kubeadmControlPlaneMaxSurgeIntOrString.String())) + if kcpTemplate.Spec.Template.Spec.RolloutStrategy == nil { + kcpTemplate.Spec.Template.Spec.RolloutStrategy = &controlplanev1.RolloutStrategy{} + } + if kcpTemplate.Spec.Template.Spec.RolloutStrategy.RollingUpdate == nil { + kcpTemplate.Spec.Template.Spec.RolloutStrategy.RollingUpdate = &controlplanev1.RollingUpdate{} } + kcpTemplate.Spec.Template.Spec.RolloutStrategy.RollingUpdate.MaxSurge = &kubeadmControlPlaneMaxSurgeIntOrString return nil } @@ -218,19 +224,19 @@ func patchKubeadmConfigTemplate(ctx context.Context, k *bootstrapv1.KubeadmConfi // Only patch the customImage if this DockerMachineTemplate belongs to a MachineDeployment or MachinePool with class "default-class" // NOTE: This works by checking the existence of a builtin variable that exists only for templates linked to MachineDeployments. - mdClass, mdFound, err := topologymutation.GetStringVariable(templateVariables, "builtin.machineDeployment.class") - if err != nil { - return errors.Wrap(err, "could not set cgroup-driver to KubeadmConfigTemplate template kubeletExtraArgs") + mdClass, err1 := topologymutation.GetStringVariable(templateVariables, "builtin.machineDeployment.class") + if err1 != nil && !patchvariables.IsNotFoundError(err1) { + return errors.Wrap(err1, "could not set cgroup-driver to KubeadmConfigTemplate template kubeletExtraArgs") } - mpClass, mpFound, err := topologymutation.GetStringVariable(templateVariables, "builtin.machinePool.class") - if err != nil { - return errors.Wrap(err, "could not set cgroup-driver to KubeadmConfigTemplate template kubeletExtraArgs") + mpClass, err2 := topologymutation.GetStringVariable(templateVariables, "builtin.machinePool.class") + if err2 != nil && !patchvariables.IsNotFoundError(err2) { + return errors.Wrap(err2, "could not set cgroup-driver to KubeadmConfigTemplate template kubeletExtraArgs") } // This is a required variable. Return an error if it's not found. // NOTE: this should never happen because it is enforced by the patch engine. - if !mdFound && !mpFound { + if patchvariables.IsNotFoundError(err1) && patchvariables.IsNotFoundError(err2) { return errors.New("could not set cgroup-driver to KubeadmConfigTemplate template kubeletExtraArgs: could find neither \"builtin.machineDeployment.class\" nor \"builtin.machinePool.class\" variable") } @@ -240,15 +246,15 @@ func patchKubeadmConfigTemplate(ctx context.Context, k *bootstrapv1.KubeadmConfi // - JoinConfiguration.KubeletExtraArgs // NOTE: MachineDeployment version might be different than Cluster.version or other MachineDeployment's versions; // the builtin variables provides the right version to use. - mdVersion, found, err := topologymutation.GetStringVariable(templateVariables, "builtin.machineDeployment.version") + mdVersion, err := topologymutation.GetStringVariable(templateVariables, "builtin.machineDeployment.version") if err != nil { + // This is a required variable. Return an error if it's not found. + if patchvariables.IsNotFoundError(err) { + return errors.New("could not set cgroup-driver to KubeadmConfigTemplate template kubeletExtraArgs: variable \"builtin.machineDeployment.version\" not found") + } return errors.Wrap(err, "could not set cgroup-driver to KubeadmConfigTemplate template kubeletExtraArgs") } - // This is a required variable. Return an error if it's not found. - if !found { - return errors.New("could not set cgroup-driver to KubeadmConfigTemplate template kubeletExtraArgs: variable \"builtin.machineDeployment.version\" not found") - } machineDeploymentVersion, err := version.ParseMajorMinorPatchTolerant(mdVersion) if err != nil { return errors.Wrap(err, "could not set cgroup-driver to KubeadmConfigTemplate template kubeletExtraArgs") @@ -274,15 +280,15 @@ func patchKubeadmConfigTemplate(ctx context.Context, k *bootstrapv1.KubeadmConfi // - JoinConfiguration.KubeletExtraArgs // NOTE: MachinePool version might be different than Cluster.version or other MachinePool's versions; // the builtin variables provides the right version to use. - mpVersion, found, err := topologymutation.GetStringVariable(templateVariables, "builtin.machinePool.version") + mpVersion, err := topologymutation.GetStringVariable(templateVariables, "builtin.machinePool.version") if err != nil { + // This is a required variable. Return an error if it's not found. + if patchvariables.IsNotFoundError(err) { + return errors.New("could not set cgroup-driver to KubeadmConfigTemplate template kubeletExtraArgs: variable \"builtin.machinePool.version\" not found") + } return errors.Wrap(err, "could not set cgroup-driver to KubeadmConfigTemplate template kubeletExtraArgs") } - // This is a required variable. Return an error if it's not found. - if !found { - return errors.New("could not set cgroup-driver to KubeadmConfigTemplate template kubeletExtraArgs: variable \"builtin.machinePool.version\" not found") - } machinePoolVersion, err := version.ParseMajorMinorPatchTolerant(mpVersion) if err != nil { return errors.Wrap(err, "could not set cgroup-driver to KubeadmConfigTemplate template kubeletExtraArgs") @@ -317,11 +323,13 @@ func patchDockerMachineTemplate(ctx context.Context, dockerMachineTemplate *infr // NOTE: ControlPlane version might be different than Cluster.version or MachineDeployment's versions; // the builtin variables provides the right version to use. // NOTE: This works by checking the existence of a builtin variable that exists only for templates linked to the ControlPlane. - cpVersion, found, err := topologymutation.GetStringVariable(templateVariables, "builtin.controlPlane.version") - if err != nil { + cpVersion, err := topologymutation.GetStringVariable(templateVariables, "builtin.controlPlane.version") + if err != nil && !patchvariables.IsNotFoundError(err) { return errors.Wrap(err, "could not set customImage to control plane dockerMachineTemplate") } - if found { + + // if found + if err == nil { semVer, err := version.ParseMajorMinorPatchTolerant(cpVersion) if err != nil { return errors.Wrap(err, "could not parse control plane version") @@ -338,25 +346,25 @@ func patchDockerMachineTemplate(ctx context.Context, dockerMachineTemplate *infr // NOTE: MachineDeployment version might be different from Cluster.version or other MachineDeployment's versions; // the builtin variables provides the right version to use. // NOTE: This works by checking the existence of a builtin variable that exists only for templates linked to MachineDeployments. - mdVersion, found, err := topologymutation.GetStringVariable(templateVariables, "builtin.machineDeployment.version") + mdVersion, err := topologymutation.GetStringVariable(templateVariables, "builtin.machineDeployment.version") if err != nil { + if patchvariables.IsNotFoundError(err) { + // If the DockerMachineTemplate didn't have variables for either a control plane or a machineDeployment return an error. + // NOTE: this should never happen because it is enforced by the patch engine. + return errors.New("no version variables found for DockerMachineTemplate patch") + } return errors.Wrap(err, "could not set customImage to MachineDeployment DockerMachineTemplate") } - if found { - semVer, err := version.ParseMajorMinorPatchTolerant(mdVersion) - if err != nil { - return errors.Wrap(err, "could not parse MachineDeployment version") - } - kindMapping := kind.GetMapping(semVer, "") - log.Info(fmt.Sprintf("Setting MachineDeployment customImage to %q", kindMapping.Image)) - dockerMachineTemplate.Spec.Template.Spec.CustomImage = kindMapping.Image - return nil + semVer, err := version.ParseMajorMinorPatchTolerant(mdVersion) + if err != nil { + return errors.Wrap(err, "could not parse MachineDeployment version") } + kindMapping := kind.GetMapping(semVer, "") - // If the DockerMachineTemplate didn't have variables for either a control plane or a machineDeployment return an error. - // NOTE: this should never happen because it is enforced by the patch engine. - return errors.New("no version variables found for DockerMachineTemplate patch") + log.Info(fmt.Sprintf("Setting MachineDeployment customImage to %q", kindMapping.Image)) + dockerMachineTemplate.Spec.Template.Spec.CustomImage = kindMapping.Image + return nil } // patchDockerMachinePoolTemplate patches the DockerMachinePoolTemplate. @@ -370,25 +378,25 @@ func patchDockerMachinePoolTemplate(ctx context.Context, dockerMachinePoolTempla // NOTE: MachinePool version might be different from Cluster.version or other MachinePool's versions; // the builtin variables provides the right version to use. // NOTE: This works by checking the existence of a builtin variable that exists only for templates linked to MachinePools. - mpVersion, found, err := topologymutation.GetStringVariable(templateVariables, "builtin.machinePool.version") + mpVersion, err := topologymutation.GetStringVariable(templateVariables, "builtin.machinePool.version") if err != nil { + // If the DockerMachinePoolTemplate didn't have variables for a machinePool return an error. + // NOTE: this should never happen because it is enforced by the patch engine. + if patchvariables.IsNotFoundError(err) { + return errors.New("no version variables found for DockerMachinePoolTemplate patch") + } return errors.Wrap(err, "could not set customImage to MachinePool DockerMachinePoolTemplate") } - if found { - semVer, err := version.ParseMajorMinorPatchTolerant(mpVersion) - if err != nil { - return errors.Wrap(err, "could not parse MachinePool version") - } - kindMapping := kind.GetMapping(semVer, "") - log.Info(fmt.Sprintf("Setting MachinePool customImage to %q", kindMapping.Image)) - dockerMachinePoolTemplate.Spec.Template.Spec.Template.CustomImage = kindMapping.Image - return nil + semVer, err := version.ParseMajorMinorPatchTolerant(mpVersion) + if err != nil { + return errors.Wrap(err, "could not parse MachinePool version") } + kindMapping := kind.GetMapping(semVer, "") - // If the DockerMachinePoolTemplate didn't have variables for a machinePool return an error. - // NOTE: this should never happen because it is enforced by the patch engine. - return errors.New("no version variables found for DockerMachinePoolTemplate patch") + log.Info(fmt.Sprintf("Setting MachinePool customImage to %q", kindMapping.Image)) + dockerMachinePoolTemplate.Spec.Template.Spec.Template.CustomImage = kindMapping.Image + return nil } // ValidateTopology implements the HandlerFunc for the ValidateTopology hook.