feat: allow pod IPs even for non-hostNetwork pods

[peterhoneder]

but only for the internalHostnameAnnotationKey which doesn't use the node's IP anyway

Description

Pod IPs and node IPs can be used for hostname annotations on pods.

The existing implementation worked this way:

• check if the pod has nodeNetwork: true
• if no -> skip
• otherwise:
  • for the internal hostname annotation -> use the pod' IP
  • for the regular hostname annotation -> use the node's IP
  • special handling for kops-dns-controller

I would like to change the implementation to cover a case, where Pod IPs can be exposed as names, but with pods where hostNetwork == false. This is a typical use case with CNI plugins like Calico CNI, where pod IPs are exposed as BGP routes directly to the ToR router/switch. So there is basically no service in front of the pod and the pod is directly exposed to the outside world of the cluster, most commonly with an internet-routable public IP, but could also be a private IP that is justed routed internally. The usefulness of this change addresses both those cases.

Since the pod's IP is already used for the internal hostname annotation, there is no need to actually limit this behavor to pods with nodeNetwork: true.

I can add public documentation for this, since the whole pod annotation part is not publicly documented yet, it might be a topic for a separate PR though.

There is no github issue for this yet.

Checklist

• Unit tests updated
• End user documentation updated

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: peterhoneder (1b9c3bd, 27e2581)

[k8s-ci-robot]

Welcome @peterhoneder!

It looks like this is your first PR to kubernetes-sigs/external-dns 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/external-dns has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[johngmyers]

It looks like external-dns is, in emulating kOps dns-controller, is assuming the pod IP is the same as a singular internal node IP. The dns.alpha.kubernetes.io/internal annotation on a pod causes dns-controller to register the host-network pod with each of its node's internal IP addresses. This can be subtly different.

Both the node's internal IPs case and pod's IP case are useful. In some networking configurations, non-host pods' internal IPs are inaccessible, yet the node internal IPs are. There is also a difference for dual-stack nodes supporting a cluster with a single-stack pod network.

[peterhoneder]

kOps dns-controller

the proposed PR currently does not change that behavior, but yes the existing implementation of the non kops compatible code path would only use NodeIPs from host network pods

[johngmyers]

I have a vague recollection of pods created in the early minutes of a new cluster having odd IP addresses, but I can't find anything like that in e2e tests. In the tests I've looked over, podIPs of host-network pods are consistently the internal IPs of the node. So there might not be a distinction after all.

Whether or not non-host-network pods are included can be controlled by whether non-host-network pods have the annotation.

There's a good question of whether it should use podIP or podIPs, but that's for after external-dns supports AAAA records.

/lgtm

[johngmyers]

I think I remember what it was: before the CNI comes up, the podIP field might not get populated. So if your CNI depends on external-dns registering the kube-apiserver pods with DNS, you need external-dns to grab the IPs from the pod's node.

As previously noted, this was broken before and remains broken after the PR. The fix would be to follow from the pod to the node for pods that are host-networking. Fixing external-dns to correctly implement kOps requirements is behind #2051.

[peterhoneder]

Hi!

my change does not affect the behavior for host-networking. It only allows an additional behavior for non host-networking pods, where the PodIP is correctly populated, and must be used instead of the host IP. The typical use case would be with a CNI like calico, where e.g. publicly routable IPv4 addresses are assigned to the pod.

[johngmyers]

To be clear, I was not asking this PR to fix the handling of kOps requirements. It does not make things worse.

[peterhoneder]

To be clear, I was not asking this PR to fix the handling of kOps requirements. It does not make things worse.

ok, thanks, perfect 😄 just wanted to make sure we have the same understanding 👍

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Mark this PR as fresh with /remove-lifecycle stale
• Close this PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[peterhoneder]

/remove-lifecycle stale

[szuecs]

The test name seems to be wrong.

And I wonder if you can increase test cases.

[mloiseleur]

@peterhoneder Do you think you can rebase this PR and takes into account review comment from @szuecs ?

[peterhoneder]

@peterhoneder Do you think you can rebase this PR and takes into account review comment from @szuecs ?

yes, I will do that

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from johngmyers. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[peterhoneder]

@peterhoneder Do you think you can rebase this PR and takes into account review comment from @szuecs ?

I rebased the PR and added more tests and more explicit comments on why the tests are there that way. I tested the scenarios described in the commit message.

Regarding the comments further up in the thread especially from @johngmyers I think this points to valid use cases that might be interesting. In our setup we are basically using the internal hostname annotation for pods where calico CNI provides addresses and later announces those over BGP. Those addresses are public addresses due to the nature of the workload. In those cases having the regular hostname annotation work similarly in cases where the pod is not in the host network would be beneficial, but due to the constraints described above this sounds to me more like a task for a future PR.

[mloiseleur]

/ok-to-test

[mloiseleur]

/lgtm

[mloiseleur]

Wdyt about removing this condition to cover all cases described in #3468 (comment) ?

[peterhoneder]

I can include that change in here, will do so later today.

[peterhoneder]

@mloiseleur please check, I added the changes and also some more docs updates.

[mloiseleur]

It's lgtm at first glance. I asked @gregorycuellar to also check, if possible.

[mloiseleur]

@gregorycuellar : This PR has been improved to include use case of #3468. Wdyt ? Anything missing ?

[gregorycuellar]

@mloiseleur It look good to me, I think it's ok

[mloiseleur]

/lgtm
/assign @szuecs

[omartheironman]

is there an update on this?

[peterhoneder]

Hi! any updates on this?

[perber]

Hi! any updates on this?

[szuecs]

I don't think DNS is a good choice for this use case. They should rather create a service type loadbalancer. Maybe calico can build a svc type loadbalancer if they see value in supporting it.

In general I would rather delete pod dns name functionality than invest more into it.

[peterhoneder]

I don't think DNS is a good choice for this use case. They should rather create a service type loadbalancer. Maybe calico can build a svc type loadbalancer if they see value in supporting it.

In general I would rather delete pod dns name functionality than invest more into it.

Let me explain the use case: there are services which cannot be put behind a service. E.g. if you have multiple connections which need to end up on the same replica but originate from different source IPs. E.g. if they are based on DNS round robin, a k8s service would not help. It would only help if a service would be created for each pod separately. The patch works very well in production and seems to be used by multiple people who even created multiple PRs for this.

[jcralbino]

Hello All,

I am a user of external DNS, and I manage, design and operate enterprise container platform.

I will like to clarify some assumptions from this thread, and state that any POD IP can be reached for external communication if they are routed to the network.

https://docs.vmware.com/en/VMware-Tanzu-Kubernetes-Grid-Integrated-Edition/1.19/tkgi/GUID-network-profiles-pods.html

That is what we use for our enterprise deployment, routed pod IPs, where SNAT is not used and all communication between/to a pod can be allowed if the security policy allows it.

It is my understanding that using this check for hostNetwork=False, and assuming that those pods should be ignored is also wrong

[jcralbino]

I don't think DNS is a good choice for this use case. They should rather create a service type loadbalancer. Maybe calico can build a svc type loadbalancer if they see value in supporting it.

In general I would rather delete pod dns name functionality than invest more into it.

I believe that use case is indeed not the only one.

Pod IPs can be routable, and allowing it to have a dns name can be relevant for many different scenarios