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

Add separate cluster for configured kiali instance #6927

Merged
merged 7 commits into from Jan 4, 2024

Conversation

nrfox
Copy link
Contributor

@nrfox nrfox commented Dec 6, 2023

** Describe the change **

This change lets you manually specify a cluster in the kiali config that there is no remote secret for. This cluster is considered "inaccessible" meaning that Kiali won't try to connect to it and the frontend won't consider it for the "multi-cluster mode" e.g. there won't be a cluster column when there's one accessible cluster and one inaccessible cluster.

To test:

  1. Create two clusters. You can use: hack/run-integration-tests.sh --test-suite frontend-multi-primary --setup-only true.
  2. Deploy kiali on both clusters including the "west" cluster. ./hack/istio/multicluster/deploy-kiali.sh --single-kiali false --cluster1-context kind-east --cluster2-context kind-west --kiali-auth-strategy anonymous --manage-kind true --kiali-use-dev-image true
  3. Add the "east" cluster to the "west" kiali config by editing the "west" kiali configmap (kubectl --context kind-west edit cm kiali -n istio-system):
    clustering:
      clusters:
        - name: east
          accessible: false
      kiali_urls:
        - cluster_name: east
          url: http://<kiali_url>/kiali
          instance_name: kiali
          namespace: istio-system
    
  4. Restart the west kiali: kubectl --context kind-west delete pod -n istio-system -l app=kiali
  5. Navigate to the west kiali's "mesh" page (you can port-forward to it via hack/kiali-port-forward.sh -kc kind-west).
    You should see two kiali instances on the mesh page and a link to the "east" kiali:
    Screenshot from 2023-12-20 13-31-19
    You should not see a cluster column on any of the app/service/istio/workload pages for the "west" kiali.

** Issue reference **

Fixes #6243

@nrfox nrfox force-pushed the kiali-6243 branch 4 times, most recently from 6744ef7 to 7ad5c4a Compare December 20, 2023 18:39
@nrfox nrfox marked this pull request as ready for review December 20, 2023 18:40
@jmazzitelli
Copy link
Collaborator

jmazzitelli commented Dec 20, 2023

I believe you need to use --kiali-use-dev-image when running the test procedure because we want the kiali server built from this PR, not the latest image from quay. So step 2 I think should be:

Deploy kiali on both clusters including the "west" cluster. ./hack/istio/multicluster/deploy-kiali.sh --single-kiali false --cluster1-context kind-east --cluster2-context kind-west --kiali-auth-strategy anonymous --manage-kind true --kiali-use-dev-image true

@jmazzitelli
Copy link
Collaborator

jmazzitelli commented Dec 20, 2023

step 3 - you need to know to pass --context kind-west in order to edit the correct configmap:

kubectl --context kind-west edit cm kiali -n istio-system

Updated the PR description with some additional things to the test procedures that make it easier on testers.

@jmazzitelli
Copy link
Collaborator

jmazzitelli commented Dec 20, 2023

I don't see it - I'll check my setup to make sure it is as expected:
image

kubectl --context kind-west -n istio-system get cm kiali -oyaml

shows:

    kiali_feature_flags:
...
      clustering:
        autodetect_secrets:
          enabled: true
          label: kiali.io/multiCluster=true
        clusters:
        - name: east
          accessible: false
        kiali_urls:
        - cluster_name: east
          url: http://localhost:20001/kiali
          name: kiali
          namespace: istio-system

Using locally built image:

 kubectl --context kind-west -n istio-system get pods -l app=kiali -ojsonpath='{.items...spec.containers[*].image}{"\n"}'
localhost/kiali/kiali:dev
$ kubectl --context kind-west -n istio-system logs -l app=kiali --tail=-1 | head -n3
2023-12-20T22:33:33Z INF Kiali: Version: v1.79.0-SNAPSHOT, Commit: 7ad5c4a242442b5603a30db0c730b81f2b72e3e9, Go: 1.20.10

$ git log
commit 7ad5c4a242442b5603a30db0c730b81f2b72e3e9 (HEAD -> nrfox-kiali-6243)
Author: Nick Fox <nfox@redhat.com>
Date:   Tue Dec 5 22:50:40 2023 -0500

    Add separate cluster for configured kiali instance

@jmazzitelli
Copy link
Collaborator

That is going to need a release notes blurb - "deprecated: feature flags.clustering has moved up one level, the feature flags.clustering will be removed in a future release."

@jmazzitelli
Copy link
Collaborator

I see this now, but notice I don't see the instance name in the Kiali column. I just see istio-system/.

image

The relevant kiali CR section:

    clustering:
      autodetect_secrets:
        enabled: true
        label: kiali.io/multiCluster=true
      clusters:
      - name: east
        accessible: false
      kiali_urls:
      - cluster_name: east
        url: http://localhost:20001/kiali
        name: kiali
        namespace: istio-system

@nrfox
Copy link
Contributor Author

