-
Notifications
You must be signed in to change notification settings - Fork 2.7k
feat(cloudflare): Enable DNS record Comment and Tags #5359
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
Conversation
[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.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Welcome @7onn! |
Hi @7onn. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
b53e5f8
to
43e366b
Compare
43e366b
to
bf518c3
Compare
/ok-to-test |
@ivankatliarchuk can you provide me some direction on how to build this container so I can use the image on my staging environment to test? :) Couldn't find anything on this repo. At least not searching quickly. |
Here is dev guide https://github.com/kubernetes-sigs/external-dns/blob/master/docs/contributing/dev-guide.md One of the option is not even build, just run |
provider/cloudflare/cloudflare.go
Outdated
|
||
comment := p.RecordComment | ||
tags := p.RecordTags | ||
for i := range ep.ProviderSpecific { |
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.
for _, providerSpecific := range ep.ProviderSpecific {
if providerSpecific.Name == source.CloudflareRecordCommentKey {
// Your logic here
}
}
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.
done :) bedfdae
provider/cloudflare/cloudflare.go
Outdated
comment = ep.ProviderSpecific[i].Value | ||
} | ||
if ep.ProviderSpecific[i].Name == source.CloudflareRecordTagsKey { | ||
tags = strings.Split(ep.ProviderSpecific[i].Value, ",") |
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 there is any validation for tags and comments that we could add? Is this functionality available to all subscriptions?
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.
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.
Ok so this only available for paid Tier.
Feels like this needs testing where or not is not breaking Free tier
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.
I agree.
Ideally, the limits can be read from the API for the specific zone and then the comment/tag verified against it.
If it doesn't match, I think a warning should be logged, but the record should still be created/updated.
I couldn't find it in the API documentation, so I've asked the Cloudflare support if that is possible.
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.
Very well raised. This would work for my case but not for people on a free Cloudflare account. I'll will look into such validations and try to make them happen dynamically so if Cloudflare changes, the software won't need to be changed. Thanks everyone.
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.
In order to prevent inappropriate tagging, do you think this is a good direction? 16d6bf7
I wonder how we'd apply the same safety filter for Comments as these are actually for everyone except varying on limited length
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.
I think that is a very good direction. I left a comment with 16d6bf7#r156796315 about the log level, other than that it looks good.
Cloudflare support confirmed to me there is no way to get the current subscription level, so for comments it's going to be more difficult.
I think the best value for time and complexity we can get here would be to skip and log a warning for anything over 500 characters.
For a Free zone, anything over 100 will then still error, so that specific error (I'd expect there to be a specific code or error message after all I know about the Cloudflare API) can be handled to print a
Comment for %s too long, see https://developers.cloudflare.com/dns/manage-dns-records/reference/record-attributes/#availability for character limits
This would have the benefit of preventing API errors for all paid plans, and if the limits change in the future, there would still be useful information in there since the only hardcoded value is the 500 character absolute maximum.
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.
Decreased the log level here d077169
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.
Nice, even better than what I had in mind.
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.
Had to bring it back though! Because if you put the value and it isn't suitable for you tier, it was producing an endless diff :/ Would welcome some suggestions on how to workaround this.
3e43f17
to
35f3202
Compare
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.
just one tiny thing left
We need some smoke testing, to make sure if tags-comments not set, the external-dns still works ;-)
providerSpecificAnnotations = append(providerSpecificAnnotations, endpoint.ProviderSpecificProperty{ | ||
Name: CloudflareRegionKey, | ||
Value: v, | ||
}) | ||
} | ||
|
||
if v, ok := ants[CloudflareRecordCommentKey]; ok { |
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.
source_test is not updated, can we cover added logic there?
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.
done bd63dc2
We need some smoke testing
Where is its Dockerfile? I'd like to build this and simply update my deployment's image in staging registry.k8s.io/external-dns/external-dns:v0.15.1 -> devbytom/external-dns:test
. Preferably, building similarly to the official way instead of building as I assume it should be.
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.
Have a loot this job https://github.com/kubernetes-sigs/external-dns/actions/runs/14976420658/job/42069819842?pr=5359. try it could be pullable
other approach is how to build image https://github.com/kubernetes-sigs/external-dns/blob/master/docs/contributing/dev-guide.md#building-local-images
how to run without doing build if you have kubectl access to cluster https://github.com/kubernetes-sigs/external-dns/blob/master/docs/contributing/dev-guide.md#execute-code-without-building-binary
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.
Tested in my staging cluster. Found an apparent bug. The records are stuck on a diff state and every minute, the app is updating it again.
The image is devbytom/external-dns:test-tom
built on top of this branch's HEAD with
KO_DOCKER_REPO=devbytom/external-dns \
VERSION=test-tom \
ko build --tags test-tom --bare --sbom none \
--image-label org.opencontainers.image.source="https://github.com/kubernetes-sigs/external-dns" \
--image-label org.opencontainers.image.revision=bd63dc21cbbfa4a1ac37e51a3cffae77a2e81aca \
--platform=linux/amd64,linux/arm64,linux/arm/v7 --push=true .
The app logs were
time="2025-05-12T16:55:19Z" level=info msg="GitCommitShort=unknown, GoVersion=go1.24.2, Platform=linux/amd64, UserAgent=ExternalDNS/test-tom"
time="2025-05-12T16:55:19Z" level=info msg="Instantiating new Kubernetes client"
time="2025-05-12T16:55:19Z" level=info msg="Using inCluster-config based on serviceaccount-token"
time="2025-05-12T16:55:19Z" level=info msg="Created Kubernetes client https://172.20.0.1:443"
time="2025-05-12T16:55:23Z" level=info msg="All records are already up to date"
time="2025-05-12T16:56:23Z" level=info msg="All records are already up to date"
time="2025-05-12T16:57:23Z" level=info msg="All records are already up to date"
time="2025-05-12T16:58:28Z" level=info msg="Changing record." action=UPDATE record=app.<redacted>.com ttl=1 type=CNAME zone=<redacted>
time="2025-05-12T16:58:30Z" level=info msg="Changing record." action=UPDATE record=app.<redacted>.com ttl=1 type=TXT zone=<redacted>
time="2025-05-12T16:58:32Z" level=info msg="Changing record." action=UPDATE record=cname-app.<redacted>.com ttl=1 type=TXT zone=<redacted>
time="2025-05-12T16:59:29Z" level=info msg="Changing record." action=UPDATE record=app.<redacted>.com ttl=1 type=CNAME zone=<redacted>
time="2025-05-12T16:59:31Z" level=info msg="Changing record." action=UPDATE record=app.<redacted>.com ttl=1 type=TXT zone=<redacted>
time="2025-05-12T16:59:33Z" level=info msg="Changing record." action=UPDATE record=cname-app.<redacted>.com ttl=1 type=TXT zone=<redacted>
... repeat
The records are correctly updated at least.

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.
Ignore the image. Images from pull requests not pushed to registries. You need to build one youerself. I'm usually just running code against sandbox clusters with fixtures.
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.
Replacing the container image and then updating existing Ingresses in staging via ArgoCD worked for me (or at least felt like the least effort). It was able to reveal this bug 🐛 . I need to understand what is holding the record on a diff state for the app to update it ceaselessly.
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.
A negative result is still a result. So from the code side of things it looks ok, but it does not do it correctly 😊
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.
It sounds like we do modification in the wrong location. You need to modify it in Records() method. What you are looking is to add tags and comments in this function groupByNameAndTypeWithCustomHostnames
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.
This have fixed the loop for good users: 052760b
But if you keep the len(comment) > 500
, the diff naturally keeps happening as it compares the full string configured with the trimmed one on Cloudflare.
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.
/test go |
@ivankatliarchuk: The specified target(s) for
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-sigs/prow repository. |
/test all |
/retest |
/label tide/merge-method-squash |
fde20c3
to
052760b
Compare
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.
The code works, I just not too sure if the changes are in the right location. If you ok, have a look at proposed changes. Just review them. For whatever reason cloudflare is quite different to other providers in terms of how Records and ApplyChanges are used.
provider/cloudflare/cloudflare.go
Outdated
comment := p.DNSRecordsConfig.Comment | ||
tags := p.DNSRecordsConfig.GetTags() | ||
for _, providerSpecific := range ep.ProviderSpecific { | ||
if providerSpecific.Name == source.CloudflareRecordCommentKey { | ||
comment = providerSpecific.Value | ||
} | ||
if providerSpecific.Name == source.CloudflareRecordTagsKey { | ||
tags = strings.Split(providerSpecific.Value, ",") | ||
} | ||
} | ||
|
||
// Free account checks | ||
if tags != nil || len(comment) > freeZoneCommentMaxLength { | ||
freeAccount := !p.ZoneHasPaidPlan(ep.DNSName) | ||
if freeAccount && tags != nil { | ||
log.Infof("DNS tags are only available for paid accounts, skipping for %s", ep.DNSName) | ||
tags = nil | ||
} | ||
if freeAccount && len(comment) > freeZoneCommentMaxLength { | ||
log.Infof("DNS record comment is limited to %d chars for free zones, trimming comment of %s", freeZoneCommentMaxLength, ep.DNSName) | ||
comment = comment[:freeZoneCommentMaxLength-1] | ||
} | ||
if len(comment) > paidZoneCommentMaxLength { | ||
log.Infof("DNS record comment is limited to %d chars, trimming comment of %s", paidZoneCommentMaxLength, ep.DNSName) | ||
comment = comment[:paidZoneCommentMaxLength-1] | ||
} | ||
} |
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.
Step 2. This will all get replaced with something like
if e.GetProviderSpecificProperty("cloudflare-is-subscribed") == "true" {
comment = extract from provider properties
tags = extract from provider properties
}
Example how simple is AWS Apply
external-dns/provider/aws/aws.go
Lines 637 to 652 in 5eb0c9f
// ApplyChanges applies a given set of changes in a given zone. | |
func (p *AWSProvider) ApplyChanges(ctx context.Context, changes *plan.Changes) error { | |
zones, err := p.zones(ctx) | |
if err != nil { | |
return provider.NewSoftErrorf("failed to list zones, not applying changes: %w", err) | |
} | |
updateChanges := p.createUpdateChanges(changes.UpdateNew, changes.UpdateOld) | |
combinedChanges := make(Route53Changes, 0, len(changes.Delete)+len(changes.Create)+len(updateChanges)) | |
combinedChanges = append(combinedChanges, p.newChanges(route53types.ChangeActionCreate, changes.Create)...) | |
combinedChanges = append(combinedChanges, p.newChanges(route53types.ChangeActionDelete, changes.Delete)...) | |
combinedChanges = append(combinedChanges, updateChanges...) | |
return p.submitChanges(ctx, combinedChanges, zones) | |
} |
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.
Have a look at method apply
func (p *CloudFlareProvider) ApplyChanges(ctx context.Context, changes *plan.Changes)
changes
should already contain all the required information
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.
Same. Advice is not correct
func (p *CloudFlareProvider) ZoneHasPaidPlan(hostname string) bool { | ||
zone, err := publicsuffix.EffectiveTLDPlusOne(hostname) | ||
if err != nil { | ||
log.Errorf("Failed to get effective TLD+1 for hostname %s %v", hostname, err) | ||
return false | ||
} | ||
zoneID, err := p.Client.ZoneIDByName(zone) | ||
if err != nil { | ||
log.Errorf("Failed to get zone %s by name %v", zone, err) | ||
return false | ||
} | ||
|
||
zoneDetails, err := p.Client.ZoneDetails(context.Background(), zoneID) | ||
if err != nil { | ||
log.Errorf("Failed to get zone %s details %v", zone, err) | ||
return false | ||
} | ||
|
||
return zoneDetails.Plan.IsSubscribed | ||
} | ||
|
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.
Step 3. This no longer required
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.
My advice wass wrong
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.
I ended up testing just to understand more. But indeed, the WithProviderSpecific
in groupByNameAndTypeWithCustomHostnames()
wasn't turned available in newCloudFlareChange()
, so it was impossible to read wether the zone was subscribed. So I brought the function back. But I'm still stuck with endless updates on the tagged resource 🤔 Even after sorting the user tags input to make sure we'll sort alphabetically like Cloudflare does. I'm suspecting of this function that compares endpoints: https://github.com/7onn/external-dns/blob/299db8d0c54fad2a75e04881d53098c0161bbb3a/internal/testutils/endpoint.go#L119
If I sort the annotation myself,
external-dns.alpha.kubernetes.io/cloudflare-record-tags: editor,external-dns
# instead of
external-dns.alpha.kubernetes.io/cloudflare-record-tags: external-dns,editor
I get the desired time="2025-05-13T11:34:19Z" level=info msg="All records are already up to date"
* Avoid infinite loop of updates
052760b
to
2b4c0ee
Compare
cf1992c
to
299db8d
Compare
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.
I would say documentation is missing . Rest seems lgtm
https://github.com/kubernetes-sigs/external-dns/blob/master/docs/tutorials/cloudflare.md
} | ||
|
||
// Free account checks | ||
if tags != nil || len(comment) > freeZoneCommentMaxLength { | ||
free := !p.ZoneHasPaidPlan(ep.DNSName) | ||
if free && tags != nil { | ||
log.Warnf("DNS tags are only available for paid accounts, skipping for %s. Please remove it from the config to avoid endless syncs.", ep.DNSName) | ||
log.Infof("DNS tags are only available for paid accounts, skipping for %s.", ep.DNSName) |
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.
It's a warning
return nil | ||
} | ||
tags := strings.Split(c.Tags, ",") | ||
return tags |
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.
could be single line
PR needs rebase. 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. |
With this bug #5359 (comment) wdyt, to split a pull request and add a support for clouflare comment first? |
@ivankatliarchuk I have split it: https://github.com/kubernetes-sigs/external-dns/pull/5411/files this way we can focus on what the open issue wants; and then I'll focus on my wanted tags on a smaller-scoped PR. |
Review as part of this PR #5411 (comment) |
I close this PR since the Comments one has been merged and there is an other opened on Tags |
@mloiseleur: Closed this PR. 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-sigs/prow repository. |
Description
This will add two flags to be used on the Cloudflare provider. One for making
comment
and othertags
available for DNS records. That's helpful when you already have a lot of records and you'd like to track what external-dns is provisioning there.Both comment and tags can be set either
--cloudflare-record-comment=test
,--cloudflare-record-tags=kubernetes,external-dns
Fixes #3934
Checklist