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

[v1] flow-collector: add source ip to site-clients process name #1857

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Karen-Schoener
Copy link
Collaborator

This results in flow metrics being generated for each external client-vm ip.

@Karen-Schoener Karen-Schoener marked this pull request as draft December 19, 2024 16:47
@Karen-Schoener
Copy link
Collaborator Author

Karen-Schoener commented Dec 19, 2024

Tested with 2 external vm-clients & 2 external vm-servers.

This change resulted in the following (total flow) metrics being reported to prometheus.

flows_total{address="vm-b-vm-backend:8080",destProcess="site-clients-0b784eab-192.168.56.118",destSite="west@_@0b784eab-13ef-45f0-b6b6-4440dbc7b238",direction="outgoing",protocol="tcp",sourceProcess="site-servers-b97f04ba",sourceSite="east@_@b97f04ba-b739-4ad8-a382-8083c039f60c"} 8
flows_total{address="vm-b-vm-backend:8080",destProcess="site-servers-b97f04ba",destSite="east@_@b97f04ba-b739-4ad8-a382-8083c039f60c",direction="incoming",protocol="tcp",sourceProcess="site-clients-0b784eab-192.168.56.118",sourceSite="west@_@0b784eab-13ef-45f0-b6b6-4440dbc7b238"} 8
flows_total{address="vm-d-vm-backend:8080",destProcess="site-clients-0b784eab-192.168.56.126",destSite="west@_@0b784eab-13ef-45f0-b6b6-4440dbc7b238",direction="outgoing",protocol="tcp",sourceProcess="site-servers-b97f04ba",sourceSite="east@_@b97f04ba-b739-4ad8-a382-8083c039f60c"} 16
flows_total{address="vm-d-vm-backend:8080",destProcess="site-servers-b97f04ba",destSite="east@_@b97f04ba-b739-4ad8-a382-8083c039f60c",direction="incoming",protocol="tcp",sourceProcess="site-clients-0b784eab-192.168.56.126",sourceSite="west@_@0b784eab-13ef-45f0-b6b6-4440dbc7b238"} 16

With this change, the Processes tab in the skupper-console shows: 2 site-clients (1 for each client IP) and 1 site-servers.

The Topology / Components page in the skupper-console shows 1 bubble for 'site-clients' and 1 bubble for 'site-servers'.
To be discussed: whether this page should show 2 site-clients (1 for each client ip).

Also, when I look at the site-servers process, I notice that only 1 address is listed.
I would have expected to see 2 addresses (since I am testing with 2 addresses).

  {
   "recType": "PROCESS",
   "identity": "3f16209a-8782-4b4a-920b-a306f2ce6618",
   "parent": "b97f04ba-b739-4ad8-a382-8083c039f60c",
   "startTime": 1734623481000000,
   "endTime": 0,
   "name": "site-servers-b97f04ba",
   "parentName": "east",
   "groupName": "site-servers",
   "groupIdentity": "b28588af-96ae-47fa-8b42-4efe2313c952",
   "processRole": "remote",
   "processBinding": "bound",
   "addresses": [
    "vm-d-vm-backend:8080@87fb79d3-a814-43e7-b91b-3c7b144bb4e8@tcp"
   ]
  },

@Karen-Schoener
Copy link
Collaborator Author

@ajssmith @c-kruse @scwhaley When you have time, it would be great to hear early feedback on this proposed change.

Thanks very much, Karen

This results in flow metrics being generated for each
external client-vm ip.

Fixes skupperproject#1859
@Karen-Schoener Karen-Schoener force-pushed the flow-collector-site-clients-ip branch from 05e6659 to e4c1b41 Compare December 19, 2024 20:00
@scwhaley
Copy link

scwhaley commented Dec 20, 2024

@Karen-Schoener For the expected 2 addresses for the site-servers process record, it appears that the LIST api method only appends 1 address per process record (the address from the associated connector record, a 1:1 relationship, even though 2 connectors send traffic to the same process).

So I think this is just the existing behavior, not a result of your change (unless i am reading this code wrong).

flow_mem_driver.go ~ln 1810

image

@Karen-Schoener Karen-Schoener marked this pull request as ready for review January 2, 2025 19:15
@Karen-Schoener
Copy link
Collaborator Author

Marking PR as ready for review.

@ajssmith @c-kruse @scwhaley I believe that the skupper console works as expected with this code change.

@c-kruse
Copy link
Contributor

c-kruse commented Jan 2, 2025

@Karen-Schoener Would you mind sharing how you set up your services to test this? The external servers and clients are off the beaten path enough that I can't recall how to get it all wired together.

@Karen-Schoener
Copy link
Collaborator Author

@Karen-Schoener Would you mind sharing how you set up your services to test this? The external servers and clients are off the beaten path enough that I can't recall how to get it all wired together.

@c-kruse I documented the steps in this ticket: #1859 (comment)

If there are any questions on the steps, please let me know... Thanks, Karen

@c-kruse
Copy link
Contributor

c-kruse commented Jan 2, 2025

Thank you @Karen-Schoener!

I think this mostly is making sense and looks like it works - on my first try I can't seem to get more than one site-client going, but I suspect I just need to further fiddle with my setup (kind and/or minikube + the docker "driver" seem to observe external traffic as originating from some gateway address 10.244.0.1 for me.) I probably need to use something VM or bare metal based with a bridge network instead?

@Karen-Schoener
Copy link
Collaborator Author

Karen-Schoener commented Jan 3, 2025

@c-kruse So far, I've tested with 2 VM clients and 2 VM servers.

I'm testing on a Windows PC that is running Virtualbox VMs. One of the VMs is running minikube / skupper.
Also, I'm using Virtualbox VMs as VM clients and VM servers.

In my test environment, the external traffic is seen as originating from the VM-client IP addresses.

@scwhaley
Copy link

scwhaley commented Jan 3, 2025

@c-kruse I have also noticed that the client IP is SNAT'd for certain kube distributions (docker seems to be the usual suspect, k3d in particular stands out in my memory). I use microk8s as a kube distro that doesn't NAT the source ip.

Copy link
Contributor

@c-kruse c-kruse left a comment

Choose a reason for hiding this comment

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

Thank you @Karen-Schoener and @scwhaley. Looks solid to me! Adding @bartoval, because he has a sharper eye for the console side of things than I do.

FWIW, I never did manage to produce a cluster that was not doing SNAT on incoming packets - even with microk8s. Instead, I was just hitting the skupper service from pods in another namespace which has the same effect as close as I can reason.

@c-kruse c-kruse requested a review from bartoval January 4, 2025 00:11
@scwhaley
Copy link

scwhaley commented Jan 10, 2025

Ran tests on my end, this seems to be working as expected. Approved!

Thanks @Karen-Schoener for the great work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants