Add DNSDomain as a proxyConfig option, used to be values.yaml only

[costinm]

See the comments - most of our configuration is now in MeshConfig/ProxyConfig or env variables - this currently is configurable via values.yaml and gets set via '--domain' flag in Istiod and agent.

This is called 'clusterDomain' in kubelet and dnsDomain in kubeadmin - I used the later as name, don't mind using the first but it would be best to be one of the 2 to reduce confusion.

It is a very old setting - only existing comment is "# CAUTION: It is important to ensure that all Istio helm charts specify the same clusterDomain value"

Use of K8S with a different suffix is rare, but some vendors and kubeadmin allows this to be set.
Despite the age - in Istio 1.0 was called proxyDomain, 1.1 changed it to clusterDomain - this is a rarely used option and test coverage is limited, but it is required if K8S is set to use a custom domain instead of cluster.local, or the generated config will break FQDNs.

The goal is to improve documentation/testing and allow setting this via MeshConfig for consistency (and simplify a bit the related code)

[istio-policy-bot]

😊 Welcome @costinm! This is either your first contribution to the Istio api repo, or it's been
a while since you've been here.

You can learn more about the Istio working groups, Code of Conduct, and contribution guidelines
by referring to Contributing to Istio.

Thanks for contributing!

Courtesy of your friendly welcome wagon.

[istio-testing]

@costinm: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
release-notes_api	6b9f8c3	link	false	/test release-notes
gencheck_api	6b9f8c3	link	true	/test gencheck

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. I understand the commands that are listed here.

[howardjohn]

This only controls istio-agent (?) but we need it in istiod as well. What does that mean for usage?

[costinm]

No, it is used in istiod as well ( --domain ). Both must match. Not sure what you mean with "What does that mean for usage" - the idea is that if this is set, it will be used instead of the old value. And probably we'll need a bunch of fixes and more tests - because .cluster.local is hardcoded in a LOT of places, but need to have the proto updated first. For context - GKE now allows creating clusters with a custom DNS name, and OSS K8S had this feature for a very long time - and so did Istio, we just never tested it much.

…

On Fri, Jun 7, 2024 at 8:41 AM John Howard ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In mesh/v1alpha1/proxy.proto <#3234 (comment)>: > + // Must match the custom domain set in the external DNS server, if CoreDNS is not used. + // + // Do not set unless you are using a custom DNS domain for your K8S cluster and K8S + // is already set to use this value. + // + // The helm charts use "default.global.proxy.clusterDomain". + // + // Istiod uses this value to generate FQDNs for services. Agent uses this value + // in setting its node ID. + // + // See: + // https://kubernetes.io/docs/reference/config-api/kubelet-config.v1beta1/ + // https://kubernetes.io/docs/reference/config-api/kubeadm-config.v1beta3/#kubeadm-k8s-io-v1beta3-DNS + // + // $hide_from_docs + string DNSDomain = 40; This only controls istio-agent (?) but we need it in istiod as well. What does that mean for usage? — Reply to this email directly, view it on GitHub <#3234 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAUR2TB5Y243UXWOBYODR3ZGHIDRAVCNFSM6AAAAABI64IWW2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCMBUHA4DKNZRGY> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[costinm]

Right now it goes to kube/controller/Options.DomainSuffix - and kube.ServiceHostname is using it, not completely broken.

…

On Fri, Jun 7, 2024 at 12:45 PM Costin Manolache ***@***.***> wrote: No, it is used in istiod as well ( --domain ). Both must match. Not sure what you mean with "What does that mean for usage" - the idea is that if this is set, it will be used instead of the old value. And probably we'll need a bunch of fixes and more tests - because .cluster.local is hardcoded in a LOT of places, but need to have the proto updated first. For context - GKE now allows creating clusters with a custom DNS name, and OSS K8S had this feature for a very long time - and so did Istio, we just never tested it much. On Fri, Jun 7, 2024 at 8:41 AM John Howard ***@***.***> wrote: > ***@***.**** commented on this pull request. > ------------------------------ > > In mesh/v1alpha1/proxy.proto > <#3234 (comment)>: > > > + // Must match the custom domain set in the external DNS server, if CoreDNS is not used. > + // > + // Do not set unless you are using a custom DNS domain for your K8S cluster and K8S > + // is already set to use this value. > + // > + // The helm charts use "default.global.proxy.clusterDomain". > + // > + // Istiod uses this value to generate FQDNs for services. Agent uses this value > + // in setting its node ID. > + // > + // See: > + // https://kubernetes.io/docs/reference/config-api/kubelet-config.v1beta1/ > + // https://kubernetes.io/docs/reference/config-api/kubeadm-config.v1beta3/#kubeadm-k8s-io-v1beta3-DNS > + // > + // $hide_from_docs > + string DNSDomain = 40; > > This only controls istio-agent (?) but we need it in istiod as well. What > does that mean for usage? > > — > Reply to this email directly, view it on GitHub > <#3234 (review)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AAAUR2TB5Y243UXWOBYODR3ZGHIDRAVCNFSM6AAAAABI64IWW2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCMBUHA4DKNZRGY> > . > You are receiving this because you were mentioned.Message ID: > ***@***.***> >

