Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add support for etcd backups with s3 storage #471

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ See my public profile with links for connecting with me [here](https://vitobotta
- [Load balancers](docs/Load_balancers.md)
- [Storage](docs/Storage.md)
- [Troubleshooting](docs/Troubleshooting.md)
- [S3 Backups](docs/S3%20backups.md)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd specify, in the title, that this is for etcd backups specifically, otherwise less experienced users might mistakenly assume that this doe some other kind of backup instead.

- [Contributing and support](docs/Contributing_and_support.md)

___
Expand Down
12 changes: 12 additions & 0 deletions docs/Creating_a_cluster.md
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,18 @@ networking:
datastore:
mode: etcd # etcd (default) or external
external_datastore_endpoint: postgres://....
# s3: # can only be enabled for etcd mode
# enabled: false
# endpoint: "s3.amazonaws.com" # optional defaults to s3.amazonaws.com
# endpoint_ca: "" # optional
# skip_ssl_verify: false # optional defaults to false
# access_key: ""
# secret_key: ""
# bucket: ""
# region: "us-east-1" # optional defaults to us-east-1
# folder: "" # optional
# insecure: false # optional defaults to false
# timeout: "5m0s" # optional defaults to 5m0s
Comment on lines +50 to +61
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would probably follow a convention I have seen often for this kind of thing, where the group of settings - for a specific type of store in this case - is named after the store type itself. So instead of naming the group s3, I would name the group etcd to convey that those settings are relative to etcd, instead of relying on a comment. Then I'd prefix the s3 specific settings with s3_ so it's clear that the storage supported is s3.


schedule_workloads_on_masters: false

Expand Down
5 changes: 5 additions & 0 deletions docs/S3 backups.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
### S3 Backups

S3 backups can be enabled for the embedded ectd mode only. You can see the explainations for each option in the [K3s docs](https://docs.k3s.io/cli/etcd-snapshot).

> "In addition to backing up the datastore itself, you must also back up the server token file at /var/lib/rancher/k3s/server/token. You must restore this file, or pass its value into the --token option, when restoring from backup. If you do not use the same token value when restoring, the snapshot will be unusable, as the token is used to encrypt confidential data within the datastore itself." [K3S Backup/Restore Docs](https://docs.k3s.io/datastore/backup-restore)
3 changes: 3 additions & 0 deletions src/configuration/datastore.cr
Original file line number Diff line number Diff line change
@@ -1,11 +1,14 @@
require "yaml"
require "./s3"

class Configuration::Datastore
include YAML::Serializable

getter mode : String = "etcd"
getter external_datastore_endpoint : String = ""

getter s3 : Configuration::S3 = Configuration::S3.new
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By convention, I'd add an intermediate namespace as in Configuration::Datastore::S3 since this configuration is specific to the datastore only.


def initialize(@mode : String = "etcd", @external_datastore_endpoint : String = "")
end
end
20 changes: 20 additions & 0 deletions src/configuration/s3.cr
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
require "yaml"

class Configuration::S3
include YAML::Serializable

getter enabled : Bool = false
getter endpoint : String?
getter endpoint_ca : String?
getter skip_ssl_verify : Bool = false
getter access_key : String = ""
getter secret_key : String = ""
getter bucket : String = ""
getter region : String?
getter folder : String?
getter insecure : Bool = false
getter timeout : String?

def initialize()
end
end
4 changes: 4 additions & 0 deletions src/configuration/settings/datastore.cr
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
require "./s3"

class Configuration::Settings::Datastore
getter errors : Array(String)
getter datastore : Configuration::Datastore
Expand All @@ -8,8 +10,10 @@ class Configuration::Settings::Datastore
def validate
case datastore.mode
when "etcd"
Settings::S3.new(errors, datastore.s3).validate
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also should ideally be Settings::Datastore::S3

when "external"
errors << "external_datastore_endpoint is required for external datastore" if datastore.external_datastore_endpoint.strip.empty?
errors << "s3 backups is only availabe on 'etcd' datastore" if datastore.s3.enabled
else
errors << "datastore mode is invalid - allowed values are 'etcd' and 'external'"
end
Expand Down
16 changes: 16 additions & 0 deletions src/configuration/settings/s3.cr
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
class Configuration::Settings::S3
getter errors : Array(String)
getter s3 : Configuration::S3

def initialize(@errors, @s3)
end

def validate
case s3.enabled
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps you could simplify this a bit since you are only checking one condition and the case statement is overkill.

Suggested change
case s3.enabled
return unless s3.enabled
errors << "access_key is required for S3 backups" if s3.access_key.strip.empty?
errors << "secret_key is required for S3 backups" if s3.secret_key.strip.empty?
errors << "bucket is required for S3 backups" if s3.bucket.strip.empty?

when true
errors << "access_key is required for S3 backups" if s3.access_key.strip.empty?
errors << "secret_key is required for S3 backups" if s3.secret_key.strip.empty?
errors << "bucket is required for S3 backups" if s3.bucket.strip.empty?
end
end
end
19 changes: 19 additions & 0 deletions src/kubernetes/installer.cr
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,7 @@ class Kubernetes::Installer
cluster_dns: settings.networking.cluster_dns,
datastore_endpoint: datastore_endpoint,
etcd_arguments: etcd_arguments,
s3_arguments: generate_s3_arguments(),
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly to Ruby, there is no need for () when invoking a method without any arguments :)

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also I'd rename this to etcd_backup_settings or etcd_snapshot_settings to make it clear that these settings are for etcd backups/snapshots.

embedded_registry_mirror_enabled: settings.embedded_registry_mirror.enabled.to_s,
})
end
Expand Down Expand Up @@ -388,4 +389,22 @@ class Kubernetes::Installer
first_master.private_ip_address
end
end

