Move DNS provider initialization to DNS controller #417

Miciah · 2020-06-23T00:17:59Z

cmd/ingress-operator/start.go (NewStartCommand): Delete initialization of the DNS provider.
(cloudCredentialsSecretName, createDNSProvider, getPlatformStatus): Move from here...
pkg/operator/controller/dns/controller.go: ...to here.
(New): Delete parameter for DNS provider; the controller now initializes it. Add config parameter. Add watches on the DNS config and Infrastructure config objects. Use ToDNSRecords mapper function to reconcile all DNSRecord objects when a config object changes.
(Config): New type to hold the namespace and operator release version, which are needed for initializing the DNS provider.
(reconciler): Add Config and infraConfig fields.
(Reconcile): Get the cluster Infrastructure config object, and initialize or re-initialize the DNS provider if it was not already initialized or if the Infrastructure config has changed.
(ToDNSRecords): New method. Return reconcilation requests for all DNSRecords.
pkg/operator/operator.go (New): Update initialization of the DNS controller to pass the controller's config object instead of the DNS provider object.
manifests/00-cluster-role.yaml: Allow the operator to list and watch the dnses and infrastructures config resources.
pkg/manifests/bindata.go: Regenerate.

danehans · 2020-06-23T19:34:24Z

@Miciah thanks for working on this refactor. This will help me with #416.

danehans · 2020-06-23T19:37:18Z

pkg/operator/controller/dns/controller.go

