health: Update Cilium agent to listen on nodeip

[tamilmani1989]

Please ensure your pull request adheres to the following guidelines:

• For first time contributors, read Submitting a pull request
• All code is covered by unit and/or runtime tests where feasible.
• All commits contain a well written commit description including a title,
  description and a Fixes: #XXX line if the commit addresses a particular
  GitHub issue.
• If your commit description contains a Fixes: <commit-id> tag, then
  please add the commit author[s] as reviewer[s] to this issue.
• All commits are signed off. See the section Developer’s Certificate of Origin
• Provide a title or release-note blurb suitable for the release notes.
• Are you a user of Cilium? Please add yourself to the Users doc
• Thanks for contributing!

This PR updates agent health server to listen on nodeip instead of all interfaces.
Fixes:
#23353

<!-- Enter the release note text here if needed or remove this section! -->

[pippolo84]

Thanks for your PR @tamilmani1989 . Changes look good on the agent side, but I think we should leave the cilium-health-responder behavior as is.

[pippolo84]

Changes look good, thanks!

One last thing: could you please change the commit message to adhere to the guide here? Maybe a brief description of the solved issue and a reference to it in a Fixes: ... line. For the title, I suggest something like health: Update daemon to listen on node ip.
Thanks! 🙏

[pippolo84]

Hi @tamilmani1989 , what I meant was to change the commit message like this:

health: Update daemon to listen on node ip

Update agenthealth to listen on internal node ip instead of listening on
all interfaces.

Fixes: #23353
    
Signed-off-by: Tamilmani <tamanoha@microsoft.com>

I was not referring to the PR title, but to the first line of the commit message. Sorry for the confusion.
Can you please update the commit message one last time so I can approve the PR? Thanks in advance! 🙏

[pippolo84]

Nice! 🚀

[pippolo84]

@tamilmani1989 CI failures are unrelated to the PR. It has already been fixed in main, could you please rebase?

[pippolo84]

/test

[tamilmani1989]

@pippolo84 anything else required for this PR?

[pippolo84]

CI failures seem legit, are you up to investigate them? I think it should be the same root cause for all of those.

[maintainer-s-little-helper]

Commit 31aaf55006f1772c0e2af222a824fcb78c4e0cbd does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

[tamilmani1989]

/test

[tamilmani1989]

/test

[tamilmani1989]

/test

[tamilmani1989]

@pippolo84 not sure why external workloads CI failing. It doesn't look like regular cilium agent running on a node. Is there anyway I can repro it locally? My guess is that cilium is running as non-hostnetworking pod in that node. If that's the case, is there anyway to detect in cilium agent code? Rest of failures look flaky.

This is the failure:

cilium-health validation failed: "cilium-agent 'kube-system/cilium-w7lzk': connectivity to path 'cilium-cilium-5707680440-1-vm/cilium-cilium-5707680440-1-vm-6.host.primary-address.http.status' is unhealthy: 'Get \"[http://10.202.0.30:4240/hello\](http://10.202.0.30:4240/hello/)": dial tcp 10.202.0.30:4240: connect: connection refused'", retrying...

NAME                              CILIUM ID   IP
cilium-cilium-5707680440-1-vm-6   21875       10.202.0.30

[pippolo84]

Thanks for this next iteration! 🙏
I've left some comments, mainly around the style.

To setup a local cluster with external workloads you can refer to the doc here.
I think you can check option.Config.JoinCluster to understand if Cilium is running in a VM, since the feature is enabled based on that config flag.

[tamilmani1989]

/test

[tamilmani1989]

Thanks for this next iteration! 🙏 I've left some comments, mainly around the style.

To setup a local cluster with external workloads you can refer to the doc here. I think you can check option.Config.JoinCluster to understand if Cilium is running in a VM, since the feature is enabled based on that config flag.

I spent some time in creating repro but couldn't. When I tried creating external workload following that doc, I'm losing connection to VM everytime I ran this script ./install-external-workload.sh . I couldn't access VM after that. I see cew resource created for that external worload though

$ k get cew
NAME     CILIUM ID   IP
extvm3   29819       10.10.0.7

I skipped this change for external node for now and if needed can open separate issue for that.

[tamilmani1989]

These CI failures look flaky as it succeeded in previous run

[pippolo84]

These CI failures look flaky as it succeeded in previous run

