Remove ExternalDNS contour ingress workaround #635

knrt10 · 2020-06-17T11:37:44Z

This PR closes #589. It has been tested.

Component Tested

ipochi

@knrt10 Thanks for the PR.

suggestion: please provide a descriptive commit message.

surajssd · 2020-06-18T07:49:02Z

@knrt10 Just one comment, we have a component httpbin for the same purpose you have deployed nginx.

surajssd · 2020-06-18T08:50:07Z

@knrt10 just a small spell mistake in the commit message:

-Due to upstream issue in contour, address fiels was not setting on
-ingress resource. We introduces a workaround to solve that issue.
+Due to upstream issue in contour, address fields were not setting on
+ingress resource. We introduced a workaround to solve that issue.

knrt10 · 2020-06-18T08:51:43Z

@surajssd applied the httpbin component too. You can verify now curl dex.kautilya.dev.lokomotive-k8s.net

knrt10 · 2020-06-18T08:54:29Z

Removing cluster now

ipochi

thanks for the PR @knrt10

There are a couple of things that needs to be taken care of:

Since we remove the ingress_hosts from contour configuration, creation of DNS entries has become a manual process, which we want external-dns to pick up.

Update Ingress manifests for Dex and Gangway with annotation
external-dns.alpha.kubernetes.io/hostname:.

The value of the annotation will be IngressHost field of dex and gangway i.e:
external-dns.alpha.kubernetes.io/hostname: {{ .IngressHost }}.

Also we need to change external-dns component configuration:

Instead of default source = ["service"] , it should be source=["ingress"]

This way when you deploy dex and gangway, external-dns will pick up the hosts in Ingress objects and create all those DNS entries.

knrt10 · 2020-06-18T10:56:42Z

@ipochi updated. Please review

ipochi

suggestion: any changes related to dex and gangway should be part of another commit.

knrt10 · 2020-06-18T11:09:45Z

Updated

ipochi · 2020-06-22T08:35:23Z

assets/components/contour/templates/02-service-envoy.yaml

@@ -12,9 +12,6 @@ metadata:
    # for information about enabling the PROXY protocol on the ELB to recover
    # the original remote IP address.
    service.beta.kubernetes.io/aws-load-balancer-backend-protocol: tcp
-    {{- if .Values.ingressHosts }}
-    external-dns.alpha.kubernetes.io/hostname: '{{- join "," .Values.ingressHosts }}'


I have some doubts whether there should be a field in contour configuration that would create a DNS entry for the envoy service. The new field in contour would act as the value of the external-dns annotation.

Not sure if this is necessary, would like others to chime in.

@iaguis

invidian

Do we have tests for this functionality? Perhaps we could add some to ensure, that this change set is working as expected?

pkg/components/dex/component.go

docs/how-to-guides/authentication-with-dex-gangway.md

knrt10 · 2020-06-25T13:58:29Z

@invidian what tests do you suggest for this?

invidian · 2020-06-25T14:27:24Z

@invidian what tests do you suggest for this?

We should test, that the ingress host (either newly created one as part of testing or one from other component) resolves to the IP address in Ingress object.

knrt10 · 2020-06-26T10:42:33Z

@invidian can you please review

invidian

Thanks @knrt10 for adding tests, the implementation looks almost correct to me. I added some comments what needs to be changed, but nothing major. Overall things looks good, nice work :) Please have a look.

Also note, that currently the CI do not gets triggered, so please run tests locally to make sure things are working as expected, until we fix the CI.

test/ingress/packet_test/match_host_test.go

knrt10 · 2020-06-29T06:27:41Z

Thanks @invidian for reviewing. Updating the PR

knrt10 · 2020-06-29T09:00:16Z

e2e tests works fine.

Due to upstream issue in contour, address fields were not setting on ingress resource. We introduced a workaround to solve that issue. Previously we were explicitly using `IngressHosts` to work with external-dns. Now since the upstream issue has been fixed in contour we have removed optional field `IngressHosts`. See projectcontour/contour#403 and 71c19e0 Signed-off-by: knrt10 <kautilya@kinvolk.io>

Since `ingress_hosts` from contour configuration is removed, now we have added a new annotation for external-dns to pickup and add it automatically to DNS entries. external-dns default source is changes to `ingress`. Signed-off-by: knrt10 <kautilya@kinvolk.io>

test/ingress/packet_test/match_host_test.go

invidian

LGTM, thanks @knrt10 for addressing the review comments in timely manner. I'd wait with merging until we fix the CI though, to make sure tests also work there.

PR fixed

knrt10 · 2020-06-30T01:27:12Z

@invidian can you please approve, stale review got removed on latest push to check CI

Signed-off-by: knrt10 <kautilya@kinvolk.io>

invidian · 2020-06-30T05:17:55Z

@knrt10 I pushed again last commit to trigger the CI one more time, as while doing some other tests I unpaused PacketARM pipeline, which is going to fail.

knrt10 · 2020-06-30T08:03:44Z

@invidian PacketARM did not get trigerred.

ipochi

LGTM.

Thanks and good work @knrt10

knrt10 requested review from invidian, ipochi, johananl, pothos and surajssd as code owners June 17, 2020 11:37

ipochi suggested changes Jun 17, 2020

View reviewed changes

knrt10 force-pushed the knrt10/fix-contour-ingress-workaround branch 3 times, most recently from 4436f05 to 17ac285 Compare June 17, 2020 12:28

knrt10 force-pushed the knrt10/fix-contour-ingress-workaround branch 2 times, most recently from babaa68 to e805e58 Compare June 18, 2020 08:39

knrt10 force-pushed the knrt10/fix-contour-ingress-workaround branch 2 times, most recently from f4d5702 to 2899f1b Compare June 18, 2020 09:14

