From 89922b7cf343cbda537d29d27cba400cbc684af7 Mon Sep 17 00:00:00 2001 From: Etienne Audet-Cobello Date: Fri, 11 Oct 2024 16:40:40 -0400 Subject: [PATCH] change snapinstalldata to pointer --- .../controllers/ck8sconfig_controller.go | 64 +++++++++++++------ pkg/cloudinit/common.go | 10 +-- pkg/cloudinit/controlplane_init_test.go | 8 +-- pkg/cloudinit/controlplane_join_test.go | 8 +-- pkg/cloudinit/worker_join_test.go | 8 +-- 5 files changed, 62 insertions(+), 36 deletions(-) diff --git a/bootstrap/controllers/ck8sconfig_controller.go b/bootstrap/controllers/ck8sconfig_controller.go index 37ed8c8d..44eb5070 100644 --- a/bootstrap/controllers/ck8sconfig_controller.go +++ b/bootstrap/controllers/ck8sconfig_controller.go @@ -258,11 +258,14 @@ func (r *CK8sConfigReconciler) joinControlplane(ctx context.Context, scope *Scop return err } - snapInstallData := r.getSnapInstallDataFromSpec(scope.Config.Spec) + snapInstallData, err := r.getSnapInstallDataFromSpec(scope.Config.Spec) + if err != nil { + return err + } // If the machine has an in-place upgrade annotation, use it to set the snap install data inPlaceInstallData := r.resolveInPlaceUpgradeRelease(machine) - if inPlaceInstallData.Option != "" && inPlaceInstallData.Value != "" { + if inPlaceInstallData != nil { snapInstallData = inPlaceInstallData } @@ -353,7 +356,10 @@ func (r *CK8sConfigReconciler) joinWorker(ctx context.Context, scope *Scope) err return err } - snapInstallData := r.getSnapInstallDataFromSpec(scope.Config.Spec) + snapInstallData, err := r.getSnapInstallDataFromSpec(scope.Config.Spec) + if err != nil { + return err + } // If the machine has an in-place upgrade annotation, use it to set the snap install data inPlaceInstallData := r.resolveInPlaceUpgradeRelease(machine) @@ -419,38 +425,38 @@ func (r *CK8sConfigReconciler) resolveFiles(ctx context.Context, cfg *bootstrapv return collected, nil } -func (r *CK8sConfigReconciler) resolveInPlaceUpgradeRelease(machine *clusterv1.Machine) cloudinit.SnapInstallData { +func (r *CK8sConfigReconciler) resolveInPlaceUpgradeRelease(machine *clusterv1.Machine) *cloudinit.SnapInstallData { mAnnotations := machine.GetAnnotations() if mAnnotations == nil { - return cloudinit.SnapInstallData{} + return nil } val, ok := mAnnotations[bootstrapv1.InPlaceUpgradeReleaseAnnotation] if !ok { - return cloudinit.SnapInstallData{} + return nil } optionKv := strings.Split(val, "=") if len(optionKv) != 2 { r.Log.Info("Invalid in-place upgrade release annotation, ignoring", "annotation", val) - return cloudinit.SnapInstallData{} + return nil } switch optionKv[0] { case "channel": - return cloudinit.SnapInstallData{ + return &cloudinit.SnapInstallData{ Option: cloudinit.InstallOptionChannel, Value: optionKv[1], } case "revision": - return cloudinit.SnapInstallData{ + return &cloudinit.SnapInstallData{ Option: cloudinit.InstallOptionRevision, Value: optionKv[1], } case "localPath": - return cloudinit.SnapInstallData{ + return &cloudinit.SnapInstallData{ Option: cloudinit.InstallOptionLocalPath, Value: optionKv[1], } @@ -458,28 +464,43 @@ func (r *CK8sConfigReconciler) resolveInPlaceUpgradeRelease(machine *clusterv1.M r.Log.Info("Unknown in-place upgrade release option, ignoring", "option", optionKv[0]) } - return cloudinit.SnapInstallData{} + return nil } -func (r *CK8sConfigReconciler) getSnapInstallDataFromSpec(spec bootstrapv1.CK8sConfigSpec) cloudinit.SnapInstallData { +func (r *CK8sConfigReconciler) getSnapInstallDataFromSpec(spec bootstrapv1.CK8sConfigSpec) (*cloudinit.SnapInstallData, error) { + // Ensure that exactly one option is set + count := 0 + if spec.Channel != "" { + count++ + } + if spec.Revision != "" { + count++ + } + if spec.LocalPath != "" { + count++ + } + if count > 1 { + return nil, fmt.Errorf("only one of Channel, Revision, or LocalPath can be set, but multiple were provided") + } + switch { case spec.Channel != "": - return cloudinit.SnapInstallData{ + return &cloudinit.SnapInstallData{ Option: cloudinit.InstallOptionChannel, Value: spec.Channel, - } + }, nil case spec.Revision != "": - return cloudinit.SnapInstallData{ + return &cloudinit.SnapInstallData{ Option: cloudinit.InstallOptionRevision, Value: spec.Revision, - } + }, nil case spec.LocalPath != "": - return cloudinit.SnapInstallData{ + return &cloudinit.SnapInstallData{ Option: cloudinit.InstallOptionLocalPath, Value: spec.LocalPath, - } + }, nil default: - return cloudinit.SnapInstallData{} + return &cloudinit.SnapInstallData{}, nil } } @@ -621,7 +642,10 @@ func (r *CK8sConfigReconciler) handleClusterNotInitialized(ctx context.Context, return ctrl.Result{}, fmt.Errorf("failed to render k8sd-proxy daemonset: %w", err) } - snapInstallData := r.getSnapInstallDataFromSpec(scope.Config.Spec) + snapInstallData, err := r.getSnapInstallDataFromSpec(scope.Config.Spec) + if err != nil { + return ctrl.Result{}, err + } cpinput := cloudinit.InitControlPlaneInput{ BaseUserData: cloudinit.BaseUserData{ diff --git a/pkg/cloudinit/common.go b/pkg/cloudinit/common.go index afad66b2..2a15cd53 100644 --- a/pkg/cloudinit/common.go +++ b/pkg/cloudinit/common.go @@ -28,7 +28,7 @@ type BaseUserData struct { // KubernetesVersion is the Kubernetes version from the cluster object. KubernetesVersion string // SnapInstallData is the snap install data. - SnapInstallData SnapInstallData + SnapInstallData *SnapInstallData // BootCommands is a list of commands to run early in the boot process. BootCommands []string // PreRunCommands is a list of commands to run prior to k8s installation. @@ -65,9 +65,11 @@ func NewBaseCloudConfig(data BaseUserData) (CloudConfig, error) { snapInstall := data.SnapInstallData // Default to k8s version if snap install option is not set or empty. - if snapInstall.Option == "" || snapInstall.Value == "" { - snapInstall.Option = InstallOptionChannel - snapInstall.Value = fmt.Sprintf("%d.%d-classic/stable", kubernetesVersion.Major(), kubernetesVersion.Minor()) + if snapInstall == nil { + snapInstall = &SnapInstallData{ + Option: InstallOptionChannel, + Value: fmt.Sprintf("%d.%d-classic/stable", kubernetesVersion.Major(), kubernetesVersion.Minor()), + } } config := CloudConfig{ diff --git a/pkg/cloudinit/controlplane_init_test.go b/pkg/cloudinit/controlplane_init_test.go index 79699693..ac266462 100644 --- a/pkg/cloudinit/controlplane_init_test.go +++ b/pkg/cloudinit/controlplane_init_test.go @@ -158,12 +158,12 @@ func TestNewInitControlPlaneSnapInstallOverrides(t *testing.T) { tests := []struct { name string - snapInstall cloudinit.SnapInstallData + snapInstall *cloudinit.SnapInstallData notOptions []cloudinit.InstallOption }{ { name: "ChannelOverride", - snapInstall: cloudinit.SnapInstallData{ + snapInstall: &cloudinit.SnapInstallData{ Option: cloudinit.InstallOptionChannel, Value: "v1.30/edge", }, @@ -171,7 +171,7 @@ func TestNewInitControlPlaneSnapInstallOverrides(t *testing.T) { }, { name: "RevisionOverride", - snapInstall: cloudinit.SnapInstallData{ + snapInstall: &cloudinit.SnapInstallData{ Option: cloudinit.InstallOptionRevision, Value: "123", }, @@ -179,7 +179,7 @@ func TestNewInitControlPlaneSnapInstallOverrides(t *testing.T) { }, { name: "LocalPathOverride", - snapInstall: cloudinit.SnapInstallData{ + snapInstall: &cloudinit.SnapInstallData{ Option: cloudinit.InstallOptionLocalPath, Value: "/path/to/k8s.snap", }, diff --git a/pkg/cloudinit/controlplane_join_test.go b/pkg/cloudinit/controlplane_join_test.go index 6f4155d3..9e27ab9d 100644 --- a/pkg/cloudinit/controlplane_join_test.go +++ b/pkg/cloudinit/controlplane_join_test.go @@ -139,25 +139,25 @@ func TestNewJoinControlPlaneSnapInstall(t *testing.T) { tests := []struct { name string - snapInstall cloudinit.SnapInstallData + snapInstall *cloudinit.SnapInstallData }{ { name: "ChannelOverride", - snapInstall: cloudinit.SnapInstallData{ + snapInstall: &cloudinit.SnapInstallData{ Option: cloudinit.InstallOptionChannel, Value: "v1.30/stable", }, }, { name: "RevisionOverride", - snapInstall: cloudinit.SnapInstallData{ + snapInstall: &cloudinit.SnapInstallData{ Option: cloudinit.InstallOptionRevision, Value: "123", }, }, { name: "LocalPathOverride", - snapInstall: cloudinit.SnapInstallData{ + snapInstall: &cloudinit.SnapInstallData{ Option: cloudinit.InstallOptionLocalPath, Value: "/path/to/k8s.snap", }, diff --git a/pkg/cloudinit/worker_join_test.go b/pkg/cloudinit/worker_join_test.go index d9a55384..c41b6d94 100644 --- a/pkg/cloudinit/worker_join_test.go +++ b/pkg/cloudinit/worker_join_test.go @@ -139,25 +139,25 @@ func TestNewJoinWorkerSnapInstall(t *testing.T) { tests := []struct { name string - snapInstall cloudinit.SnapInstallData + snapInstall *cloudinit.SnapInstallData }{ { name: "ChannelOverride", - snapInstall: cloudinit.SnapInstallData{ + snapInstall: &cloudinit.SnapInstallData{ Option: cloudinit.InstallOptionChannel, Value: "v1.30/stable", }, }, { name: "RevisionOverride", - snapInstall: cloudinit.SnapInstallData{ + snapInstall: &cloudinit.SnapInstallData{ Option: cloudinit.InstallOptionRevision, Value: "123", }, }, { name: "LocalPathOverride", - snapInstall: cloudinit.SnapInstallData{ + snapInstall: &cloudinit.SnapInstallData{ Option: cloudinit.InstallOptionLocalPath, Value: "/path/to/k8s.snap", },