From e55ca45e13e772d006860522395a38c22e2b4141 Mon Sep 17 00:00:00 2001 From: eaudetcobello Date: Tue, 15 Oct 2024 09:16:31 -0400 Subject: [PATCH] Set SnapInstallData from CK8sConfigSpec (#62) adds option to specify channel, revision or localPath in the CK8sConfig template --- bootstrap/api/v1beta2/ck8sconfig_types.go | 14 ++ bootstrap/api/v1beta2/condition_consts.go | 6 + ...ootstrap.cluster.x-k8s.io_ck8sconfigs.yaml | 13 ++ ....cluster.x-k8s.io_ck8sconfigtemplates.yaml | 13 ++ .../controllers/ck8sconfig_controller.go | 126 ++++++++++++++---- ...ne.cluster.x-k8s.io_ck8scontrolplanes.yaml | 13 ++ ...er.x-k8s.io_ck8scontrolplanetemplates.yaml | 14 ++ pkg/ck8s/workload_cluster.go | 5 + pkg/cloudinit/common.go | 10 +- pkg/cloudinit/controlplane_init_test.go | 23 +++- pkg/cloudinit/controlplane_join_test.go | 8 +- pkg/cloudinit/worker_join_test.go | 8 +- 12 files changed, 208 insertions(+), 45 deletions(-) diff --git a/bootstrap/api/v1beta2/ck8sconfig_types.go b/bootstrap/api/v1beta2/ck8sconfig_types.go index 5d1c4a9d..e2bf963e 100644 --- a/bootstrap/api/v1beta2/ck8sconfig_types.go +++ b/bootstrap/api/v1beta2/ck8sconfig_types.go @@ -66,6 +66,20 @@ type CK8sConfigSpec struct { // +optional SnapstoreProxyID string `json:"snapstoreProxyId,omitempty"` + // Channel is the channel to use for the snap install. + // +optional + Channel string `json:"channel,omitempty"` + + // Revision is the revision to use for the snap install. + // If Channel is set, this will be ignored. + // +optional + Revision string `json:"revision,omitempty"` + + // LocalPath is the path of a local snap file in the workload cluster to use for the snap install. + // If Channel or Revision are set, this will be ignored. + // +optional + LocalPath string `json:"localPath,omitempty"` + // CK8sControlPlaneConfig is configuration for the control plane node. // +optional ControlPlaneConfig CK8sControlPlaneConfig `json:"controlPlane,omitempty"` diff --git a/bootstrap/api/v1beta2/condition_consts.go b/bootstrap/api/v1beta2/condition_consts.go index 5ccb6f46..7e9ee0c6 100644 --- a/bootstrap/api/v1beta2/condition_consts.go +++ b/bootstrap/api/v1beta2/condition_consts.go @@ -69,3 +69,9 @@ const ( // an error while retrieving certificates for a joining node. CertificatesCorruptedReason = "CertificatesCorrupted" ) + +const ( + SnapInstallDataValidatedCondition clusterv1.ConditionType = "SnapInstallDataValidated" + + SnapInstallValidationFailedReason = "SnapInstallValidationFailed" +) diff --git a/bootstrap/config/crd/bases/bootstrap.cluster.x-k8s.io_ck8sconfigs.yaml b/bootstrap/config/crd/bases/bootstrap.cluster.x-k8s.io_ck8sconfigs.yaml index d468a1d1..f7845a1d 100644 --- a/bootstrap/config/crd/bases/bootstrap.cluster.x-k8s.io_ck8sconfigs.yaml +++ b/bootstrap/config/crd/bases/bootstrap.cluster.x-k8s.io_ck8sconfigs.yaml @@ -51,6 +51,9 @@ spec: items: type: string type: array + channel: + description: Channel is the channel to use for the snap install. + type: string controlPlane: description: CK8sControlPlaneConfig is configuration for the control plane node. @@ -192,6 +195,11 @@ spec: the default CNI. type: boolean type: object + localPath: + description: |- + LocalPath is the path of a local snap file in the workload cluster to use for the snap install. + If Channel or Revision are set, this will be ignored. + type: string nodeName: description: |- NodeName is the name to use for the kubelet of this node. It is needed for clouds @@ -210,6 +218,11 @@ spec: items: type: string type: array + revision: + description: |- + Revision is the revision to use for the snap install. + If Channel is set, this will be ignored. + type: string snapstoreProxyDomain: description: The snap store proxy domain type: string diff --git a/bootstrap/config/crd/bases/bootstrap.cluster.x-k8s.io_ck8sconfigtemplates.yaml b/bootstrap/config/crd/bases/bootstrap.cluster.x-k8s.io_ck8sconfigtemplates.yaml index 1ee229cc..ad549272 100644 --- a/bootstrap/config/crd/bases/bootstrap.cluster.x-k8s.io_ck8sconfigtemplates.yaml +++ b/bootstrap/config/crd/bases/bootstrap.cluster.x-k8s.io_ck8sconfigtemplates.yaml @@ -58,6 +58,9 @@ spec: items: type: string type: array + channel: + description: Channel is the channel to use for the snap install. + type: string controlPlane: description: CK8sControlPlaneConfig is configuration for the control plane node. @@ -201,6 +204,11 @@ spec: enable the default CNI. type: boolean type: object + localPath: + description: |- + LocalPath is the path of a local snap file in the workload cluster to use for the snap install. + If Channel or Revision are set, this will be ignored. + type: string nodeName: description: |- NodeName is the name to use for the kubelet of this node. It is needed for clouds @@ -219,6 +227,11 @@ spec: items: type: string type: array + revision: + description: |- + Revision is the revision to use for the snap install. + If Channel is set, this will be ignored. + type: string snapstoreProxyDomain: description: The snap store proxy domain type: string diff --git a/bootstrap/controllers/ck8sconfig_controller.go b/bootstrap/controllers/ck8sconfig_controller.go index c0d694f0..ebb13c47 100644 --- a/bootstrap/controllers/ck8sconfig_controller.go +++ b/bootstrap/controllers/ck8sconfig_controller.go @@ -258,7 +258,20 @@ func (r *CK8sConfigReconciler) joinControlplane(ctx context.Context, scope *Scop return err } - snapInstallData := r.resolveInPlaceUpgradeRelease(machine) + snapInstallData, err := r.getSnapInstallDataFromSpec(scope.Config.Spec) + if err != nil { + return fmt.Errorf("failed to get snap install data from spec: %w", err) + } + + // If the machine has an in-place upgrade annotation, use it to set the snap install data + inPlaceInstallData := r.resolveInPlaceUpgradeRelease(machine) + if inPlaceInstallData != nil { + snapInstallData = inPlaceInstallData + } + + // log snapinstalldata + scope.Info("SnapInstallData Spec", "Option", scope.Config.Spec.Channel, "Value", scope.Config.Spec.Revision, "LocalPath", scope.Config.Spec.LocalPath) + scope.Info("SnapInstallData", "Option", snapInstallData.Option, "Value", snapInstallData.Value) input := cloudinit.JoinControlPlaneInput{ BaseUserData: cloudinit.BaseUserData{ @@ -343,7 +356,16 @@ func (r *CK8sConfigReconciler) joinWorker(ctx context.Context, scope *Scope) err return err } - snapInstallData := r.resolveInPlaceUpgradeRelease(machine) + snapInstallData, err := r.getSnapInstallDataFromSpec(scope.Config.Spec) + if err != nil { + return fmt.Errorf("failed to get snap install data from spec: %w", err) + } + + // If the machine has an in-place upgrade annotation, use it to set the snap install data + inPlaceInstallData := r.resolveInPlaceUpgradeRelease(machine) + if inPlaceInstallData != nil { + snapInstallData = inPlaceInstallData + } input := cloudinit.JoinWorkerInput{ BaseUserData: cloudinit.BaseUserData{ @@ -403,39 +425,83 @@ 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{} + if mAnnotations == nil { + return nil } val, ok := mAnnotations[bootstrapv1.InPlaceUpgradeReleaseAnnotation] - if ok { - optionKv := strings.Split(val, "=") - - switch optionKv[0] { - case "channel": - return cloudinit.SnapInstallData{ - Option: cloudinit.InstallOptionChannel, - Value: optionKv[1], - } - case "revision": - return cloudinit.SnapInstallData{ - Option: cloudinit.InstallOptionRevision, - Value: optionKv[1], - } - case "localPath": - return cloudinit.SnapInstallData{ - Option: cloudinit.InstallOptionLocalPath, - Value: optionKv[1], - } - default: - r.Log.Info("Unknown in-place upgrade release option, ignoring", "option", optionKv[0]) + if !ok { + return nil + } + + optionKv := strings.Split(val, "=") + + if len(optionKv) != 2 { + r.Log.Info("Invalid in-place upgrade release annotation, ignoring", "annotation", val) + return nil + } + + switch optionKv[0] { + case "channel": + return &cloudinit.SnapInstallData{ + Option: cloudinit.InstallOptionChannel, + Value: optionKv[1], + } + case "revision": + return &cloudinit.SnapInstallData{ + Option: cloudinit.InstallOptionRevision, + Value: optionKv[1], + } + case "localPath": + return &cloudinit.SnapInstallData{ + Option: cloudinit.InstallOptionLocalPath, + Value: optionKv[1], } + default: + r.Log.Info("Unknown in-place upgrade release option, ignoring", "option", optionKv[0]) + } + + return nil +} + +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") } - return cloudinit.SnapInstallData{} + switch { + case spec.Channel != "": + return &cloudinit.SnapInstallData{ + Option: cloudinit.InstallOptionChannel, + Value: spec.Channel, + }, nil + case spec.Revision != "": + return &cloudinit.SnapInstallData{ + Option: cloudinit.InstallOptionRevision, + Value: spec.Revision, + }, nil + case spec.LocalPath != "": + return &cloudinit.SnapInstallData{ + Option: cloudinit.InstallOptionLocalPath, + Value: spec.LocalPath, + }, nil + default: + return &cloudinit.SnapInstallData{}, nil + } } // resolveSecretFileContent returns file content fetched from a referenced secret object. @@ -576,7 +642,11 @@ func (r *CK8sConfigReconciler) handleClusterNotInitialized(ctx context.Context, return ctrl.Result{}, fmt.Errorf("failed to render k8sd-proxy daemonset: %w", err) } - snapInstallData := r.resolveInPlaceUpgradeRelease(machine) + snapInstallData, err := r.getSnapInstallDataFromSpec(scope.Config.Spec) + if err != nil { + conditions.MarkFalse(scope.Config, bootstrapv1.SnapInstallDataValidatedCondition, bootstrapv1.SnapInstallValidationFailedReason, clusterv1.ConditionSeverityError, err.Error()) + return ctrl.Result{Requeue: true}, fmt.Errorf("failed to get snap install data from spec: %w", err) + } cpinput := cloudinit.InitControlPlaneInput{ BaseUserData: cloudinit.BaseUserData{ diff --git a/controlplane/config/crd/bases/controlplane.cluster.x-k8s.io_ck8scontrolplanes.yaml b/controlplane/config/crd/bases/controlplane.cluster.x-k8s.io_ck8scontrolplanes.yaml index ba5ce55c..8499bebe 100644 --- a/controlplane/config/crd/bases/controlplane.cluster.x-k8s.io_ck8scontrolplanes.yaml +++ b/controlplane/config/crd/bases/controlplane.cluster.x-k8s.io_ck8scontrolplanes.yaml @@ -246,6 +246,9 @@ spec: items: type: string type: array + channel: + description: Channel is the channel to use for the snap install. + type: string controlPlane: description: CK8sControlPlaneConfig is configuration for the control plane node. @@ -389,6 +392,11 @@ spec: the default CNI. type: boolean type: object + localPath: + description: |- + LocalPath is the path of a local snap file in the workload cluster to use for the snap install. + If Channel or Revision are set, this will be ignored. + type: string nodeName: description: |- NodeName is the name to use for the kubelet of this node. It is needed for clouds @@ -407,6 +415,11 @@ spec: items: type: string type: array + revision: + description: |- + Revision is the revision to use for the snap install. + If Channel is set, this will be ignored. + type: string snapstoreProxyDomain: description: The snap store proxy domain type: string diff --git a/controlplane/config/crd/bases/controlplane.cluster.x-k8s.io_ck8scontrolplanetemplates.yaml b/controlplane/config/crd/bases/controlplane.cluster.x-k8s.io_ck8scontrolplanetemplates.yaml index b8c41013..ca5f7fc7 100644 --- a/controlplane/config/crd/bases/controlplane.cluster.x-k8s.io_ck8scontrolplanetemplates.yaml +++ b/controlplane/config/crd/bases/controlplane.cluster.x-k8s.io_ck8scontrolplanetemplates.yaml @@ -221,6 +221,10 @@ spec: items: type: string type: array + channel: + description: Channel is the channel to use for the snap + install. + type: string controlPlane: description: CK8sControlPlaneConfig is configuration for the control plane node. @@ -366,6 +370,11 @@ spec: to enable the default CNI. type: boolean type: object + localPath: + description: |- + LocalPath is the path of a local snap file in the workload cluster to use for the snap install. + If Channel or Revision are set, this will be ignored. + type: string nodeName: description: |- NodeName is the name to use for the kubelet of this node. It is needed for clouds @@ -384,6 +393,11 @@ spec: items: type: string type: array + revision: + description: |- + Revision is the revision to use for the snap install. + If Channel is set, this will be ignored. + type: string snapstoreProxyDomain: description: The snap store proxy domain type: string diff --git a/pkg/ck8s/workload_cluster.go b/pkg/ck8s/workload_cluster.go index 89e0a550..a52ae00f 100644 --- a/pkg/ck8s/workload_cluster.go +++ b/pkg/ck8s/workload_cluster.go @@ -243,6 +243,11 @@ func (w *Workload) RefreshMachine(ctx context.Context, machine *clusterv1.Machin request := apiv1.SnapRefreshRequest{} response := &apiv1.SnapRefreshResponse{} optionKv := strings.Split(upgradeOption, "=") + + if len(optionKv) != 2 { + return "", fmt.Errorf("invalid in-place upgrade release annotation: %s", upgradeOption) + } + switch optionKv[0] { case "channel": request.Channel = optionKv[1] 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 d497d1d0..8c2647d2 100644 --- a/pkg/cloudinit/controlplane_init_test.go +++ b/pkg/cloudinit/controlplane_init_test.go @@ -152,31 +152,37 @@ func TestNewInitControlPlaneSnapInstall(t *testing.T) { g.Expect(config.WriteFiles).ToNot(ContainElement(HaveField("Path", fmt.Sprintf("/capi/etc/snap-%s", cloudinit.InstallOptionRevision)))) g.Expect(config.WriteFiles).ToNot(ContainElement(HaveField("Path", fmt.Sprintf("/capi/etc/snap-%s", cloudinit.InstallOptionLocalPath)))) }) +} +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/stable", + Value: "v1.30/edge", }, + notOptions: []cloudinit.InstallOption{cloudinit.InstallOptionRevision, cloudinit.InstallOptionLocalPath}, }, { name: "RevisionOverride", - snapInstall: cloudinit.SnapInstallData{ + snapInstall: &cloudinit.SnapInstallData{ Option: cloudinit.InstallOptionRevision, Value: "123", }, + notOptions: []cloudinit.InstallOption{cloudinit.InstallOptionChannel, cloudinit.InstallOptionLocalPath}, }, { name: "LocalPathOverride", - snapInstall: cloudinit.SnapInstallData{ + snapInstall: &cloudinit.SnapInstallData{ Option: cloudinit.InstallOptionLocalPath, Value: "/path/to/k8s.snap", }, + notOptions: []cloudinit.InstallOption{cloudinit.InstallOptionChannel, cloudinit.InstallOptionRevision}, }, } @@ -199,6 +205,13 @@ func TestNewInitControlPlaneSnapInstall(t *testing.T) { "Path": Equal(fmt.Sprintf("/capi/etc/snap-%s", tt.snapInstall.Option)), "Content": Equal(tt.snapInstall.Value), }))) + + // Check that the incorrect files are not written + for _, notOption := range tt.notOptions { + g.Expect(config.WriteFiles).NotTo(ContainElement(gstruct.MatchFields(gstruct.IgnoreExtras, gstruct.Fields{ + "Path": Equal(fmt.Sprintf("/capi/etc/snap-%s", notOption)), + }))) + } }) } } 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", },