OCPBUGS-31521: Don't publish duplicate DNS records #1229

rfredette · 2025-05-07T17:37:11Z

Before attempting to publish a domain to a zone, check if that domain is already being published to the same zone.

openshift-ci · 2025-05-07T17:37:15Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

openshift-ci · 2025-05-07T17:37:40Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign frobware for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

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

Miciah · 2025-05-07T19:18:53Z

pkg/operator/controller/dns/controller.go

+	for _, existingRecord := range records.Items {
+		// we only care if the domain name is published by a different record, so ignore the matching record if it
+		// already exists.
+		// TODO: There's got to be a better way to match the same object


Compare by UID instead of name.

This is still a relevant comment, but I moved the function, which made github think it's outdated. I'll make this change in my next update

Miciah · 2025-05-07T19:19:46Z

pkg/operator/controller/dns/controller.go

@@ -352,6 +361,24 @@ func (r *reconciler) publishRecordToZones(zones []configv1.DNSZone, record *iov1
 			}
 		} else if isRecordPublished {
 			condition, err = r.replacePublishedRecord(zones[i], record)
+		} else if isDomainPublished, err = domainIsAlreadyPublishedInZone(context.Background(), r.cache, record, &zones[i]); err != nil {


You don't need to declare isDomainPublished outside the else if clause. Better to keep isDomainPublished and err scoped to the else if clauses.

Suggested change

} else if isDomainPublished, err = domainIsAlreadyPublishedInZone(context.Background(), r.cache, record, &zones[i]); err != nil {

} else if isDomainPublished, err := domainIsAlreadyPublishedInZone(context.Background(), r.cache, record, &zones[i]); err != nil {

Edit: Discussed on a call. Line 388 uses the err value. This logic is a bit subtle and could use some refactoring.

Miciah · 2025-05-07T19:21:02Z

pkg/operator/controller/dns/controller.go

+				Reason:             "DomainAlreadyInUse",
+				Status:             string(operatorv1.ConditionUnknown),
+				Type:               iov1.DNSRecordPublishedConditionType,
+				LastTransitionTime: metav1.Now(),


I believe mergeConditions will set LastTransitionTime, so you don't need to set it here (lines 371 and 380).

523461a updated mergeConditions to set LastTransitionTime. That's a fairly recent change, so older code (e.g. line 360) might be setting LastTransitionTime because it used to be necessary but no longer is. Scratch that, mergeConditions was setting LastTransitionTime even before that commit.

Before attempting to publish a domain to a zone, check if that domain is already being published to the same zone.

Miciah · 2025-05-15T19:13:06Z

pkg/operator/controller/dns/controller.go

+func (r *reconciler) MapOnRecordDelete(ctx context.Context, o client.Object) []reconcile.Request {
+	deletedRecord, ok := o.(*iov1.DNSRecord)
+	if !ok {
+		log.Info("failed to read DNS record")


Suggested change

log.Info("failed to read DNS record")

log.Infof("Got unexpected object; expected type DNSRecord, got type %T", o)

Or something more like this:

Suggested change

log.Info("failed to read DNS record")

log.Error(nil, "Got unexpected type of object", "expected", "DNSRecord", "actual", fmt.Sprintf("%T", o))

Miciah · 2025-05-15T19:14:01Z

pkg/operator/controller/dns/controller.go

+	// When a DNS record is deleted, there may be a conflicting record that should be published. Watch exclusively for
+	// deletes, and queue a reconcile request for the appropriate conflicting record, if applicable.
+	if err := c.Watch(source.Kind[client.Object](operatorCache, &iov1.DNSRecord{}, handler.EnqueueRequestsFromMapFunc(reconciler.MapOnRecordDelete), predicate.Funcs{


Make the comment a little more explicit that, yes, we have two watches on the same resource (dnsrecords), and the reason is so that we can have a predicate and mapfunc to do something special for deletes.

Miciah · 2025-05-15T19:14:28Z

pkg/operator/controller/dns/controller.go

+	// When a DNS record is deleted, there may be a conflicting record that should be published. Watch exclusively for
+	// deletes, and queue a reconcile request for the appropriate conflicting record, if applicable.
+	if err := c.Watch(source.Kind[client.Object](operatorCache, &iov1.DNSRecord{}, handler.EnqueueRequestsFromMapFunc(reconciler.MapOnRecordDelete), predicate.Funcs{
+		CreateFunc:  func(e event.CreateEvent) bool { return false },
+		DeleteFunc:  func(e event.DeleteEvent) bool { return true },


Add a comment with your findings that the delete event happens when the object is actually deleted, not when it is merely marked for deletion (that is, when deletionTimestamp is set).

Miciah · 2025-05-15T19:27:59Z

pkg/operator/controller/dns/controller.go

@@ -580,6 +646,37 @@ func (r *reconciler) ToDNSRecords(ctx context.Context, o client.Object) []reconc
 	return requests
 }

+func (r *reconciler) MapOnRecordDelete(ctx context.Context, o client.Object) []reconcile.Request {


Suggested change

func (r *reconciler) MapOnRecordDelete(ctx context.Context, o client.Object) []reconcile.Request {

func (r *reconciler) mapOnRecordDelete(ctx context.Context, o client.Object) []reconcile.Request {

rfredette · 2025-05-15T19:28:12Z

pkg/operator/controller/dns/controller.go

@@ -580,6 +646,37 @@ func (r *reconciler) ToDNSRecords(ctx context.Context, o client.Object) []reconc
 	return requests
 }

+func (r *reconciler) MapOnRecordDelete(ctx context.Context, o client.Object) []reconcile.Request {


Needs a godoc comment

rfredette · 2025-05-15T19:28:42Z

pkg/operator/controller/dns/controller_test.go

@@ -840,3 +847,26 @@ func Test_customCABundle(t *testing.T) {
 		})
 	}
 }
+
+func buildFakeCache(t *testing.T) *testutil.FakeCache {


Also could use a godoc comment

Miciah · 2025-05-15T19:32:11Z

pkg/operator/controller/dns/controller_test.go

 // correctly merges status updates.
-func TestPublishRecordToZonesMergesStatus(t *testing.T) {
+func Test_publishRecordToZonesMergesStatus(t *testing.T) {


TestPublishRecordToZonesMergesStatus is an appropriate name for the test as there is no publishRecordToZonesMergesStatus function.

Probably I am also missing something here, but is there a new test case that will check if the condition is set?

openshift-ci-robot · 2025-08-13T21:21:28Z

@rfredette: This pull request references Jira Issue OCPBUGS-31521, which is invalid:

expected the bug to target either version "4.20." or "openshift-4.20.", but it targets "4.19.z" instead

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

Before attempting to publish a domain to a zone, check if that domain is already being published to the same zone.

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.

openshift-ci · 2025-08-14T01:44:40Z

@rfredette: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-aws-ovn-hypershift-conformance	`d38f7bf`	link	true	`/test e2e-aws-ovn-hypershift-conformance`
ci/prow/e2e-gcp-operator	`d38f7bf`	link	true	`/test e2e-gcp-operator`
ci/prow/e2e-aws-operator	`d38f7bf`	link	true	`/test e2e-aws-operator`
ci/prow/e2e-azure-operator	`d38f7bf`	link	true	`/test e2e-azure-operator`
ci/prow/e2e-aws-ovn	`d38f7bf`	link	true	`/test e2e-aws-ovn`
ci/prow/e2e-aws-ovn-single-node	`d38f7bf`	link	false	`/test e2e-aws-ovn-single-node`
ci/prow/e2e-aws-ovn-techpreview	`d38f7bf`	link	false	`/test e2e-aws-ovn-techpreview`
ci/prow/okd-scos-e2e-aws-ovn	`d38f7bf`	link	false	`/test okd-scos-e2e-aws-ovn`
ci/prow/hypershift-e2e-aks	`d38f7bf`	link	true	`/test hypershift-e2e-aks`
ci/prow/verify	`d38f7bf`	link	true	`/test verify`
ci/prow/e2e-azure-ovn	`d38f7bf`	link	false	`/test e2e-azure-ovn`

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-sigs/prow repository. I understand the commands that are listed here.

rikatz · 2025-08-14T13:02:05Z

pkg/operator/controller/dns/controller.go

+	}
+	oldestExistingRecord := iov1.DNSRecord{}
+	for _, existingRecord := range otherRecords.Items {
+		if oldestExistingRecord.CreationTimestamp.IsZero() || existingRecord.CreationTimestamp.Before(&oldestExistingRecord.CreationTimestamp) {


as a suggestion, first check if existingRecord is marked for deletion, so you don't consider it for reconciliation if it is also on the queue for removal:

if !existingRecord.DeletionTimestamp.IsZero() { continue }

That seems reasonable. On the one hand, as noted in #1229 (comment), the reconciliation triggers when the DNSRecord CR is actually deleted, not merely marked for deletion. On the other hand, we could be in a situation where multiple dnsrecords are being deleted at the same time, in which case your suggestion might avoid some unnecessary reconciliations.

yeap, I should have been more explicit but my idea here is to ignore any other DNS record that would be considered older but also being on the deletion queue.

rikatz · 2025-08-14T13:22:57Z

pkg/operator/controller/dns/controller.go

+	for _, existingRecord := range records.Items {
+		// we only care if the domain name is published by a different record, so ignore the matching record if it
+		// already exists.
+		// TODO: There's got to be a better way to match the same object


You can match the UID here. I would say that during admission webhooks matching UID is a bad idea, as the UID will be added once persisted by apiserver, but in case of a reconciliation process it means that the UID was already generated and cannot be changed anymore.

rikatz · 2025-08-14T13:29:57Z

pkg/operator/controller/dns/controller_test.go

+	iov1.AddToScheme(scheme)
+	fakeClient := fake.NewClientBuilder().
+		WithScheme(scheme).
+		WithIndex(&iov1.DNSRecord{}, dnsRecordIndexFieldName, func(o client.Object) []string {


nit (no need to fix now!): do we care about making this indexer function some sort of utils/specific function that can be used both on the operatorCache.IndexField and on fakeCache to keep consistency?

Do you mean a util function that the controller logic and test logic would share? That does make sense, though I do caution against re-using controller logic in tests if doing so could mask a defect in the controller logic.

yes, the idea was to make some util/shared function, but also I see your concerns here, so makes sense also to not share and in case something changes on the main reconciliation logic, the test that has a different cache logic will catch the regression.

candita · 2025-08-20T15:21:03Z

/assign @rikatz

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 7, 2025

Miciah reviewed May 7, 2025

View reviewed changes

Don't publish duplicate DNS records

d38f7bf

Before attempting to publish a domain to a zone, check if that domain is already being published to the same zone.

rfredette force-pushed the no-conflicting-dns branch from 9fece4b to d38f7bf Compare May 15, 2025 17:33

Miciah reviewed May 15, 2025

View reviewed changes

rfredette commented May 15, 2025

View reviewed changes

Miciah reviewed May 15, 2025

View reviewed changes

rfredette changed the title ~~Don't publish duplicate DNS records~~ OCPBUGS-31521: Don't publish duplicate DNS records Aug 13, 2025

rfredette marked this pull request as ready for review August 13, 2025 21:21

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 13, 2025

openshift-ci bot requested review from miheer and Thealisyed August 13, 2025 21:22

rfredette mentioned this pull request Aug 13, 2025

OCPBUGS-31521: Don't allow DNS records for gateway to be created if they clash with any existing ingress controller. #1145

Closed

rikatz reviewed Aug 14, 2025

View reviewed changes

openshift-ci bot assigned rikatz Aug 20, 2025

	} else if isDomainPublished, err = domainIsAlreadyPublishedInZone(context.Background(), r.cache, record, &zones[i]); err != nil {
	} else if isDomainPublished, err := domainIsAlreadyPublishedInZone(context.Background(), r.cache, record, &zones[i]); err != nil {

	log.Info("failed to read DNS record")
	log.Infof("Got unexpected object; expected type DNSRecord, got type %T", o)

	log.Info("failed to read DNS record")
	log.Error(nil, "Got unexpected type of object", "expected", "DNSRecord", "actual", fmt.Sprintf("%T", o))

	func (r *reconciler) MapOnRecordDelete(ctx context.Context, o client.Object) []reconcile.Request {
	func (r *reconciler) mapOnRecordDelete(ctx context.Context, o client.Object) []reconcile.Request {

OCPBUGS-31521: Don't publish duplicate DNS records #1229

Are you sure you want to change the base?

OCPBUGS-31521: Don't publish duplicate DNS records #1229

Uh oh!

Conversation

rfredette commented May 7, 2025

Uh oh!

openshift-ci bot commented May 7, 2025

Uh oh!

openshift-ci bot commented May 7, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Miciah May 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Miciah May 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Miciah May 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openshift-ci-robot commented Aug 13, 2025

Uh oh!

openshift-ci bot commented Aug 14, 2025

Uh oh!

rikatz Aug 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

candita commented Aug 20, 2025

Uh oh!

Uh oh!

Miciah May 7, 2025 •

edited

Loading

Miciah May 7, 2025 •

edited

Loading

Miciah May 15, 2025 •

edited

Loading

rikatz Aug 14, 2025 •

edited

Loading