Support Ambassador Host resources as sources #1642

inercia · 2020-06-23T12:14:42Z

Ambassador can be configured with Host resources (based on the Host CRD) for defining the external DNS host name. This is an example of a Host resource:

  apiVersion: getambassador.io/v2
  kind: Host
  metadata:
    name: e78cb61d56b6.eu.ngrok.io
    annotations:
      external-dns.ambassador-service: my-service
  spec:
    acmeProvider:
      authority: https://acme-v02.api.letsencrypt.org/directory
      email: alvaro.saurin@gmail.com
    ambassadorId:
    - default
    hostname: e78cb61d56b6.eu.ngrok.io
    selector:
      matchLabels:
        hostname: e78cb61d56b6.eu.ngrok.io
    tlsSecret:
      name: e78cb61d56b6.eu.ngrok.io

This code adds a new source, ambassador-host, that

watches Hosts resources with
a) a hostname, and
b) a external-dns.ambassador-service annotation that refers to an existing Service srv.
obtains the load balancer IP ip address from svc.Status.LoadBalancer.Ingress
returns the Endpoint with hostname and ip

Closes #1552

k8s-ci-robot · 2020-06-23T12:15:40Z

Welcome @inercia!

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

inercia · 2020-07-08T16:18:34Z

/assign @njuettner

source/store.go

source/ambassador_host.go

kflynn · 2020-12-03T03:21:41Z

@njuettner, I think that all of your comments have been addressed. I've dealt with the merge conflicts, and I'd like to get this landed.

kflynn · 2020-12-04T14:55:11Z

@njuettner @Lemmons @hjacobs @Raffo So, what's the path forward here? It would be lovely to get this landed before yet another merge conflict blocks it. 🙂

donofriov · 2020-12-11T09:55:56Z

Hey @njuettner just following up again as this unblocks some work for me, is there anything else needed or that I can help with to get this merged and released?

Raffo

I added two comments and mostly waiting for @njuettner's stamp. Changes look good to me.

Raffo · 2020-12-23T12:48:16Z

source/ambassador_host.go

+func parseAmbLoadBalancerService(service string) (namespace, name string, err error) {
+	parts := strings.Split(service, "/")
+	if len(parts) == 2 {
+		namespace, name = parts[0], parts[1]
+	} else {
+		parts = strings.Split(service, ".")
+		if len(parts) == 2 {
+			name, namespace = parts[0], parts[1]
+		} else {
+			namespace, name = api.NamespaceDefault, service
+		}
+	}
+	return
+}


I would prefer if we can avoid using named returns, we try to avoid them where possible.

Raffo · 2020-12-23T12:49:06Z

source/ambassador_host.go

+			continue
+		}
+
+		targets, err := sc.targetsFromAmbassadorLoadBalancer(ctx, service)


I'm also not sure if there is anything that can be done to get this. Do you have other suggestions @njuettner ?

kyle-co · 2021-01-05T21:17:20Z

This is definitely of interest to my cohorts and I. Any way I can help get this shipped?

donofriov · 2021-01-19T13:33:16Z

@Raffo @njuettner This is the last thing we need before we can enable our new production pipeline. Is there anything I can do to help this get merged this week? cc: @kflynn

springroll12 · 2021-01-19T13:41:01Z

@njuettner Our team would also love to see this land ASAP.

richarddli · 2021-01-19T19:17:15Z

@Raffo is there anyone other than @njuettner who can merge this? clearly there's a lot of interest and I can't help but notice that @njuettner really hasn't had any commits since June 2020.

(This PR has been open almost 6 months now.)

kflynn · 2021-01-19T19:55:33Z

