connectivity: Report IP families that are being tested

[Neilblaze]

The objective is to improve the connectivity reporting by displaying which IP families (i.e., v4 or v6) are being tested.
Additionally, each Action description should contain the family too. E.g., we could report curl-v4-1, curl-v6-2, etc.

Fixes #1410
cc: @brb, @gandro
Signed-off-by: Pratyay Banerjee

[Neilblaze]

@brb Can you please provide some feedback on where I'm going wrong? 😅

Even one of the error log state this? → could not import github.com/cilium/cilium-cli/connectivity/tests

[gandro]

Thanks for the PR! You have a syntax error here. This func here is never closed, hence all the errors you see.

I'd recommend testing the code out locally too, that will make it easier to spot defects.

[Neilblaze]

Ah silly 😭
Thank you so much @gandro for the catch! Because of the fact that I work on two different PC's (Ubuntu 22.04 LTS + Windows 10), the mistake happened while I was shifting the code from one system to another. 😅

And thanks for the advice, I'll make sure to test the code out locally too to avoid such silly mistakes. 😄

[Neilblaze]

Also, @gandro this → github.com/cilium/cilium-cli/connectivity/check is throwing 404. Additionally, how can I check the IP-class then? (refer to this)

Should I have to use this?

[gandro]

The proper import path should be "github.com/cilium/cilium-cli/connectivity/check". This is a Go import path and therefore not a valid GitHub URL. The GitHub URL for that file would be https://github.com/cilium/cilium-cli/tree/master/connectivity/check

Should I have to use this?

Yes, that's the one to use.

[maintainer-s-little-helper]

Commit c6e9d77 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

[maintainer-s-little-helper]

Commit 2fbe9aa 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

[maintainer-s-little-helper]

Commit 2fbe9aa 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

[maintainer-s-little-helper]

Commit 2fbe9aa 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

[Neilblaze]

@gandro I think it's fine now. Phew!
The implemented code could be optimized using the ternary operator. I tried this:

t.ForEachIPFamily(func(ipFam check.IPFamily) {
    ipFamStr = ipFam == check.IPFamilyV4 ? "v4" : "v6"
// ...

instead of

t.ForEachIPFamily(func(ipFam check.IPFamily) {
    if ipFam == check.IPFamilyV4 {
        ipFamStr = "v4"
} else if ipFam == check.IPFamilyV6 {
    ipFamStr = "v6"
}
// ...

It should work, but apparently, I was getting an error log stating "?" as illegal character. On doing some research, I found this on StackOverflow. Hence, I proceeded with the previous one. Also, instead of performing two checks, we can simply check whether it's ipv4 or not, else it should be ipv6.

If the above one sounds fine to you, then this can be implemented:

t.ForEachIPFamily(func(ipFam check.IPFamily) {
    if ipFam == check.IPFamilyV4 {
        ipFamStr = "v4"
    } else {
        ipFamStr = "v6"
    }
// ...

Once you confirm, then I'll push the final changes & squash it.
Refer to this. And apologies for creating this simple thing a mess. 😅

[maintainer-s-little-helper]

Commit 555bfaf 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

[Neilblaze]

@gandro All the tests are were passing (including EKS(ENI), GKE & Multicluster). But after I squashed all the commits together (after adding the DCO), those two are specifically failing. Could be a flake, would you mind re-triggering this job again? And if everything checks out, please merge :)

[gandro]

Thanks for the update. Yes, those look like unrelated flakes (it seems the github container registry has been struggling since yesterday, which is used for the actions too).

But before we validate CI, I think there are still a few things to address in the code. I've left some inline feedback.

In addition, there are more usages of ForEachIPFamily to patch (I recommend searchg for ForEachIPFamily in your IDE to make sure you get them all):

cilium-cli/connectivity/tests/client.go

Line 42 in 3388360

t.ForEachIPFamily(func(ipFam check.IPFamily) {

cilium-cli/connectivity/tests/pod.go

Line 108 in acebe27

t.ForEachIPFamily(func(ipFam check.IPFamily) {

cilium-cli/connectivity/tests/encryption.go

Line 155 in 3388360

t.NewAction(s, "curl", client, server, ipFam).Run(func(a *check.Action) {

cilium-cli/connectivity/tests/encryption.go

Line 160 in 3388360

t.NewAction(s, "ping", client, server, ipFam).Run(func(a *check.Action) {

[Neilblaze]

@gandro I underestimated how simple it was, and as a result, I delved deeply into every aspect, leading me into all this unnecessary trouble lol. Hopefully this time it should be fine.

[gandro]

Thanks! Yes, this looks good to me know. Let's run it through CI.

[gandro]

GKE failed connecting to google.com - possibly due to external factors. Restarting

https://github.com/cilium/cilium-cli/actions/runs/4449884381/jobs/7814654455?pr=1440

[gandro]

Re-adding the required review from the CLI codeowners. Please do not remove automatically assigned code-owners, since not every reviewer (like myself) belongs to all codeowner teams.

[Neilblaze]

Please do not remove automatically assigned code-owners, since not every reviewer (like myself) belongs to all codeowner teams.

@gandro Sorry about the confusion, but as far as I can remember, I'm pretty sure I didn't unassign anyone 😅, but it seems like GitHub auto-removes any other reviewers when we select this 👇🏼

[gandro]

Please do not remove automatically assigned code-owners, since not every reviewer (like myself) belongs to all codeowner teams.

@gandro Sorry about the confusion, but as far as I can remember, I'm pretty sure I didn't unassign anyone sweat_smile, but it seems like GitHub auto-removes any other reviewers when we select this 👇🏼

Oh, interesting, thanks for the clarification!

--

This PR is ready to be merged.