Add ExternalDNS Operator Enhancement Proposal #786

sgreene570 · 2021-05-18T14:54:11Z

This PR revives and expands upon the original ExternalDNS Operator enhancement (#456).

This is in support of https://issues.redhat.com/browse/NE-303

sgreene570 · 2021-05-21T14:17:44Z

/assign @Miciah @danehans @knobunc
/cc @rfredette @candita @frobware @miheer @alebedev87

alebedev87

Overall LGTM, just some questions.

alebedev87 · 2021-06-11T19:03:41Z

enhancements/dns/externaldns-operator.md

+surface (common fields on relevant ExternalDNS CRDs, consistent labeling of created resources, etc.), yet is easy to build.
+* Build an operator that is suitable for production use of ExternalDNS.
+* Focus on supporting the OpenShift Route resource as a source for ExternalDNS.
+* Additionally support the Kubernetes [service source](https://github.com/kubernetes-sigs/external-dns/blob/master/source/service.go) (namely LoadBalancer services)


Aren't services of type LoadBalancer already supported by ExternalDNS? FAQ

Right, the initial ExternalDNS Operator will only support a subset of the source resources supported by ExternalDNS.

sgreene570 · 2021-06-11T19:51:48Z

enhancements/dns/externaldns-operator.md

+
+* Create a GitHub repository to host the operator source code in OpenShift's GitHub org,
+with the hopes of moving the operator's source code into an upstream k8s-sigs repository in the future.
+* Leverage popular frameworks and libraries, e.g. controller-runtime and kubebuilder, to simplify development


Good question. We polled the openshift networking team, and took @danehans 's experience with Contour operator into account when we decided to go forward with kubebuilder + controller-runtime, as opposed to Operator SDK. Kubebuilder + controller-runtime seems to be the more popular approach upstream.

enhancements/dns/externaldns-operator.md

+of the operator and align with upstream communities.
+* Manage dependencies through [Go Modules](https://blog.golang.org/using-go-modules).
+* Create user and developer documentation for running ExternalDNS operator (on both OpenShift and vanilla Kubernetes).
+* Create tooling to simplify building, running, testing, etc. the operator.


alebedev87 · 2021-06-11T19:40:25Z

enhancements/dns/externaldns-operator.md

+proper label selector logic for all source types (instead of label selector based annotations), work may need to be done in the upstream ExternalDNS
+community accordingly.
+
+As a side note, ExternalDNS deployments will be configured to run ExternalDNS pods on cluster control-plane nodes.


Maybe a small explanation why on the control plane nodes?

See Dane's prior comment:

I think it's common practice for add-on control-plane functionality to use the node roles to run on master nodes only by default.

I'll elaborate in the EP. Thanks!

seanmalloy · 2021-06-14T15:32:57Z

@vinny-sabatini @seegeekrow @sanbornick @trzejos @jaideepbellani please review when you have some time. Thanks!

Miciah · 2021-06-15T21:46:33Z

enhancements/dns/externaldns-operator.md

+    - openshift-route:
+      ignoreHostnameAnnotation: "true"
+      labelSelector:
+        foo: bar
+    - CRD
+      resource: dnsrecords
+    - service:
+      namespace: foo
+      publishInternal: "true"


This will probably change anyway, but syntactically, the union type subfields need to be indented. Also, you're missing a colon (but so was my earlier suggestion—oops!), and the field names should be more jsonny/yamlish.

Suggested change

- openshift-route:

ignoreHostnameAnnotation: "true"

labelSelector:

foo: bar

- CRD

resource: dnsrecords

- service:

namespace: foo

publishInternal: "true"

- openshiftRoute:

ignoreHostnameAnnotation: "true"

labelSelector:

foo: bar

- crd:

resource: dnsrecords

- service:

namespace: foo

publishInternal: "true"

Great catch. Fixed! (for now, at least 😁 )

Miciah · 2021-06-15T21:54:16Z

enhancements/dns/externaldns-operator.md

+* Expand upon the existing OpenShift [ingressoperator/dnsrecord](https://github.com/openshift/api/blob/master/operatoringress/v1/types.go)
+CRD to implement the ExternalDNS base [CRD source](https://github.com/kubernetes-sigs/external-dns/blob/master/docs/contributing/crd-source.md).


We moved this to non-goals (for 4.9), so we need to be clear here that we are not committing to making this change as part of this specific enhancement.

Sure. I added a note here that this is work for post 4.9.

Miciah · 2021-06-15T22:01:00Z

enhancements/dns/externaldns-operator.md

+[tutorial section of the ExternalDNS docs](https://github.com/kubernetes-sigs/external-dns/blob/master/docs/tutorials).
+In general, ExternalDNS needs proper permissions to view, update, and delete DNS resource records in the relevant DNS zones.
+
+Support for OpenShift clusters running AWS that


Can we just say "Support for AWS clusters that use STS"? The CredentialsRequest API is OpenShift-specific, but given that we'll need some way to provide credentials on vanilla Kubernetes, does support for STS need to be OpenShift-specific?

Right, we can. Fixed.

seanmalloy · 2021-06-15T22:04:33Z

enhancements/dns/externaldns-operator.md

+in a cluster, as well as for cluster administrators who want to verify that their requested resources have been created properly.
+
+Currently, the upstream ExternalDNS CRD source only supports record creation for A or CNAME type records. See
+[upstream issue 164](https://github.com/kubernetes-sigs/external-dns/issues/164) for more context to that end. Contributions may need to be made to


Upstream issue 164 seems to be related to leader election. I suspect you are referencing the wrong issue here.

Looks like I accidentally dropped the 7 from kubernetes-sigs/external-dns#1647.

Fixed. Thanks!

Miciah · 2021-06-15T22:18:40Z

enhancements/dns/externaldns-operator.md

+source-specific parameters. In this example, ExternalDNS would create DNS Records for Route resources that match the label (read: annotation, more on this later)
+`foo: bar`, and would ignore the ExternalDNS Host Target Override Annotation (See [Implementation Details](#implementation-details) for more details about ExternalDNS
+annotations). This example would also instruct ExternalDNS to create DNS Records for CR instances of type `dnsrecords` (not a part of OCP 4.9 implementation).
+Finally, this example would instruct ExternalDNS to create DNS Records in the given Private zone for Service resource instances in namespace `foo`.


"Records", "See", and "Private" can be lowercase. My memory is bad, and by the time I reach this paragraph, the example has scrolled off the screen, and I've figuratively and literally lost sight of it; two things could help: (1) an introductory sentence at the start of this section, such as, "The following example CR will be used throughout this section to explain the proposed CRD", and (2) a gentle reminder of where the example is when referencing it.

Suggested change

source-specific parameters. In this example, ExternalDNS would create DNS Records for Route resources that match the label (read: annotation, more on this later)

`foo: bar`, and would ignore the ExternalDNS Host Target Override Annotation (See [Implementation Details](#implementation-details) for more details about ExternalDNS

annotations). This example would also instruct ExternalDNS to create DNS Records for CR instances of type `dnsrecords` (not a part of OCP 4.9 implementation).

Finally, this example would instruct ExternalDNS to create DNS Records in the given Private zone for Service resource instances in namespace `foo`.

source-specific parameters. For the example ExternalDNS CR above, the ExternalDNS Operator would create DNS records for Route resources that match the label (read: annotation, more on this later)

`foo: bar`, and would ignore the ExternalDNS Host Target Override Annotation (see [Implementation Details](#implementation-details) for more details about ExternalDNS

annotations). This example would also instruct ExternalDNS to create DNS records for CR instances of type `dnsrecords` (not a part of OCP 4.9 implementation).

Finally, this example would instruct ExternalDNS to create DNS records in the given private zone for Service resource instances in namespace `foo`.

By the way, do you prefer "the ExternalDNS operator" (because that's what it is), "the ExternalDNS Operator", or "ExternalDNS Operator" (without an article because we're using it as a name)?

Great suggestions! I've gone ahead and settled on the ExternalDNS operator. I've also fixed all of the typos you highlighted, as well as added the requested details from (1) and (2). Thanks!

This commit contains the original externalDNS operator enhancement proposal create by Daneyon Hansen as a part of openshift#456 (which was closed due to inactivity).

This commit relocates and expands upon the original ExternalDNS Operator enhancement. This commit is in support of https://issues.redhat.com/browse/NE-303.

Miciah · 2021-06-16T16:10:52Z

Thanks! Let's merge and iterate.
/lgtm

Miciah · 2021-06-16T16:12:41Z

/approve

openshift-ci · 2021-06-16T16:13:00Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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~~ [Miciah]

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

sgreene570 · 2021-06-16T16:16:53Z

We are up to 200 comments so we decided to merge this enhancement as is and create follow up PRs as needed. Thanks for the input all! Please feel free to comment here or reach out to me offline with any further comments.

seanmalloy · 2021-06-16T17:06:09Z

@sgreene570 I reviewed the proposal and it looks pretty good. I have two point below that you may want to consider in the future.

For excluding domains external-dns provides several different options. I'm leaning towards using --regex-domain-filter or --regex-domain-exclusion, but we haven't tested these yet in our environment. What I want to avoid is having to list out a large list of domains to filter. Also, in our environment the list of sub-domains may change, so I don't want to keep the list updated. I'm hoping one of the regex options accomplish this.

  --domain-filter= ...           Limit possible target zones by a domain suffix; specify multiple times for multiple domains (optional)
  --exclude-domains= ...         Exclude subdomains (optional)
  --regex-domain-filter=         Limit possible domains and target zones by a Regex filter; Overrides domain-filter (optional)
  --regex-domain-exclusion=      Regex filter that excludes domains and target zones matched by regex-domain-filter (optional)
  --zone-name-filter= ...        Filter target zones by zone domain (For now, only AzureDNS provider is using this flag); specify multiple times for multiple zones (optional)
  --zone-id-filter= ...          Filter target zones by hosted zone id; specify multiple times for multiple zones (optional)

Also, it would be nice to handle DNS for applications deployed across multiple clusters at some point in the future. We have not done any testing with this in our environment, but we think k8s external-dns might help us automate this in the future. It would be super cool if OpenShift could make it easy to automate this in the future.

sgreene570 · 2021-06-16T19:30:32Z

For excluding domains external-dns provides several different options. I'm leaning towards using --regex-domain-filter or --regex-domain-exclusion...

@seanmalloy thank you for pointing this out to me! After talking with my team, sounds like we will come up with a way to pass either a slice of domains, or a domain regex, to the domains and excludeDomains fields, or at least something along those lines. I'll be sure to open another PR (and cc you) to update this enhancement document once we have a better idea of what the ExternalDNS operator API will look like.

Also, it would be nice to handle DNS for applications deployed across multiple clusters at some point in the future. We have not done any testing with this in our environment, but we think k8s external-dns might help us automate this in the future. It would be super cool if OpenShift could make it easy to automate this in the future.

This is an interesting use case that I had not thought about. @seanmalloy, could you maybe help me understand what future work in this space would look like?

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 18, 2021

openshift-ci bot requested review from derekwaynecarr and smarterclayton May 18, 2021 14:57

sgreene570 force-pushed the external-dns-operator-revived branch 14 times, most recently from da9fad6 to bbdf379 Compare May 20, 2021 19:28

openshift-ci bot assigned danehans, knobunc and Miciah May 21, 2021

openshift-ci bot requested review from alebedev87, candita, frobware, miheer and rfredette May 21, 2021 14:17

sgreene570 force-pushed the external-dns-operator-revived branch 2 times, most recently from 71ef92d to 08741e6 Compare May 26, 2021 18:31

sgreene570 changed the title ~~[WIP] Add ExternalDNS Operator Enhancement Proposal~~ Add ExternalDNS Operator Enhancement Proposal May 26, 2021

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 26, 2021

sgreene570 force-pushed the external-dns-operator-revived branch 6 times, most recently from 180aaac to ab2c65b Compare June 10, 2021 20:12

alebedev87 reviewed Jun 11, 2021

View reviewed changes

sgreene570 force-pushed the external-dns-operator-revived branch from ab2c65b to 9fe6d62 Compare June 11, 2021 20:02

Miciah reviewed Jun 15, 2021

View reviewed changes

seanmalloy reviewed Jun 15, 2021

View reviewed changes

Miciah reviewed Jun 15, 2021

View reviewed changes

Adds initial ExternalDNS Operator Enhancement Proposal

bb76acb

This commit contains the original externalDNS operator enhancement proposal create by Daneyon Hansen as a part of openshift#456 (which was closed due to inactivity).

sgreene570 force-pushed the external-dns-operator-revived branch 2 times, most recently from 2b096a4 to 4eade5e Compare June 16, 2021 13:31

sgreene570 mentioned this pull request Jun 16, 2021

Add Support for an ExternalDNS Operator kubernetes-sigs/external-dns#1730

Closed

Bring Externaldns Operator enhancement up to speed

b0f67e9

This commit relocates and expands upon the original ExternalDNS Operator enhancement. This commit is in support of https://issues.redhat.com/browse/NE-303.

sgreene570 force-pushed the external-dns-operator-revived branch from 4eade5e to b0f67e9 Compare June 16, 2021 15:36

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 16, 2021

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 16, 2021

openshift-merge-robot merged commit c723451 into openshift:master Jun 16, 2021

chinmayi-chandrasekar mentioned this pull request Oct 6, 2021

Add README for CRC local development openshift/external-dns-operator#30

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ExternalDNS Operator Enhancement Proposal #786

Add ExternalDNS Operator Enhancement Proposal #786

sgreene570 commented May 18, 2021 •

edited

Loading

sgreene570 commented May 21, 2021

alebedev87 left a comment

alebedev87 Jun 11, 2021

sgreene570 Jun 11, 2021

This comment was marked as resolved.

sgreene570 Jun 11, 2021

This comment was marked as resolved.

alebedev87 Jun 11, 2021

sgreene570 Jun 11, 2021

sgreene570 Jun 11, 2021

seanmalloy commented Jun 14, 2021

Miciah Jun 15, 2021

sgreene570 Jun 16, 2021

Miciah Jun 15, 2021

sgreene570 Jun 16, 2021

Miciah Jun 15, 2021

sgreene570 Jun 16, 2021

seanmalloy Jun 15, 2021

sgreene570 Jun 16, 2021

sgreene570 Jun 16, 2021

Miciah Jun 15, 2021

sgreene570 Jun 16, 2021

Miciah commented Jun 16, 2021

Miciah commented Jun 16, 2021

openshift-ci bot commented Jun 16, 2021

sgreene570 commented Jun 16, 2021 •

edited

Loading

seanmalloy commented Jun 16, 2021

sgreene570 commented Jun 16, 2021

		* Expand upon the existing OpenShift [ingressoperator/dnsrecord](https://github.com/openshift/api/blob/master/operatoringress/v1/types.go)
		CRD to implement the ExternalDNS base [CRD source](https://github.com/kubernetes-sigs/external-dns/blob/master/docs/contributing/crd-source.md).

Add ExternalDNS Operator Enhancement Proposal #786

Add ExternalDNS Operator Enhancement Proposal #786

Conversation

sgreene570 commented May 18, 2021 • edited Loading

sgreene570 commented May 21, 2021

alebedev87 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanmalloy commented Jun 14, 2021

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

Miciah commented Jun 16, 2021

Miciah commented Jun 16, 2021

openshift-ci bot commented Jun 16, 2021

sgreene570 commented Jun 16, 2021 • edited Loading

seanmalloy commented Jun 16, 2021

sgreene570 commented Jun 16, 2021

sgreene570 commented May 18, 2021 •

edited

Loading

sgreene570 commented Jun 16, 2021 •

edited

Loading