nrfox commented Dec 21, 2023

I see this now, but notice I don't see the instance name in the Kiali column. I just see istio-system/.

image

The relevant kiali CR section:

    clustering:
      autodetect_secrets:
        enabled: true
        label: kiali.io/multiCluster=true
      clusters:
      - name: east
        accessible: false
      kiali_urls:
      - cluster_name: east
        url: http://localhost:20001/kiali
        name: kiali
        namespace: istio-system

@jmazzitelli I had the config wrong initially in the setup instructions. It should be instance_name not name.

@jmazzitelli
Copy link
Collaborator

ok. now I see it.
image

jmazzitelli
jmazzitelli previously approved these changes Dec 21, 2023
hhovsepy
hhovsepy previously approved these changes Dec 21, 2023
Copy link
Contributor

@hhovsepy hhovsepy left a comment

Choose a reason for hiding this comment

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

With the correct location of clustering:, running locally:
Screenshot from 2023-12-21 22-10-34

@nrfox
Copy link
Contributor Author

nrfox commented Jan 3, 2024

Added an e2e integration test for this and removed the Accessible field for config.Cluster. There's now just a config.Clustering.InaccessibleClusters field. This is more clear since clusters added manually to the config this way are only for inaccessible clusters and before setting config.Cluster.Accessible = true did nothing.

@nrfox
Copy link
Contributor Author

nrfox commented Jan 3, 2024

Also added a kiali.Instance abstraction for the e2e tests that should hopefully make it easier to update the config and restart kiali from within an e2e test.

@hhovsepy
Copy link
Contributor

hhovsepy commented Jan 3, 2024

I confirm seeing the inaccessible 'east' cluster in mash page with configured link.
Question, if a remote cluster is accessible and not mentioned in inaccessible_clusters, it still can be configured by setting a URL, right?

@nrfox
Copy link
Contributor Author

nrfox commented Jan 3, 2024

I confirm seeing the inaccessible 'east' cluster in mash page with configured link. Question, if a remote cluster is accessible and not mentioned in inaccessible_clusters, it still can be configured by setting a URL, right?

What URL? The clustering.kiali_urls[*].url? No the way it works is kiali_url.cluster_name is just a reference to a cluster. That cluster is defined in clustering.inaccessible_clusters or it's an "accessible" cluster that is defined by a cluster secret and kiali will have access to it and those kialis should be automatically discovered so you wouldn't need to add the kiali manually to the config anyways. kiali_url.cluster_name should just reference a cluster in clustering.inaccessible_clusters.

I'm not sure I love the name inaccessible_clusters but I want the field to convey that these are clusters that kiali doesn't have access to so you need to specify them manually in the config. Hopefully that's clear?

@hhovsepy
Copy link
Contributor

hhovsepy commented Jan 3, 2024

@nrfox to be more clear, is it possible to override "accessible" cluster's auto discovered Kiali's url by setting clustering.kiali_urls[*].url ?

@nrfox
Copy link
Contributor Author

nrfox commented Jan 3, 2024

@nrfox to be more clear, is it possible to override "accessible" cluster's auto discovered Kiali's url by setting clustering.kiali_urls[*].url ?

Ah ok I haven't tested that out yet but I didn't change that part of the code so it should still work:

kiali/business/mesh.go

Lines 383 to 395 in b1dd7b2

var instances []kubernetes.KialiInstance
for _, d := range services {
kiali := convertKialiServiceToInstance(&d, cluster)
// If URL is already populated (because of an annotation), trust that because it's user configuration.
// But if Kiali URL configured per cluster name, instance name and namespace, then use that URL.
for _, cfgurl := range in.conf.Clustering.KialiURLs {
if cfgurl.ClusterName == clusterName && cfgurl.InstanceName == kiali.ServiceName && cfgurl.Namespace == kiali.Namespace {
kiali.Url = cfgurl.URL
}
}
instances = append(instances, kiali)
}
return instances

@nrfox
Copy link
Contributor Author

nrfox commented Jan 3, 2024

Test failure seems to be persistent so I'll have to investigate what's wrong there with TestAuthPolicyPrincipalsError.

Fixed

Using config.Unmarshal will apply defaults to the config which is necessary since the config struct doesn't marshal/unmarshal properly without the defaults applied.
Copy link
Contributor

@hhovsepy hhovsepy left a comment

Choose a reason for hiding this comment

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

Regression tested also accessible cluster's discovered Kiali url override via kiali_urls: config:
Screenshot from 2024-01-04 17-55-03

@nrfox nrfox merged commit f443264 into kiali:master Jan 4, 2024
8 checks passed
@nrfox nrfox deleted the kiali-6243 branch January 4, 2024 20:00
@nrfox nrfox added requires operator PR It requires update in operator code docs It requires changes on kiali.io labels Jan 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs It requires changes on kiali.io requires operator PR It requires update in operator code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Multicluster] provide links to external Kialis without requiring istio secrets
3 participants