add support for etcd backups with s3 storage #471
Conversation
|
| # 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 |
There was a problem hiding this comment.
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.
| @@ -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) | |||
There was a problem hiding this comment.
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.
|
|
||
| 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.
By convention, I'd add an intermediate namespace as in Configuration::Datastore::S3 since this configuration is specific to the datastore only.
| @@ -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.
This also should ideally be Settings::Datastore::S3
| end | ||
|
|
||
| def validate | ||
| case s3.enabled |
There was a problem hiding this comment.
Perhaps you could simplify this a bit since you are only checking one condition and the case statement is overkill.
| 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? |
| @@ -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.
Similarly to Ruby, there is no need for () when invoking a method without any arguments :)
There was a problem hiding this comment.
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.
K3S has built in support for uploading snapshots of the embedded etcd database to S3. This PR adds the settings to enable that to the configuration file.
If the s3 settings are not included it will default to not being enabled, to ensure backward compatibility.
I also added a check to make sure it is only enabled when using the etcd datastore mode.
It may also work with other S3 compatible providers. I cannot give a list, however I tested it with Backblaze B2 and it worked.
An example when uploading to AWS S3 in the us-east-1 region: