[NE-568] Add option to manage DNS to ingresscontrollers

[thejasn]

This enhancement provides the customer the ability to completely
disable DNS management on the Ingress Controller so that DNS can be
managed externally by them.

Signed-off-by: thejasn thn@redhat.com

[thejasn]

/assign @sherine-k @alebedev87

[sherine-k]

Hi @thejasn,
as promised, a few typo fixes suggested.
along with 1 question...
Great job

[thejasn]

/label tide/merge-method-squash

[alebedev87]

/retitle [NE-568] Add option to manage DNS to ingresscontrollers

[brandisher]

@thejasn overall I understand the intent but I did have a few questions/comments and some feedback to share. For any of my API related comments, defer to the API Team for final say. I mostly wanted to share my recent API experience to help move the needle.

[brandisher]

I'd expect this to be DNSManagementPolicy so it's more clear at a glance what it's affecting.

[thejasn]

Are you suggesting sharing the type variable between the fields? I wanted to avoid that, hence the different names. Sharing the type doesn't seem correct, since it would couple the two objects LoadBalancerStrategy and DNSRecord when any new types are added (eg: externalDNS). WDYT?

[brandisher]

Ah, that's a great question! I wasn't as clear as I intended to be; this is the change I'm suggesting for clarity.

Suggested change

	DNS LoadBalancerDNSManagementPolicy `json:"dns"`
	DNSManagementPolicy LoadBalancerDNSManagementPolicy `json:"dns"`

That way you're maintaining the distinction in type between LoadBalancerDNSManagementPolicy and DNSManagementPolicy but when a cluster admin is reading the API/config it's clear that they're changing the loadBalancerStrategy.dnsManagementPolicy which is, in my opinion, more clear than loadBalancerStrategy.dns. Also just a reminder that ultimately the API team should make the call on the shape of the API but I suspect they'd err on the side of making configuration options more obvious.

[brandisher]

Similarly here as above, I'd expect this to be DNSManagementPolicy. Although with this one it's sort of implicit what the management policy is informing so this is probably even more nit-like than above.

[brandisher]

I read this as "DNS management policy migration from managed to unmanaged". If that's the correct way to read this then I think it should be made more explicit as you do allow for migration from managed to unmanaged when the IC domain is the same as the cluster domain in the Modifying/Updating an ingresscontroller section.

[thejasn]

Ahh, this was meant to state traffic management is not going to be considered during moving from Managed to Unmanaged. I've updated the point, to make it more clear.

[alebedev87]

Suggested change

	The ingresscontroller CRD is extended by adding an optional field `DNS`
	The ingresscontroller CRD is extended by adding an optional field `DNSManagementPolicy`

[thejasn]

Added deletion workflow.

[thejasn]

Clarified the implementation details on how deletion of ingresscontrollers will be done.

[alebedev87]

I know that there is probably no easy way out. But I think it still a bit contradictory. In this comment we say that DNSRecord CR won't be created/updated/deleted, however the deletion will still happen: deletionTimestamp will be added. Also the same comment says that it's the full responsibility of the cluster admin to deal with DNSRecord CRs however the cluster admin won't do oc delete dnsrecord it has to be oc edit dnsrecord (as not removed finalization label will block the deletion) which is kinda not intuitive.

I think I'm rather for one of the following:

• "fully orphaned" unmanaged DNSRecord CR: cluster admin has to do aws route53 delete resource record + oc delete dnsrecord. The orphaned DNSRecord CRs can be found with the command you suggested.
• "cloud provider orphaned" unmanaged DNSRecord CR: cluster admin has to do aws route53 delete resource record only. DNSRecord will be cleared from the finalization label without doing the actual finalization.

@thejasn, @brandisher: WDYT guys?

[thejasn]

Based on our discussion, decided to treat Unmanaged as the DNS record on the cloud provider will be unmanaged by the operator. (going ahead with latter)

[thejasn]

Clearly stated what the 2 new states denote and their scope.

[alebedev87]

/lgtm

[Miciah]

Suggested change

	- Recover from degraded state of the `cluster-ingress-operator` during upgrades
	- Enable migration

[Miciah]

I wouldn't say "partial support" or "provided by the installer"; it's an existing, course-grained option in the cluster DNS config, which the cluster administrator can configure at install time (by customizing installer manifests) or after installation (using oc patch dnses.config.openshift.io/cluster --type=merge --patch='{"spec":{"privateZone":null,"publicZone":null}}). The goal, as your next paragraph says, is to add a more fine-grained option.

[Miciah]

Why "recreation"? Is the test going to delete the DNS record after setting dnsManagementPolicy to "Unmanaged"?

[thejasn]

Yes, that is what I had in mind. Thinking of testing both the scenarios to verify there were no collisions during recreation.

[Miciah]

Thanks, that makes sense; it just isn't clear from the test plan as written.

[thejasn]

The test plan might need to be revisited. I've mostly implemented this feature and turns out we can't have updates from Unmanaged to Managed since domain cannot be updated. Essentially, to update an ingresscontroller from Unmanaged to Managed becomes an DELETE and CREATE (replace) due to the limitation on domain.

Although it is possible to update from Managed to Unmanaged users would be forced to re-use the domain leading to again DELETE and CREATE of the ingresscontroller so that a new unmanaged domain can be given.

So I'm thinking it is better to make dnsManagementPolicy also not updatable similar to domain (figured this would be better than making domain updatable`), then the test plan would be straight forward. Just test with an Unmanaged ingresscontroller.

[Miciah]

I don't understand. Why would it matter that domain cannot be updated? The operator should still be able to do an upsert when the management state is changed from Unmanaged to Managed, the same as it does an upsert if a managed DNSRecord's targets change. On some platforms, an upsert may actually be implemented as discrete delete and create steps within a transaction, but that's an implementation detail that is abstracted behind the dns.Provider interface.

[Miciah]

Is the idea here to create the IngressController with "Unmanaged", update it to "Managed", and verify that the operator creates the DNS record upon the update to the IngressController? Here, "DNSRecord" means the actual DNS record and not the CR, right?

[thejasn]

Yes, updated the text.

[Miciah]

The ingresscontroller will still handle any traffic that it receives, so I'm not sure exactly what this means. Does this refer to DNS traffic, or to ingress (HTTP/TLS) traffic?

[Miciah]

Would we do something (such as trigger an alert or set Upgradeable=False) to convey that the cluster is no longer in a supported state?

[Miciah]

What conditions?

[Miciah]

Thanks, that makes sense; it just isn't clear from the test plan as written.

[openshift-ci]

@thejasn: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

[Miciah]

Thanks!
/approve
/lgtm

[openshift-ci]

[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