add service annotation to set public/private iface for NodePort

[rbtr]

Scenario: in AWS with a private hosted zone, and instances with both public and private IPs, I would like some NodePort services to be registered in DNS with the node's private IP so that they're only resolvable within my VPC.

This is my reattempt at #898 since it appears abandoned, and I think that having the public/private interface selection should happen on a per-service basis rather than globally.

I'm open to suggestions on what the annotation should be renamed to, consider "access" a placeholder...
Finally, this change should be backwards compatible - if the annotation is not present we fall through to the previous behavior.

[k8s-ci-robot]

Welcome @rbtr!

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. 😃

[rbtr]

/assign @linki

[rbtr]

honestly, this could be the start of a larger story on being able to appropriately segregate private/public IPs and zones - like maybe if the aws-zone-type is set to public use public IPs and if it's set to private use private IPs, or maybe the zone type could be detected, or maybe I could set multiple zones and different hostname per zone in a single service for split horizon configs...
but this seems like the minimally invasive change required to be able to create records with the instance private IPs, in a private zone, when those instances also have public IPs

[rbtr]

bump? @linki @hjacobs @njuettner

[jonathan-mothership]

Duplicate of #1212?

[rbtr]

@jonathan-mothership yeah, this and that PR are very similar in effect, if not identical.

i won't close this yet because i think, conceptually, saying "ignore public" is not the best way to characterize this datapath - it would be better to allow the user to be explicit in saying "use public" or "use private" - and that's a significant distinction in our implementations.

but i think it's safe to say neither PR author thinks that the current behavior of external-dns assuming which interface we want to use for us is correct either 😅

[jonathan-mothership]

I agree with you completely, @rbtr, this feature is useful for private EKS clusters. FWIW I've taken the other PR and modified the annotation name and value to be nodePortTarget="internal". Hopefully one of these PRs gets merged soon!
#1212 (comment)

[rbtr]

@linki @hjacobs @njuettner
does this have any chance of making it in? it's the only way external-dns works for me

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[seanmalloy]

@seanmalloy any updates here?

@rbtr no updates yet. It is in my queue to review. It looks like there is conflict in source/service_test.go. Thanks for your patience.

[vinny-sabatini]

/assign
/unassign @seanmalloy

[vinny-sabatini]

Thanks for the PR @rbtr !
I gave this a review and the code change looks good to me. Thanks for the clear update of the docs and including the necessary test cases.

If you can fix the merge conflicts and rebase, I'll throw my LGTM label on there and will work with the official maintainers to get this reviewed and hopefully merged soon.

[rbtr]

Wilco, thanks for the update

[rbtr]

@vinny-sabatini rebased

[rbtr]

/assign @Raffo

[vinny-sabatini]

/lgtm

[vinny-sabatini]

/assign @njuettner

[Raffo]

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Raffo, rbtr

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

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [Raffo]

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

[rbtr]

thanks all!