New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
OCPBUGS-10875: gateway-service-dns: Set DNS policy appropriately #934
OCPBUGS-10875: gateway-service-dns: Set DNS policy appropriately #934
Conversation
@Miciah: This pull request references Jira Issue OCPBUGS-10875, 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 kubernetes/test-infra repository. |
/jira refresh |
@Miciah: This pull request references Jira Issue OCPBUGS-10875, 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: 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. |
26ca122
to
c5b6502
Compare
@Miciah: This pull request references Jira Issue OCPBUGS-10875, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
/retest-required |
/assign @gcs278 |
}, | ||
expectUpdate: []client.Object{}, | ||
expectDelete: []client.Object{}, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth adding 2 simple test cases: one for dnsConfig
is null and another for infraConfig
is null and nothing should be created?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
c5b6502
to
8b20178
Compare
8b20178
to
038190e
Compare
Whoops, https://github.com/openshift/cluster-ingress-operator/compare/8b20178b30c5e9516ac68e8c46649ba2b9d4b701..038190ece2d957a26615bbc666ad9164a2e67846 re-adds code from #949 that got inadvertently got dropped in the rebase. |
038190e
to
0eb49fb
Compare
I'll rebase again once #949 (comment) is resolved. |
* pkg/operator/controller/gateway-service-dns/controller_test.go (Test_Reconcile): Add labels to DNS records so that deleteStaleDNSRecordsForGateway doesn't filter them out. Verify that objects that are expected to be deleted are indeed deleted. Add a test case to verify that deletion works properly. (fakeClientRecorder): Add "deleted" field. (Create, Delete, Update): Log actions. (Delete): Record deleted objects.
0eb49fb
to
26492db
Compare
@@ -229,11 +229,20 @@ func dnsRecordChanged(current, expected *iov1.DNSRecord) (bool, *iov1.DNSRecord) | |||
// once we know there are no users depending on this. | |||
// See https://bugzilla.redhat.com/show_bug.cgi?id=2041616 | |||
func ManageDNSForDomain(domain string, status *configv1.PlatformStatus, dnsConfig *configv1.DNS) bool { | |||
if len(domain) == 0 { | |||
if len(domain) == 0 || len(dnsConfig.Spec.BaseDomain) == 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we unit test this new change? BaseDomain = ""
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return false | ||
} | ||
|
||
mustContain := "." + dnsConfig.Spec.BaseDomain | ||
|
||
// Ignore any trailing dot for comparison. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we unit test comparison of domains and base domains that end with .
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
26492db
to
7a32d62
Compare
When creating a DNSRecord CR for a gateway listener, set the DNS management policy to "Unmanaged" if the DNSRecord's domain is outside the cluster's base domain. Before this commit, the operator would attempt to create DNS records for gateway listeners even if they had domains outside of the cluster's base domain zone, resulting in "failed to publish DNS record to zone" errors. This commit fixes OCPBUGS-10875. https://issues.redhat.com/browse/OCPBUGS-10875 * pkg/operator/controller/gateway-service-dns/controller.go (Reconcile): Get the dns and infrastructure config objects, and pass them to ensureDNSRecordsForGateway. (ensureDNSRecordsForGateway): Add infraConfig and dnsConfig parameters. Use them and the ManageDNSForDomain function to determine the appropriate policy (managed or unmanaged) for each DNS record, and pass that policy to EnsureDNSRecord. * pkg/resources/dnsrecord/dns.go (EnsureDNSRecord): Add a dnsPolicy parameter, and pass it to desiredDNSRecord. (ManageDNSForDomain): Ignore trailing dots when comparing the given domain and the cluster's base domain. * pkg/resources/dnsrecord/dns_test.go (TestManageDNSForDomain): Add test cases for empty base domain and for various combinations of trailing dots on the domain or base domain. * pkg/operator/controller/gateway-service-dns/controller_test.go (Test_Reconcile): Add expectError to the test inputs, and assert that the expected error is returned if expectError is not empty. Add dnsConfig and infraConfig to existingObjects in test cases. Add test cases to verify that the reconciler returns the expected error if the cluster DNS config or cluster infrastructure config is not found. Add a test case to verify that the DNS management policy is set to "Unmanaged" if the DNS name doesn't belong to the cluster's base domain.
7a32d62
to
620c930
Compare
@Miciah: This pull request references Jira Issue OCPBUGS-10875, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: 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-10875, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
e2e-aws-ovn-serial failed on
Search.ci isn't currently loading for me, so I cannot check whether this failure is occurring on other PRs. e2e-hypershift failed. It isn't clear why. I see numerous errors in test output, but no actual test failure. I think the tests just timed out:
@enxebre, do you know how to diagnose these failures? |
Sorry for the delay, I missed the notifications on this one. Updates look good, thanks. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gcs278 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:
Approvers can indicate their approval by writing |
e2e-gcp-operator failed because must-gather failed. Also, #968 merged, so we need to rerun all tests. |
@Miciah: The specified target(s) for
The following commands are available to trigger optional jobs:
Use
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. |
/test all |
/hold Revision 620c930 was retested 3 times: holding |
e2e-aws-operator failed because must-gather failed. e2e-aws-ovn-serial failed because
This is a known issue: OCPBUGS-14930. /hold cancel |
/test e2e-aws-ovn-serial |
1 similar comment
/test e2e-aws-ovn-serial |
@Miciah: 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: Jira Issue OCPBUGS-10875: All pull requests linked via external trackers have merged: Jira Issue OCPBUGS-10875 has been moved to the MODIFIED 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 kubernetes/test-infra repository. |
gateway-service-dns: Add test coverage for deletes
pkg/operator/controller/gateway-service-dns/controller_test.go
(Test_Reconcile
): Add labels to DNS records so thatdeleteStaleDNSRecordsForGateway
doesn't filter them out. Verify that objects that are expected to be deleted are indeed deleted. Add a test case to verify that deletion works properly.(
fakeClientRecorder
): Add "deleted" field.(
Create
,Delete
,Update
): Log actions.(
Delete
): Record deleted objects.gateway-service-dns: Set DNS policy appropriately
When creating a DNSRecord CR for a gateway listener, set the DNS management policy to "Unmanaged" if the DNSRecord's domain is outside the cluster's base domain.
Before this change, the operator would attempt to create DNS records for gateway listeners even if they had domains outside of the cluster's base domain zone, resulting in "failed to publish DNS record to zone" errors.
pkg/operator/controller/gateway-service-dns/controller.go
(Reconcile
): Get the dns and infrastructure config objects, and pass them toensureDNSRecordsForGateway
.(
ensureDNSRecordsForGateway
): AddinfraConfig
anddnsConfig
parameters. Use them and theManageDNSForDomain
function to determine the appropriate policy (managed or unmanaged) for each DNS record, and pass that policy toEnsureDNSRecord
.pkg/resources/dnsrecord/dns.go
(EnsureDNSRecord
): Add adnsPolicy
parameter, and pass it todesiredDNSRecord
.(
ManageDNSForDomain
): Ignore trailing dots when comparing the given domain and the cluster's base domain.pkg/resources/dnsrecord/dns_test.go
(TestManageDNSForDomain
): Add test cases for empty base domain and for various combinations of trailing dots on the domain or base domain.pkg/operator/controller/gateway-service-dns/controller_test.go
(Test_Reconcile
): AdddnsConfig
andinfraConfig
toexistingObjects
in test cases. Add a test case to verify that the DNS management policy is set to "Unmanaged" if the DNS name doesn't belong to the cluster's base domain.