connectivity: use the right test namespace in cnps

[bmcustodio]

Previously, the CNPs created during the connectivity test referenced cilium-test in matchLabels as opposed to the user-specified namespace. This commit addresses that by looking up potential matches and doing the proper replacement.

[ti-mo]

Related: #275. Also, see #276, which makes spinning down test namespaces quite time-consuming at the moment. Should be an easy fix, though. Let's hold off on generating random namespaces for now.

I think a solution to this problem might require some more thought. Populating the namespace field is trivial, but the target field of matchLabel annotations looks a bit trickier. Any thoughts?

[ti-mo]

I'm not a fan of the placeholder idea, that will create complexity sprawl in the long run.

My idea was to have policies in yaml form with e.g. the namespace field empty and populate them in Go during execution. (empty fields to make sure we don't inadvertently clobber them) This would mean these policies can no longer be applied using kubectl, but that's not the case with this approach either.

[bmcustodio]

This would mean these policies can no longer be applied using kubectl, but that's not the case with this approach either.

cat connectivity/manifests/client-egress-to-echo.yaml| TEST_NAMESPACE=foo envsubst | k apply -f- works 🙂 Of course it isn't as simple as just kubectling, but it does actually work.

[ti-mo]

This would require passing the correct test namespace to cilium uninstall as well, which a user would quite easily overlook. Not a comment on this PR, but we've discussed before that this code shouldn't be here, only cilium test should be responsible for managing the lifecycle of the test namespace.

[bmcustodio]

I think a solution to this problem might require some more thought.

@ti-mo that's why I was proposing this as a quick, interim fix until we figure it out. 🙂 At least in the meantime users wouldn't be running into this issue.

[nbusseneau]

Same comments as Timo, I'll approve because I do think the quick fix is sound. I would suggest tracking in an issue if we end up merging this.

[tklauser]

I'm not a big fan of the placeholder idea either and I think that it will probably stick around for quite a while, even if we create an issue to eventually replace it 😉

How about modifying the policy in Go code as suggested by @ti-mo? Or is there any problem with that approach?

Would something like along the lines of the following, naïve, untested change work?

diff --git a/connectivity/check/test.go b/connectivity/check/test.go
index 634ee982acc0..31606ad6917a 100644
--- a/connectivity/check/test.go
+++ b/connectivity/check/test.go
@@ -22,6 +22,7 @@ import (
        "sync"
        "time"
 
+       k8sConst "github.com/cilium/cilium/pkg/k8s/apis/cilium.io"
        ciliumv2 "github.com/cilium/cilium/pkg/k8s/apis/cilium.io/v2"
 )
 
@@ -209,6 +210,19 @@ func (t *Test) WithPolicy(policy string) *Test {
                t.Fatal("Error parsing policy YAML: %w", err)
        }
 
+       for i := range pl {
+               pl[i].Namespace = t.ctx.params.TestNamespace
+               if pl[i].Spec != nil {
+                       for _, e := range pl[i].Spec.Egress {
+                               for _, es := range e.ToEndpoints {
+                                       if _, ok := es.MatchLabels[k8sConst.PodNamespaceLabel]; ok {
+                                               es.MatchLabels[k8sConst.PodNamespaceLabel] = t.ctx.params.TestNamespace
+                                       }
+                               }
+                       }
+               }
+       }
+
        if err := t.addCNPs(pl...); err != nil {
                t.Fatal("adding CNPs to policy context: %w", err)
        }

[bmcustodio]

@tklauser nice one — I have based the new approach on that, with slight changes (cc @ti-mo @jrajahalme).

[tklauser]

🚀

[tklauser]

Removed the ready-to-merge label to give @christarazi a chance to review again re. #330 (comment)

[christarazi]

Let's squash the last commit into the first

[tklauser]

Let's squash the last commit into the first

Done and force pushed.