Skip to content

Commit c2276d8

Browse files
authored
feat(cli): migrate kingpin to cobra - dual parity (#5836)
* feat(cli): add Cobra binder and backend switch * add FlagBinder with Kingpin and Cobra implementations * support --cli-backend and EXTERNAL_DNS_CLI (default: kingpin) * add tests for binders and CLI switch Refs: #5379 Signed-off-by: Tobias Harnickell <[email protected]> * feat(cli): centralize flag registration and add Cobra parity Started moving CLI flag registration into a common binder function, avoiding duplication between Kingpin and Cobra. Refs: #5820 Signed-off-by: Tobias Harnickell <[email protected]> * feat(cli): enforce Cobra parity with Kingpin * Add `regexpValue` and `RegexpVar` to Cobra binder with `setRegexpDefault` * Enforce `--provider` presence and validate against `providerNames * require at least one `--source` and validate against new `allowedSources` * Expand tests for Kingpin and Cobra Refs: #5379 Signed-off-by: Tobias Harnickell <[email protected]> * feat(cli): Commit go-lint edits Signed-off-by: Tobias Harnickell <[email protected]> * feat(cli): add kingpin vs cobra binder parity * Test parity assertion across binders * Test Cobra-specific incapabilities (`--no-<flag>` and env vars) * Deduplicate regexp flag handling Refs: #5379 Signed-off-by: Tobias Harnickell <[email protected]> * feat(cli): Rebuild flags documentation Signed-off-by: Tobias Harnickell <[email protected]> --------- Signed-off-by: Tobias Harnickell <[email protected]>
1 parent 0d7b2c2 commit c2276d8

File tree

5 files changed

+1314
-225
lines changed

5 files changed

+1314
-225
lines changed

docs/flags.md

Lines changed: 10 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -31,9 +31,9 @@
3131
| `--[no-]exclude-unschedulable` | Exclude nodes that are considered unschedulable (default: true) |
3232
| `--[no-]expose-internal-ipv6` | When using the node source, expose internal IPv6 addresses (optional, default: false) |
3333
| `--fqdn-template=""` | A templated string that's used to generate DNS names from sources that don't define a hostname themselves, or to add a hostname suffix when paired with the fake source (optional). Accepts comma separated list for multiple global FQDN. |
34-
| `--gateway-label-filter=GATEWAY-LABEL-FILTER` | Filter Gateways of Route endpoints via label selector (default: all gateways) |
35-
| `--gateway-name=GATEWAY-NAME` | Limit Gateways of Route endpoints to a specific name (default: all names) |
36-
| `--gateway-namespace=GATEWAY-NAMESPACE` | Limit Gateways of Route endpoints to a specific namespace (default: all namespaces) |
34+
| `--gateway-label-filter=""` | Filter Gateways of Route endpoints via label selector (default: all gateways) |
35+
| `--gateway-name=""` | Limit Gateways of Route endpoints to a specific name (default: all names) |
36+
| `--gateway-namespace=""` | Limit Gateways of Route endpoints to a specific namespace (default: all namespaces) |
3737
| `--[no-]ignore-hostname-annotation` | Ignore hostname annotation when generating DNS names, valid only when --fqdn-template is set (default: false) |
3838
| `--[no-]ignore-ingress-rules-spec` | Ignore the spec.rules section in Ingress resources (default: false) |
3939
| `--[no-]ignore-ingress-tls-spec` | Ignore the spec.tls section in Ingress resources (default: false) |
@@ -43,17 +43,15 @@
4343
| `--managed-record-types=A...` | Record types to manage; specify multiple times to include many; (default: A,AAAA,CNAME) (supported records: A, AAAA, CNAME, NS, SRV, TXT) |
4444
| `--namespace=""` | Limit resources queried for endpoints to a specific namespace (default: all namespaces) |
4545
| `--nat64-networks=NAT64-NETWORKS` | Adding an A record for each AAAA record in NAT64-enabled networks; specify multiple times for multiple possible nets (optional) |
46-
| `--openshift-router-name=OPENSHIFT-ROUTER-NAME` | if source is openshift-route then you can pass the ingress controller name. Based on this name external-dns will select the respective router from the route status and map that routerCanonicalHostname to the route host while creating a CNAME record. |
46+
| `--openshift-router-name=""` | if source is openshift-route then you can pass the ingress controller name. Based on this name external-dns will select the respective router from the route status and map that routerCanonicalHostname to the route host while creating a CNAME record. |
4747
| `--pod-source-domain=""` | Domain to use for pods records (optional) |
4848
| `--[no-]publish-host-ip` | Allow external-dns to publish host-ip for headless services (optional) |
4949
| `--[no-]publish-internal-services` | Allow external-dns to publish DNS records for ClusterIP services (optional) |
5050
| `--service-type-filter=SERVICE-TYPE-FILTER` | The service types to filter by. Specify multiple times for multiple filters to be applied. (optional, default: all, expected: ClusterIP, NodePort, LoadBalancer or ExternalName) |
51-
| `--source=source` | The resource types that are queried for endpoints; specify multiple times for multiple sources (required, options: service, ingress, node, pod, fake, connector, gateway-httproute, gateway-grpcroute, gateway-tlsroute, gateway-tcproute, gateway-udproute, istio-gateway, istio-virtualservice, cloudfoundry, contour-httpproxy, gloo-proxy, crd, empty, skipper-routegroup, openshift-route, ambassador-host, kong-tcpingress, f5-virtualserver, f5-transportserver, traefik-proxy) |
5251
| `--target-net-filter=TARGET-NET-FILTER` | Limit possible targets by a net filter; specify multiple times for multiple possible nets (optional) |
5352
| `--[no-]traefik-enable-legacy` | Enable legacy listeners on Resources under the traefik.containo.us API Group |
5453
| `--[no-]traefik-disable-new` | Disable listeners on Resources under the traefik.io API Group |
5554
| `--events-emit=EVENTS-EMIT` | Events that should be emitted. Specify multiple times for multiple events support (optional, default: none, expected: RecordReady, RecordDeleted, RecordError) |
56-
| `--provider=provider` | The DNS provider where the DNS records will be created (required, options: akamai, alibabacloud, aws, aws-sd, azure, azure-dns, azure-private-dns, civo, cloudflare, coredns, digitalocean, dnsimple, exoscale, gandi, godaddy, google, inmemory, linode, ns1, oci, ovh, pdns, pihole, plural, rfc2136, scaleway, skydns, transip, webhook) |
5755
| `--provider-cache-time=0s` | The time to cache the DNS provider record list requests. |
5856
| `--domain-filter=` | Limit possible target zones by a domain suffix; specify multiple times for multiple domains (optional) |
5957
| `--exclude-domains=` | Exclude subdomains (optional) |
@@ -95,7 +93,7 @@
9593
| `--cloudflare-custom-hostnames-certificate-authority=none` | When using the Cloudflare provider with the Custom Hostnames, specify which Certificate Authority will be used. A value of none indicates no Certificate Authority will be sent to the Cloudflare API (default: none, options: google, ssl_com, lets_encrypt, none) |
9694
| `--cloudflare-dns-records-per-page=100` | When using the Cloudflare provider, specify how many DNS records listed per page, max possible 5,000 (default: 100) |
9795
| `--[no-]cloudflare-regional-services` | When using the Cloudflare provider, specify if Regional Services feature will be used (default: disabled) |
98-
| `--cloudflare-region-key=CLOUDFLARE-REGION-KEY` | When using the Cloudflare provider, specify the default region for Regional Services. Any value other than an empty string will enable the Regional Services feature (optional) |
96+
| `--cloudflare-region-key=""` | When using the Cloudflare provider, specify the default region for Regional Services. Any value other than an empty string will enable the Regional Services feature (optional) |
9997
| `--cloudflare-record-comment=""` | When using the Cloudflare provider, specify the comment for the DNS records (default: '') |
10098
| `--coredns-prefix="/skydns/"` | When using the CoreDNS provider, specify the prefix name |
10199
| `--akamai-serviceconsumerdomain=""` | When using the Akamai provider, specify the base URL (required when --provider=akamai and edgerc-path not specified) |
@@ -105,7 +103,7 @@
105103
| `--akamai-edgerc-path=""` | When using the Akamai provider, specify the .edgerc file path. Path must be reachable form invocation environment. (required when --provider=akamai and *-token, secret serviceconsumerdomain not specified) |
106104
| `--akamai-edgerc-section=""` | When using the Akamai provider, specify the .edgerc file path (Optional when edgerc-path is specified) |
107105
| `--oci-config-file="/etc/kubernetes/oci.yaml"` | When using the OCI provider, specify the OCI configuration file (required when --provider=oci |
108-
| `--oci-compartment-ocid=OCI-COMPARTMENT-OCID` | When using the OCI provider, specify the OCID of the OCI compartment containing all managed zones and records. Required when using OCI IAM instance principal authentication. |
106+
| `--oci-compartment-ocid=""` | When using the OCI provider, specify the OCID of the OCI compartment containing all managed zones and records. Required when using OCI IAM instance principal authentication. |
109107
| `--oci-zone-scope=GLOBAL` | When using OCI provider, filter for zones with this scope (optional, options: GLOBAL, PRIVATE). Defaults to GLOBAL, setting to empty value will target both. |
110108
| `--[no-]oci-auth-instance-principal` | When using the OCI provider, specify whether OCI IAM instance principal authentication should be used (instead of key-based auth via the OCI config file). |
111109
| `--oci-zones-cache-duration=0s` | When using the OCI provider, set the zones list cache TTL (0s to disable). |
@@ -119,11 +117,11 @@
119117
| `--[no-]pdns-skip-tls-verify` | When using the PowerDNS/PDNS provider, disable verification of any TLS certificates (optional when --provider=pdns) (default: false) |
120118
| `--ns1-endpoint=""` | When using the NS1 provider, specify the URL of the API endpoint to target (default: https://api.nsone.net/v1/) |
121119
| `--[no-]ns1-ignoressl` | When using the NS1 provider, specify whether to verify the SSL certificate (default: false) |
122-
| `--ns1-min-ttl=NS1-MIN-TTL` | Minimal TTL (in seconds) for records. This value will be used if the provided TTL for a service/ingress is lower than this. |
120+
| `--ns1-min-ttl=0` | Minimal TTL (in seconds) for records. This value will be used if the provided TTL for a service/ingress is lower than this. |
123121
| `--digitalocean-api-page-size=50` | Configure the page size used when querying the DigitalOcean API. |
124122
| `--godaddy-api-key=""` | When using the GoDaddy provider, specify the API Key (required when --provider=godaddy) |
125123
| `--godaddy-api-secret=""` | When using the GoDaddy provider, specify the API secret (required when --provider=godaddy) |
126-
| `--godaddy-api-ttl=GODADDY-API-TTL` | TTL (in seconds) for records. This value will be used if the provided TTL for a service/ingress is not provided. |
124+
| `--godaddy-api-ttl=0` | TTL (in seconds) for records. This value will be used if the provided TTL for a service/ingress is not provided. |
127125
| `--[no-]godaddy-api-ote` | When using the GoDaddy provider, use OTE api (optional, default: false, when --provider=godaddy) |
128126
| `--tls-ca=""` | When using TLS communication, the path to the certificate authority to verify server communications (optionally specify --tls-client-cert for two-way TLS) |
129127
| `--tls-client-cert=""` | When using TLS communication, the path to the certificate to present as a client (not required for TLS) |
@@ -174,11 +172,12 @@
174172
| `--[no-]once` | When enabled, exits the synchronization loop after the first iteration (default: disabled) |
175173
| `--[no-]dry-run` | When enabled, prints DNS record changes rather than actually performing them (default: disabled) |
176174
| `--[no-]events` | When enabled, in addition to running every interval, the reconciliation loop will get triggered when supported sources change (default: disabled) |
177-
| `--min-ttl=MIN-TTL` | Configure global TTL for records in duration format. This value is used when the TTL for a source is not set or set to 0. (optional; examples: 1m12s, 72s, 72) |
178175
| `--log-format=text` | The format in which log messages are printed (default: text, options: text, json) |
179176
| `--metrics-address=":7979"` | Specify where to serve the metrics and health check endpoint (default: :7979) |
180177
| `--log-level=info` | Set the level of logging. (default: info, options: panic, debug, info, warning, error, fatal) |
181178
| `--webhook-provider-url="http://localhost:8888"` | The URL of the remote endpoint to call for the webhook provider (default: http://localhost:8888) |
182179
| `--webhook-provider-read-timeout=5s` | The read timeout for the webhook provider in duration format (default: 5s) |
183180
| `--webhook-provider-write-timeout=10s` | The write timeout for the webhook provider in duration format (default: 10s) |
184181
| `--[no-]webhook-server` | When enabled, runs as a webhook server instead of a controller. (default: false). |
182+
| `--provider=provider` | The DNS provider where the DNS records will be created (required, options: akamai, alibabacloud, aws, aws-sd, azure, azure-dns, azure-private-dns, civo, cloudflare, coredns, digitalocean, dnsimple, exoscale, gandi, godaddy, google, inmemory, linode, ns1, oci, ovh, pdns, pihole, plural, rfc2136, scaleway, skydns, transip, webhook) |
183+
| `--source=source` | The resource types that are queried for endpoints; specify multiple times for multiple sources (required, options: service, ingress, node, pod, fake, connector, gateway-httproute, gateway-grpcroute, gateway-tlsroute, gateway-tcproute, gateway-udproute, istio-gateway, istio-virtualservice, cloudfoundry, contour-httpproxy, gloo-proxy, crd, empty, skipper-routegroup, openshift-route, ambassador-host, kong-tcpingress, f5-virtualserver, f5-transportserver, traefik-proxy) |

pkg/apis/externaldns/binders.go

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,8 @@ limitations under the License.
1717
package externaldns
1818

1919
import (
20+
"fmt"
21+
"regexp"
2022
"strconv"
2123
"time"
2224

@@ -33,6 +35,13 @@ type FlagBinder interface {
3335
Int64Var(name, help string, def int64, target *int64)
3436
StringsVar(name, help string, def []string, target *[]string)
3537
EnumVar(name, help, def string, target *string, allowed ...string)
38+
// StringsEnumVar binds a repeatable string flag with an allowed set.
39+
// Implementations may not enforce allowed values.
40+
StringsEnumVar(name, help string, def []string, target *[]string, allowed ...string)
41+
// StringMapVar binds key=value repeatable flags into a map.
42+
StringMapVar(name, help string, target *map[string]string)
43+
// RegexpVar binds a regular expression value.
44+
RegexpVar(name, help string, def *regexp.Regexp, target **regexp.Regexp)
3645
}
3746

3847
// KingpinBinder implements FlagBinder using github.com/alecthomas/kingpin/v2.
@@ -81,6 +90,60 @@ func (b *KingpinBinder) EnumVar(name, help, def string, target *string, allowed
8190
b.App.Flag(name, help).Default(def).EnumVar(target, allowed...)
8291
}
8392

93+
func (b *KingpinBinder) StringsEnumVar(name, help string, def []string, target *[]string, allowed ...string) {
94+
if len(def) > 0 {
95+
b.App.Flag(name, help).Default(def...).EnumsVar(target, allowed...)
96+
return
97+
}
98+
b.App.Flag(name, help).EnumsVar(target, allowed...)
99+
}
100+
101+
func (b *KingpinBinder) StringMapVar(name, help string, target *map[string]string) {
102+
b.App.Flag(name, help).StringMapVar(target)
103+
}
104+
105+
func (b *KingpinBinder) RegexpVar(name, help string, def *regexp.Regexp, target **regexp.Regexp) {
106+
defStr := ""
107+
if def != nil {
108+
defStr = def.String()
109+
}
110+
b.App.Flag(name, help).Default(defStr).RegexpVar(target)
111+
}
112+
113+
type regexpValue struct {
114+
target **regexp.Regexp
115+
}
116+
117+
func (rv *regexpValue) String() string {
118+
if rv == nil || rv.target == nil || *rv.target == nil {
119+
return ""
120+
}
121+
return (*rv.target).String()
122+
}
123+
124+
func (rv *regexpValue) Set(s string) error {
125+
re, err := regexp.Compile(s)
126+
if err != nil {
127+
return err
128+
}
129+
*rv.target = re
130+
return nil
131+
}
132+
133+
func (rv *regexpValue) Type() string { return "regexp" }
134+
135+
type regexpSetter interface {
136+
Set(string) error
137+
}
138+
139+
func setRegexpDefault(rs regexpSetter, def *regexp.Regexp, name string) {
140+
if def != nil {
141+
if err := rs.Set(def.String()); err != nil {
142+
panic(fmt.Errorf("invalid default regexp for flag %s: %w", name, err))
143+
}
144+
}
145+
}
146+
84147
// CobraBinder implements FlagBinder using github.com/spf13/cobra.
85148
type CobraBinder struct {
86149
Cmd *cobra.Command
@@ -119,3 +182,20 @@ func (b *CobraBinder) StringsVar(name, help string, def []string, target *[]stri
119182
func (b *CobraBinder) EnumVar(name, help, def string, target *string, allowed ...string) {
120183
b.Cmd.Flags().StringVar(target, name, def, help)
121184
}
185+
186+
func (b *CobraBinder) StringsEnumVar(name, help string, def []string, target *[]string, allowed ...string) {
187+
// pflag does not enforce enums.
188+
b.Cmd.Flags().StringArrayVar(target, name, def, help)
189+
}
190+
191+
func (b *CobraBinder) StringMapVar(name, help string, target *map[string]string) {
192+
// Use StringToStringVar for key=value pairs.
193+
b.Cmd.Flags().StringToStringVar(target, name, map[string]string{}, help)
194+
}
195+
196+
func (b *CobraBinder) RegexpVar(name, help string, def *regexp.Regexp, target **regexp.Regexp) {
197+
rv := &regexpValue{target: target}
198+
// set default value to mirror kingpin's Default(def.String()) behavior
199+
setRegexpDefault(rv, def, name)
200+
b.Cmd.Flags().Var(rv, name, help)
201+
}

pkg/apis/externaldns/binders_test.go

Lines changed: 142 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,8 @@ limitations under the License.
1717
package externaldns
1818

1919
import (
20+
"errors"
21+
"regexp"
2022
"testing"
2123
"time"
2224

@@ -26,6 +28,10 @@ import (
2628
"github.com/stretchr/testify/require"
2729
)
2830

31+
type badSetter struct{}
32+
33+
func (b *badSetter) Set(s string) error { return errors.New("bad default") }
34+
2935
func TestKingpinBinderParsesAllTypes(t *testing.T) {
3036
app := kingpin.New("test", "")
3137
b := NewKingpinBinder(app)
@@ -137,3 +143,139 @@ func TestCobraBinderEnumNotValidatedHere(t *testing.T) {
137143
require.NoError(t, err)
138144
assert.Equal(t, "c", e)
139145
}
146+
147+
// Cobra requires --<flag>=false
148+
func TestCobraBinderNoBooleanNegationFormUnsupported(t *testing.T) {
149+
cmd := &cobra.Command{Use: "test"}
150+
b := NewCobraBinder(cmd)
151+
152+
var v bool
153+
b.BoolVar("v", "bool flag", true, &v)
154+
155+
cmd.SetArgs([]string{"--no-v"})
156+
err := cmd.Execute()
157+
require.Error(t, err)
158+
}
159+
160+
func TestCobraRegexValueSetStringType(t *testing.T) {
161+
var r *regexp.Regexp
162+
rv := &regexpValue{target: &r}
163+
164+
require.Equal(t, "regexp", rv.Type())
165+
// empty when target nil
166+
assert.Empty(t, rv.String())
167+
168+
// invalid pattern returns error
169+
err := rv.Set("(")
170+
require.Error(t, err)
171+
172+
// valid pattern sets target
173+
err = rv.Set("^foo$")
174+
require.NoError(t, err)
175+
require.NotNil(t, r)
176+
assert.Equal(t, "^foo$", r.String())
177+
assert.Equal(t, "^foo$", rv.String())
178+
}
179+
180+
func TestCobraRegexpVarDefaultAndInvalidValue(t *testing.T) {
181+
cmd := &cobra.Command{Use: "test"}
182+
b := NewCobraBinder(cmd)
183+
184+
var r *regexp.Regexp
185+
// Provide a valid default: should set target immediately
186+
b.RegexpVar("re", "help", regexp.MustCompile("^x+$"), &r)
187+
require.NotNil(t, r)
188+
assert.Equal(t, "^x+$", r.String())
189+
190+
// Executing with an invalid value should produce an error
191+
cmd2 := &cobra.Command{Use: "test2"}
192+
b2 := NewCobraBinder(cmd2)
193+
var r2 *regexp.Regexp
194+
b2.RegexpVar("re", "help", nil, &r2)
195+
cmd2.SetArgs([]string{"--re=invalid("})
196+
err := cmd2.Execute()
197+
require.Error(t, err)
198+
}
199+
200+
func TestCobraStringMapVarDefaultEmpty(t *testing.T) {
201+
cmd := &cobra.Command{Use: "test"}
202+
b := NewCobraBinder(cmd)
203+
204+
var m map[string]string
205+
b.StringMapVar("m", "help", &m)
206+
207+
cmd.SetArgs([]string{})
208+
err := cmd.Execute()
209+
require.NoError(t, err)
210+
require.NotNil(t, m)
211+
assert.Empty(t, m)
212+
}
213+
214+
func TestKingpinRegexpVarDefaultAndParse(t *testing.T) {
215+
app := kingpin.New("test", "")
216+
b := NewKingpinBinder(app)
217+
218+
var r *regexp.Regexp
219+
b.RegexpVar("re", "help", regexp.MustCompile("^a+$"), &r)
220+
221+
_, err := app.Parse([]string{})
222+
require.NoError(t, err)
223+
require.NotNil(t, r)
224+
assert.Equal(t, "^a+$", r.String())
225+
226+
// user-provided value should override default
227+
var r2 *regexp.Regexp
228+
app2 := kingpin.New("test2", "")
229+
b2 := NewKingpinBinder(app2)
230+
b2.RegexpVar("re", "help", nil, &r2)
231+
_, err = app2.Parse([]string{"--re=^b+$"})
232+
require.NoError(t, err)
233+
require.NotNil(t, r2)
234+
assert.Equal(t, "^b+$", r2.String())
235+
}
236+
237+
func TestKingpinStringsEnumVarWithAndWithoutDefault(t *testing.T) {
238+
app := kingpin.New("test", "")
239+
b := NewKingpinBinder(app)
240+
241+
var vals []string
242+
b.StringsEnumVar("se", "help", []string{"a", "b"}, &vals, "a", "b", "c")
243+
_, err := app.Parse([]string{})
244+
require.NoError(t, err)
245+
assert.ElementsMatch(t, []string{"a", "b"}, vals)
246+
247+
// without default
248+
app2 := kingpin.New("test2", "")
249+
b2 := NewKingpinBinder(app2)
250+
var vals2 []string
251+
b2.StringsEnumVar("se", "help", nil, &vals2, "a", "b", "c")
252+
_, err = app2.Parse([]string{"--se=a", "--se=c"})
253+
require.NoError(t, err)
254+
assert.ElementsMatch(t, []string{"a", "c"}, vals2)
255+
}
256+
257+
func TestCobraStringsEnumVarWithAndWithoutDefault(t *testing.T) {
258+
cmd := &cobra.Command{Use: "test"}
259+
b := NewCobraBinder(cmd)
260+
261+
var vals []string
262+
b.StringsEnumVar("se", "help", []string{"x", "y"}, &vals, "x", "y")
263+
cmd.SetArgs([]string{})
264+
require.NoError(t, cmd.Execute())
265+
assert.ElementsMatch(t, []string{"x", "y"}, vals)
266+
267+
// without default
268+
cmd2 := &cobra.Command{Use: "test2"}
269+
b2 := NewCobraBinder(cmd2)
270+
var vals2 []string
271+
b2.StringsEnumVar("se", "help", nil, &vals2, "x", "y")
272+
cmd2.SetArgs([]string{"--se=a", "--se=b"})
273+
require.NoError(t, cmd2.Execute())
274+
assert.ElementsMatch(t, []string{"a", "b"}, vals2)
275+
}
276+
277+
func TestSetRegexDefaultPanicsOnInvalidDefault(t *testing.T) {
278+
bs := &badSetter{}
279+
def := regexp.MustCompile("^")
280+
require.Panics(t, func() { setRegexpDefault(bs, def, "flag") })
281+
}

0 commit comments

Comments
 (0)