fix(connectivity): Add node-local-dns entitiy match for local ip usage case

[eminaktas]

When node-local-dns is deployed with local IP,
policies fail to verify that the connection goes to
node-local-dns. This change allows such DNS
traffics in the connectivity test policy yamls.

Signed-off-by: eminaktas [email protected]

[eminaktas]

ping @aditighag

[tklauser]

Thanks for the PR @eminaktas.

Could you please add a short description why the change is needed and/or an issue reference to the commit message?

[eminaktas]

Thanks for the PR @eminaktas.

Could you please add a short description why the change is needed and/or an issue reference to the commit message?

Thanks for the comment @tklauser . It's done.

[aditighag]

Barring the connectivity/manifests/client-egress-to-entities-world.yaml, I don't think enabling access to the world entity is recommended, it's too permissive.
See cilium/cilium#20683 (comment).

[squeed]

Does #995 fix this?

[eminaktas]

Does #995 fix this?

Hi @squeed

Yes, it does. However, it might introduce a security issue because this commit uses the world label to allow the nodelocaldns requests. Like @aditighag mentions that it's too permissive. To prevent it, Cilium should label the nodelcaldns as host.

Kubespray's default nodelocaldns IP is 169.254.25.10. It creates a network interface on the node to provide a connection to nodelocaldns.

[squeed]

@eminaktas #995 no longer opens access to world. Does it fix your use case, or do we need further work? @aditighag ?

[eminaktas]

@eminaktas #995 no longer opens access to world. Does it fix your use case, or do we need further work? @aditighag ?

@squeed I guess I misunderstood you. #995 fixes only when node-local-dns is deployed as k8s service. This commit is to fix when node-local-dns is deployed with local IP.

[aditighag]

@eminaktas #995 no longer opens access to world. Does it fix your use case, or do we need further work? @aditighag ?

#995 only extends the connectivity tests for LRP based node-local dns deployment. It doesn't have the identity issue because LRP works with pod ips, not the 169.254... address.

[eminaktas]

I added a comment here for something that I noticed.

[eminaktas]

I am kindly pinging for this case @squeed @aditighag.

I updated the PR to prevent failing the connectivity test for the environment where nodelocaldns is configured with a local IP labelled world by Cilium. In addition, I updated the policies with the narrowest authorization, so the packages do not drop.

I am looking forward to your feedback 👀
Thanks 🙏

[squeed]

Rather than adding blanket world access, can you wait for #1267 then template the node local dns address in? And, is there an easy way for cilium-cli to autodetect the dns IP?

Or, are we just being too picky here, @aditighag?

[squeed]

Oh bah, this might not work. See cilium/cilium#16308

Fine, I give up :-)

[aditighag]

@eminaktas I thought we discussed it on the other PR, see - cilium/cilium#20683 (comment). Did something change after your reply - cilium/cilium#20683 (comment)?

[eminaktas]

Hi, @aditighag
Actually, there is no change Cilium still can't mark the local IPs with a host label. I think this is currently the only solution for the test cases. Also, this will save time for other people. Like @squeed asked, are we just being too picky on this?

[aditighag]

Hi @eminaktas! I went over all the policies that the PR is modifying, and it looks we seem to relax them only for DNS traffic going to node_local pods. I deployed a couple of policies, and validated that rest of the traffic to world is blocked.
While it's not ideal to poke holes in the connectivity manifests as the world identity is fully permissive, until we have better support to add custom identities, we probably need this workaround. We should follow up on -cilium/cilium#18644 (comment). This also includes making LRP as stable so that users won't have to deal with such IP addresses in their cluster.
I've requested changes at one place, rest of the changes are fine.

[aditighag]

Why not narrow this down further -

- matchExpressions:
          - { key: 'k8s-app', operator: In, values: [ "node-local-dns", "nodelocaldns" ] }
          - { key: 'io.kubernetes.pod.namespace', operator: In, values: [ "kube-system" ] }

[eminaktas]

Yeah, I added and tested there wasn't any issue. thanks.

[eminaktas]

The new change caused a failure for Multicluster test. You can find the logs here.

[eminaktas]

I did not understand the why Multicluster test failed. So, I took the change back.

[maintainer-s-little-helper]

Commit 505d7ef 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

[eminaktas]

Hi @eminaktas! I went over all the policies that the PR is modifying, and it looks we seem to relax them only for DNS traffic going to node_local pods. I deployed a couple of policies, and validated that rest of the traffic to world is blocked. While it's not ideal to poke holes in the connectivity manifests as the world identity is fully permissive, until we have better support to add custom identities, we probably need this workaround. We should follow up on -cilium/cilium#18644 (comment). This also includes making LRP as stable so that users won't have to deal with such IP addresses in their cluster. I've requested changes at one place, rest of the changes are fine.

Thanks for the help @aditighag.