Manage routing wildcard domain #59

ironcladlou · 2018-11-09T17:01:41Z

Add routing wildcard DNS management for AWS. This should be replaced ASAP by something more generalized (e.g. external-dns).

The records are managed in a way that's compatible with the installer and are correctly cleaned up during a cluster destroy.

Does not yet handle updates or deletes.

ironcladlou · 2018-11-09T17:01:50Z

/hold

Miciah · 2018-11-09T17:42:47Z

pkg/manifests/manifests.go

@@ -49,7 +49,7 @@ func (f *Factory) DefaultClusterIngress(ic *util.InstallConfig) (*ingressv1alpha
 	if err != nil {
 		return nil, err
 	}
-	ingressDomain := fmt.Sprintf("%s.%s", ic.Metadata.Name, ic.BaseDomain)
+	ingressDomain := fmt.Sprintf("*.apps.%s.%s", ic.Metadata.Name, ic.BaseDomain)


This will break TestDefaultClusterIngress. Also, do we want to publish the "*." in the canonical name in routes?

No, canonical name is the name of the DNS wildcard root domain and API consumers should assume that it is ("%s.%s", myName, canonicalHostname). I.e. canonical should always be reported as a.b.c and consumers must assume their name is d.a.b.c

So we should add the "*." in ensureDNSForLoadBalancer. Not sure about the "apps." though—probably should add "apps." here and adjust TestDefaultClusterIngress to keep it passing.

We don't yet have any cluster config for the app domain, so I'm just making things up here. Do agree that the *. should be prefixed at runtime in ensureDNSForLoadBalancer().

ironcladlou · 2018-11-09T18:21:51Z

Extracted #60 and #61. Will rebase this soon.

ironcladlou · 2018-11-09T21:42:27Z

@smarterclayton @abhinavdahiya thoughts on how we can approach hosted zone config/discovery here? We need to know:

The zone in which the alias should live
The zone of the alias target

I'm not entirely clear how 1 is managed in the first place, and for 2 all we have to start with is the hostname attached to the Service status.

abhinavdahiya · 2018-11-09T22:14:39Z

here's how hive deletes r53 zones https://github.com/openshift/hive/tree/master/contrib/pkg/awstagdeprovision#L1366-L1421

smarterclayton · 2018-11-09T23:45:45Z

It should be the same zone as the apiserver masters unless we have a really clever use case. Or we just make the installer create it. On Nov 9, 2018, at 4:42 PM, Dan Mace <notifications@github.com> wrote: @smarterclayton <https://github.com/smarterclayton> @abhinavdahiya <https://github.com/abhinavdahiya> thoughts on how we can approach hosted zone config/discovery here? We need to know: 1. The zone in which the alias should live 2. The zone of the alias target I'm not entirely clear how 1 is managed in the first place, and for 2 all we have to start with is the hostname attached to the Service status. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#59 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_p2wcssdn-Y-4UaUUny_58d-_sbOZks5utfbEgaJpZM4YW8ay> .

ironcladlou · 2018-11-13T22:41:02Z

Did some more work on this to auto-discover all the inputs... probably worth another review at this point before I go any further. PTAL @openshift/sig-network-edge @smarterclayton

ironcladlou · 2018-11-14T01:26:58Z

@dgoodwin @joelddiaz @smarterclayton @abhinavdahiya

One cleanup problem here I just thought of. The hive DNS cleanup algorithm is basically to delete public zone records which match the cluster's private zone records. So, to get hive to clean up public zone A records created by the ingress operator, I think matching private zone records would also need to also be created. I don't have the private hosted zone ID in cluster config, and all clusters' zones share a domain name, leaving me with some less than ideal lookup options (e.g. "find the private zone containing an A record for $cluster_name-api .$basedomain").

Thoughts?

smarterclayton · 2018-11-14T03:15:38Z

pkg/stub/handler.go

@@ -242,6 +242,12 @@ func (h *Handler) ensureRouterForIngress(ci *ingressv1alpha1.ClusterIngress) err
 				} else if !errors.IsAlreadyExists(err) {
 					return fmt.Errorf("failed to create router service %s/%s: %v", service.Namespace, service.Name, err)
 				}
