Allow cluster-dns-ip to be set to a list of IP Addresses
#2176
Conversation
cbgbt
force-pushed
the
clusterdnsip
branch
2 times, most recently
from
c5aacb6
to
c8ba9db
Compare
|
Fixed formatting. I always forget to run check-fmt 😮💨 |
| {{#each settings.kubernetes.cluster-dns-ip}} | ||
| - {{this}} | ||
| {{else}} | ||
| - {{settings.kubernetes.cluster-dns-ip}} | ||
| {{/each}} |
There was a problem hiding this comment.
Need to make sure this ends up in 1.23 as well, if #2140 lands first.
| /// for backwards compatibility. | ||
| #[derive(Debug, Clone, Eq, PartialEq, Hash, Serialize, Deserialize)] | ||
| #[serde(untagged)] | ||
| pub enum KubernetesClusterDNSIp { |
There was a problem hiding this comment.
Maybe this instead?
| pub enum KubernetesClusterDNSIp { | |
| pub enum KubernetesClusterDnsIp { |
|
|
||
| /// KubernetesClusterDNSIp represents the --cluster-dns settings for kubelet. | ||
| /// | ||
| /// The default is determined via information from IMDS in AWS. |
There was a problem hiding this comment.
Do we need to mention where the default comes from? Feels like a different concern.
| #[test] | ||
| fn test_parse_cluster_dns_ip_from_list() { | ||
| assert_eq!( | ||
| serde_json::from_str::<KubernetesClusterDNSIp>(r#"[]"#).unwrap(), | ||
| KubernetesClusterDNSIp::Vector(vec![]) | ||
| ); |
There was a problem hiding this comment.
If we don't allow an empty string, should we allow an empty list?
I am OK either way as long as this doesn't cause trouble with kubelet when rendering the template:
[settings.kubernetes]
cluster-dns-ip = []
There was a problem hiding this comment.
The guard in the kubelet configs protects the config from an empty list, as empty lists are considered "falsey" by handlebars.
I've tested empirically and confirmed that an empty list results in clusterDNS being absent from the kubelet config.
I think an empty string is "less correct" model-wise than an empty list. I'd like the consistency of disallowing both, but I'm not sure it's worth writing the custom deserializer since there's not an easy way to enforce the constraint in serde otherwise.
| pub fn first(&self) -> Option<&IpAddr> { | ||
| self.iter().next() | ||
| } |
There was a problem hiding this comment.
Why add a special first method vs. making callers write iter().next() ? It seems like giving them the iterator and letting them use any of the trait methods is more powerful.
| .and_then(|ip_array| ip_array.iter().next()) | ||
| .map(|ip_value| ip_value.clone()); |
There was a problem hiding this comment.
Checking my understanding here also -
If we have a value and it's the empty list, we'll have a None here, and remove the setting from the datastore. The previous template had a guard for whether the setting was defined, so this is safe - we'll render a config file without that section.
|
Force push to address feedback from @bcressey. |
| } | ||
| } | ||
|
|
||
| #[cfg(test)] |
There was a problem hiding this comment.
Nice tests.
Edit: somehow my other comment on the migration tests was lost. Basically, awesome that we have migration tests! 🚀
| let cluster_dns_ip = if let Some(ip) = aws_k8s_info.cluster_dns_ip { | ||
| let configured_cluster_dns_ip = aws_k8s_info | ||
| .cluster_dns_ip | ||
| .and_then(|cluster_ip| cluster_ip.iter().next().cloned()); |
There was a problem hiding this comment.
It didn't have any tips here, but it did suggest some others in pluto, so I pushed some small changes.
While clusterDNS is modeled as a list in the kubelet config, we allow the settings to be given as either a string or list in the API for backwards compatibility. The API will store and return the data as-provided, and the model provides a unified iterator view over both models.
|
Rebase on develop. Fix two clippy warnings in pluto. |
Issue number: #2132
Description of changes:
This allows
settings.kubernetes.cluster-dns-ipto be set to a list. The value may also remain as a string, for backwards compatibility. The model will store the data using the type submitted, so existing code expecting string types will continue to work.If a list is present, downwards migrations will truncate the list, using the first element as the configured value.
Testing done:
/etc/kubernetes/kubelet/configcontainsclusterDNSsettings, regardless of if the setting is a string or a list/etc/resolv.confcontains themTerms 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.