ipochi suggested changes Jun 18, 2020

View reviewed changes

knrt10 force-pushed the knrt10/fix-contour-ingress-workaround branch from 2899f1b to b676c29 Compare June 18, 2020 10:55

ipochi previously requested changes Jun 18, 2020

View reviewed changes

knrt10 force-pushed the knrt10/fix-contour-ingress-workaround branch from b676c29 to 3593fb4 Compare June 18, 2020 11:08

ipochi reviewed Jun 22, 2020

View reviewed changes

invidian suggested changes Jun 22, 2020

View reviewed changes

pkg/components/dex/component.go Outdated Show resolved Hide resolved

docs/how-to-guides/authentication-with-dex-gangway.md Show resolved Hide resolved

knrt10 force-pushed the knrt10/fix-contour-ingress-workaround branch 2 times, most recently from 00a5066 to 0b8adc0 Compare June 22, 2020 13:29

knrt10 force-pushed the knrt10/fix-contour-ingress-workaround branch from 0b8adc0 to d1e3b6c Compare June 26, 2020 10:32

knrt10 force-pushed the knrt10/fix-contour-ingress-workaround branch 3 times, most recently from bf8c21f to 4cdf7c8 Compare June 28, 2020 07:25

invidian suggested changes Jun 29, 2020

View reviewed changes

test/ingress/packet_test/match_host_test.go Outdated Show resolved Hide resolved

test/ingress/packet_test/match_host_test.go Outdated Show resolved Hide resolved

test/ingress/packet_test/match_host_test.go Show resolved Hide resolved

knrt10 force-pushed the knrt10/fix-contour-ingress-workaround branch from 4cdf7c8 to 89273cd Compare June 29, 2020 08:59

knrt10 requested a review from invidian June 29, 2020 08:59

knrt10 force-pushed the knrt10/fix-contour-ingress-workaround branch from 89273cd to 0c5283b Compare June 29, 2020 09:01

knrt10 added 2 commits June 29, 2020 14:32

invidian reviewed Jun 29, 2020

View reviewed changes

test/ingress/packet_test/match_host_test.go Outdated Show resolved Hide resolved

knrt10 force-pushed the knrt10/fix-contour-ingress-workaround branch from 0c5283b to 2306e5e Compare June 29, 2020 11:30

knrt10 requested a review from invidian June 29, 2020 11:30

invidian previously approved these changes Jun 29, 2020

View reviewed changes

knrt10 dismissed invidian’s stale review via 82fb910 June 29, 2020 21:22

knrt10 force-pushed the knrt10/fix-contour-ingress-workaround branch from 2306e5e to 82fb910 Compare June 29, 2020 21:22

invidian previously approved these changes Jun 30, 2020

View reviewed changes

Add test for ingress host matching

57d9314

Signed-off-by: knrt10 <kautilya@kinvolk.io>

invidian dismissed their stale review via 57d9314 June 30, 2020 05:17

invidian force-pushed the knrt10/fix-contour-ingress-workaround branch from 82fb910 to 57d9314 Compare June 30, 2020 05:17

invidian approved these changes Jun 30, 2020

View reviewed changes

ipochi approved these changes Jun 30, 2020

View reviewed changes

knrt10 merged commit b54382f into master Jun 30, 2020

knrt10 deleted the knrt10/fix-contour-ingress-workaround branch June 30, 2020 08:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove ExternalDNS contour ingress workaround #635

Remove ExternalDNS contour ingress workaround #635

knrt10 commented Jun 17, 2020 •

edited

Loading

ipochi left a comment

surajssd commented Jun 18, 2020

surajssd commented Jun 18, 2020

knrt10 commented Jun 18, 2020 •

edited

Loading

knrt10 commented Jun 18, 2020

ipochi left a comment •

edited

Loading

knrt10 commented Jun 18, 2020

ipochi left a comment

knrt10 commented Jun 18, 2020

ipochi Jun 22, 2020

invidian left a comment

knrt10 commented Jun 25, 2020

invidian commented Jun 25, 2020

knrt10 commented Jun 26, 2020

invidian left a comment

knrt10 commented Jun 29, 2020

knrt10 commented Jun 29, 2020

invidian left a comment

knrt10 commented Jun 30, 2020

invidian commented Jun 30, 2020

knrt10 commented Jun 30, 2020

ipochi left a comment

Remove ExternalDNS contour ingress workaround #635

Remove ExternalDNS contour ingress workaround #635

Conversation

knrt10 commented Jun 17, 2020 • edited Loading

Component Tested

ipochi left a comment

Choose a reason for hiding this comment

surajssd commented Jun 18, 2020

surajssd commented Jun 18, 2020

knrt10 commented Jun 18, 2020 • edited Loading

knrt10 commented Jun 18, 2020

ipochi left a comment • edited Loading

Choose a reason for hiding this comment

knrt10 commented Jun 18, 2020

ipochi left a comment

Choose a reason for hiding this comment

knrt10 commented Jun 18, 2020

ipochi Jun 22, 2020

Choose a reason for hiding this comment

invidian left a comment

Choose a reason for hiding this comment

knrt10 commented Jun 25, 2020

invidian commented Jun 25, 2020

knrt10 commented Jun 26, 2020

invidian left a comment

Choose a reason for hiding this comment

knrt10 commented Jun 29, 2020

knrt10 commented Jun 29, 2020

invidian left a comment

Choose a reason for hiding this comment

knrt10 commented Jun 30, 2020

invidian commented Jun 30, 2020

knrt10 commented Jun 30, 2020

ipochi left a comment

Choose a reason for hiding this comment

knrt10 commented Jun 17, 2020 •

edited

Loading

knrt10 commented Jun 18, 2020 •

edited

Loading

ipochi left a comment •

edited

Loading