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

Kiali render hostnames as individual service instead of serviceentry as whole #6962

Closed
im-Amitto opened this issue Dec 15, 2023 · 24 comments · Fixed by #6984
Closed

Kiali render hostnames as individual service instead of serviceentry as whole #6962

im-Amitto opened this issue Dec 15, 2023 · 24 comments · Fixed by #6984
Assignees
Labels
backlog Triaged Issue added to backlog bug Something isn't working enhancement This is the preferred way to describe new end-to-end features.

Comments

@im-Amitto
Copy link

Describe the bug

Loom Video :-> https://www.loom.com/share/2c1906c2ffe042dcbf6d96ca9e4f7ef4
A serviceentry is supposed to be visualised as single icon in graph does not matter how many number of hostnames it has in it.

apiVersion: networking.istio.io/v1alpha3
kind: ServiceEntry
metadata:
  name: api.headout.com
spec:
  hosts:
  - api.example.com
  - api.example2.com
  ports:
  - number: 443
    name: https
    protocol: HTTPS
  resolution: DNS
  location: MESH_EXTERNAL

It should be shown as a single icon but kiali is showing 1 service icon per hostname. It does not happen always. It does show the service entry as a single icon sometime but goes back to showing one icon per hostname on refresh.

Expected Behavior

Single icon for a single service entry

What are the steps to reproduce this bug?

  1. Create a serviceentry with two hostname
  2. Visualise the traffic

Environment

  • Kiali version: v1.76.1 (0f96891)
  • Istio version: 1.18.2
  • Kubernetes version: v1.28.4-eks-8cb36c9
  • Prometheus 2.48.0
@im-Amitto im-Amitto added the bug Something isn't working label Dec 15, 2023
@hhovsepy
Copy link
Contributor

I confirm seeing SE per host in the graph, this is because Istio is generating a Service per SE host in Service Registry.

@im-Amitto
Copy link
Author

@hhovsepy but it does not happen everytime, sometime i get the visualisation that i want where it's shown as a single entity.
Here mb-domain contain more than 400 hostname.
Screenshot 2023-12-18 at 9 40 49 AM

@hhovsepy
Copy link
Contributor

Seems like Kiali would need a new functionality in a Graph. A checkbox to merge all URLs of the ServiceEntry into one or show it separately.

@hhovsepy hhovsepy added enhancement This is the preferred way to describe new end-to-end features. backlog Triaged Issue added to backlog labels Dec 19, 2023
@jshaughn
Copy link
Collaborator

Seems like Kiali would need a new functionality in a Graph. A checkbox to merge all URLs of the ServiceEntry into one or show it separately.

@hhovsepy After thinking about this I think this should not be an option, I think the requested behavior is the intended behavior, and only a single ServiceEntry node should exist for all of the hosts. And so, this does seem like a bug, and actually a regression, as I believe the code intends to consolidate into a single node.

Note that this is still namespace specific, so if a ServiceEntry is exported to multiple namespaces, a separate ServiceEntry node could show up in each of the namespaces, because Kiali treats each export as a separate SE.

@im-Amitto
Copy link
Author

im-Amitto commented Dec 19, 2023

Seems like Kiali would need a new functionality in a Graph. A checkbox to merge all URLs of the ServiceEntry into one or show it separately.

@hhovsepy After thinking about this I think this should not be an option, I think the requested behavior is the intended behavior, and only a single ServiceEntry node should exist for all of the hosts. And so, this does seem like a bug, and actually a regression, as I believe the code intends to consolidate into a single node.

Yes, that's what i wanted to point out

Note that this is still namespace specific, so if a ServiceEntry is exported to multiple namespaces, a separate ServiceEntry node could show up in each of the namespaces, because Kiali treats each export as a separate SE.

Yes, currently it shows graph in two ways. They are seen randomly on page refresh.

  1. SE in namespace level, one icon for each SE (The one the feels nice)
  2. Each host as a seprate service, in this the service are not shown at namespace level.

I would actually love if we can have a 3rd way where we see 1 symbol per SE but globally instead of showing at namespace level.

Anyway, even the expected behaviour[1], works completely fine.

If someone can help me find the piece od code that renders the graph i might be able to figure out the fix.

@jshaughn
Copy link
Collaborator

I would actually love if we can have a 3rd way where we see 1 symbol per SE but globally instead of showing at namespace level.

This has been requested before, but sort of breaks the model, so unlikely to do this option.

@jshaughn
Copy link
Collaborator

@im-Amitto Just to confirm, when you see the bad behavior, the many nodes are actually service nodes (triangles), not service entry nodes (5 sides), right?

@im-Amitto
Copy link
Author