@Raffo Related, is there a hard requirement for no named parameters, or would it be OK to land this and then fix that in a followup PR? (I'd prefer to land this one, then do a second one, simply for my own ease of testing.)

springroll12 · 2021-01-25T14:58:37Z

@Raffo Any movement on this?

Raffo · 2021-01-25T18:02:10Z

I can get this merged, but the fact that we can't address a minor comment is of course a bit of a concern in terms of how we can keep this code maintained. I'm going to re-review this this Wednesday when I go through the most urgent PRs.

kflynn · 2021-01-25T18:50:36Z

@Raffo That's actually why I was asking how strong your "I would prefer" was. 🙂 Let me see about addressing that...

kflynn · 2021-01-27T16:43:54Z

@Raffo OK, no more named returns in parseAmbLoadBalancerService -- and I made it more closely match Ambassador's historical semantics, added comments, and added a test for the parsing rules.

Ambassador can be configured with `Host` resources (based on the `Host` CRD) for defining the external DNS host name. This code adds a new source, `ambassador-host`, that looks for the `ambassador/ambassador` Service and and uses the `hostname` from the `Host` resource. Signed-off-by: Alvaro Saurin <alvaro.saurin@gmail.com> Signed-off-by: Flynn <flynn@datawire.io>

Raffo · 2021-01-27T22:12:54Z

Perfect, thanks for the update. Reviewing this first thing tomorrow morning.

Raffo · 2021-01-28T08:07:17Z

/approve
/lgtm

k8s-ci-robot · 2021-01-28T08:07:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: inercia, 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

kflynn · 2021-01-28T14:03:16Z

@Raffo Thanks!! 🙂

nikolay · 2021-02-02T20:03:14Z

@Raffo Any chance to have this released soon?

vsimon · 2021-02-12T19:30:25Z

Hi @Raffo @njuettner, I know you're busy but would you know when there might be a release bump that would include this?

FriedCircuits · 2021-05-04T18:29:41Z

Thanks for getting this released. I found it doesn't work when you are using ALBs with Ambassador because the LB info is on the ingress controller vs the ambassador service. I will have to continue adding all the DNS records we need on the ALB annotations.

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 23, 2020

k8s-ci-robot requested review from hjacobs and Raffo June 23, 2020 12:14

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 23, 2020

inercia force-pushed the inercia/ambassador_host branch from 2183db8 to e091ecc Compare June 23, 2020 13:13

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 23, 2020

inercia force-pushed the inercia/ambassador_host branch 2 times, most recently from f87651d to 7af7f26 Compare June 23, 2020 21:26

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 23, 2020

inercia force-pushed the inercia/ambassador_host branch 3 times, most recently from 30b6513 to dbdb1b2 Compare June 24, 2020 09:18

inercia changed the title ~~[WIP] Support Ambassador Host resources as sources~~ Support Ambassador Host resources as sources Jun 24, 2020

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 24, 2020

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 1, 2020

inercia force-pushed the inercia/ambassador_host branch from dbdb1b2 to 2ddae2a Compare July 6, 2020 15:15

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 6, 2020

inercia force-pushed the inercia/ambassador_host branch from 2ddae2a to abfdb3f Compare July 8, 2020 15:41

k8s-ci-robot assigned njuettner Jul 8, 2020

njuettner reviewed Jul 22, 2020

View reviewed changes

source/store.go Show resolved Hide resolved

source/ambassador_host.go Outdated Show resolved Hide resolved

source/ambassador_host.go Show resolved Hide resolved

source/ambassador_host.go Outdated Show resolved Hide resolved

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 29, 2020

inercia force-pushed the inercia/ambassador_host branch from abfdb3f to a21e8fd Compare August 5, 2020 14:41

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 5, 2020

richarddli mentioned this pull request Aug 5, 2020

Add support (might just be docs) for external DNS emissary-ingress/emissary#1057

Closed

inercia force-pushed the inercia/ambassador_host branch from a21e8fd to 78e0a5e Compare August 7, 2020 13:47

Raffo reviewed Dec 23, 2020

View reviewed changes

kflynn force-pushed the inercia/ambassador_host branch from df29d37 to 2b5309b Compare January 27, 2021 16:42

kflynn force-pushed the inercia/ambassador_host branch from 2b5309b to 6eeef96 Compare January 27, 2021 17:47

k8s-ci-robot assigned Raffo Jan 28, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 28, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 28, 2021

k8s-ci-robot merged commit 9759d6b into kubernetes-sigs:master Jan 28, 2021

vsimon mentioned this pull request Feb 17, 2021

Failing to sync cache for ambassador-host #1965

Closed

FriedCircuits mentioned this pull request Feb 19, 2021

[bitnami/external-dns] Ambassador RBAC Rules bitnami/charts#5565

Merged

2 tasks

jmjohnson-slalom mentioned this pull request May 7, 2021

Support Ambassador host resource as source #2075

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Ambassador Host resources as sources #1642

Support Ambassador Host resources as sources #1642

inercia commented Jun 23, 2020 •

edited

Loading

k8s-ci-robot commented Jun 23, 2020

inercia commented Jul 8, 2020

kflynn commented Dec 3, 2020

kflynn commented Dec 4, 2020

donofriov commented Dec 11, 2020

Raffo left a comment

Raffo Dec 23, 2020

kflynn Jan 27, 2021

Raffo Dec 23, 2020

kyle-co commented Jan 5, 2021

donofriov commented Jan 19, 2021

springroll12 commented Jan 19, 2021

richarddli commented Jan 19, 2021

kflynn commented Jan 19, 2021

springroll12 commented Jan 25, 2021

Raffo commented Jan 25, 2021

kflynn commented Jan 25, 2021

kflynn commented Jan 27, 2021

Raffo commented Jan 27, 2021

Raffo commented Jan 28, 2021

k8s-ci-robot commented Jan 28, 2021

kflynn commented Jan 28, 2021

nikolay commented Feb 2, 2021

vsimon commented Feb 12, 2021

FriedCircuits commented May 4, 2021

Support Ambassador Host resources as sources #1642

Support Ambassador Host resources as sources #1642

Conversation

inercia commented Jun 23, 2020 • edited Loading

k8s-ci-robot commented Jun 23, 2020

inercia commented Jul 8, 2020

kflynn commented Dec 3, 2020

kflynn commented Dec 4, 2020

donofriov commented Dec 11, 2020

Raffo left a comment

Choose a reason for hiding this comment

Raffo Dec 23, 2020

Choose a reason for hiding this comment

kflynn Jan 27, 2021

Choose a reason for hiding this comment

Raffo Dec 23, 2020

Choose a reason for hiding this comment

kyle-co commented Jan 5, 2021

donofriov commented Jan 19, 2021

springroll12 commented Jan 19, 2021

richarddli commented Jan 19, 2021

kflynn commented Jan 19, 2021

springroll12 commented Jan 25, 2021

Raffo commented Jan 25, 2021

kflynn commented Jan 25, 2021

kflynn commented Jan 27, 2021

Raffo commented Jan 27, 2021

Raffo commented Jan 28, 2021

k8s-ci-robot commented Jan 28, 2021

kflynn commented Jan 28, 2021

nikolay commented Feb 2, 2021

vsimon commented Feb 12, 2021

FriedCircuits commented May 4, 2021

inercia commented Jun 23, 2020 •

edited

Loading