feat: add isSource template function to scope --fqdn-template by source - #6625
feat: add isSource template function to scope --fqdn-template by source#6625ivankatliarchuk wants to merge 12 commits into
Conversation
Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>
|
[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. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>
Coverage Report for CI Build 32010655162Coverage decreased (-0.008%) to 81.849%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions82 previously-covered lines in 3 files lost coverage.
Coverage Stats
💛 - Coveralls |
Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>
Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>
Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>
Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>
| // source is the ExternalDNS source name (e.g. "service", "traefik-proxy") this Engine | ||
| // was scoped to via WithSource. Empty for an unscoped Engine. | ||
| source string |
There was a problem hiding this comment.
| // source is the ExternalDNS source name (e.g. "service", "traefik-proxy") this Engine | |
| // was scoped to via WithSource. Empty for an unscoped Engine. | |
| source string |
Unless I'm missing something, this parameter is written but never read nor used.
There was a problem hiding this comment.
It was assigned, but not used. Pushed change
| "isIPv4": isIPv4, | ||
| "hasKey": hasKey, | ||
| "fromJson": fromJson, | ||
| // stub: the source isn't known at parse time; Engine.WithSource rebinds this per source. |
There was a problem hiding this comment.
It may makes unknown source name indistinguishable from "not this source".
Wdyt of adding a validation on this ?
Something like this, in source/template/validate.go:
func validateIsSourceArgs(tmpl *template.Template, flag string) error {
unknown := sets.New[string]()
for _, t := range tmpl.Templates() {
if t.Tree == nil {
continue
}
walkCommands(t.Root, func(cmd *parse.CommandNode) {
if len(cmd.Args) == 0 {
return
}
id, ok := cmd.Args[0].(*parse.IdentifierNode)
if !ok || id.Ident != "isSource" {
return
}
for _, arg := range cmd.Args[1:] {
if s, ok := arg.(*parse.StringNode); ok && !types.IsKnown(s.Text) {
unknown.Insert(s.Text)
}
}
})
}
if len(unknown) == 0 {
return nil
}
return fmt.Errorf("parse %s: isSource: unknown source %q (valid: %s)",
flag, strings.Join(sets.Sorted(unknown), `", "`), strings.Join(types.All, ", "))
}There was a problem hiding this comment.
We could validate values, sanitize templates against values. For walkCommands - a correct walk has to recurse into IfNode/RangeNode/WithNode/TemplateNode bodies and nested structs, not just top-level t.Root commands - {{ if eq (isSource "ingres") true }} as example needs to be caught too and many more cases. A shallow walker gives false confidence (validates the easy cases, misses nested ones). I understand the gain, is just looks a bit too complex to correctly resolve.
There was a problem hiding this comment.
Added validation directly to the function for now
Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>
Co-authored-by: Michel Loiseleur <97035654+mloiseleur@users.noreply.github.com>
Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>
…isSource-scoping' into feat/fqdn-template-isSource-scoping * refs/remotes/origin/feat/fqdn-template-isSource-scoping: feat: Add isSource template function to scope --fqdn-template by source
Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>
| func BuildWithConfig(ctx context.Context, source string, p ClientGenerator, cfg *Config) (Source, error) { | ||
| // Scope the template engine to this source so templates can use isSource "name". | ||
| var err error | ||
| if cfg.TemplateEngine, err = cfg.TemplateEngine.WithSource(source); err != nil { |
There was a problem hiding this comment.
It's modifying the Config, so it could get stuck on last source name, when called multiples times.
FTM, we are good, but we may need at some point to snapshot & restore on defer
| func TestAllContainsEveryConstant(t *testing.T) { | ||
| expected := []Type{ | ||
| Node, Service, Ingress, Pod, | ||
| GatewayHttpRoute, GatewayGrpcRoute, GatewayTlsRoute, GatewayTcpRoute, GatewayUdpRoute, | ||
| IstioGateway, IstioVirtualService, | ||
| AmbassadorHost, ContourHTTPProxy, GlooProxy, TraefikProxy, OpenShiftRoute, | ||
| Fake, Connector, CRD, SkipperRouteGroup, KongTCPIngress, | ||
| F5VirtualServer, F5TransportServer, Unstructured, | ||
| } | ||
| assert.ElementsMatch(t, expected, All) | ||
| } |
There was a problem hiding this comment.
This test compares a hand-copy in types.go to a hand-copy in this file.
Mmmh 🤔
@ivankatliarchuk Maybe we can do better by ensuring we are aligned between the source code and the CLI code. When we add a new source, we definitely do not want to miss to fill the two lists (All in source/types/type.go and the allowedSources in pkg/apis/externaldns/types.go.
It could be something like that in pkg/apis/externaldns/types_test.go:
func TestAllowedSourcesMatchesSourceTypes(t *testing.T) {
want := append(slices.Clone(types.All), "empty")
slices.Sort(want)
got := slices.Clone(allowedSources)
slices.Sort(got)
assert.Equal(t, want, got, "allowedSources and source/types.All have drifted")
}@ivankatliarchuk Wdyt ?
What does it do ?
Motivation
Adds a way to scope templates to specific sources. Relates #6593
Complement a solution like #6611
More