@im-Amitto Just to confirm, when you see the bad behavior, the many nodes are actually service nodes (triangles), not service entry nodes (5 sides), right?

yes, exactly

@lonnixrdc
Copy link

I have had the same behavior even when my SE only has 1 host in it. Sometimes I see the SE icon, other times I just see a triangle and the host listed out. I think there are 2 issues in play, both related to how Kiali displays ServiceEntries

  1. Multiple hosts showing up (or not showing up) for the same SE
  2. The SE sometimes not showing up and other times showing up

@jshaughn jshaughn self-assigned this Dec 19, 2023
@jshaughn
Copy link
Collaborator

jshaughn commented Dec 19, 2023

I'm seeing the behavior, very strange...

Update: I see the problem, it stems from recent multi-cluster work and it is due to some random ordering of nodes, so that is why it is intermittent. Still figuring out a solution, but should have a fix fairly soon.

jshaughn added a commit to jshaughn/kiali that referenced this issue Dec 20, 2023
- I'm not sure if telemetry changed recently but when making requests
  of a service entry the telemetry may not include any cluster or namespace
  information, just the destination_service_name (i.e. the requested host)
- The subsequent "unknown" values for cluster and namespace can not be used
  to help fetch the ServiceEntry config for matching. Depending on
  random node ordering, sometimes we would "get lucky" and fetch the needed
  information, but the whole approach was inherently flawed.
- ServiceEntry definitions are really used by the source node, to properly
  route out of the mesh, and so the SE definition should really be fetched
  using the source node's cluster+namespace information (or so I currently
  think).  This PR makes that change.
- The PR also adds a useful optimization that can speed up this appender;
  or namespaces that only have injected service nodes we will no longer
  bother with loading SE definitions or doing the matching, because injected
  service nodes are not going to be converted to a ServiceEntry.

kiali#6962
jshaughn added a commit to jshaughn/kiali that referenced this issue Dec 20, 2023
- I'm not sure if telemetry changed recently but when making requests
  of a service entry the telemetry may not include any cluster or namespace
  information, just the destination_service_name (i.e. the requested host)
- The subsequent "unknown" values for cluster and namespace can not be used
  to help fetch the ServiceEntry config for matching. Depending on
  random node ordering, sometimes we would "get lucky" and fetch the needed
  information, but the whole approach was inherently flawed.
- ServiceEntry definitions are really used by the source node, to properly
  route out of the mesh, and so the SE definition should really be fetched
  using the source node's cluster+namespace information (or so I currently
  think).  This PR makes that change.
- The PR also adds a useful optimization that can speed up this appender;
  or namespaces that only have injected service nodes we will no longer
  bother with loading SE definitions or doing the matching, because injected
  service nodes are not going to be converted to a ServiceEntry.

kiali#6962
jshaughn added a commit to jshaughn/kiali that referenced this issue Dec 21, 2023
- I'm not sure if telemetry changed recently but when making requests
  of a service entry the telemetry may not include any cluster or namespace
  information, just the destination_service_name (i.e. the requested host)
- The subsequent "unknown" values for cluster and namespace can not be used
  to help fetch the ServiceEntry config for matching. Depending on
  random node ordering, sometimes we would "get lucky" and fetch the needed
  information, but the whole approach was inherently flawed.
- mesh_external ServiceEntry definitions are really used by the source node,
  to properly route out of the mesh, and so the SE definition needs to be
  fetched using the source node's cluster+namespace information. This PR makes
  that change. It also better handles the mesh_internal scenario, which
  *does* use the cluster+namespace provided.
- The PR also adds a useful optimization that can speed up this appender;
  for namespaces that only have injected service nodes we will no longer
  bother with loading SE definitions or doing the matching, because injected
  service nodes are not going to be converted to a ServiceEntry.
- The tests are updated to not use an "unknown" node as the SE requestor,
  that is not a valid scenario.  Also, fixed an issue in the mocking of one
  test.

kiali#6962
jshaughn added a commit that referenced this issue Dec 22, 2023
- I'm not sure if telemetry changed recently but when making requests
  of a service entry the telemetry may not include any cluster or namespace
  information, just the destination_service_name (i.e. the requested host)
- The subsequent "unknown" values for cluster and namespace can not be used
  to help fetch the ServiceEntry config for matching. Depending on
  random node ordering, sometimes we would "get lucky" and fetch the needed
  information, but the whole approach was inherently flawed.
- mesh_external ServiceEntry definitions are really used by the source node,
  to properly route out of the mesh, and so the SE definition needs to be
  fetched using the source node's cluster+namespace information. This PR makes
  that change. It also better handles the mesh_internal scenario, which
  *does* use the cluster+namespace provided.
