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.

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

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

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 🤔

nicole-lihui · 2024-06-28T05:53:29Z

Not stale

nicole-lihui · 2024-07-15T07:44:34Z

waiting add clusterDomain to meshconfig : istio/api#3265

istio-testing · 2024-07-15T07:51:38Z

@nicole-lihui: 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
unit-tests-arm64_istio	`0f81715`	link	true	`/test unit-tests-arm64`
gencheck_istio	`0f81715`	link	true	`/test gencheck`
integ-cni_istio	`0f81715`	link	true	`/test integ-cni`
integ-distroless_istio	`0f81715`	link	true	`/test integ-distroless`
integ-ambient_istio	`0f81715`	link	true	`/test integ-ambient`
integ-operator-controller_istio	`0f81715`	link	true	`/test integ-operator-controller`
integ-pilot-istiodremote_istio	`0f81715`	link	true	`/test integ-pilot-istiodremote`
integ-helm_istio	`0f81715`	link	true	`/test integ-helm`
integ-pilot-istiodremote-mc_istio	`0f81715`	link	true	`/test integ-pilot-istiodremote-mc`
integ-security_istio	`0f81715`	link	true	`/test integ-security`
integ-ipv6_istio	`0f81715`	link	true	`/test integ-ipv6`
integ-pilot_istio	`0f81715`	link	true	`/test integ-pilot`
integ-ds_istio	`0f81715`	link	true	`/test integ-ds`
integ-pilot-multicluster_istio	`0f81715`	link	true	`/test integ-pilot-multicluster`
integ-basic-arm64_istio	`0f81715`	link	true	`/test integ-basic-arm64`
unit-tests_istio	`0f81715`	link	true	`/test unit-tests`
macbuildcheck_istio	`0f81715`	link	true	`/test macbuildcheck`
release-test_istio	`0f81715`	link	true	`/test release-test`
lint_istio	`0f81715`	link	true	`/test lint`
integ-telemetry-discovery_istio	`0f81715`	link	true	`/test integ-telemetry-discovery`
integ-security-istiodremote_istio	`0f81715`	link	true	`/test integ-security-istiodremote`
integ-telemetry-istiodremote_istio	`0f81715`	link	true	`/test integ-telemetry-istiodremote`
integ-telemetry-mc_istio	`0f81715`	link	true	`/test integ-telemetry-mc`
integ-telemetry_istio	`0f81715`	link	true	`/test integ-telemetry`
integ-security-multicluster_istio	`0f81715`	link	true	`/test integ-security-multicluster`

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.

>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

istio-policy-bot added the lifecycle/stale Indicates a PR or issue hasn't been manipulated by an Istio team member for a while label Jun 28, 2024

istio-policy-bot removed the lifecycle/stale Indicates a PR or issue hasn't been manipulated by an Istio team member for a while label Jun 28, 2024

Merge remote-tracking branch 'upstream/master' into fix-33174

4700912

nicole-lihui mentioned this pull request Jul 15, 2024

add cluster domain to meshConfig istio/api#3265

Open

use meshConfig clusterDomain

0f81715

nicole-lihui requested a review from a team as a code owner July 15, 2024 07:46

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

Loading

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

nicole-lihui May 16, 2024

MorrisLaw May 15, 2024

howardjohn May 28, 2024

nicole-lihui May 29, 2024

nicole-lihui commented Jun 28, 2024

nicole-lihui commented Jul 15, 2024

istio-testing commented Jul 15, 2024

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 Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicole-lihui commented Jun 28, 2024

nicole-lihui commented Jul 15, 2024

istio-testing commented Jul 15, 2024

nicole-lihui commented May 15, 2024 •

edited by istio-policy-bot

Loading