Skip to content

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

Closed
wants to merge 15 commits into from

Conversation

7onn
Copy link
Contributor

@7onn 7onn commented May 5, 2025

Description

This will add two flags to be used on the Cloudflare provider. One for making comment and other tags 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

  • As a program flag --cloudflare-record-comment=test, --cloudflare-record-tags=kubernetes,external-dns
  • Or as Ingress annotations, taking precedence over the program args
annotations:
  external-dns.alpha.kubernetes.io/cloudflare-record-comment: "test"
  external-dns.alpha.kubernetes.io/cloudflare-record-tags: "kubernetes,external-dns,my-app"

Fixes #3934

Checklist

  • Unit tests updated
  • End user documentation updated

@k8s-ci-robot
Copy link
Contributor

[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 raffo 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:

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

Copy link

linux-foundation-easycla bot commented May 5, 2025

CLA Signed

The committers listed above are authorized under a signed CLA.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label May 5, 2025
@k8s-ci-robot
Copy link
Contributor

Welcome @7onn!

It looks like this is your first PR to kubernetes-sigs/external-dns 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/external-dns has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot
Copy link
Contributor

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels May 5, 2025
@7onn 7onn force-pushed the 7onn/cloudflare-tags-comments branch from b53e5f8 to 43e366b Compare May 6, 2025 00:04
@7onn 7onn force-pushed the 7onn/cloudflare-tags-comments branch from 43e366b to bf518c3 Compare May 6, 2025 00:12
@ivankatliarchuk
Copy link
Contributor

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 6, 2025
@7onn
Copy link
Contributor Author

7onn commented May 7, 2025

@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.

@ivankatliarchuk
Copy link
Contributor

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 go run main.go with required arguments, as long as cluster context is available from terminal


comment := p.RecordComment
tags := p.RecordTags
for i := range ep.ProviderSpecific {
Copy link
Contributor

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
	}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done :) bedfdae

comment = ep.ProviderSpecific[i].Value
}
if ep.ProviderSpecific[i].Name == source.CloudflareRecordTagsKey {
tags = strings.Split(ep.ProviderSpecific[i].Value, ",")
Copy link
Contributor

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

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

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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

Copy link
Contributor

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.

Copy link
Contributor Author

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

Copy link
Contributor

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.

Copy link
Contributor Author

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.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 11, 2025
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 11, 2025
@7onn 7onn force-pushed the 7onn/cloudflare-tags-comments branch from 3e43f17 to 35f3202 Compare May 11, 2025 12:34
@7onn 7onn requested a review from ivankatliarchuk May 11, 2025 12:35
Copy link
Contributor

@ivankatliarchuk ivankatliarchuk left a 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 {
Copy link
Contributor

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?

Copy link
Contributor Author

@7onn 7onn May 12, 2025

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

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.
image

image

Copy link
Contributor

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.

Copy link
Contributor Author

@7onn 7onn May 12, 2025

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.

Copy link
Contributor

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 😊

Copy link
Contributor

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

Copy link
Contributor Author

@7onn 7onn May 12, 2025

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.

Copy link
Contributor Author

@7onn 7onn May 12, 2025

Choose a reason for hiding this comment

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

Interestingly, I just discovered another infinite loop issue. This time, credited to the tag sorting :)

In cloudflare, I have
image

But In code:
image

So I have to sort in order to avoid this behavior.

@ivankatliarchuk
Copy link
Contributor

/test go

@k8s-ci-robot
Copy link
Contributor

@ivankatliarchuk: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test pull-external-dns-licensecheck
/test pull-external-dns-unit-test

Use /test all to run all jobs.

In response to this:

/test go

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.

@ivankatliarchuk
Copy link
Contributor

/test all

@ivankatliarchuk
Copy link
Contributor

/retest

@ivankatliarchuk
Copy link
Contributor

/label tide/merge-method-squash

@k8s-ci-robot k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label May 12, 2025
@7onn 7onn force-pushed the 7onn/cloudflare-tags-comments branch 2 times, most recently from fde20c3 to 052760b Compare May 12, 2025 19:28
Copy link
Contributor

@ivankatliarchuk ivankatliarchuk left a 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.

Comment on lines 875 to 903
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]
}
}
Copy link
Contributor

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

// 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)
}

Copy link
Contributor

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

Copy link
Contributor

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

Comment on lines +369 to +390
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
}

Copy link
Contributor

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

Copy link
Contributor

Choose a reason for hiding this comment

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

My advice wass wrong

Copy link
Contributor Author

@7onn 7onn May 13, 2025

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
@7onn 7onn force-pushed the 7onn/cloudflare-tags-comments branch from 052760b to 2b4c0ee Compare May 12, 2025 20:31
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 13, 2025
@7onn 7onn force-pushed the 7onn/cloudflare-tags-comments branch from cf1992c to 299db8d Compare May 13, 2025 11:21
Copy link
Contributor

@ivankatliarchuk ivankatliarchuk left a comment

Choose a reason for hiding this comment

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

}

// 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)
Copy link
Contributor

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
Copy link
Contributor

Choose a reason for hiding this comment

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

could be single line

@k8s-ci-robot
Copy link
Contributor

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.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 14, 2025
@ivankatliarchuk
Copy link
Contributor

With this bug #5359 (comment) wdyt, to split a pull request and add a support for clouflare comment first?

@7onn
Copy link
Contributor Author

7onn commented May 16, 2025

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.

@ivankatliarchuk
Copy link
Contributor

Review as part of this PR #5411 (comment)

@mloiseleur
Copy link
Collaborator

I close this PR since the Comments one has been merged and there is an other opened on Tags
Feel free to re-open it or open a new one if I missed something.
/close

@k8s-ci-robot
Copy link
Contributor

@mloiseleur: Closed this PR.

In response to this:

I close this PR since the Comments one has been merged and there is an other opened on Tags
Feel free to re-open it or open a new one if I missed something.
/close

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cloudflare: support comment field
5 participants