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

hubble: Hubble node_name field should contain cluster name #15933

Merged
merged 1 commit into from
May 17, 2021

Conversation

Maddy007-maha
Copy link
Contributor

Description:
This field node_name currently only contains the node name, without its cluster-mesh prefix.
If the cluster name is not the default cluster name then it should show the node_name along with the prefixed cluster name

Its should be shown as below:
cluster/<node_name>
Eg: cluster1/node-ip.compute.internal

$ hubble observe --last 2 -o compact --print-node-name
Nov 3 11:40:45.407 [cluster5/ip-172-0-51-175.us-west-2.compute.internal]: [...]
Nov 3 11:40:46.554 [cluster7/ip-172-0-121-242.us-west-2.compute.internal ]: [...]

Fixes: #13872

Signed-off-by: Maddy007-maha mahadev.panchal@accuknox.com

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Apr 29, 2021
@Maddy007-maha
Copy link
Contributor Author

Hi, I have edited the configMap of cilium with following cluster information as below:
changed from:

cluster-id: ""
cluster-name: default

to:

cluster-id: "1"
cluster-name: cluster1

Then restarted the cilium pods and hubble pods, its been observed that local node is unavailable. Am I missing something and do we have any doc to follow to setup single cluster.
$ hubble status

Handling connection for 4245
Healthcheck (via localhost:4245): Ok
Current/Max Flows: 0/0
Flows/s: N/A
Connected Nodes: 0/1
Unavailable Nodes: 1
  - cluster1/minikube

$ hubble observe -o compact --last 2 --print-node-name

Handling connection for 4245
Apr 28 15:16:12.878 [hubble-relay-765f884d5d-vfb4q]: 1 nodes are unavailable: cluster1/minikube

I can see the cilium node list and its showing properly

$ kubectl -n kube-system exec -ti cilium-2pq7h -- cilium node list
Defaulted container "cilium-agent" out of: cilium-agent, clean-cilium-state (init)
Name                IPv4 Address   Endpoint CIDR   IPv6 Address   Endpoint CIDR
cluster1/minikube   192.168.49.2   10.0.0.0/24
I see some failures in the hubble-relay saying that peer is not reachable from the hubble-relay logs:
level=warning msg="Failed to create gRPC client" address="192.168.49.2:4244" error="context deadline exceeded" hubble-tls=true next-try-in=1h25m20s peer=cluster1/minikube subsys=hubble-relay
level=info msg="No connection to peer cluster1/minikube, skipping" address="192.168.49.2:4244" subsys=hubble-relay
level=info msg=Connecting address="192.168.49.2:4244" hubble-tls=true peer=cluster1/minikube subsys=hubble-relay
level=warning msg="Failed to create gRPC client" address="192.168.49.2:4244" error="context deadline exceeded" hubble-tls=true next-try-in=1h30m0s peer=cluster1/minikube subsys=hubble-relay 

Kindly throw some light :)

@glibsm glibsm added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Apr 29, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Apr 29, 2021
@Maddy007-maha Maddy007-maha force-pushed the fix_13872 branch 2 times, most recently from 48e5d07 to e8c15bb Compare May 3, 2021 16:15
@Maddy007-maha Maddy007-maha marked this pull request as ready for review May 3, 2021 16:17
@Maddy007-maha Maddy007-maha requested a review from a team May 3, 2021 16:17
@Maddy007-maha Maddy007-maha requested a review from a team as a code owner May 3, 2021 16:17
@Maddy007-maha Maddy007-maha requested a review from glibsm May 3, 2021 16:17
@Maddy007-maha
Copy link
Contributor Author

Any update on reviewing this PR?

Copy link
Member

@gandro gandro left a comment

Choose a reason for hiding this comment

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

Apologies for the delayed review. Thanks a lot for the PR, this is overall the right approach.

There are some failing unit tests that need to be fixed in github.com/cilium/cilium/pkg/hubble/monitor:

https://travis-ci.com/github/cilium/cilium/jobs/502667881#L14680

pkg/node/types/nodename.go Show resolved Hide resolved
pkg/node/types/nodename.go Outdated Show resolved Hide resolved
Description:
This field node_name currently only contains the node name,
without it's cluster-mesh prefix.  If the cluster name is not
the default cluster name then it should show the node_name along
with prefixed cluster name.

Its should be shown as below:
cluster<id>/<node_name>
Eg: cluster1/node-ip.compute.internal

$ hubble observe --last 2 -o compact --print-node-name
Nov  3 11:40:45.407 [cluster5/ip-172-0-51-175.us-west-2.compute.internal]: [...]
Nov  3 11:40:46.554 [cluster7/ip-172-0-121-242.us-west-2.compute.internal ]: [...]

Tested Output:
vagrant@k8s1:~/go/src/github.com/cilium/cilium$ hubble observe --print-node-name
TIMESTAMP             NODE        SOURCE              DESTINATION        TYPE        VERDICT     SUMMARY
May  3 15:47:10.176   maha/k8s1   10.11.1.226:51792   10.11.0.190:4240   to-stack    FORWARDED   TCP Flags: SYN
May  3 15:47:10.177   maha/k8s1   10.11.1.226:51792   10.11.0.190:4240   from-host   FORWARDED   TCP Flags: SYN
May  3 15:47:10.177   maha/k8s1   10.11.1.226:51792   10.11.0.190:4240   to-stack    FORWARDED   TCP Flags: SYN

$ hubble observe --last 2 -o compact --print-node-name
May  3 15:50:18.290 [maha/k8s1]: [fd00::1:8665]:52188 -> [fd00::ebfa]:4240 to-endpoint FORWARDED (TCP Flags: ACK)
May  3 15:50:22.638 [maha/k8s1]: fe80::436:9bff:fedc:1763 -> ff02::2 from-endpoint FORWARDED (ICMPv6 RouterSolicitation)

Fixes: cilium#13872

Signed-off-by: Maddy007-maha <mahadev.panchal@accuknox.com>
Copy link
Member

@gandro gandro left a comment

Choose a reason for hiding this comment

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

Awesome, thanks again!

@gandro
Copy link
Member

gandro commented May 12, 2021

test-me-please

@gandro
Copy link
Member

gandro commented May 12, 2021

@Maddy007-maha

Kindly throw some light :)

As for the issue regarding clustermesh, I don't think minikube is the right way to test it, as it doesn't really have node connectivity. I recommend using kind instead and following the guide: https://docs.cilium.io/en/latest/gettingstarted/clustermesh/clustermesh/#gs-clustermesh

@gandro
Copy link
Member

gandro commented May 12, 2021

test-1.21-4.9

Copy link
Member

@gandro gandro left a comment

Choose a reason for hiding this comment

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

@Maddy007-maha No need to re-request a review, this just needs to pass CI at this point (which it has) and requires a review from a janitor as well. Then it will be merged.

Edit: I'm also re-running the conformance tests. This slightly changes the behavior of Hubble's output in clustermesh, thus we want to make sure it doesn't break the new cilium-cli based CI

@gandro gandro removed the request for review from glibsm May 13, 2021 11:00
@aanm aanm merged commit 3203df9 into cilium:master May 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hubble node_name field should contain cluster name
4 participants