fix istioctl analyze vs error when a custom cluster domain #51064

nicole-lihui · 2024-05-15T09:12:03Z

resolved #33174

Please provide a description of this PR:

hzxuzhonghu · 2024-05-15T10:12:34Z

pkg/config/analysis/analyzers/util/service_lookup.go

@@ -61,6 +62,17 @@ func InitServiceEntryHostMap(ctx analysis.Context) map[ScopedFqdn]*v1alpha3.Serv
 		return true
 	})

+	// use meshConfig.trustDomain changed the default domain
+	// todo: replace by `values.global.proxy.clusterDomain`
+	ctx.ForEach(gvk.MeshConfig, func(r *resource.Instance) bool {


can we get MeshConfig with this function? I am not sure, MeshConig is from configmap

there's a hack for MeshConfig.

Yes, the configmap was saved in cache as MeshConfig for analysis during initialization.

hanxiaop · 2024-05-15T15:27:01Z

pkg/config/analysis/testing/fixtures/context.go

@@ -43,7 +43,9 @@ func (ctx *Context) Exists(config.GroupVersionKind, resource.FullName) bool { re
 // ForEach implements analysis.Context
 func (ctx *Context) ForEach(_ config.GroupVersionKind, fn analysis.IteratorFn) {
 	for _, r := range ctx.Resources {
-		fn(r)
+		if !fn(r) {


Why change this?

+1 to @hanxiaop question

I want to end the loop when the loop gets meshconfig, so added here

howardjohn · 2024-05-15T17:41:04Z

pkg/config/analysis/analyzers/util/hosts.go

+var configClusterLocalDomain string
+
+func SetConfigClusterLocalDomain(domain string) {
+	configClusterLocalDomain = "svc." + domain


Using globals like this is not safe or acceptable, this will cause data races. note this package is used in long running servers, not just the CLI

ok, that's indeed a problem, I'll find a new methods.

MorrisLaw · 2024-05-15T17:50:47Z

pkg/config/analysis/analyzers/util/service_lookup.go

+		meshConfig := r.Message.(*meshconfig.MeshConfig)
+		if meshConfig.GetTrustDomain() != "" {
+			SetConfigClusterLocalDomain(meshConfig.GetTrustDomain())
+			return false


I think the following questions are related to @hanxiaop question below about !fn(r).

Why do we return false when GetTrustDomain() is not empty? Is the goal to stop iterating once we get the trust domain?, hence:

if !fn(r) { break }

as seen in the ctx.ForEach(...) function update you added.

Yeah 😄

I think we need a break here and then add a loop break for the ForEach func.

MorrisLaw · 2024-05-15T17:55:37Z

pkg/config/analysis/analyzers/util/hosts.go

+
+var configClusterLocalDomain string
+
+func SetConfigClusterLocalDomain(domain string) {


Apologies for my ignorance here but can you explain the reason behind checking for a non-empty Trust Domain(as returned via GetTrustDomain()) and setting Config Cluster Local Domain to that value?

backgraound:

This issue originates from #33174, where the user customized the k8s cluster domain and used several related Istio configurations.

meshConfig.trustDomain
values.global.trustDomain
values.global.proxy.clusterDomain

However, when using istioctl analyze, it consistently shows the error service-name.namespace.svc.customDnsDomain IST0101 Error (Referenced host not found).

why use trusDomain?
Actually, the up configuration keys, values.global.proxy.clusterDomain aligns better semantically.
and then it only used in the sidecar template and is not get value through any runtime configuration. Implementing this would be a complexity task.

so, considering the user scenario(when user need changed cluster domain), they generally configure these parameters simultaneously. As a result, trustDomain can get from meshConfig, and can be used as a reference value.

MorrisLaw · 2024-05-15T17:59:38Z

pkg/config/analysis/analyzers/util/service_lookup.go

@@ -61,6 +62,17 @@ func InitServiceEntryHostMap(ctx analysis.Context) map[ScopedFqdn]*v1alpha3.Serv
 		return true
 	})

+	// use meshConfig.trustDomain changed the default domain
+	// todo: replace by `values.global.proxy.clusterDomain`


What exactly is the todo? Also, does not doing that work now effect the resolution of the issue in any way?

Based on the description above( why use ** trusDomain** ??), I believe the current solution is only a preliminary workaround. A more optimal solution is needed and requires further discussion.

And I'm not sure how the community plans and designs this aspect, and whether there's a need for unification. Additionally, I think it would be helpful to include some usage documentation, as many people might encounter the same issue.

@zirain @howardjohn @hzxuzhonghu @hanxiaop

From a bug fix perspective, trustDomain can already resolved the user's issue.

However, I want to implement a more standardized and user-friendly solution. Therefore, I'll add a TODO to drive this improvement forward.

MorrisLaw · 2024-05-15T18:06:39Z

pkg/config/analysis/testing/fixtures/context.go

@@ -43,7 +43,9 @@ func (ctx *Context) Exists(config.GroupVersionKind, resource.FullName) bool { re
 // ForEach implements analysis.Context
 func (ctx *Context) ForEach(_ config.GroupVersionKind, fn analysis.IteratorFn) {
 	for _, r := range ctx.Resources {
-		fn(r)
+		if !fn(r) {


+1 to @hanxiaop question

howardjohn · 2024-05-28T17:39:28Z

pkg/config/analysis/analyzers/util/service_lookup.go

 	"istio.io/api/networking/v1alpha3"
 	"istio.io/istio/pkg/config/analysis"
 	"istio.io/istio/pkg/config/resource"
 	"istio.io/istio/pkg/config/schema/gvk"
 )

+func GetCustomClusterDomain(ctx analysis.Context) string {
+	cusClusterDomain := ""
+	// use meshConfig.trustDomain changed the default domain


trustDomain and cluster domain are 100% orthogonal to each other. There is absolutely no relationship at all (beyond the same default) and they should not be used interchange-ably like this

Let me try to find a better solution 🤔

>fix istioctl analyze vs error when a custom cluster domain

6cc85ca

resolved istio#33174

nicole-lihui requested review from a team as code owners May 15, 2024 09:12

istio-policy-bot added area/user experience release-notes-none Indicates a PR that does not require release notes. labels May 15, 2024

istio-testing added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 15, 2024

hzxuzhonghu reviewed May 15, 2024

View reviewed changes

hanxiaop reviewed May 15, 2024

View reviewed changes

howardjohn requested changes May 15, 2024

View reviewed changes

MorrisLaw reviewed May 15, 2024

View reviewed changes

fix get cluster domain

72a0191

istio-testing added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 16, 2024

nicole-lihui requested a review from zirain May 16, 2024 09:43

zirain changed the title ~~>fix istioctl analyze vs error when a custom cluster domain~~ fix istioctl analyze vs error when a custom cluster domain May 16, 2024

nicole-lihui requested review from MorrisLaw, hanxiaop, hzxuzhonghu and howardjohn May 27, 2024 10:18

howardjohn requested changes May 28, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix istioctl analyze vs error when a custom cluster domain #51064

fix istioctl analyze vs error when a custom cluster domain #51064

nicole-lihui commented May 15, 2024 •

edited by istio-policy-bot

hzxuzhonghu May 15, 2024

zirain May 15, 2024

hanxiaop May 15, 2024

hanxiaop May 15, 2024

MorrisLaw May 15, 2024

nicole-lihui May 16, 2024

howardjohn May 15, 2024

nicole-lihui May 16, 2024

MorrisLaw May 15, 2024

nicole-lihui May 16, 2024

MorrisLaw May 15, 2024

nicole-lihui May 16, 2024

nicole-lihui May 16, 2024 •

edited

MorrisLaw May 15, 2024

nicole-lihui May 16, 2024

nicole-lihui May 16, 2024

MorrisLaw May 15, 2024

howardjohn May 28, 2024

nicole-lihui May 29, 2024


		var configClusterLocalDomain string

		func SetConfigClusterLocalDomain(domain string) {

fix istioctl analyze vs error when a custom cluster domain #51064

Are you sure you want to change the base?

fix istioctl analyze vs error when a custom cluster domain #51064

Conversation

nicole-lihui commented May 15, 2024 • edited by istio-policy-bot

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicole-lihui May 16, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicole-lihui commented May 15, 2024 •

edited by istio-policy-bot

nicole-lihui May 16, 2024 •

edited