+				if ci.Spec.IngressDomain != nil {
+					err = h.ensureDNSForLoadBalancer(ci, service)


we definitely need to think about how to rate limit how often this can be ensured (i.e. should we have a rate limiter that requeues us when the limit expires, should we have a periodic timer, etc).

smarterclayton · 2018-11-14T03:17:03Z

pkg/stub/dns.go

+	creds := credentials.NewStaticCredentials(string(awsCreds.Data["aws_access_key_id"]), string(awsCreds.Data["aws_secret_access_key"]), "")
+	sess, err := session.NewSession(&aws.Config{
+		Credentials: creds,
+		Region:      aws.String(h.InstallConfig.Platform.AWS.Region),


TODO here to pull this from the Infrastructure config, not InstallConfig.

joelddiaz · 2018-11-14T14:15:22Z

The private zones do get some tagging applied to them (eg tectonicClusterID)... Not sure how helpful that is (or whether we want to embed this kind of tag lookup in the operator/controllers).

I'd feel more comfortable with the tags if the hosted zones were being tagged with the kubernetes.io/cluster/$clustername tag. There's an old discussion on this topic here openshift/installer#458

The uninstall code can also be updated to not require the matching private zone, but the question then becomes how would it unambiguously identify the records that need removal in the shared public zone?

ironcladlou · 2018-11-14T16:53:02Z

Thanks, @joelddiaz. I didn't realize the zone was tagged. I think lookup using the tectonicClusterID tag on the private zone and creating a private zone record will have to be good enough for now.

ironcladlou · 2018-11-16T16:09:42Z

@openshift/sig-network-edge @smarterclayton okay, I think this is ready for near final review... I've been using some uncommitted tests for it; we probably need some e2e coverage.

Overall I'm extremely uncomfortable maintaining this code and it's probably unsustainable beyond like 2 platforms (aws/gcp), but I'm hoping it's good enough to keep us moving...

ironcladlou · 2018-11-16T19:07:08Z

I added some cheesy logic to cache updates operator-side to stop spamming AWS with UPSERT calls. Can come up with a better way later.

ironcladlou · 2018-11-16T19:57:05Z

@smarterclayton

error: error reading /tmp/cluster/telemeter-token: no such file or directory
error: no objects passed to apply
deployment.apps/telemeter-client created
/usr/bin/openshift-tests
which: no extended.test in (/usr/libexec/origin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin)
must provide TEST_SUITE variable

Looks like something's up with our job

ironcladlou · 2018-11-16T20:13:21Z

/hold cancel

ironcladlou · 2018-11-16T20:14:17Z

@csrwng you might want to review what we're doing here as part of the external-dns use case analysis.

csrwng · 2018-11-16T20:36:49Z

pkg/dns/aws/dns.go

+	// Find the target hosted zone of the load balancer attached to the service.
+	// TODO: cache it?
+	var targetHostedZoneID string
+	loadBalancers, err := m.ELB.DescribeLoadBalancers(&elb.DescribeLoadBalancersInput{})


You'll need the elbv2 version of this call to catch the new network load balancers we're using

Can you explain further? I'm having no issues w/ the ELBs created by the k8s cloud provider for our services.

ahh, sorry I was thinking of the nlbs that get created by the installer for masters, but those are not the ones created by the cloud provider.

csrwng · 2018-11-16T20:38:24Z

pkg/dns/aws/dns.go

+}
+
+// findClusterPublicZone finds the public zone given the base domain.
+func (m *Manager) findClusterPublicZone() *route53.HostedZone {


I was hoping to send something like this upstream to external-dns ... add the ability to lookup zones by tags. In the meantime, the code will live in the operator.

csrwng · 2018-11-16T20:40:29Z

pkg/dns/aws/dns.go

+	// Create or update an alias from the wildcard domain to the service load
+	// balancer hostname in both zones.
+	// TODO: only make this call if the records don't exist.
+	updateRecord := func(zoneID string) error {


This is the part that external-dns can do for you right now. Btw, if I set the --registry=noop flag, I don't have it create the ugly TXT records.

ironcladlou · 2018-11-16T20:53:55Z

P.S., current code won't handle deletes or updates to the ClusterIngress. Can address that in a followup; need to get some sort of e2e coverage on this.

ironcladlou · 2018-11-16T20:56:41Z

/retest

Miciah · 2018-11-16T21:20:50Z

cmd/cluster-ingress-operator/main.go

-		Namespace:       namespace,
-		ManifestFactory: manifests.NewFactory(),
-	}
+	handler, err := createHandler(namespace)


Should check err.

Miciah · 2018-11-16T21:21:59Z

cmd/cluster-ingress-operator/main.go

+
+	ic, err := util.GetInstallConfig(kubeClient)
+	if err != nil {
+		return nil, fmt.Errorf("could't get installconfig: %v", err)


Typo: "coudn't" (granted, it was already present before your refactoring).

Miciah · 2018-11-16T21:23:49Z

cmd/cluster-ingress-operator/main.go

+		return nil, fmt.Errorf("Failed to get cvoClient: %v", err)
+	}
+
+	ic, err := util.GetInstallConfig(kubeClient)


Why separate declaration of kubeClient from use? Really, because there is only one use, we could do away with the variable entirely.

Miciah · 2018-11-16T21:34:27Z

pkg/dns/aws/dns.go

+// records Manager creates in the public zone will also be created in the
+// private zone to ensure public zone records for the cluster can be identified
+// and cleaned up later.
+type Manager struct {


The names seem backwards: in the aws package, you define Manager, so we have aws.Manager, and in the dns package, you define DNSManager, so we have dns.DNSManager. It would make better sense to have aws.DNSManager and dns.Manager.

Alternatively, if we anticipate that the aws package will be imported as "awsdns" and if it doesn't make the code too confusing, we could change the name only in the aws package so that we will have dns.Manager and awsdns.Manager.

Good thoughts... I went with option 2 (dns.Manager and awsdns.Manager).

Miciah · 2018-11-16T21:35:59Z

pkg/dns/aws/dns.go

+// and cleaned up later.
+type Manager struct {
+	ELB     *elb.ELB
+	Route53 *route53.Route53


Why are these two fields public?

Miciah · 2018-11-16T22:39:54Z

cmd/cluster-ingress-operator/main.go

 	sdk.Handle(handler)
 	sdk.Run(context.TODO())
 }
+
+func createHandler(namespace string) (*stub.Handler, error) {


This should be moved to pkg/stub/handler.go and maybe renamed to "NewHandler".

I almost didn't even refactor this -- I think we have more to think about here in terms of setup. Okay to revisit after the sdk upgrade?

Sure, it's only a suggestion.

Miciah · 2018-11-16T22:44:11Z

manifests/01-kube-system-aws-creds-role-binding.yaml

@@ -0,0 +1,14 @@
+# Binds the operator role to its Service Account.


s/operator role to its/aws-creds-secret-reader role to the operator's/

Miciah · 2018-11-16T22:49:15Z

pkg/dns/aws/dns.go

+// topology established by the OpenShift installer. Specifically, this implies:
+//
+// 1. A public zone shared by all clusters with <domain-name>
+// 2. A private zone for the cluster with <domain-name>


Instead of "<domain-name>", I think it would be clearer to say "domain name equal to the BaseDomain".

Miciah · 2018-11-16T23:01:01Z

pkg/stub/handler.go

+// ensureDNSForLoadBalancer configures a wildcard DNS alias for a ClusterIngress
+// targeting the given service.
+func (h *Handler) ensureDNSForLoadBalancer(ci *ingressv1alpha1.ClusterIngress, service *corev1.Service) error {
+	// Ensure DNS is configured for the load balancer.


This comment doesn't seem helpful.

Miciah · 2018-11-16T23:06:38Z

cmd/cluster-ingress-operator/main.go

+			},
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "aws-creds",
+				Namespace: "kube-system",


Could use metav1.NamespaceSystem here.

Miciah · 2018-11-16T23:43:28Z

cmd/cluster-ingress-operator/main.go

+
+	var dnsManager dns.Manager
+	switch {
+	case ic.Platform.AWS != nil:


We could have an InstallConfig with a nil Platform, so we should check that here.

Platform is a value type, can't be nil

Ah, right, sorry. Never mind!

Miciah · 2018-11-16T23:49:34Z

cmd/cluster-ingress-operator/main.go

+			AccessID:   string(awsCreds.Data["aws_access_key_id"]),
+			AccessKey:  string(awsCreds.Data["aws_secret_access_key"]),
+			Region:     ic.Platform.AWS.Region,
+			BaseDomain: ic.BaseDomain + ".",


As far as I can tell, BaseDomain is allowed to have a final dot, so it might be prudent to do a TrimSuffix to avoid ending up with two dots.

Miciah · 2018-11-16T23:50:59Z

pkg/dns/aws/dns.go

+
+var _ dns.Manager = &Manager{}
+
+// Manager is a DNSManager for AWS which is tightly coupled to the DNS zone


I guess that should be "is a dns.Manager" now.

Revised comment

Miciah · 2018-11-16T23:54:41Z

pkg/dns/aws/dns.go

+		},
+	})
+	if err != nil {
+		return fmt.Errorf("couldn't update DNS record for in zone %s: %v", zoneID, err)


You have two prepositions and only one prepositional object: "for in zone %s".

Add routing wildcard DNS management for AWS. This should be replaced ASAP by something more generalized (e.g. external-dns). The records are managed in a way that's compatible with the installer and are correctly cleaned up during a cluster destroy. *Does not yet handle updates or deletes.*

ironcladlou · 2018-11-17T01:13:39Z

/retest

ironcladlou · 2018-11-17T13:12:07Z

/retest

Miciah · 2018-11-17T14:55:24Z

Are the changes under hack/ intended to be included in this PR? If so, let me know, and I'll go ahead and tag this PR.

ironcladlou · 2018-11-19T14:07:51Z

Yeah, the hack/ changes are just some bonus fixes/features

Miciah · 2018-11-19T14:12:47Z

/lgtm

Miciah · 2018-11-19T14:55:27Z

/lgtm

openshift-ci-robot · 2018-11-19T14:55:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ironcladlou, Miciah

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~~ [ironcladlou]

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

ironcladlou · 2018-11-19T16:19:37Z

/retest

openshift-bot · 2018-11-19T17:43:36Z

/retest

Please review the full test history for this PR and help us cut down flakes.

Miciah · 2018-11-19T19:35:32Z

/retest

openshift-bot · 2018-11-19T21:45:59Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Nov 9, 2018

openshift-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Nov 9, 2018

openshift-ci-robot requested review from imcsk8 and pravisankar November 9, 2018 17:01

Miciah reviewed Nov 9, 2018

View reviewed changes

ironcladlou force-pushed the external-dns branch from d7a32ae to 008f4fd Compare November 9, 2018 21:30

ironcladlou force-pushed the external-dns branch 2 times, most recently from 03443f3 to 1ad40b3 Compare November 13, 2018 22:36

smarterclayton reviewed Nov 14, 2018

View reviewed changes

Add aws-sdk-go dependency

a315b9b

ironcladlou force-pushed the external-dns branch from 1ad40b3 to b02d234 Compare November 16, 2018 16:06

ironcladlou force-pushed the external-dns branch 2 times, most recently from fc7292e to 37601fb Compare November 16, 2018 19:06

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 16, 2018

csrwng reviewed Nov 16, 2018

View reviewed changes

Miciah reviewed Nov 16, 2018

View reviewed changes

ironcladlou force-pushed the external-dns branch 3 times, most recently from 2fe56f3 to 8c13306 Compare November 16, 2018 23:36

Miciah reviewed Nov 16, 2018

View reviewed changes

ironcladlou force-pushed the external-dns branch from 8c13306 to 2b9a2f6 Compare November 17, 2018 00:34

openshift-ci-robot assigned Miciah Nov 19, 2018

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 19, 2018

openshift-merge-robot merged commit 9475264 into openshift:master Nov 19, 2018

Miciah mentioned this pull request Jun 27, 2023

NE-1129: Make ingress operator optional on HyperShift openshift/enhancements#1415

Merged

		@@ -0,0 +1,14 @@
		# Binds the operator role to its Service Account.


		var _ dns.Manager = &Manager{}

		// Manager is a DNSManager for AWS which is tightly coupled to the DNS zone

Manage routing wildcard domain #59

Manage routing wildcard domain #59

Conversation

ironcladlou commented Nov 9, 2018 • edited Loading

ironcladlou commented Nov 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ironcladlou commented Nov 9, 2018

ironcladlou commented Nov 9, 2018

abhinavdahiya commented Nov 9, 2018

smarterclayton commented Nov 9, 2018 via email

ironcladlou commented Nov 13, 2018

ironcladlou commented Nov 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joelddiaz commented Nov 14, 2018

ironcladlou commented Nov 14, 2018

ironcladlou commented Nov 16, 2018

ironcladlou commented Nov 16, 2018

ironcladlou commented Nov 16, 2018

ironcladlou commented Nov 16, 2018

ironcladlou commented Nov 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ironcladlou commented Nov 16, 2018

ironcladlou commented Nov 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ironcladlou commented Nov 17, 2018

ironcladlou commented Nov 17, 2018

Miciah commented Nov 17, 2018

ironcladlou commented Nov 19, 2018

Miciah commented Nov 19, 2018

Miciah commented Nov 19, 2018

openshift-ci-robot commented Nov 19, 2018

ironcladlou commented Nov 19, 2018

openshift-bot commented Nov 19, 2018

Miciah commented Nov 19, 2018

openshift-bot commented Nov 19, 2018

ironcladlou commented Nov 9, 2018 •

edited

Loading