kubelet: add setting for configuring cloudProvider #1494
Conversation
jpculp
changed the title
| /// kubelet. It stores the original string and makes it accessible through standard traits. | ||
| #[derive(Debug, Clone, Eq, PartialEq, Hash)] | ||
| pub struct KubernetesCloudProvider { | ||
| inner: String, |
There was a problem hiding this comment.
Maybe an enum would be better. Not a strong preference but it looks like we have enums for other settings.
There was a problem hiding this comment.
I think I'd reach for an enum as well. :)
There was a problem hiding this comment.
I'm not crazy about the enum for something this small. This block of code used KubernetesAuthenticationMode as a reference.
There was a problem hiding this comment.
I'm open to replacing it with an enum later, but we would want to address #1501 first.
README.md
Outdated
| @@ -308,6 +308,7 @@ The following settings can be optionally set to customize the node labels and ta | |||
| The following settings are optional and allow you to further configure your cluster. | |||
| * `settings.kubernetes.cluster-domain`: The DNS domain for this cluster, allowing all Kubernetes-run containers to search this domain before the host's search domains. Defaults to `cluster.local`. | |||
| * `settings.kubernetes.standalone-mode`: Whether to run the kubelet in standalone mode, without connecting to an API server. Defaults to `false`. | |||
| * `settings.kubernetes.cloud-provider`: The cloud platform for this cluster. Defaults to `aws` for AWS variants, and `external` for other variants. | |||
There was a problem hiding this comment.
We should probably say "cloud provider" rather than "cloud platform".
There was a problem hiding this comment.
I was trying to avoid repeating the name of the setting, but if folks feel strongly or have a less redundant suggestion I'm open to changing it.
There was a problem hiding this comment.
I think we should keep it in line with the term that the k8s official doc uses to remove any ambiguity on what this is for. See https://v1-18.docs.kubernetes.io/docs/concepts/cluster-administration/cloud-providers/
| /// kubelet. It stores the original string and makes it accessible through standard traits. | ||
| #[derive(Debug, Clone, Eq, PartialEq, Hash)] | ||
| pub struct KubernetesCloudProvider { | ||
| inner: String, |
There was a problem hiding this comment.
I think I'd reach for an enum as well. :)
jpculp
force-pushed
the
kubelet-cloud-provider-setting
branch
from
f022822
to
99d6483
Compare
|
|
| {{~/unless}} | ||
| --cloud-provider {{settings.kubernetes.cloud-provider}} \ |
There was a problem hiding this comment.
We need the old logic here so that we pass an empty string to cloud-provider if standalone mode is enabled.
Adds a new setting `kubernetes.cloud-provider` for configuring whether the cloud provider is `aws` or `external`. Prior to this, the argument was hard-coded to `aws`.
Adds a migration for the new `kubernetes.cloud-provider` setting.
jpculp
force-pushed
the
kubelet-cloud-provider-setting
branch
from
99d6483
to
104122a
Compare
|
|
Tested migration with a variant that did not have a default cloud provider setting set. Launched as |
Issue number:
#1459
Description of changes:
Adds a new setting
kubernetes.cloud-providerfor configuring whether the cloud provider isawsorexternal.Prior to this, the argument was hard-coded to
aws.Adds a migration for the new
kubernetes.cloud-providersetting.Testing done:
aws-k8s-1.19ami.sudo sheltie.aws-k8s-1.18.aws-k8s-1.17.aws-k8s-1.16.Terms of contribution:
By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.