-
-
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?
Conversation
Quality Gate passedIssues Measures |
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.
Wow, you made me VERY happy with this PR! It was on my list, so I appreciate you took the time to implement it.
I requested some changes if you don't mind, but these are mostly minor things and quick changes as you have done a great job really.
Thanks a lot!
# 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.
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.
@@ -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.
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.
|
||
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 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.
@@ -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 comment
The reason will be displayed to describe this comment to others. Learn more.
This also should ideally be Settings::Datastore::S3
end | ||
|
||
def validate | ||
case s3.enabled |
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.
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.
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 :)
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.
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: