OCPBUGS-60492: Skip Gateway Service DNS record creation on platforms with ClusterHostedDNS is configured#1269
Conversation
|
@sadasu: This pull request references Jira Issue OCPBUGS-60492, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn 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. |
|
/jira refresh |
|
@sadasu: This pull request references Jira Issue OCPBUGS-60492, 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: DetailsIn 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. |
| if !checkPlatformSupport(infraConfig) { | ||
| log.Info("infrastructure 'cluster' has unsupported configuration; reconciliation will be skipped") | ||
| return reconcile.Result{}, nil | ||
| } | ||
|
|
||
| domains := getGatewayHostnames(&gateway) | ||
| var errs []error | ||
| errs = append(errs, r.ensureDNSRecordsForGateway(ctx, &gateway, &service, domains.List(), infraConfig, dnsConfig)...) |
There was a problem hiding this comment.
It can be useful to create the DNSRecord CR, but with dnsManagementState: Unmanaged, to serve as a reference for what DNS record the cluster-admin is expected to create. So I would suggest still calling ensureDNSRecordsForGateway but changing it to to set dnsManagementState: Unmanaged when the platform is not supported. Does that make sense?
There was a problem hiding this comment.
My reasoning for putting the check here vs ensureDNSRecordsForGateway was that this check would give the same outcome for each domain within ensureDNSRecordsForGateway.
I see that creating the DNSRecord CR could be useful, so updated implementation.
There was a problem hiding this comment.
My reasoning for putting the check here vs
ensureDNSRecordsForGatewaywas that this check would give the same outcome for each domain withinensureDNSRecordsForGateway.
If you're concerned about the performance impact, you can call checkClusterHostedDNS (previously checkPlatformSupport) in Reconcile and pass the Boolean result to ensureDNSRecordsForGateway.
| // checkPlatformSupport return true if the platform supports gateway service dns. | ||
| // Gateway service dns is currently not supported when in-cluster DNS is configured | ||
| // for AWS or GCP platforms. | ||
| func checkPlatformSupport(infraConfig *configv1.Infrastructure) bool { |
There was a problem hiding this comment.
Minor grammar and capitalization corrections:
| // checkPlatformSupport return true if the platform supports gateway service dns. | |
| // Gateway service dns is currently not supported when in-cluster DNS is configured | |
| // for AWS or GCP platforms. | |
| func checkPlatformSupport(infraConfig *configv1.Infrastructure) bool { | |
| // checkPlatformSupport returns true if the platform supports gateway service DNS. | |
| // Gateway service DNS is currently not supported when in-cluster DNS is configured | |
| // for AWS or GCP platforms. | |
| func checkPlatformSupport(infraConfig *configv1.Infrastructure) bool { |
Also, I wonder whether the name might be misleading. We don't manage DNS at all on some platforms, but that isn't what this function checks; it only checks whether platforms on which we can manage the DNS records have some alternative custom DNS record management. Maybe a name like checkUsePlatformDefaultDNS? Or invert the Boolean value and condition, and rename the function to checkClusterHostedDNS?
There was a problem hiding this comment.
Updated to invert the Boolean value and condition, and rename the function to checkClusterHostedDNS.
When ClusterHostedDNS is enabled, create Gateway Service DNS records with DNS Policy `Unmanaged`.
8d7424c to
fb9ca0b
Compare
|
/retest-required |
|
It would be good to investigate this CI failure. It seems related to me, being GCP and DNS.
|
| if checkClusterHostedDNS(infraConfig) { | ||
| dnsPolicy = iov1.UnmanagedDNS | ||
| } |
There was a problem hiding this comment.
Can you please explain in a comment how this solves the problem of missing DNS name for Gateway objects?
There was a problem hiding this comment.
custom-dns is enabled by the customer when they do not want OpenShift to use the cloud provider's default DNS. So, setting the dnsPolicy to iov1.UnmanagedDNS to cause the Gateway API controller to skip configuring the cloud's DNS.
There was a problem hiding this comment.
In that case, can the tests provide whatever DNS is likely to be used instead, or at least something resembling DNS? We can't just assume it all works without testing it. Changing the name of this PR might help too.
There was a problem hiding this comment.
In that case, can the tests provide whatever DNS is likely to be used instead, or at least something resembling DNS? We can't just assume it all works without testing it. Changing the name of this PR might help too.
The custom DNS CI workflows do that: install using on-cluster networking and then add user-provisioned DNS after the install. We will need to add those jobs to run in this repo.
Results are in, and it looks like the installer configuring the DNS zones to So I think we do need a check in the ingress controller, but I would not make it specific to custom DNS, but rather based on the presence of the cloud |
|
/hold |
|
/approve I prefer if the Git commit message references the Jira issue and follows the Kubernetes commit message guidelines, but the changes look fine to me. |
|
[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 DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/hold cancel |
|
/retest-required |
1 similar comment
1 similar comment
|
@sadasu: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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-sigs/prow repository. I understand the commands that are listed here. |
|
/hold We've identified that with custom-dns, when no Public and Private zones are specified in the DNS CR, no additional changes are required in the CIO for Gateway controller. |
|
/jira refresh The requirements for Jira bugs have changed (Jira issues linked to PRs on main branch need to target different OCP), recalculating validity. |
|
@openshift-bot: This pull request references Jira Issue OCPBUGS-60492, which is invalid:
Comment DetailsIn 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. |
|
This fix is currently not considered necessary. |
|
@sadasu: This pull request references Jira Issue OCPBUGS-60492. The bug has been updated to no longer refer to the pull request using the external bug tracker. DetailsIn 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. |
When in-cluster DNS/custom-DNS is configured on AWS and GCP, skip creating Gateway Service DNS record.