feat: additional template functions #3949

matkam · 2023-09-21T20:18:02Z

Description

Adds a few useful text/template functions:

replaceAll: strings.replaceAll
isIPv6: exposes an existing function that returns a bool to let you know if a string is an IPv6 address
isIPv4: a new function that returns a bool to let you know if a string is an IPv4 address

N/A

Checklist

Unit tests updated
End user documentation updated

linux-foundation-easycla · 2023-09-21T20:18:05Z

The committers listed above are authorized under a signed CLA.

✅ login: matkam / name: Mathew Kamkar (b5a6cbd, 5798069, b37cb51, 664ea01, 3f77b0a, 8d52be2, 20e5eb9, db35373, ae43729, a6851a2, db55aa1)

k8s-ci-robot · 2023-09-21T20:18:11Z

Welcome @matkam!

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 · 2023-09-21T20:18:12Z

Hi @matkam. 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/test-infra repository.

johngmyers · 2023-09-22T00:50:45Z

Please submit a CLA per the instructions above.

matkam · 2023-09-22T03:25:04Z

Please submit a CLA per the instructions above.

Yes 👍
I'm just waiting on my employer to click the button.

matkam · 2023-09-26T23:17:50Z

@johngmyers we are good to go here 👍

johngmyers · 2023-09-27T00:12:43Z

source/source.go