private def generate_s3_arguments
opts = [] of String

opts << "--etcd-s3" if settings.datastore.s3.enabled
opts << "--etcd-s3-endpoint=#{settings.datastore.s3.endpoint}" if present?(settings.datastore.s3.endpoint)
opts << "--etcd-s3-endpoint-ca=#{settings.datastore.s3.endpoint_ca}" if present?(settings.datastore.s3.endpoint_ca)
opts << "--etcd-s3-skip-ssl-verify" if settings.datastore.s3.skip_ssl_verify
opts << "--etcd-s3-access-key=#{settings.datastore.s3.access_key}" if present?(settings.datastore.s3.access_key)
opts << "--etcd-s3-secret-key=#{settings.datastore.s3.secret_key}" if present?(settings.datastore.s3.secret_key)
opts << "--etcd-s3-bucket=#{settings.datastore.s3.bucket}" if present?(settings.datastore.s3.bucket)
opts << "--etcd-s3-region=#{settings.datastore.s3.region}" if present?(settings.datastore.s3.region)
opts << "--etcd-s3-folder=#{settings.datastore.s3.folder}" if present?(settings.datastore.s3.folder)
opts << "--etcd-s3-insecure" if settings.datastore.s3.insecure
opts << "--etcd-s3-timeout=#{settings.datastore.s3.timeout}" if present?(settings.datastore.s3.timeout)

opts.uniq.sort.join(" ")
end
end
4 changes: 4 additions & 0 deletions src/kubernetes/util.cr
Original file line number Diff line number Diff line change
Expand Up @@ -83,4 +83,8 @@ module Kubernetes::Util

port_open?(ip_address, port, timeout = 1.0)
end

private def present?(value : String?)
value.try(&.strip).try(&.empty?) == false
end
end
2 changes: 1 addition & 1 deletion templates/master_install_script.sh
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ curl -sfL https://get.k3s.io | INSTALL_K3S_VERSION="{{ k3s_version }}" K3S_TOKEN
--kube-controller-manager-arg="bind-address=0.0.0.0" \
--kube-proxy-arg="metrics-bind-address=0.0.0.0" \
--kube-scheduler-arg="bind-address=0.0.0.0" \
{{ taint }} {{ extra_args }} {{ etcd_arguments }} $FLANNEL_SETTINGS $EMBEDDED_REGISTRY_MIRROR \
{{ taint }} {{ extra_args }} {{ etcd_arguments }} {{ s3_arguments }} $FLANNEL_SETTINGS $EMBEDDED_REGISTRY_MIRROR \
--advertise-address=$PRIVATE_IP \
--node-ip=$PRIVATE_IP \
--node-external-ip=$PUBLIC_IP \
Expand Down