feat: add support for external IP in ambassador host source

[fad3t]

Description

Hello!

I would like to submit a PR to add support for external IPs in Ambassador Host resources.
Most of the code has been shamelessly copied from the Service source implementation (https://github.com/kubernetes-sigs/external-dns/blob/master/source/service.go#L591).

I couldn't add any tests as the ones I currently see for Ambassador are rather basic (e.g. making sure the annotation format is correct) while in my case I would need to mock the Kubernetes API and I'm not sure how to do it -- happy to give it a try if somebody can guide me a bit.

Note this fixes #3733.

Let me know if anything is missing or if you have improvement ideas.

Thanks,
Fred

[k8s-ci-robot]

Welcome @fad3t!

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]

Is it intentional that the code also includes the ExternalIPs of Services of type other than LoadBalancer?

The code could be more simply expressed using the refactor in #3728

Would it make sense to call extractLoadBalancerTargets() instead of (incompletely) duplicating the logic?

[johngmyers]

I'm wondering if we should refactor serviceSource.generateEndpoints() to extract a common function for getting the Endpoints for a Service. This would then be called by sources such as Ambassador and Istio (#3720) that obtain their endpoints from one or more associated Services.

What do you think @szuecs ?

[fad3t]

Is it intentional that the code also includes the ExternalIPs of Services of type other than LoadBalancer?

It is not, although I'm not sure there are many deployments of Ambassador using another service type. Anyway, it would be good to add that check, thanks.

The code could be more simply expressed using the refactor in #3728

Right, thanks.

Would it make sense to call extractLoadBalancerTargets() instead of (incompletely) duplicating the logic?

Indeed, I didn't think about reusing that function - maybe to avoid confusion as it's part of the Service source. Might be good to have some kind of common utils functions in a separate file, so that all similar implementations can reuse them (and test them only once). This ties in with your second comment about extracting a common function for this purpose.

How would you like to proceed next?

[johngmyers]

I would like to first get some consensus from approvers on my proposed direction for sources that consume endpoints from associated Services.

Would it make sense for Ambassador to be associated with a Service of type NodePort? It seems to me it might, especially on clouds without good load balancers. Similarly, would it make sense to be associated with a headless Service?

After that, we would refactor the service source to make the target extraction usable by other sources.

[fad3t]

Hi,

Any update on this one? Could we maybe consider going for an intermediate solution (= reuse the existing extractLoadBalancerTargets function) while discussions are ongoing -- and refactor in a second step?

Thx,
Fred

[johngmyers]

There needs to be a little refactoring since the TTL shouldn't come from the service's annotation.

[szuecs]

@johngmyers I think it's fine like this and we can do refactoring after that, or if @fad3t would do it even better.

/ok-to-test

[fad3t]

Hi, I replaced my code with the existing extractLoadBalancerTargets function as suggested. I won't be able to look at the refactoring now, but I'll keep this in mind for later (I'd also need to get a bit more familiar with the project code).

Thanks!

[fad3t]

/test pull-external-dns-lint

[mloiseleur]

/lgtm

[Raffo]

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Raffo

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