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 (fbd4bc5, 365c0b0)

[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

[didlawowo]

any news ?

[k8s-ci-robot]

New changes are detected. LGTM label has been removed.

[raoulbhatia]

FYI We have rebased the current patch on behalf of @peterhoneder onto latest master to ensure we are up2date

[thecmdradama]

Hi guys, any progress on getting this PR merged in? @mloiseleur

[raoulbhatia]

/remove-lifecycle stale

[foyerunix]

Hello,

It seem I duplicated a great part of your effort in my own PR that also handle the reverse PTR creation: #4782

Perhaps there is a way to merge the two into something that could be accepted ?

Best Regards.

[peterhoneder]

Hello,

It seem I duplicated a great part of your effort in my own PR that also handle the reverse PTR creation: #4782

Perhaps there is a way to merge the two into something that could be accepted ?

Best Regards.

I would be happy to see both of the PRs merged, especially since we use this PR already for a long time in production.

[szuecs]

As long as it's only enabled for a certain new feature flag, I will not block this pr. Same for the other mentioned one.

In general I don't think it is a good idea but I will not block IMO broken architectures (don't try to explain why it is not broken 😉). Some are likely not in control of these assumptions and cannot easily change them.

Happy new year everyone, hope we can get this through the line for you.

[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 mloiseleur. For more information see the 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

[mloiseleur]

@peterhoneder Do that work for you ? It seems to me that since #4782 already implements it behind a feature flag, we should focus on this one. Anything I missed ?

[peterhoneder]

@peterhoneder Do that work for you ? It seems to me that since #4782 already implements it behind a feature flag, we should focus on this one. Anything I missed ?

I'm not completely sure why this would need a feature flag, since you specifically have to add the annotation to enable the PodIP to be used anyway? Seems like a double opt-in then for me. Could you explain?

[foyerunix]

Hello @peterhoneder,

I believe my PR also accommodates your use case. As I understand it, using --ignore-non-host-network-pods without my second new option --pod-source-domain should achieve the desired results.

Do you see any improvements we could make to better handle your use case?

Best regards.

[raoulbhatia]

@foyerunix et al – we implemented our requirement based on #4782 with --no-ignore-non-host-network-pods.
We are still in in a verification/monitoring mode, but overall things look good at this point.

We will keep an eye on our deployment.
If everything is well, we can close this PR in favor of #4782 next week.

Thanks @foyerunix for pushing this :-)

[mloiseleur]

@peterhoneder Now that #4782 has been merged, I understand that we can close this one. Feel free to re-open it if I missed something.
/close

[k8s-ci-robot]

@mloiseleur: Closed this PR.

In response to this:

@peterhoneder Now that #4782 has been merged, I understand that we can close this one. Feel free to re-open it if I missed something.
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[k8s-ci-robot]

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.