OCPBUGS-31521: Don't publish duplicate DNS records#1229
OCPBUGS-31521: Don't publish duplicate DNS records#1229rfredette wants to merge 1 commit intoopenshift:masterfrom
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
| 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 |
There was a problem hiding this comment.
Compare by UID instead of name.
There was a problem hiding this comment.
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
| } | ||
| } else if isRecordPublished { | ||
| condition, err = r.replacePublishedRecord(zones[i], record) | ||
| } else if isDomainPublished, err = domainIsAlreadyPublishedInZone(context.Background(), r.cache, record, &zones[i]); err != nil { |
There was a problem hiding this comment.
You don't need to declare isDomainPublished outside the else if clause. Better to keep isDomainPublished and err scoped to the else if clauses.
| } 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.
rfredette
force-pushed
the
no-conflicting-dns
branch
from
9fece4b to
d38f7bf
Compare
| 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") |
There was a problem hiding this comment.
| log.Info("failed to read DNS record") | |
| log.Infof("Got unexpected object; expected type DNSRecord, got type %T", o) |
There was a problem hiding this comment.
Or something more like this:
| log.Info("failed to read DNS record") | |
| log.Error(nil, "Got unexpected type of object", "expected", "DNSRecord", "actual", fmt.Sprintf("%T", o)) |
| // 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{ |
There was a problem hiding this comment.
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.
| // 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 }, |
There was a problem hiding this comment.
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).
| // Test_publishRecordToZonesMergesStatus verifies that publishRecordToZones | ||
| // correctly merges status updates. | ||
| func TestPublishRecordToZonesMergesStatus(t *testing.T) { | ||
| func Test_publishRecordToZonesMergesStatus(t *testing.T) { |
There was a problem hiding this comment.
TestPublishRecordToZonesMergesStatus is an appropriate name for the test as there is no publishRecordToZonesMergesStatus function.
There was a problem hiding this comment.
Probably I am also missing something here, but is there a new test case that will check if the condition is set?
rfredette
changed the title
openshift-ci-robot
added
jira/severity-moderate
|
@rfredette: This pull request references Jira Issue OCPBUGS-31521, 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. |
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
| iov1.AddToScheme(scheme) | ||
| fakeClient := fake.NewClientBuilder(). | ||
| WithScheme(scheme). | ||
| WithIndex(&iov1.DNSRecord{}, dnsRecordIndexFieldName, func(o client.Object) []string { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
The intent here is to use the same index function that was added in lines 117-122. Since the fake cache doesn't go through all the setup steps that the actual one does, it needed to be added manually. In this case, having the logic match what's used in the actual controller probably is the way to go.
|
/assign @rikatz |
rfredette
force-pushed
the
no-conflicting-dns
branch
from
d38f7bf to
e48bfe1
Compare
|
[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. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
rfredette
force-pushed
the
no-conflicting-dns
branch
from
e48bfe1 to
4822baa
Compare
|
/cc |
| oldestExistingRecord := iov1.DNSRecord{} | ||
| for _, existingRecord := range otherRecords.Items { | ||
| // Exclude records that are marked for deletion. | ||
| if !existingRecord.DeletionTimestamp.IsZero() { |
There was a problem hiding this comment.
just for safety (sorry for realizing this just now): DeletionTimestamp is a nullable field / a pointer (https://github.com/kubernetes/apimachinery/blob/d74026bbe3beeff64c3dc7259a29be7708aa834f/pkg/apis/meta/v1/types.go#L209) and as so, I would recommend checking if it is null, and then checking if it is zero.
There was a problem hiding this comment.
The IsZero method has an nil check on its receiver, so I think the caller can omit the nil check?
I would be happy with a unit test case in lieu of a nil check.
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@Miciah: This pull request references Jira Issue OCPBUGS-31521, which is valid. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira (iamin@redhat.com), skipping review request. 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. |
|
marking bug as verified, since it is fixed through pre-merge testing
/verified by rhamini3 |
openshift-ci-robot
added
the
verified
|
@rhamini3: This PR has been marked as verified by 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. |
|
E2E test failure seen on this, reported by @rikatz in https://issues.redhat.com/browse/OCPBUGS-64675.
|
|
/retest |
|
/retest-required |
|
we may want to revive this soon. @candita @rfredette do you folks think this can be worked next week, if Ryan has the capacity? Otherwise I think we should prioritize it for 4.23 as this is long overdue. thanks!! |
rfredette
force-pushed
the
no-conflicting-dns
branch
from
4822baa to
0dbf8fe
Compare
openshift-ci-robot
removed
the
verified
📝 WalkthroughWalkthroughThe DNS controller now indexes DNSRecord.Spec.DNSName and validates domain conflicts before publishing. publishRecordToZones performs pre-publish checks that set DNSRecordPublished to InternalError or DomainAlreadyInUse on failure and avoid publishing when a conflict exists. Delete events use a specialized map function to enqueue the oldest matching non-unmanaged record for reconciliation. Tests were added/updated with a fake controller-runtime cache and conflict-focused scenarios. 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/operator/controller/dns/controller.go`:
- Around line 426-453: The current domainIsAlreadyPublishedToZone uses
recordIsAlreadyPublishedToZone (which reads a Published=True status) so races
allow duplicate publishes; change the guard to determine single ownership by
comparing eligible records' identity/order instead of published status: in
domainIsAlreadyPublishedToZone (and when iterating records.Items) ignore records
that are not eligible (e.g., with deletionTimestamp set or that cannot be
authoritative), then find the canonical owner for record.Spec.DNSName by
choosing the record with the earliest CreationTimestamp (tie-break by UID) and
return true (block) if any other eligible record is earlier than the current
record. Replace the call to recordIsAlreadyPublishedToZone with this
order/ownership check so Ensure cannot publish when a prior eligible record
exists.
- Around line 663-680: The current requeue logic selects the oldest non-deleting
record (oldestExistingRecord) from otherRecords.Items without checking whether
that record is eligible to publish (e.g. UnmanagedDNS), which can requeue an
ineligible record and leave a publishable record stranded; update the selection
loop to first filter otherRecords.Items to only include eligible records (skip
those with DeletionTimestamp set and skip records marked UnmanagedDNS or
otherwise known-unable-to-publish), then pick the oldest CreationTimestamp among
that filtered set (the variable oldestExistingRecord), and if no eligible
records remain return an empty []reconcile.Request rather than a request for an
ineligible record. Ensure the logic references otherRecords.Items,
oldestExistingRecord and the UnmanagedDNS/eligibility check used elsewhere in
this controller.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 3dcd6230-2887-4bd2-940f-260c2abd37f8
📒 Files selected for processing (2)
pkg/operator/controller/dns/controller.gopkg/operator/controller/dns/controller_test.go
| // domainIsAlreadyPublishedToZone returns true if the domain name in the | ||
| // provided DNSRecord is already published by another existing dnsRecord. | ||
| func domainIsAlreadyPublishedToZone(ctx context.Context, cache cache.Cache, record *iov1.DNSRecord, zone *configv1.DNSZone) (bool, error) { | ||
| records := iov1.DNSRecordList{} | ||
| if err := cache.List(ctx, &records, client.MatchingFields{dnsRecordIndexFieldName: record.Spec.DNSName}); err != nil { | ||
| return false, err | ||
| } | ||
|
|
||
| if len(records.Items) == 0 { | ||
| log.Info(fmt.Sprintf("No existing records found for domain %q", record.Spec.DNSName)) | ||
| return false, nil | ||
| } | ||
|
|
||
| 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. | ||
| if record.UID == existingRecord.UID { | ||
| continue | ||
| } | ||
| if record.Spec.DNSName != existingRecord.Spec.DNSName { | ||
| continue | ||
| } | ||
| if recordIsAlreadyPublishedToZone(&existingRecord, zone) { | ||
| return true, nil | ||
| } | ||
| } | ||
| return false, nil | ||
| } |
There was a problem hiding this comment.
This guard still allows duplicate publishes on stale status.
A conflicting record only blocks once its cached zone condition is already Published=True. Because Ensure runs before the later status write/cache update, a second record can still pass this check and publish the same DNS name under back-to-back reconciles. If the intended policy is single ownership per DNS name, the guard needs to key off another eligible record's existence/order, not post-publish status.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/operator/controller/dns/controller.go` around lines 426 - 453, The
current domainIsAlreadyPublishedToZone uses recordIsAlreadyPublishedToZone
(which reads a Published=True status) so races allow duplicate publishes; change
the guard to determine single ownership by comparing eligible records'
identity/order instead of published status: in domainIsAlreadyPublishedToZone
(and when iterating records.Items) ignore records that are not eligible (e.g.,
with deletionTimestamp set or that cannot be authoritative), then find the
canonical owner for record.Spec.DNSName by choosing the record with the earliest
CreationTimestamp (tie-break by UID) and return true (block) if any other
eligible record is earlier than the current record. Replace the call to
recordIsAlreadyPublishedToZone with this order/ownership check so Ensure cannot
publish when a prior eligible record exists.
| oldestExistingRecord := iov1.DNSRecord{} | ||
| for _, existingRecord := range otherRecords.Items { | ||
| // Exclude records that are marked for deletion. | ||
| if !existingRecord.DeletionTimestamp.IsZero() { | ||
| continue | ||
| } | ||
| if oldestExistingRecord.CreationTimestamp.IsZero() || existingRecord.CreationTimestamp.Before(&oldestExistingRecord.CreationTimestamp) { | ||
| oldestExistingRecord = existingRecord | ||
| } | ||
| } | ||
| return []reconcile.Request{ | ||
| { | ||
| NamespacedName: types.NamespacedName{ | ||
| Name: oldestExistingRecord.Name, | ||
| Namespace: oldestExistingRecord.Namespace, | ||
| }, | ||
| }, | ||
| } |
There was a problem hiding this comment.
Only requeue an eligible replacement record here.
This currently picks the oldest non-deleting record even if it is UnmanagedDNS and can never publish. In a sequence like unmanaged A → published B → blocked managed C, deleting B requeues A and leaves C stranded with no follow-up event. Once you filter to records that can actually publish, also return no requests when none remain.
🐛 Possible fix
- oldestExistingRecord := iov1.DNSRecord{}
- for _, existingRecord := range otherRecords.Items {
+ var oldestExistingRecord *iov1.DNSRecord
+ for i := range otherRecords.Items {
+ existingRecord := &otherRecords.Items[i]
// Exclude records that are marked for deletion.
if !existingRecord.DeletionTimestamp.IsZero() {
continue
}
- if oldestExistingRecord.CreationTimestamp.IsZero() || existingRecord.CreationTimestamp.Before(&oldestExistingRecord.CreationTimestamp) {
- oldestExistingRecord = existingRecord
+ if existingRecord.Spec.DNSManagementPolicy == iov1.UnmanagedDNS {
+ continue
+ }
+ if oldestExistingRecord == nil || existingRecord.CreationTimestamp.Before(&oldestExistingRecord.CreationTimestamp) {
+ oldestExistingRecord = existingRecord
}
}
+ if oldestExistingRecord == nil {
+ return nil
+ }
return []reconcile.Request{
{
NamespacedName: types.NamespacedName{
Name: oldestExistingRecord.Name,
Namespace: oldestExistingRecord.Namespace,
},
},
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/operator/controller/dns/controller.go` around lines 663 - 680, The
current requeue logic selects the oldest non-deleting record
(oldestExistingRecord) from otherRecords.Items without checking whether that
record is eligible to publish (e.g. UnmanagedDNS), which can requeue an
ineligible record and leave a publishable record stranded; update the selection
loop to first filter otherRecords.Items to only include eligible records (skip
those with DeletionTimestamp set and skip records marked UnmanagedDNS or
otherwise known-unable-to-publish), then pick the oldest CreationTimestamp among
that filtered set (the variable oldestExistingRecord), and if no eligible
records remain return an empty []reconcile.Request rather than a request for an
ineligible record. Ensure the logic references otherRecords.Items,
oldestExistingRecord and the UnmanagedDNS/eligibility check used elsewhere in
this controller.
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
0dbf8fe to
793a451
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/operator/controller/dns/controller.go (1)
663-684:⚠️ Potential issue | 🟡 MinorReturn empty slice when no eligible record exists.
After filtering out records that are being deleted (line 666) or are unmanaged (line 670), if no eligible records remain,
oldestExistingRecordstays as a zero-value struct with emptyNameandNamespace. Lines 677-684 then return a request for this empty record, which wastes reconciliation resources.🛡️ Proposed fix
oldestExistingRecord := iov1.DNSRecord{} for _, existingRecord := range otherRecords.Items { // Exclude records that are marked for deletion. if !existingRecord.DeletionTimestamp.IsZero() { continue } // Exclude unmanaged DNS records. if existingRecord.Spec.DNSManagementPolicy == iov1.UnmanagedDNS { continue } if oldestExistingRecord.CreationTimestamp.IsZero() || existingRecord.CreationTimestamp.Before(&oldestExistingRecord.CreationTimestamp) { oldestExistingRecord = existingRecord } } + // No eligible record found to requeue. + if oldestExistingRecord.Name == "" { + return []reconcile.Request{} + } return []reconcile.Request{ { NamespacedName: types.NamespacedName{ Name: oldestExistingRecord.Name, Namespace: oldestExistingRecord.Namespace, }, }, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/operator/controller/dns/controller.go` around lines 663 - 684, The loop that computes oldestExistingRecord (iterating otherRecords.Items and filtering by DeletionTimestamp and Spec.DNSManagementPolicy) can leave oldestExistingRecord as the zero-value DNSRecord, causing a reconcile.Request with empty Name/Namespace to be returned; update the code after the loop to check whether oldestExistingRecord.CreationTimestamp.IsZero() (or Name/Namespace empty) and if so return an empty []reconcile.Request{}, otherwise build and return the reconcile.Request using oldestExistingRecord.Name and .Namespace. Ensure the check references oldestExistingRecord, otherRecords, and does not create a request when no eligible record exists.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/operator/controller/dns/controller.go`:
- Around line 663-684: The loop that computes oldestExistingRecord (iterating
otherRecords.Items and filtering by DeletionTimestamp and
Spec.DNSManagementPolicy) can leave oldestExistingRecord as the zero-value
DNSRecord, causing a reconcile.Request with empty Name/Namespace to be returned;
update the code after the loop to check whether
oldestExistingRecord.CreationTimestamp.IsZero() (or Name/Namespace empty) and if
so return an empty []reconcile.Request{}, otherwise build and return the
reconcile.Request using oldestExistingRecord.Name and .Namespace. Ensure the
check references oldestExistingRecord, otherRecords, and does not create a
request when no eligible record exists.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 669de4c2-b3a1-4111-9891-1fb7b543fc2c
📒 Files selected for processing (2)
pkg/operator/controller/dns/controller.gopkg/operator/controller/dns/controller_test.go
|
/cc will review again soon! |
|
@rfredette: The following tests 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. |
|
@coderabbitai given it has been too long since you reviewed this PR, I want you to review again. You must report any critical or major finding, you must do a review line by line. Please report any possible inconsistency or behavior that can cause disruption, corruption. Report as much findings as possible, do not leave any review to be verified on a future commit, I want you to do a single report with EVERYTHING that must be fixed on a single review shot |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 15938 🏁 Script executed: Length of output: 36999 🏁 Script executed: Length of output: 7056
🔴 Critical BugsBug 1 —
|
| Scenario | Expected result |
|---|---|
| No matching records | []reconcile.Request{} |
| All matching records are terminating | []reconcile.Request{} ← Bug 1 scenario |
| All matching records are unmanaged | []reconcile.Request{} ← Bug 1 scenario |
| Multiple matching records, pick oldest | request for the oldest record |
| Deleted record itself is still in cache | must exclude it |
Issue 8 — TOCTOU race condition between conflict check and publish
File: pkg/operator/controller/dns/controller.go
} else if isDomainPublished, err := domainIsAlreadyPublishedToZone(...); err != nil {
...
} else if isDomainPublished {
...
} else {
condition, err = r.publishRecord(zones[i], record)
...
}Between the moment domainIsAlreadyPublishedToZone returns false and r.publishRecord is called, another record could be published to the same zone and domain. This is a classic check-then-act race. While the practical frequency is low for DNS record creation, it can lead to duplicate DNS records being published if two gateways/ingresscontrollers are created concurrently with the same domain. This should at minimum be documented, and ideally caught at the DNS provider level.
🟡 Significant Observations
Issue 9 — dnsRecord test helper creates records without a Namespace
File: pkg/operator/controller/dns/controller_test.go
func dnsRecord(name string, ...) *iov1.DNSRecord {
return &iov1.DNSRecord{
ObjectMeta: metav1.ObjectMeta{
Name: fmt.Sprintf("record-%s", name),
UID: uuid.NewUUID(),
// ⚠️ No Namespace set
},
...
}
}Real DNS records live in openshift-ingress-operator. The absence of a namespace in test helpers means that cross-namespace conflict scenarios (e.g., two records in different namespaces with the same domain name) are not tested. The index dnsRecordIndexFieldName is namespace-agnostic (it only indexes by Spec.DNSName), so a record in namespace A could block a record in namespace B — this behavior should be explicitly tested.
Issue 10 — mapOnRecordDelete silently swallows cache.List errors
File: pkg/operator/controller/dns/controller.go
if err := r.cache.List(ctx, &otherRecords, client.MatchingFields{dnsRecordIndexFieldName: deletedRecord.Spec.DNSName}); err != nil {
log.Error(err, "failed to list DNS records")
return []reconcile.Request{} // ⚠️ Silently returns, no retry
}When cache.List fails here, the error is logged but no reconcile request is returned. There is no mechanism to retry the lookup. Combined with Issue 3, if this happens when a conflicting record is deleted, the blocked record stays in DomainAlreadyInUse forever. Consider returning a reconcile for the deleted record itself as a fallback, or at minimum logging a more prominent warning.
Issue 11 — UnmanagedDNS condition lost its LastTransitionTime initialization (behavioral regression)
File: pkg/operator/controller/dns/controller.go
-condition = iov1.DNSZoneCondition{
- ...
- LastTransitionTime: metav1.Now(),
-}
+condition = iov1.DNSZoneCondition{
+ ...
+ // No LastTransitionTime
+}The new InternalError and DomainAlreadyInUse conditions also omit LastTransitionTime. While mergeConditions does set it when adding new conditions, this is an inconsistency relative to replacePublishedRecord and publishRecord which initialize it explicitly. This is low risk but increases cognitive overhead.
Issue 12 — No e2e test covering the "conflicting then unblocking" lifecycle
The E2E test testGatewayAPIDNS/gatewayListenersWithOverlappingHostname panicked. Even after Bug 1 is fixed, there should be an explicit end-to-end test that verifies:
- Record A publishes successfully.
- Record B with the same domain gets
DomainAlreadyInUse. - Record A is deleted.
- Record B is eventually published (exercising
mapOnRecordDelete).
This lifecycle is the entire purpose of the PR and should be covered by at least one E2E test.
Summary Table
| # | Severity | Location | Description |
|---|---|---|---|
| 1 | 🔴 Critical | mapOnRecordDelete |
Returns empty-name reconcile request when all candidates filtered → E2E panic |
| 2 | 🔴 Critical | mapOnRecordDelete |
Does not exclude deleted record by UID |
| 3 | 🟠 Major | publishRecordToZones |
DomainAlreadyInUse never requeues → stuck records if mapOnRecordDelete fails |
| 4 | 🟠 Major | publishRecordToZones |
context.Background() instead of reconcile context |
| 5 | 🟠 Major | domainIsAlreadyPublishedToZone |
Redundant dead-code name check (index already filters by name) |
| 6 | 🟠 Major | domainIsAlreadyPublishedToZone |
Noisy log.Info on every successful reconcile |
| 7 | 🟠 Major | controller_test.go |
Zero unit tests for mapOnRecordDelete |
| 8 | 🟠 Major | publishRecordToZones |
TOCTOU race between conflict check and actual publish |
| 9 | 🟡 Significant | controller_test.go |
dnsRecord helper has no namespace; cross-namespace conflicts untested |
| 10 | 🟡 Significant | mapOnRecordDelete |
cache.List errors silently swallowed with no retry |
| 11 | 🟡 Significant | publishRecordToZones |
LastTransitionTime missing from new conditions |
| 12 | 🟡 Significant | E2E | No E2E test for full conflict-then-unblock lifecycle |
The immediate blocker is Bug 1 — the missing guard in mapOnRecordDelete when all candidates are filtered is causing the nil-pointer panic in E2E. This must be fixed before merge.
6 participants
Before attempting to publish a domain to a zone, check if that domain is already being published to the same zone.