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.

Sign up for GitHub

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

feat: additional template functions #3949

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

feat: additional template functions #3949

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
8d52be2
template functions: replaceAll, isIPv6, isIPv4
matkam Sep 21, 2023
b37cb51
typeo
matkam Sep 21, 2023
b5a6cbd
unit test
matkam Sep 22, 2023
3f77b0a
docs
matkam Sep 22, 2023
5798069
johngmyers review: use sprig, fix ipv4 check
matkam Oct 27, 2023
db35373
update test and docs
matkam Oct 27, 2023
ae43729
Update docs/tutorials/nodes.md
matkam Nov 10, 2023
db55aa1
document isIPv4 and isIPv6 template functions
matkam Feb 12, 2024
a6851a2
Merge branch 'master' into master
matkam Jan 7, 2025
664ea01
use netip to determine ipv4 and ipv6
matkam Jan 7, 2025
20e5eb9
slim-sprig v3
matkam Jan 7, 2025
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
5 changes: 3 additions & 2 deletions docs/sources/nodes.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ Using nodes (`--source=node`) as source is possible to synchronize a DNS zone wi
The node source adds an `A` record per each node `externalIP` (if not found, any IPv4 `internalIP` is used instead).
It also adds an `AAAA` record per each node IPv6 `internalIP`.
The TTL of the records can be set with the `external-dns.alpha.kubernetes.io/ttl` node annotation.
The FQDN template provides more than 100+ functions, documented [here](https://go-task.github.io/slim-sprig/). For instance, it includes a function to replace all `.` with `-` in the node name, which can be useful with cloud providers that include dots in the node name. There are two additional functions available on top of the standard sprig functions: `isIPv4` and `isIPv6`. The functions can be used to test a string for being an IPv4 or IPv6 address.
Copy link
Contributor

@ivankatliarchuk ivankatliarchuk Jan 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

could we also have a markdown table with all currently supported or added functions, description and short usage example?


Nodes marked as **Unschedulable** as per [core/v1/NodeSpec](https://pkg.go.dev/k8s.io/api@v0.31.1/core/v1#NodeSpec) are excluded.
This avoid exposing Unhealthy, NotReady or SchedulingDisabled (cordon) nodes.
Expand Down Expand Up @@ -40,7 +41,7 @@ spec:
- --domain-filter=external-dns-test.my-org.com
- --aws-zone-type=public
- --registry=txt
- --fqdn-template={{.Name}}.external-dns-test.my-org.com
- --fqdn-template={{.Name | replace "." "-"}}.external-dns-test.my-org.com
- --txt-owner-id=my-identifier
- --policy=sync
- --log-level=debug
Expand Down Expand Up @@ -111,7 +112,7 @@ spec:
- --domain-filter=external-dns-test.my-org.com
- --aws-zone-type=public
- --registry=txt
- --fqdn-template={{.Name}}.external-dns-test.my-org.com
- --fqdn-template={{.Name | replace "." "-"}}.external-dns-test.my-org.com
Copy link
Contributor

Choose a reason for hiding this comment

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

I never tried this fqdn-template, could you create a page or a section with bit more information about this fqdn-template and it's use cases and advantages?

I can see an example for raw kubernetes manifest. How this behave in helm chart? as the syntax is very similar. I would assume helm generation will fail, am I correct?

- --txt-owner-id=my-identifier
- --policy=sync
- --log-level=debug
Expand Down
1 change: 1 addition & 0 deletions go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ require (
github.com/ffledgling/pdns-go v0.0.0-20180219074714-524e7daccd99
github.com/go-gandi/go-gandi v0.7.0
github.com/go-logr/logr v1.4.2
github.com/go-task/slim-sprig/v3 v3.0.0
Copy link
Contributor

Choose a reason for hiding this comment

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

the only concern is that the libary had last release 2+ years ago in 2023. Could you justify/convince the library maintainer to

  • refresh a release tag
  • refresh go.mod with go.sum
  • would be nice to sync go version as well, but not critical

have you checked this library for known vulnerabilities?

trivy repo --scanners vuln .

or with snyk open source checkers

Copy link
Contributor

Choose a reason for hiding this comment

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

This actually quite tricky. For example github.com/Masterminds/sprig is heavier, but it has 4k stars and almost ~100 contributors.... So worth to share a comparison on why this library vs upstream one

Copy link
Author

Choose a reason for hiding this comment

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

It doesn't make a difference for my use case. I only need the replace function. Context here. Would you prefer importing github.com/Masterminds/sprig instead, or even creating a similar function here and avoiding the import altogether?

Copy link
Contributor

Choose a reason for hiding this comment

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

I have no opinion. If you refactor logic as per review, and cover functions with tests, we will decide on a library at the very end. Both libraries have they advantages. Or as you mention, we may not need any libraries at this time, as only few functions required.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you only need one function - so no import required. Better to creat own function. It's more about compatibility, we should have tests for all the functions provided.

github.com/google/go-cmp v0.6.0
github.com/google/uuid v1.6.0
github.com/gophercloud/gophercloud v1.14.1
Expand Down
1 change: 0 additions & 1 deletion go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -426,7 +426,6 @@ github.com/go-sql-driver/mysql v1.4.0/go.mod h1:zAC/RDZ24gD3HViQzih4MyKcchzm+sOG
github.com/go-sql-driver/mysql v1.4.1/go.mod h1:zAC/RDZ24gD3HViQzih4MyKcchzm+sOG5ZlKdlhCg5w=
github.com/go-stack/stack v1.8.0/go.mod h1:v0f6uXyyMGvRgIKkXu+yp6POWl0qKG85gN/melR3HDY=
github.com/go-task/slim-sprig v0.0.0-20210107165309-348f09dbbbc0/go.mod h1:fyg7847qk6SyHyPtNmDHnmrv/HOrqktSC+C9fM+CJOE=
github.com/go-task/slim-sprig v0.0.0-20230315185526-52ccab3ef572 h1:tfuBGBXKqDEevZMzYi5KSi8KkcZtzBcTgAUUtapy0OI=
github.com/go-task/slim-sprig/v3 v3.0.0 h1:sUs3vkvUymDpBKi3qH1YSqBQk9+9D/8M2mN1vB6EwHI=
github.com/go-task/slim-sprig/v3 v3.0.0/go.mod h1:W848ghGpv3Qj3dhTPRyJypKRiqCdHZiAzKg9hl15HA8=
github.com/gobuffalo/envy v1.7.0/go.mod h1:n7DRkBerg/aorDM8kbduw5dN3oXGswK5liaSCx4T5NI=
Expand Down
5 changes: 5 additions & 0 deletions source/node_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,11 @@ func testNodeSourceNewNodeSource(t *testing.T) {
expectError: false,
fqdnTemplate: "{{.Name}}-{{.Namespace}}.ext-dns.test.com",
},
{
title: "complex template",
expectError: false,
fqdnTemplate: "{{range .Status.Addresses}}{{if and (eq .Type \"ExternalIP\") (isIPv4 .Address)}}{{.Address | replace \".\" \"-\"}}{{break}}{{end}}{{end}}.ext-dns.test.com",
},
{
title: "non-empty annotation filter label",
expectError: false,
Expand Down
28 changes: 25 additions & 3 deletions source/source.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import (
"time"
"unicode"

sprig "github.com/go-task/slim-sprig/v3"
log "github.com/sirupsen/logrus"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/labels"
Expand Down Expand Up @@ -147,9 +148,11 @@ func parseTemplate(fqdnTemplate string) (tmpl *template.Template, err error) {
if fqdnTemplate == "" {
return nil, nil
}
funcs := template.FuncMap{
"trimPrefix": strings.TrimPrefix,
}

funcs := sprig.HermeticTxtFuncMap()
Copy link
Contributor

@ivankatliarchuk ivankatliarchuk Jan 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

extra := template.FuncMap{
"isIPv6" : isIPv6String
"isIPv4" : isIPv4String
}

and add them to funcs if it's possible

Copy link
Contributor

Choose a reason for hiding this comment

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

this code most likely better to move to

func funcMap() template.FuncMap {}

so we could have an isolated code coverage for multiple functions

funcs["isIPv6"] = isIPv6String
funcs["isIPv4"] = isIPv4String

return template.New("endpoint").Funcs(funcs).Parse(fqdnTemplate)
}

Expand Down Expand Up @@ -266,6 +269,25 @@ func suitableType(target string) string {
return endpoint.RecordTypeCNAME
}

// isIPv6String reports whether the target string is an IPv6 address,
// including IPv4-mapped IPv6 addresses.
func isIPv6String(target string) bool {
netIP, err := netip.ParseAddr(target)
Copy link
Contributor

Choose a reason for hiding this comment

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

could you share code coveragate for this functions?

Can we have specific tests just in source_test.go

I know it's a private function, but we may move them at some point to utils.go, this will help with maintenance

Worth to add BenchmarkTest... as well, and if you could, share results

if err != nil {
return false
}
return netIP.Is6()
}

// isIPv4String reports whether the target string is an IPv4 address.
func isIPv4String(target string) bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

same as other function

netIP, err := netip.ParseAddr(target)
if err != nil {
return false
}
return netIP.Is4()
}

// endpointsForHostname returns the endpoint objects for each host-target combination.
func endpointsForHostname(hostname string, targets endpoint.Targets, ttl endpoint.TTL, providerSpecific endpoint.ProviderSpecific, setIdentifier string, resource string) []*endpoint.Endpoint {
var endpoints []*endpoint.Endpoint
Expand Down
Loading