-
-
Notifications
You must be signed in to change notification settings - Fork 139
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
|
||
schedule_workloads_on_masters: false | ||
|
||
|
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) |
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. By convention, I'd add an intermediate namespace as in |
||
|
||
def initialize(@mode : String = "etcd", @external_datastore_endpoint : String = "") | ||
end | ||
end |
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 |
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 | ||
|
@@ -8,8 +10,10 @@ class Configuration::Settings::Datastore | |
def validate | ||
case datastore.mode | ||
when "etcd" | ||
Settings::S3.new(errors, datastore.s3).validate | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This also should ideally be |
||
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 | ||
|
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 | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Suggested change
|
||||||||||||||
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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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(), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similarly to Ruby, there is no need for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also I'd rename this to |
||
embedded_registry_mirror_enabled: settings.embedded_registry_mirror.enabled.to_s, | ||
}) | ||
end | ||
|
@@ -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 |
There was a problem hiding this comment.
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.