feat: pods can have hostPorts without hostNetwork

[gregorycuellar]

Description

Pods can have hostPorts without hostNetwork.
I propose to remove the check that prevent checking annotations on pods that are not on hostNetwork.

Checklist

• Unit tests updated
• End user documentation updated

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: gregorycuellar / name: CUELLAR Grégory (f2cb5ed)

[k8s-ci-robot]

Welcome @gregorycuellar!

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]

This appears to be incorrect. In many CNIs, connections to non-host-network pods would need to go to the pod IP, not the node IP.

[gregorycuellar]

@johngmyers can you explain ?

What's the point of having an hostPort and send traffic to the Pod IP ? If you can send the traffic directly to the Pod IP, you don't need hostPorts.

Hostports and hostnetwork are two separated concepts, which ( as far as I know ) are not linked together.
Also, on many cloud providers, you can only define hostports and not hostnetwork.

I don't know all CNIs so maybe there are exceptions, I don't know about.

[johngmyers]

In CNIs, such as the AWS VPC CNI, that don't use an overlay network you can send traffic to the pod IP.

This PR simply removes the check, so will create records for pods that don't have any hostPorts. It also ignores the ports.hostIP fields. So it will publish DNS records with incorrect or non-working IPs.

[johngmyers]

If you want to extend the pod source's support for non-host-network pods, you will likely need to handle them as a separate case.

[gregorycuellar]

I think, I understood the case you are mentioning.

For me, it was covered as you have the possibility to have internal hostname ( which point to pod IP ) and hostname ( to node IP ).
Also, with the check in place, none are defined if there is not hostnetwork, so this use case is not working today.

Without the PR :

• with hostnetwork and no annotation -> nothing defined
• with hostnetwork and internal hostname annotation -> Pod IP
• with hostnetwork and hostname annotation -> Node IP
• without hostnetwork and no annotation -> nothing defined
• without hostnetwork and internal hostname annotation -> nothing defined
• without hostnetwork and hostname annotation -> nothing defined
• ports.hostIP is ignored

With the PR :

• with hostnetwork and no annotation -> nothing defined
• with hostnetwork and internal hostname annotation -> Pod IP
• with hostnetwork and hostname annotation -> Node IP
• without hostnetwork and no annotation -> nothing defined
• without hostnetwork and internal hostname annotation -> Pod IP
• without hostnetwork and hostname annotation -> Node IP
• ports.hostIP is ignored

I will try, to rework the PR, to have :

• with hostnetwork and no annotation -> nothing defined
• with hostnetwork and internal hostname annotation -> Pod IP
• with hostnetwork and hostname annotation -> Node IP
• without hostnetwork and no annotation -> nothing defined
• without hostnetwork and internal hostname annotation -> Pod IP
• without hostnetwork and hostname annotation -> Node IP if hostPort is defined, Pod IP otherwise
• ports.hostIP is used if defined

[johngmyers]

#3174 is similar

I have a branch https://github.com/johngmyers/external-dns/tree/pod-from-node which switches the source of internal IPs for pods from the podIP to the nodes. This is necessary for IPv6, when the pod network is single-stack but nodes are dual-stack. That pending change is likely to interact with extending the pod source to handle non-host-network pods. (I withdraw this comment.)

[johngmyers]

I concur with the proposed semantics of May 7.

[johngmyers]

I believe if there are multiple ports it should use the union of all hostIPs if said union is non-empty.

[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 assign johngmyers for approval. 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

[mloiseleur]

/ok-to-test

[mloiseleur]

@gregorycuellar Wdyt of #3174 ? Would this solve your issue ? If not, do you think you can rebase and fix tests ?

[k8s-ci-robot]

@gregorycuellar: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-external-dns-lint	d900018	link	true	/test pull-external-dns-lint

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

[gregorycuellar]

@mloiseleur No, with 3174, it's still not possible to Node IP if hostNetwork is not defined. ( cf L103 )

PR has been rebased and tests fixed.

[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

[gregorycuellar]

@mloiseleur do you think it can be merged or should it be abandoned ?