Skip to content
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

Gh49 distributed dns plan #79

Merged
merged 4 commits into from
Apr 20, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion internal/controller/dnsrecord_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -356,6 +356,7 @@ func (r *DNSRecordReconciler) applyChanges(ctx context.Context, dnsRecord *v1alp
Policies: []externaldnsplan.Policy{policy},
Current: zoneEndpoints,
Desired: specEndpoints,
Previous: statusEndpoints,
//Note: We can't just filter domains by `managedZone.Spec.DomainName` it needs to be the exact root domain for this particular record
DomainFilter: externaldnsendpoint.MatchAllDomainFilters{&rootDomainFilter},
ManagedRecords: managedDNSRecordTypes,
Expand All @@ -365,6 +366,10 @@ func (r *DNSRecordReconciler) applyChanges(ctx context.Context, dnsRecord *v1alp

plan = plan.Calculate()

if err = plan.ConflictError(); err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice so conflicts in this pr now also

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, when i was writing the issue for it (#85), i was looking into the best way to do it and pretty much just did it, so carried on. Kept it fairly simple anyway, just collect errors and expose them from the plan to query later.

Was thinking this morning i might make it more generic so we can just add all types of possible errors to it. We can use it for the missing managed record errors we want for the likes of default geo etc..

return noRequeueDuration, err
}

dnsRecord.Status.ValidFor = defaultRequeueTime.String()
dnsRecord.Status.QueuedAt = reconcileStart
if plan.Changes.HasChanges() {
Expand All @@ -380,7 +385,6 @@ func (r *DNSRecordReconciler) applyChanges(ctx context.Context, dnsRecord *v1alp
logger.Error(err, "Giving up on trying to maintain desired state of the DNS record - changes are being overridden")
return noRequeueDuration, err
}

}
dnsRecord.Status.ValidFor = validationRequeueTime.String()
setDNSRecordCondition(dnsRecord, string(v1alpha1.ConditionTypeReady), metav1.ConditionFalse, "AwaitingValidation", "Awaiting validation")
Expand Down
77 changes: 77 additions & 0 deletions internal/controller/dnsrecord_controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,14 @@ import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/utils/ptr"
"sigs.k8s.io/controller-runtime/pkg/client"
externaldnsendpoint "sigs.k8s.io/external-dns/endpoint"

"github.com/kuadrant/dns-operator/api/v1alpha1"
)

var _ = Describe("DNSRecordReconciler", func() {
var dnsRecord *v1alpha1.DNSRecord
var dnsRecord2 *v1alpha1.DNSRecord
var managedZone *v1alpha1.ManagedZone
var testNamespace string

Expand Down Expand Up @@ -62,6 +64,10 @@ var _ = Describe("DNSRecordReconciler", func() {
err := k8sClient.Delete(ctx, dnsRecord)
Expect(client.IgnoreNotFound(err)).ToNot(HaveOccurred())
}
if dnsRecord2 != nil {
err := k8sClient.Delete(ctx, dnsRecord)
Expect(client.IgnoreNotFound(err)).ToNot(HaveOccurred())
}
if managedZone != nil {
err := k8sClient.Delete(ctx, managedZone)
Expect(client.IgnoreNotFound(err)).ToNot(HaveOccurred())
Expand Down Expand Up @@ -159,4 +165,75 @@ var _ = Describe("DNSRecordReconciler", func() {
}, TestTimeoutLong, time.Second).Should(Succeed())
})

It("should not allow second record to change the type", func() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

dnsRecord2 = &v1alpha1.DNSRecord{
ObjectMeta: metav1.ObjectMeta{
Name: "foo2.example.com",
Namespace: testNamespace,
},
Spec: v1alpha1.DNSRecordSpec{
ManagedZoneRef: &v1alpha1.ManagedZoneReference{
Name: managedZone.Name,
},
Endpoints: []*externaldnsendpoint.Endpoint{
{
DNSName: "foo.example.com",
Targets: []string{
"v1",
},
RecordType: "CNAME",
SetIdentifier: "",
RecordTTL: 60,
Labels: nil,
ProviderSpecific: nil,
},
},
},
}
Expect(k8sClient.Create(ctx, dnsRecord2)).To(Succeed())
Eventually(func(g Gomega) {
err := k8sClient.Get(ctx, client.ObjectKeyFromObject(dnsRecord2), dnsRecord2)
g.Expect(err).NotTo(HaveOccurred())
g.Expect(dnsRecord2.Status.Conditions).To(
ContainElement(MatchFields(IgnoreExtras, Fields{
"Type": Equal(string(v1alpha1.ConditionTypeReady)),
"Status": Equal(metav1.ConditionFalse),
"Reason": Equal("ProviderError"),
"Message": ContainSubstring("record type conflict, cannot update 'foo.example.com' with record type 'CNAME' when record already exists with record type 'A'"),
"ObservedGeneration": Equal(dnsRecord2.Generation),
})),
)
}, TestTimeoutMedium, time.Second).Should(Succeed())
})

It("should not allow owned record to update it", func() {
dnsRecord2 = &v1alpha1.DNSRecord{
ObjectMeta: metav1.ObjectMeta{
Name: "foo2.example.com",
Namespace: testNamespace,
},
Spec: v1alpha1.DNSRecordSpec{
OwnerID: ptr.To("owner1"),
ManagedZoneRef: &v1alpha1.ManagedZoneReference{
Name: managedZone.Name,
},
Endpoints: getTestEndpoints(),
},
}
Expect(k8sClient.Create(ctx, dnsRecord2)).To(Succeed())
Eventually(func(g Gomega) {
err := k8sClient.Get(ctx, client.ObjectKeyFromObject(dnsRecord2), dnsRecord2)
g.Expect(err).NotTo(HaveOccurred())
g.Expect(dnsRecord2.Status.Conditions).To(
ContainElement(MatchFields(IgnoreExtras, Fields{
"Type": Equal(string(v1alpha1.ConditionTypeReady)),
"Status": Equal(metav1.ConditionFalse),
"Reason": Equal("ProviderError"),
"Message": ContainSubstring("owner conflict, cannot update 'foo.example.com' with owner when existing record is not owned"),
"ObservedGeneration": Equal(dnsRecord2.Generation),
})),
)
}, TestTimeoutMedium, time.Second).Should(Succeed())
})

})
6 changes: 6 additions & 0 deletions internal/external-dns/plan/labels.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
package plan

const (
// OwnerLabelDeliminator is a deliminator used between owners in the OwnerLabelKey value when multiple owners are assigned.
OwnerLabelDeliminator = "&&"
)
Loading
Loading