+	}
+
+	// Initialize or re-initialize the DNS provider.
+	if r.infraConfig == nil || !reflect.DeepEqual(infraConfig.Spec, r.infraConfig.Spec) || !reflect.DeepEqual(infraConfig.Status, r.infraConfig.Status) {


@Miciah why is it necessary to compare spec of configv1.Infrastructure? According to the EP, operators should use status:

But since the users are going to be specifying the service endpoints for APIs, there is chance of user error and operators picking up invalid or incorrect information. Therefore, the service endpoints will be mirrored to the status section after basic validations by a [controller][TODO-link-to-section] and the cluster operators will use the information from the status section.

Thanks! I've removed the || !reflect.DeepEqual(infraConfig.Spec, r.infraConfig.Spec).

* cmd/ingress-operator/start.go (NewStartCommand): Delete initialization of the DNS provider. (cloudCredentialsSecretName, createDNSProvider, getPlatformStatus): Move from here... * pkg/operator/controller/dns/controller.go: ...to here. (New): Delete parameter for DNS provider; the controller now initializes it. Add config parameter. Add watches on the DNS config and Infrastructure config objects. Use ToDNSRecords mapper function to reconcile all DNSRecord objects when a config object changes. (Config): New type to hold the namespace and operator release version, which are needed for initializing the DNS provider. (reconciler): Add Config and infraConfig fields. (Reconcile): Get the cluster Infrastructure config object, and initialize or re-initialize the DNS provider if it was not already initialized or if the Infrastructure config has changed. (ToDNSRecords): New method. Return reconcilation requests for all DNSRecords. * pkg/operator/operator.go (New): Update initialization of the DNS controller to pass the controller's config object instead of the DNS provider object. * manifests/00-cluster-role.yaml: Allow the operator to list and watch the dnses and infrastructures config resources. * pkg/manifests/bindata.go: Regenerate.

Miciah · 2020-06-24T01:39:28Z

e2e-aws-operator failed on TestRouteAdmissionPolicy; the failure is probably not caused by the changes in this PR (see #400).

Miciah · 2020-06-24T04:59:59Z

Failed to provision the cluster.

/test e2e-aws-operator

Miciah · 2020-06-24T05:54:19Z

Failed on TestRouteAdmissionPolicy again.

sgreene570 · 2020-06-24T20:25:23Z

pkg/operator/controller/dns/controller.go

+// configuration.
+func (r *reconciler) createDNSProvider(dnsConfig *configv1.DNS, platformStatus *configv1.PlatformStatus) (dns.Provider, error) {
+	// If no DNS configuration is provided, don't try to set up provider clients.
+	// TODO: the provider configuration can be refactored into the provider


Does it make sense to work on this TODO now, since the DNS controller is already being moved around? If you think this would be helpful at the moment, I could work on it.

Well, I guess it would make sense to wait til #416 is finished to make things less confusing, but that's fine.

Does it make sense to work on this TODO now, since the DNS controller is already being moved around?

I prefer that the TODO is addressed in a separate PR due to time constraints and to keep this PR focused on the refactor needed for supporting AWS/Azure custom endpoints.

danehans · 2020-06-24T21:08:05Z

--- FAIL: TestRouteAdmissionPolicy (41.33s)
    operator_test.go:875: failed to observe expected conditions: timed out waiting for the condition
    operator_test.go:1248: deleted ingresscontroller routeadmission

/test e2e-aws-operator

If this continues, we may need to bump the timeout in waitForRouteIngressConditions().

Miciah · 2020-06-24T22:11:29Z

If this continues, we may need to bump the timeout in waitForRouteIngressConditions().

#400 should fix it. (Hint hint! * grin *.)

danehans · 2020-06-25T23:21:20Z

Same failure as #417 (comment). Retesting since #400 has yet to merge (waiting for CI to pass) and hopefully we get lucky. If not, hold until #400 merges.

danehans · 2020-06-25T23:22:06Z

/test e2e-aws-operator

Miciah · 2020-06-26T01:03:53Z

Failed on TestInternalLoadBalancer because, The DNS provider failed to ensure the record: failed to update alias in zone Z08828212CZGG65VKV5I4: couldn't update DNS record in zone Z08828212CZGG65VKV5I4: Throttling: Rate exceeded\n\tstatus code: 400, request id: aa5754ee-ef4a-4e3d-b775-b87fdbdc45d8. It looks like the DNS controller did not retry after that.

danehans · 2020-06-26T14:25:44Z

Now that #400 has merged, let's try again.

/test e2e-aws-operator

danehans · 2020-06-26T21:47:06Z

/lgtm

openshift-ci-robot · 2020-06-26T21:47:28Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

openshift-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

openshift-ci-robot requested review from frobware and knobunc June 23, 2020 00:18

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 23, 2020

Miciah mentioned this pull request Jun 23, 2020

Adds Support for AWS Custom Endpoints #416

Merged

danehans reviewed Jun 23, 2020

View reviewed changes

Miciah force-pushed the move-DNS-provider-initialization-to-DNS-controller branch 2 times, most recently from b837a23 to a5ef198 Compare June 24, 2020 00:28

Miciah force-pushed the move-DNS-provider-initialization-to-DNS-controller branch from a5ef198 to 476710f Compare June 24, 2020 01:39

Miciah changed the title ~~WIP: Move DNS provider initialization to DNS controller~~ Move DNS provider initialization to DNS controller Jun 24, 2020

openshift-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

sgreene570 reviewed Jun 24, 2020

View reviewed changes

Miciah mentioned this pull request Jun 26, 2020

dns: Improve logging of reconciliation and updates #421

Merged

openshift-ci-robot assigned danehans Jun 26, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 26, 2020

openshift-merge-robot merged commit 07b4a56 into openshift:master Jun 26, 2020

Miciah mentioned this pull request Jul 11, 2020

support other Azure cloud environments #403

Merged

Miciah mentioned this pull request Jul 13, 2020

Bug 1854383: dns: Reread cloud credentials secret if it changes #425

Merged

Miciah mentioned this pull request Aug 22, 2020

[release-4.5] Bug 1868257: dns: Reread cloud credentials secret if it changes #443

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move DNS provider initialization to DNS controller #417

Move DNS provider initialization to DNS controller #417

Miciah commented Jun 23, 2020 •

edited

danehans commented Jun 23, 2020

danehans Jun 23, 2020 •

edited

Miciah Jun 24, 2020

Miciah commented Jun 24, 2020

Miciah commented Jun 24, 2020

Miciah commented Jun 24, 2020

sgreene570 Jun 24, 2020

sgreene570 Jun 24, 2020

danehans Jun 24, 2020

danehans commented Jun 24, 2020

Miciah commented Jun 24, 2020

danehans commented Jun 25, 2020

danehans commented Jun 25, 2020

Miciah commented Jun 26, 2020

danehans commented Jun 26, 2020

danehans commented Jun 26, 2020

openshift-ci-robot commented Jun 26, 2020

Move DNS provider initialization to DNS controller #417

Move DNS provider initialization to DNS controller #417

Conversation

Miciah commented Jun 23, 2020 • edited

danehans commented Jun 23, 2020

danehans Jun 23, 2020 • edited

Choose a reason for hiding this comment

Miciah Jun 24, 2020

Choose a reason for hiding this comment

Miciah commented Jun 24, 2020

Miciah commented Jun 24, 2020

Miciah commented Jun 24, 2020

sgreene570 Jun 24, 2020

Choose a reason for hiding this comment

sgreene570 Jun 24, 2020

Choose a reason for hiding this comment

danehans Jun 24, 2020

Choose a reason for hiding this comment

danehans commented Jun 24, 2020

Miciah commented Jun 24, 2020

danehans commented Jun 25, 2020

danehans commented Jun 25, 2020

Miciah commented Jun 26, 2020

danehans commented Jun 26, 2020

danehans commented Jun 26, 2020

openshift-ci-robot commented Jun 26, 2020

Miciah commented Jun 23, 2020 •

edited

danehans Jun 23, 2020 •

edited