-
Notifications
You must be signed in to change notification settings - Fork 5k
Add a default cluster dns name config #21064
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
base: master
Are you sure you want to change the base?
Conversation
|
Welcome @wt! |
Hi @wt. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: wt The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Can one of the admins verify this patch? |
I asked about this feature here: https://kubernetes.slack.com/archives/C1F5CT6Q1/p1752280310063959 |
cb277f6
to
1e79d8f
Compare
1e79d8f
to
e0004af
Compare
e0004af
to
51f6a55
Compare
I have updated the commit message with details about the flag vs the config option. I hope this makes sense. |
fcb4218
to
d830514
Compare
I think I have fixed all the lints/tests. I added to the docs for the config command as well. |
/ok-to-test |
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.
please put before/after this PR with full example to point out what is changing in this PR exactly and why do we need this
I already add a Before/After section to the commit message of the PR. Can you please help me understand what is missing from the description? Do you want me to put a commandline sequence and describe the difference like I did above the Before/After sections? |
70d095f
to
b67355e
Compare
b67355e
to
23f5856
Compare
@medyagh I added a bunch of before/after context. I think this addresses your latest comment. Please let me know if more is needed. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
the Before/After Description is most useful in the PR Description for the reviwer and also in the changelog for the users to click on PRs to see what a PR is about, we dont utilize commit messages to be Long format, I prefer commit messages to be short and relevant to the commit and any other extra info should be in the PR Description |
The PR message is hard to find when inspecting git history later, so all important content should be accessible In this case the commit message looks very good (but I did not ready it yet), but the PR mesage is not helpful. @wt you can copy the commit message to the PR message to help reviewers. |
I updated the original PR message to match the current commit message. I didn't realize you wanted me to change that before. Thanks for spelling that out. |
FWIW, I have had some time to think about this change a bit more. I realized that a more powerful version of this change's idea might allow one to config only the This can be added on top of this change. I do not think that the scope of this change should be expanded to include this. I just wanted to see if y'all had any thoughts on this. |
7c179e3
to
2a6ab97
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@wt I just tested minikube on HEAD, here are example of output of Before/After that would help with review of PR: Beforein this example in minikube head, I start a cluster with --dns-domain="cluster.example.com"
regarding things only affecting clusters After, that is okay as long as it is same spirit for other commands, I wouldn't want to it to be a special case. so I think what your PR wants to do is, Allow setting the "dns-domain" globally for all clusters? (checking is that what your intend it?)
and then verify p2 and p3 have the Desired dns-domain as I did in the above ^^ |
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.
lets remove the go.work files and also do a go mod tidy and then I think should be good to go
(and also check if the logic for global config is same as other Global Configs)
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.
Looks useful, but the names are very confusing.
- We use defaultClusterDNSDomain for value which is not the default
- We use the term "cluster dns domain" in the code but "dns-domain" in the options
- The config name does not match the option name CamelCase vs lower-case.
Config names are a mess of CamelCase and lower-case. Let's not add more CamelCase names. In Go code we don't have a choice, but users should not pay this.
2a6ab97
to
aea0d5b
Compare
Addressed all of @nirs's comments. Need to write the test. |
It is very handy to be able to store the default cluster DNS name so that I can fire up and teardown clusters easily without losing that setting. You can think of this as a default value for the `start --dns-domain <val>` flag, but only when starting a new cluster. In my particular case, I would like to set my default cluster dna domain to a subdomain under a domain that I control. E.g. `cluster.wt.user.dev.example.com`. I want to do this to make it easier to manage TLS certs for my dev cluster. For context, the `start --dns-domain <val>` flag works like this: If you fire up a new cluster with that flag, you will get a new cluster with the domain name base. If you start an existing cluster, the config will be updated to the new domain name base and then started. This config will work a little differently. The change implements a config that will only affect newly started cluster. Here are some examples to show that difference: Newly started cluster example: ``` $ minikube config set default-dns-domain cluster2.local $ minikube start ... ``` Stop and starting the cluster with `--dns-domain` flag: ``` $ minikube stop ... $ minikub start --dns-domain cluster3.local ... ``` Stopping and starting a new cluster with a non-default domain name and without the `--dns-domain` flag: ``` $ minikube stop ... $ minikub start ... ``` The reason for this behavior is that I am not configuring the name of a cluster. That is a cluster configuration option. I am setting the default for that. If I manually overrode the name of a cluster when started previously, I don't want a default option to override my cluster configuration. Before: User cannot cannot change the default DNS domain name for a cluster to something other than cluster.local. On cluster creation/start: If a user runs `minikube start` without a `--dnsDomain` flag, the cluster will have the dns domain "cluster.local". A cluster setting is persisted. If a user runs 'minikube start --dnsDomain cluster.example.com`, the cluster will hae the dns domain "cluster.example.com". A cluster setting is persisted. On cluster restart: If a user run `minikube start`, the cluster's existing dns domain (pulled from the persisted cluster config) dns domain will be used. If a user runs `minikube start --dnsDomain cluster.example.com`, the cluster's persisted dns name will be updated to the new name and the cluster will be started with the new domain ("cluster.example.com"). Example command sequence: ``` $ minikube start --dnsDomain=cluster.example.com # cluster now has dns domain "cluster.example.com" $ minikube delete $ minikube start # cluster now has dns domain "cluster.local" ``` After: The behavior of the minikube start flag `--dns-domain` does not change at all. When the new configuration value is not set, all of the behaviors from the "Before" section above will not change. User can set the default cluster domain name to whatever they want via the config option `default-dns-domain`. E.g.: `minikube config set default-dns-domain cluster.wt.users.dev.example.com` Since the case with no configuration is the same as before, here is a description of what happens only with the new configuration set like above. On cluster creation/start: If a user runs `minikube start` without a `--dnsDomain` flag, the cluster will have the dns domain "cluster.wt.users.dev.example.com". A cluster setting is persisted. This is the only thing that changes with the configuration option set. If a user runs 'minikube start --dnsDomain cluster.example.com`, the cluster will hae the dns domain "cluster.example.com". A cluster setting is persisted. On cluster restart: If a user run `minikube start`, the cluster's existing dns domain (pulled from the persisted cluster config not from the new config) dns domain will be used. If a user runs `minikube start --dnsDomain cluster.example.com`, the cluster's persisted dns name will be updated to the new name and the cluster will be started with the new domain ("cluster.example.com"). Notice that the only case that changes with the new setting defined is the case of starting a new cluster. Setting the config will change the behavior of the "Before -> Example command sequence" section as described: Example command sequence: ``` $ minikube config set default-dns-domain cluster.wt.users.dev.example.com $ minikube start --dnsDomain=cluster.example.com # cluster now has dns domain "cluster.example.com" $ minikube delete $ minikube start # cluster now has dns domain "cluster.wt.users.dev.example.com" ``` The important piece here is that I can now change the default dnsDomain so that I don't have to specify it explicitly. This is useful when I am starting and deleting a single cluster where I want the top-level domain default to be a subdomain of a domain I control.
aea0d5b
to
8b82ebb
Compare
This comment has been minimized.
This comment has been minimized.
kvm2 driver with docker runtime
Times for minikube ingress: 15.0s 14.5s 14.5s 15.5s 14.5s Times for minikube (PR 21064) start: 48.7s 49.4s 52.3s 52.8s 50.5s docker driver with docker runtime
Times for minikube start: 21.9s 25.5s 24.2s 22.6s 24.7s Times for minikube ingress: 11.2s 12.3s 13.8s 12.8s 10.3s docker driver with containerd runtime
Times for minikube start: 21.0s 22.5s 21.3s 24.9s 20.7s Times for minikube ingress: 23.3s 38.8s 22.8s 23.3s 22.8s |
Add a default cluster DNS name config
It is very handy to be able to store the default cluster DNS name so
that I can fire up and teardown clusters easily without losing that
setting. You can think of this as a default value for the
start --dns-domain <val>
flag, but only when starting a new cluster.In my particular case, I would like to set my default cluster dna domain
to a subdomain under a domain that I control. E.g.
cluster.wt.user.dev.example.com
. I want to do this to make it easierto manage TLS certs for my dev cluster.
For context, the
start --dns-domain <val>
flag works like this:If you fire up a new cluster with that flag, you will get a new cluster
with the domain name base. If you start an existing cluster, the
config will be updated to the new domain name base and then started.
This config will work a little differently. The change implements a
config that will only affect newly started cluster. Here are some
examples to show that difference:
Newly started cluster example:
Stop and starting the cluster with
--dns-domain
flag:Stopping and starting a new cluster with a non-default domain name and
without the
--dns-domain
flag:The reason for this behavior is that I am not configuring the name of a
cluster. That is a cluster configuration option. I am setting the
default for that.
If I manually overrode the name of a cluster when started previously, I
don't want a default option to override my cluster configuration.
Before:
User cannot cannot change the default DNS domain name for a cluster
to something other than cluster.local.
On cluster creation/start:
If a user runs
minikube start
without a--dnsDomain
flag, thecluster will have the dns domain "cluster.local". A cluster setting
is persisted.
On cluster restart:
If a user run
minikube start
, the cluster's existing dns domain(pulled from the persisted cluster config) dns domain will be used.
Example command sequence:
After:
The behavior of the minikube start flag
--dns-domain
does notchange at all. When the new configuration value is not set, all of the
behaviors from the "Before" section above will not change.
User can set the default cluster domain name to whatever they want
via the config option DefaultDNSDomain. E.g.:
minikube config set DefaultDNSDomain cluster.wt.users.dev.example.com
Since the case with no configuration is the same as before, here is a
description of what happens only with the new configuration set like
above.
On cluster creation/start:
If a user runs
minikube start
without a--dnsDomain
flag, thecluster will have the dns domain "cluster.wt.users.dev.example.com".
A cluster setting is persisted. This is the only thing that changes
with the configuration option set.
On cluster restart:
If a user run
minikube start
, the cluster's existing dns domain(pulled from the persisted cluster config not from the new config)
dns domain will be used.
Notice that the only case that changes with the new setting defined is
the case of starting a new cluster. Setting the config will change the
behavior of the "Before -> Example command sequence" section as
described:
Example command sequence:
The important piece here is that I can now change the default
dnsDomain so that I don't have to specify it explicitly. This is
useful when I am starting and deleting a single cluster where I want
the top-level domain default to be a subdomain of a domain I control.