Agree. Let's try a re-run and in case flakyness is confirmed I'm gonna open an issue.

[pippolo84]

CI is green now, awesome! 💯
Apart from this last feedback to address, one minor thing left: could you please add a comment where we check for option.Config.JoinCluster to explain why that is needed? 🙏 Thanks!

[tamilmani1989]

/test

[pippolo84]

Hey @tamilmani1989 , I took a closer look at the last changes and I think we can avoid the introduction of the helpers GetInternalIPv4 and GetInternalIPv6, relying on the already existing node.GetIPv4 and node.GetIPv6 instead.
Regarding the external workloads, I think that what you observed is due to the fact that the agent is running on a VM. In that case, we just have to leave the old logic since there is no such thing as an InternalIP. To do that, I suggest to introduce a helper to get the right addresses based on the global options. I did it here and it is working fine (don't mind the failures, they are just flakes, the health related tests are passing).
So, I suggest to:

• remove the helpers GetInternalIPv4 and GetInternalIPv6
• refactor the code to introduce a helper for the server addresses as done here
• Change the comment removing the reference to the issue and stating that in the external workload case we listen on all interfaces and all address families
• squash the commits and fix all their messages to be compliant to the guidelines here (please take a careful look at point 5)

Thanks in advance!

[tamilmani1989]

Hey @tamilmani1989 , I took a closer look at the last changes and I think we can avoid the introduction of the helpers GetInternalIPv4 and GetInternalIPv6, relying on the already existing node.GetIPv4 and node.GetIPv6 instead. Regarding the external workloads, I think that what you observed is due to the fact that the agent is running on a VM. In that case, we just have to leave the old logic since there is no such thing as an InternalIP. To do that, I suggest to introduce a helper to get the right addresses based on the global options. I did it here and it is working fine (don't mind the failures, they are just flakes, the health related tests are passing). So, I suggest to:

• remove the helpers GetInternalIPv4 and GetInternalIPv6
• refactor the code to introduce a helper for the server addresses as done here
• Change the comment removing the reference to the issue and stating that in the external workload case we listen on all interfaces and all address families
• squash the commits and fix all their messages to be compliant to the guidelines here (please take a careful look at point 5)

Thanks in advance!

The reason i added new api GetInternalIPv4 is that existing node.GetIPv4 may return node External IP if internal ip doesn't exist and cilium would be listening on <external_ip>:4240. is that desired behavior?

The comment for that function reads:

// GetIPv4 returns one of the IPv4 node address available with the following
// priority:
// - NodeInternalIP
// - NodeExternalIP
// - other IP address type.
// It must be reachable on the network.

[pippolo84]

The reason i added new api GetInternalIPv4 is that existing node.GetIPv4 may return node External IP if internal ip doesn't exist and cilium would be listening on <external_ip>:4240. is that desired behavior?

The comment for that function reads:

// GetIPv4 returns one of the IPv4 node address available with the following // priority: // - NodeInternalIP // - NodeExternalIP // - other IP address type. // It must be reachable on the network.

The only case I can think of where the InternalIP won't be available if the external workload, but we already take care of that separately. Anyway, there's no harm in having extra care, so let's leave your helpers as you suggested 👍

[tamilmani1989]

If GetIPv4 or GetIPV6 fails for some reason, then cilium agent would not listen on that ip family as its not appended to address list. so we may need this piece of code as well to address it (it was removed in your version)

	if len(addresses) != 0 {
		if option.Config.EnableIPv4 && ipv4 == nil {
			addresses = append(addresses, "0.0.0.0")
		}
		if option.Config.EnableIPv6 && ipv6 == nil {
			addresses = append(addresses, "::")
		}
	}

[tamilmani1989]

@pippolo84 I updated PR based on above comments and what I felt right. I'm open to hear if there any alternate suggestions and can update accordingly. Thanks for taking time to review this. Appreciate it. 👍

[tamilmani1989]

/test

[pippolo84]

Thank you for the further iteration 💯
I agree with your analysis regarding the GetInternalIPv4 and GetInternalIPv4 returnng nil. I've left a suggestion to improve the readability of the code when handling those cases.

[pippolo84]

Thanks! This turned out to be trickier than expected, great job! 🚀

[pippolo84]

/test

[ldelossa]

All required tests passing. Merging