WIP: OCPBUGS-8526: gateway-service-dns: Use gateway addresses to configure DNS #969
Conversation
* pkg/operator/controller/gateway-service-dns/controller_test.go (Test_Reconcile): Change dnsrecord helper to parameterize the DNS record type in test cases for Test_Reconcile.
* pkg/operator/controller/gateway-service-dns/controller_test.go (Test_Reconcile): Change gw helper's listeners parameter from a variadic parameter to slice.
* pkg/operator/controller/gateway-service-dns/controller.go (managedByIstioLabelKey): Remove const, which is no longer needed after subsequent changes. (NewUnmanaged): Remove the watch on services and the watch's associated predicates. Enqueue reconciliation requests for gateways instead of services. Define a predicate that checks the gateway addresses, using the new gatewayAddressesChanged helper function, and use this predicate to enqueue a reconciliation request for a gateway when any of its addresses changes. (gatewayAddressesChanged): New function. Return a Boolean value indicating whether the given gateway's addresses have changed. (Reconcile): Expect the reconciliation request to be for a gateway instead of a service. Use the new getGatewayAddresses helper function to get the gateway's addresses, and pass these addresses to ensureDNSRecordsForGateway and deleteStaleDNSRecordsForGateway. (getGatewayAddresses): New helper function. Return a list of addresses that can be used as targets for a DNSRecord CR, as well as a record type. (ensureDNSRecordsForGateway): Replace the service parameter with "targets" and "recordType" parameters. Pass targets and recordType to EnsureDNSRecord. Change the owner reference for the DNSRecord CR to specify the gateway instead of the service. (deleteStaleDNSRecordsForGateway): Replace the service parameter with a "targets" parameter. Use the new targets parameter and existing domains parameter to preserve any DNSRecord CR that has both a valid domain and a valid target. * pkg/operator/controller/gateway-service-dns/controller_test.go (Test_Reconcile): Add addresses to gateway test inputs, and remove services as test inputs. Add a test case for a gateway with an address but no listeners, as well as a test case for various combinations of addresses. (Test_gatewayAddressesChanged): New test. Verify that gatewayAddressesChanged behaves as expected. * pkg/resources/dnsrecord/dns.go (EnsureDNSRecord): Replace the service parameter with "targets" and "recordType" parameters. Pass these new parameters to desiredDNSRecord. (desiredDNSRecord): Replace the service parameter with "targets" and "recordType" parameters. Move service-specific logic from here... (desiredWildcardDNSRecord): ...to here.
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
openshift-ci-robot
added
jira/severity-moderate
|
@Miciah: This pull request references Jira Issue OCPBUGS-8526, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: The bug has been updated to refer to the pull request using the external bug tracker. In response to this:
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. |
openshift-ci-robot
added
the
jira/valid-bug
|
/assign |
| // Set the DNS management policy on the dnsrecord to "Unmanaged" if ingresscontroller has "Unmanaged" DNS policy or | ||
| // if the ingresscontroller domain isn't a subdomain of the cluster's base domain. |
There was a problem hiding this comment.
Where is this referenced condition examined: "or if the ingresscontroller domain isn't a subdomain of the cluster's base domain" ?
| haveWC, current, err := CurrentDNSRecord(client, name) | ||
| if err != nil { | ||
| return false, nil, err | ||
| } | ||
|
|
||
| switch { | ||
| case wantWC && !haveWC: | ||
| case !haveWC: |
There was a problem hiding this comment.
The WC suffix can be removed, just use have.
| func EnsureDNSRecord(client client.Client, name types.NamespacedName, dnsRecordLabels map[string]string, ownerRef metav1.OwnerReference, domain string, service *corev1.Service) (bool, *iov1.DNSRecord, error) { | ||
| wantWC, desired := desiredDNSRecord(name, dnsRecordLabels, ownerRef, domain, iov1.ManagedDNS, service) | ||
| func EnsureDNSRecord(client client.Client, name types.NamespacedName, dnsRecordLabels map[string]string, ownerRef metav1.OwnerReference, domain string, targets []string, recordType iov1.DNSRecordType) (bool, *iov1.DNSRecord, error) { | ||
| desired := desiredDNSRecord(name, dnsRecordLabels, ownerRef, domain, iov1.ManagedDNS, targets, recordType) |
There was a problem hiding this comment.
desiredDNSRecord used to do a bit of validation and return false under certain circumstances. That logic was moved to desiredWildcardDNSRecord. Is L69 supposed to be calling the latter?
openshift-merge-robot
added
the
needs-rebase
|
PR needs rebase. 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. |
|
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
openshift-ci
bot
added
the
lifecycle/stale
|
/remove-lifecycle stale |
openshift-ci
bot
removed
the
lifecycle/stale
|
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
openshift-ci
bot
added
the
lifecycle/stale
|
Stale issues rot after 30d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle rotten |
openshift-ci
bot
added
lifecycle/rotten
|
Rotten issues close after 30d of inactivity. Reopen the issue by commenting /close |
|
@openshift-bot: Closed this PR. In response to this:
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. |
|
@Miciah: This pull request references Jira Issue OCPBUGS-8526. The bug has been updated to no longer refer to the pull request using the external bug tracker. All external bug links have been closed. The bug has been moved to the NEW state. In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
/reopen |
|
@candita: Reopened this PR. In response to this:
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. |
openshift-ci-robot
added
jira/invalid-bug
|
@Miciah: This pull request references Jira Issue OCPBUGS-8526, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
gateway-service-dns:
Test_Reconcile: Parameterize record typepkg/operator/controller/gateway-service-dns/controller_test.go(Test_Reconcile): Changednsrecordhelper to parameterize the DNS record type in test cases forTest_Reconcile.gateway-service-dns:
Test_Reconcile: Change listeners to slicepkg/operator/controller/gateway-service-dns/controller_test.go(Test_Reconcile): Changegwhelper'slistenersparameter from a variadic parameter to slice.gateway-service-dns: Use gateway addresses to configure DNS records
pkg/operator/controller/gateway-service-dns/controller.go(managedByIstioLabelKey): Remove const, which is no longer needed after subsequent changes.(
NewUnmanaged): Remove the watch on services and the watch's associated predicates. Enqueue reconciliation requests for gateways instead of services. Define a predicate that checks the gateway addresses, using the newgatewayAddressesChangedhelper function, and use this predicate to enqueue a reconciliation request for a gateway when any of its addresses changes.(
gatewayAddressesChanged): New function. Return a Boolean value indicating whether the given gateway's addresses have changed.(
Reconcile): Expect the reconciliation request to be for a gateway instead of a service. Use the newgetGatewayAddresseshelper function to get the gateway's addresses, and pass these addresses toensureDNSRecordsForGatewayanddeleteStaleDNSRecordsForGateway.(
getGatewayAddresses): New helper function. Return a list of addresses that can be used as targets for a DNSRecord CR, as well as a record type.(
ensureDNSRecordsForGateway): Replace theserviceparameter with "targets" and "recordType" parameters. PasstargetsandrecordTypetoEnsureDNSRecord. Change the owner reference for the DNSRecord CR to specify the gateway instead of the service.(
deleteStaleDNSRecordsForGateway): Replace theserviceparameter with a "targets" parameter. Use the newtargetsparameter and existingdomainsparameter to preserve any DNSRecord CR that has both a valid domain and a valid target.pkg/operator/controller/gateway-service-dns/controller_test.go(Test_Reconcile): Add addresses to gateway test inputs, and remove services as test inputs. Add a test case for a gateway with an address but no listeners, as well as a test case for various combinations of addresses.(
Test_gatewayAddressesChanged): New test. Verify thatgatewayAddressesChangedbehaves as expected.pkg/resources/dnsrecord/dns.go(EnsureDNSRecord): Replace theserviceparameter with "targets" and "recordType" parameters. Pass these new parameters todesiredDNSRecord.(
desiredDNSRecord): Replace theserviceparameter with "targets" and "recordType" parameters. Move service-specific logic from here...(
desiredWildcardDNSRecord): ...to here.