+
+func isIPv4String(input string) bool {
+	netIP := net.ParseIP(input)
+	return netIP != nil && netIP.To4() != nil


Is this function intended to return true for IPv4-mapped IPv6 addresses?

It just returns true when the input string is an IPv4 address. It'll return false if its IPv6 or otherwise invalid IP address.

https://go.dev/play/p/2SNDvrwxdh-

how about something like this?

func isIPv4String(input string) bool { netIP := net.ParseIP(input) return netIP != nil && netIP.To4() != nil && !strings.Contains(input, ":") }

johngmyers · 2023-09-27T00:17:20Z

Sprig has replace, which is similar. Perhaps we should follow their syntax?

johngmyers · 2023-09-27T00:21:24Z

/ok-to-test

matkam · 2023-09-27T00:32:41Z

Sprig has replace, which is similar. Perhaps we should follow their syntax?

does replace work differently? I could also add strings.replace to the FuncMap.

johngmyers · 2023-09-27T00:45:28Z

I believe replace has the arguments in a different order. See the documentation in https://masterminds.github.io/sprig/strings.html

matkam · 2023-09-27T04:07:13Z

I believe replace has the arguments in a different order. See the documentation in https://masterminds.github.io/sprig/strings.html

sprig looks like a nice package. how about just importing it and using hermeticTxtFuncMap like this:

func parseTemplate(fqdnTemplate string) (tmpl *template.Template, err error) {
	if fqdnTemplate == "" {
		return nil, nil
	}
	
	funcs := sprig.HermeticTxtFuncMap()
	funcs["isIPv6"] = isIPv6String
	funcs["isIPv4"] = isIPv4String

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

matkam · 2023-10-27T01:39:23Z

@johngmyers let me know what you think. I've updated the template functions to include Sprig's hermetic text functions, and fixed the isIPv4String function so that it only returns true for valid IPv4 strings.

szuecs · 2024-05-30T09:02:41Z

source/source.go

@@ -29,6 +29,7 @@ import (
 	"time"
 	"unicode"

+	sprig "github.com/go-task/slim-sprig"


Please no import for a single func.

@szuecs it includes several useful go template functions: https://github.com/go-task/slim-sprig/blob/master/functions.go

Would you rather not include these functions, and only keeping isIPv4?

k8s-triage-robot · 2024-11-05T06:40:58Z

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle stale
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

Co-authored-by: Michel Loiseleur <97035654+mloiseleur@users.noreply.github.com>

matkam · 2024-11-12T16:00:42Z

/remove-lifecycle stale

k8s-ci-robot · 2025-01-07T18:01:03Z

[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 ask for approval from mloiseleur. 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:

OWNERS

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

ivankatliarchuk

Hi @matkam. Overall looks ok, if you still have time and interest to add this to a master branch, I've shared few bits that worth addressing.

Could you also provide a set of manual test steps using manifests and kubectl commands? This will help in understanding better the change and verifying the implementation.

/assign

ivankatliarchuk · 2025-01-25T10:02:44Z

go.mod

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


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

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

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?

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.

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.

ivankatliarchuk · 2025-01-25T10:23:07Z

source/source.go

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


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

ivankatliarchuk · 2025-01-25T10:23:21Z

source/source.go

+}
+
+// isIPv4String reports whether the target string is an IPv4 address.
+func isIPv4String(target string) bool {


same as other function

ivankatliarchuk · 2025-01-25T10:31:24Z

docs/sources/nodes.md

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


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?

ivankatliarchuk · 2025-01-25T10:39:21Z

May I also ask to write tests similar to like helm does as well https://github.com/helm/helm/blob/0d66425d9a745d8a289b1a5ebb6ccc744436da95/pkg/engine/funcs_test.go#L27

We probably should have same structure for files as helm too, aka move this functions to template_functions.go or smth as agree, this is quite a powerful concept

ivankatliarchuk · 2025-01-25T10:41:11Z

docs/sources/nodes.md

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


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

ivankatliarchuk · 2025-01-25T10:51:18Z

source/source.go

-		"trimPrefix": strings.TrimPrefix,
-	}
+
+	funcs := sprig.HermeticTxtFuncMap()


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

and add them to funcs if it's possible

this code most likely better to move to

func funcMap() template.FuncMap {}

so we could have an isolated code coverage for multiple functions

matkam · 2025-01-27T23:21:45Z

Hi @matkam. Overall looks ok, if you still have time and interest to add this to a master branch, I've shared few bits that worth addressing.

Hi @ivankatliarchuk, thanks for looking at this PR. I definitely do want to get it merged in. I will look for some time in the coming weeks to address your review items.

ivankatliarchuk · 2025-01-30T21:11:49Z

/label tide/merge-method-squash

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 21, 2023

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Sep 21, 2023

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 21, 2023

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Sep 21, 2023

k8s-ci-robot requested review from johngmyers and njuettner September 21, 2023 20:18

matkam changed the title ~~[WIP] Additional template functions~~ Additional template functions Sep 22, 2023

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 22, 2023

k8s-ci-robot added 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 Sep 26, 2023

johngmyers reviewed Sep 27, 2023

View reviewed changes

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 Sep 27, 2023

matkam requested a review from johngmyers October 9, 2023 23:18

szuecs reviewed May 30, 2024

View reviewed changes

k8s-ci-robot added lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 5, 2024

matkam and others added 8 commits November 12, 2024 07:58

template functions: replaceAll, isIPv6, isIPv4

8d52be2

typeo

b37cb51

unit test

b5a6cbd

docs

3f77b0a

johngmyers review: use sprig, fix ipv4 check

5798069

update test and docs

db35373

Update docs/tutorials/nodes.md

ae43729

Co-authored-by: Michel Loiseleur <97035654+mloiseleur@users.noreply.github.com>

document isIPv4 and isIPv6 template functions

db55aa1

matkam force-pushed the master branch from b0f3beb to db55aa1 Compare November 12, 2024 15:58

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 12, 2024

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 12, 2024

Merge branch 'master' into master

a6851a2

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 7, 2025

matkam added 2 commits January 7, 2025 10:17

use netip to determine ipv4 and ipv6

664ea01

slim-sprig v3

20e5eb9

ivankatliarchuk suggested changes Jan 25, 2025

View reviewed changes

k8s-ci-robot assigned ivankatliarchuk Jan 25, 2025

ivankatliarchuk suggested changes Jan 25, 2025

View reviewed changes

k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Jan 30, 2025

feat: additional template functions #3949

Are you sure you want to change the base?

feat: additional template functions #3949

Conversation

matkam commented Sep 21, 2023 • edited Loading

linux-foundation-easycla bot commented Sep 21, 2023 • edited Loading

k8s-ci-robot commented Sep 21, 2023

k8s-ci-robot commented Sep 21, 2023

johngmyers commented Sep 22, 2023

matkam commented Sep 22, 2023

matkam commented Sep 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johngmyers commented Sep 27, 2023

johngmyers commented Sep 27, 2023

matkam commented Sep 27, 2023

johngmyers commented Sep 27, 2023

matkam commented Sep 27, 2023

matkam commented Oct 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-triage-robot commented Nov 5, 2024

matkam commented Nov 12, 2024

k8s-ci-robot commented Jan 7, 2025

ivankatliarchuk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivankatliarchuk commented Jan 25, 2025

ivankatliarchuk Jan 25, 2025 • edited Loading

Choose a reason for hiding this comment

ivankatliarchuk Jan 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matkam commented Jan 27, 2025

ivankatliarchuk commented Jan 30, 2025

matkam commented Sep 21, 2023 •

edited

Loading

linux-foundation-easycla bot commented Sep 21, 2023 •

edited

Loading

matkam commented Oct 27, 2023 •

edited

Loading

ivankatliarchuk Jan 25, 2025 •

edited

Loading

ivankatliarchuk Jan 25, 2025 •

edited

Loading