- The PR also adds a useful optimization that can speed up this appender;
  for namespaces that only have injected service nodes we will no longer
  bother with loading SE definitions or doing the matching, because injected
  service nodes are not going to be converted to a ServiceEntry.
- The tests are updated to not use an "unknown" node as the SE requestor,
  that is not a valid scenario.  Also, fixed an issue in the mocking of one
  test.

#6962
@jshaughn
Copy link
Collaborator

A fix has been merged and it will be included in Kiali 1.79, due for release Jan 22. If you need a backport to an existing version, please let me know.

@im-Amitto
Copy link
Author

A fix has been merged and it will be included in Kiali 1.79, due for release Jan 22. If you need a backport to an existing version, please let me know.

No urgency, but would love to understand how to backport stuff to an existing version.

@jshaughn
Copy link
Collaborator

jshaughn commented Jan 8, 2024

No urgency, but would love to understand how to backport stuff to an existing version.

@im-Amitto Backporting typically consists of using a git cherry-pick of a merged PR from the current version to an older version. If it doesn't map perfectly then manual edits are needed to make sure the change is compatible. Once backported we have defined some git actions to release a new patch version upstream.

@im-Amitto
Copy link
Author

@jshaughn Issue still seems to be persisting

Kiali : v1.79.0 (d506895)
Kiali Container : v1.79.0

Service Mesh
Istio : 1.18.2
Prometheus : 2.48.0
Kubernetes : v1.28.4-eks-8cb36c9

Do i need to re-create the service entries?

@jshaughn
Copy link
Collaborator

@im-Amitto , no, I don't think recreating the service entries would matter. @lonnixrdc , have you been able to try out v1.79?

@jshaughn
Copy link
Collaborator

@im-Amitto Can you try it with and without the "Show Service Nodes" Graph Display option enabled? If you are still having a problem we may need to start the debugging process again, as I'm wondering if it's the same issue. Prior to the fix it was easily reproducible, but after the fix were were unable to reproduce on our side.

@jshaughn
Copy link
Collaborator

jshaughn commented Feb 7, 2024

@im-Amitto @lonnixrdc , anything else to report here, I'm not sure what could still be the issue.

@im-Amitto
Copy link
Author

im-Amitto commented Feb 12, 2024

@im-Amitto @lonnixrdc , anything else to report here, I'm not sure what could still be the issue.

@jshaughn I debugged it a little more to be sure that i am not missing anything on my end but everything seems to be correct.

Kiali Info
image

Config for service entry with reference to multiple domains
Screenshot 2024-02-12 at 1 19 55 PM

Graph is still showing individual external service in the graph
Screenshot 2024-02-12 at 1 18 23 PM

@jshaughn
Copy link
Collaborator

@im-Amitto Thanks for the feedback, I will look into this again. Just to be clear, the previous behavior would "flip-flop" between the single node and the multiple nodes. Is that still true, or now do you always see expanded number of nodes?

@im-Amitto
Copy link
Author

@jshaughn Correct, since the upgrade i have not seen the single node even once.
Graph always comes in expanded version.

@jshaughn
Copy link
Collaborator

@im-Amitto Just to confirm, the ServiceEntry in your screenshot is still defined as in the original description? I see above that themb-domains ServiceEntry is defined in the istio-system namespace. By default it would be exported to all namespaces, but if you have defined an export_to, something like:

  exportTo:
  - "."

Then there could be an issue. I just need to make sure that the headout-mystique namespace can access the mb-domains ServiceEntry definition.

One other test you could maybe try, just for sanity, would be to define themb-domains serviceEntry directly in the headout-mystique namespace, and therefore not rely on exportTo. If that worked it would indicate that Kiali has a bug in its handling of exportTo. Actually, this would be a great test, because I do see an assumption in the code that makes me wonder if we are handling exportTo correctly.

@im-Amitto
Copy link
Author

im-Amitto commented Feb 27, 2024

@jshaughn Looks like we are on correct track

export_to

Aware about this, we are not using it in any service entry.

would be to define the mb-domains serviceEntry directly in the headout-mystique namespace

What worked

  • Creating SE in headout-mystique

What did not

  • Creating SE with exportTo set to * or even with -headout-mystique

I can't really use it as the SE are pretty common among different micro-service around the namespaces.

@jshaughn
Copy link
Collaborator

@im-Amitto OK, thanks for the quick feedback. It seems Kiali has an issue dealing with exportTo, it's not picking up the fact that, by default, your definition should be available to all namespaces. I'll look into it more today...

@jshaughn
Copy link
Collaborator

@im-Amitto I have opened #7153 for this new problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlog Triaged Issue added to backlog bug Something isn't working enhancement This is the preferred way to describe new end-to-end features.
Projects
Development

Successfully merging a pull request may close this issue.

4 participants