[howardjohn]

proxy config is a per-pod setting so I don't see how it can impact mesh-wide settings in istiod

[costinm]

Right now Istiod gets "--domain" flag from default.global.proxy.clusterDomain, just like the injection template. Istiod is using quite a few of the 'default' proxy config settings - but there are very few leftovers of 'helm only configs' like this one - we normally use MeshConfig and the default ProxyConfig plus the env variables. This PR is just making it part of ProxyConfig - we'll obviously keep the clusterDomain setting (no backward incompatible changes). It remains configurable via "--domain" CLI as well - but I would deprecate both.

…

On Fri, Jun 7, 2024 at 12:59 PM John Howard ***@***.***> wrote: proxy config is a per-pod setting so I don't see how it can impact mesh-wide settings in istiod — Reply to this email directly, view it on GitHub <#3234 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAUR2R5NSNTY24FVNNUQWTZGIGIZAVCNFSM6AAAAABI64IWW2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNJVGQ3DOMBQGE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[costinm]

Well, it is arguable if it ever makes sense for a sidecar to use a different value from Istiod - and I don't think the code can correctly handle this. There is a use case - having a central Istiod ( not sure how stable that mode is ), and each cluster having a different DNS domain. I think we'll need quite a few more code changes to get that working - but the API and use case seem legitimate.

…

On Fri, Jun 7, 2024 at 1:18 PM Costin Manolache ***@***.***> wrote: Right now Istiod gets "--domain" flag from default.global.proxy.clusterDomain, just like the injection template. Istiod is using quite a few of the 'default' proxy config settings - but there are very few leftovers of 'helm only configs' like this one - we normally use MeshConfig and the default ProxyConfig plus the env variables. This PR is just making it part of ProxyConfig - we'll obviously keep the clusterDomain setting (no backward incompatible changes). It remains configurable via "--domain" CLI as well - but I would deprecate both. On Fri, Jun 7, 2024 at 12:59 PM John Howard ***@***.***> wrote: > proxy config is a per-pod setting so I don't see how it can impact > mesh-wide settings in istiod > > — > Reply to this email directly, view it on GitHub > <#3234 (comment)>, or > unsubscribe > <https://github.com/notifications/unsubscribe-auth/AAAUR2R5NSNTY24FVNNUQWTZGIGIZAVCNFSM6AAAAABI64IWW2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNJVGQ3DOMBQGE> > . > You are receiving this because you were mentioned.Message ID: > ***@***.***> >

[hzxuzhonghu]

And there is TrustDomain there, we usually set this same as DNSDomain, so what's the difference between them

[costinm]

Very different things (unfortunately). Trust domain is mesh wide - DNSDomain is cluster wide, i.e. each cluster will have a different zone ( cluster1.prod.example.com ). At least on GKE the trustdomain is project_id.svc.goog or cluster.local. In an ideal world - yes, they would be related ( prod.example.com trust domain, cluster1.prod.example.com the DNSDomain for that cluster ) - and we could also issue DNS certs that work with any client.

…

On Mon, Jun 10, 2024 at 6:44 PM hzxuzhonghu ***@***.***> wrote: And there is TrustDomain there, we usually set this same as DNSDomain, so what's the difference between them — Reply to this email directly, view it on GitHub <#3234 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAUR2VS4XB5WKBPEGBHIQTZGZJANAVCNFSM6AAAAABI64IWW2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNJZGYYTAMJZG4> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[costinm]

I think there are better solutions - and ProxyConfig is indeed confusing since it's also used for client. We'll probably need multiple domains anyways.