Skip to content
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

Configurable cluster domain #988

Merged
merged 4 commits into from
Jul 15, 2020

Conversation

spoonofpower
Copy link
Contributor

Issue number:
N/A

Description of changes:
This change allows users to set the value for the kubelet's clusterDomain setting. By default the clusterDomain is set to cluster.local, but users can set it to any valid domain. A description of the clusterDomain setting can be found here.

We have multiple EKS clusters and each uses a different clusterDomain. We'd prefer not to have all our EKS clusters using cluster.local.

Testing done:
Tested using the aws-k8s-1.15 variant on our EKS clusters. If no value is set for settings.kubernetes.cluster-domain in the user data, the kubelet will be configured to use cluster.local for the clusterDomain. This is the current behaviour. By default this PR doesn't change the current behaviour. If there is a value for settings.kubernetes.cluster-domain, the kubelet will be configure to use that value for the clusterDomain.

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.

@tjkirch tjkirch self-requested a review July 13, 2020 15:10
Copy link
Contributor

@tjkirch tjkirch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution! We'll also test the other variants (and respond here) since this is something that affects runtime behavior.

README.md Outdated Show resolved Hide resolved
sources/models/defaults.toml Outdated Show resolved Hide resolved
sources/models/src/modeled_types/shared.rs Outdated Show resolved Hide resolved
sources/models/src/modeled_types/shared.rs Outdated Show resolved Hide resolved
sources/models/src/modeled_types/shared.rs Outdated Show resolved Hide resolved
sources/models/src/modeled_types/shared.rs Outdated Show resolved Hide resolved
sources/models/src/modeled_types/shared.rs Show resolved Hide resolved
@tjkirch
Copy link
Contributor

tjkirch commented Jul 13, 2020

@etungsten was kind enough to verify the functionality on all of the Kubernetes variants. :)

@tjkirch tjkirch requested a review from etungsten July 13, 2020 18:56
- Improve description in README.md
- Move default setting to the correct location
- Use url::Host::parse for the validation
@spoonofpower
Copy link
Contributor Author

@tjkirch Thanks for the review. I think I addressed all your concerns. Let me know if there's more I can do.

Copy link
Contributor

@tjkirch tjkirch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, just a couple simplification suggestions. I'll poke someone else to review as well.

sources/models/src/modeled_types/shared.rs Outdated Show resolved Hide resolved
sources/models/src/modeled_types/shared.rs Outdated Show resolved Hide resolved
sources/models/src/modeled_types/shared.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@etungsten etungsten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@spoonofpower
Copy link
Contributor Author

spoonofpower commented Jul 15, 2020

I'm happy with the changes, and all my testing looks good. Let me know if you need anything else changed.

@tjkirch tjkirch merged commit 55aee0a into bottlerocket-os:develop Jul 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants