From abbf7ef5f2bba4b105b00ffbf827823629e12ba1 Mon Sep 17 00:00:00 2001 From: Michael Bridgen Date: Tue, 4 Jun 2019 17:22:30 +0100 Subject: [PATCH 1/6] Expand on the recognised fluxd flags This adds to the list of fluxd flags that the agent will recognise and include in URLs. Since there are now more than four, and they don't all appear together necessarily, I've changed how the URL is constructed. --- agent/flux.go | 96 ++++++++++++++++++++++++++++++++++++++++++---- agent/main.go | 3 +- agent/main_test.go | 34 +++++++++++++++- 3 files changed, 122 insertions(+), 11 deletions(-) diff --git a/agent/flux.go b/agent/flux.go index 991ca470..315e7545 100644 --- a/agent/flux.go +++ b/agent/flux.go @@ -1,7 +1,10 @@ package main import ( + "net/url" + "strconv" "strings" + "time" flags "github.com/jessevdk/go-flags" "github.com/weaveworks/launcher/pkg/kubectl" @@ -9,10 +12,91 @@ import ( // FluxConfig stores existing flux arguments which will be used when updating WC agents type FluxConfig struct { - GitLabel string `long:"git-label"` - GitURL string `long:"git-url"` - GitPath string `long:"git-path"` - GitBranch string `long:"git-branch"` + // git parameters + GitLabel *string `long:"git-label"` + GitURL *string `long:"git-url"` + // This arg is multi-valued, or can be passed as comma-separated + // values. This accounts for either form. + GitPath []string `long:"git-path"` + GitBranch *string `long:"git-branch"` + GitTimeout *time.Duration `long:"git-timeout"` + GitPollInterval *time.Duration `long:"git-poll-interval"` + GitSetAuthor *bool `long:"git-set-author"` + GitCISkip *bool `long:"git-ci-skip"` + + // sync behaviour + GarbageCollection *bool `long:"sync-garbage-collection"` + + // For specifying ECR region from outside AWS (fluxd detects it when inside AWS) + RegistryECRRegion []string `long:"registry-ecr-region"` + // For requiring a particular registry to be accessible, else crash + RegistryRequire []string `long:"registry-require"` + // Can be used to switch image registry scanning off for some or + // all images (with glob patterns) + RegistryExcludeImage []string `long:"registry-exclude-image"` + + // This is now hard-wired to empty in launch-generator, to tell flux + // _not_ to use service discovery. But: maybe someone needs to use + // service discovery. + MemcachedService *string `long:"memcached-service"` + + // Just in case we more explicitly support restricting Weave + // Cloud, or just Flux to particular namespaces + AllowNamespace []string `long:"k8s-allow-namepace"` +} + +// AsQueryParams returns the configuration as a fragment of query +// string, so it can be interpolated into a text template. +func (c *FluxConfig) AsQueryParams() string { + // Nothing clever here. + vals := url.Values{} + + // String-valued arguments + for arg, val := range map[string]*string{ + "git-label": c.GitLabel, + "git-url": c.GitURL, + "git-branch": c.GitBranch, + "memcached-service": c.MemcachedService, + } { + if val != nil { + vals.Add(arg, *val) + } + } + + // []string-valued arguments + for arg, slice := range map[string][]string{ + "git-path": c.GitPath, + "registry-ecr-region": c.RegistryECRRegion, + "registry-require": c.RegistryRequire, + "registry-exclude-image": c.RegistryExcludeImage, + "k8s-allow-namespace": c.AllowNamespace, + } { + for _, val := range slice { + vals.Add(arg, val) + } + } + + // duration-valued arguments + for arg, dur := range map[string]*time.Duration{ + "git-timeout": c.GitTimeout, + "git-poll-interval": c.GitPollInterval, + } { + if dur != nil { + vals.Add(arg, dur.String()) + } + } + + for arg, flag := range map[string]*bool{ + "sync-garbage-collection": c.GarbageCollection, + "git-set-author": c.GitSetAuthor, + "git-ci-skip": c.GitCISkip, + } { + if flag != nil { + vals.Add(arg, strconv.FormatBool(*flag)) + } + } + + return vals.Encode() } func getFluxConfig(k kubectl.Client, namespace string) (*FluxConfig, error) { @@ -28,9 +112,5 @@ func getFluxConfig(k kubectl.Client, namespace string) (*FluxConfig, error) { return nil, err } - if cfg.GitBranch == "" && cfg.GitLabel == "" && cfg.GitPath == "" && cfg.GitURL == "" { - return nil, nil - } - return cfg, nil } diff --git a/agent/main.go b/agent/main.go index c964e514..fe7e2c70 100644 --- a/agent/main.go +++ b/agent/main.go @@ -40,8 +40,7 @@ const ( defaultWCPollURL = "https://{{.WCHostname}}/k8s.yaml" + "?k8s-version={{.KubernetesVersion}}&t={{.Token}}&omit-support-info=true" + "{{if .FluxConfig}}" + - "&git-label={{.FluxConfig.GitLabel}}&git-url={{.FluxConfig.GitURL}}" + - "&git-path={{.FluxConfig.GitPath}}&git-branch={{.FluxConfig.GitBranch}}" + + "&{{.FluxConfig.AsQueryParams}}" + "{{end}}" + "{{if .CRIEndpoint}}" + "&cri-endpoint={{.CRIEndpoint}}" + diff --git a/agent/main_test.go b/agent/main_test.go index c04f91af..7056ddc0 100644 --- a/agent/main_test.go +++ b/agent/main_test.go @@ -5,6 +5,7 @@ import ( "net/url" "reflect" "testing" + "time" "github.com/stretchr/testify/assert" ) @@ -37,7 +38,7 @@ func TestGetMinorMajorVersion(t *testing.T) { return } if v != c.version { - t.Errorf("Version was wrongl expected: %s got %s", c.version, v) + t.Errorf("Version was wrong; expected: %s got %s", c.version, v) } } } @@ -54,3 +55,34 @@ func TestAgentManifestURL(t *testing.T) { } assert.Contains(t, manifestURL, v.Encode()) } + +func TestAgentFluxURL(t *testing.T) { + // Not an exhaustive test; just representative + gitPath := []string{"config/helloworld"} + memcachedService := "" + gitCISkip := false + gitTimeout := 40 * time.Second + + fluxCfg := &FluxConfig{ + GitPath: gitPath, + MemcachedService: &memcachedService, + GitCISkip: &gitCISkip, + GitTimeout: &gitTimeout, + } + + cfg := &agentConfig{ + AgentPollURLTemplate: defaultWCPollURL, + FluxConfig: fluxCfg, + } + + manifestURL := agentManifestURL(cfg) + + v := url.Values{ + "git-path": gitPath, + "memcached-service": []string{""}, + "git-ci-skip": []string{"false"}, + "git-timeout": []string{"40s"}, + } + + assert.Contains(t, manifestURL, v.Encode()) +} From 9dab8c1fbcbafa32264f19f1244eb925926dd4e6 Mon Sep 17 00:00:00 2001 From: Simon Howe Date: Wed, 5 Jun 2019 12:08:44 +0200 Subject: [PATCH 2/6] More docs / test output --- README.md | 3 ++- agent/main_test.go | 2 +- integration-tests/tests/flux-config.sh | 5 +++-- service/static/install.sh | 5 +++-- 4 files changed, 9 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index cab029e7..21830a96 100644 --- a/README.md +++ b/README.md @@ -38,7 +38,8 @@ Start by setting up a minikube instance to run the tests on: Run the tests: ``` -make integration-tests +make +make integration-tests WEAVE_CLOUD_TOKEN= ``` This script will first ensure the dependencies are built and then run: diff --git a/agent/main_test.go b/agent/main_test.go index 7056ddc0..13104cdb 100644 --- a/agent/main_test.go +++ b/agent/main_test.go @@ -58,7 +58,7 @@ func TestAgentManifestURL(t *testing.T) { func TestAgentFluxURL(t *testing.T) { // Not an exhaustive test; just representative - gitPath := []string{"config/helloworld"} + gitPath := []string{"config/helloworld", "config/hej-world"} memcachedService := "" gitCISkip := false gitTimeout := 40 * time.Second diff --git a/integration-tests/tests/flux-config.sh b/integration-tests/tests/flux-config.sh index 0e75ffe4..b34e5a9c 100755 --- a/integration-tests/tests/flux-config.sh +++ b/integration-tests/tests/flux-config.sh @@ -55,7 +55,8 @@ sleep 40 echo "• Check flux configuration still exists" args=$(kubectl get pod -n weave -l name=weave-flux-agent -o jsonpath='{.items[?(@.metadata.labels.name=="weave-flux-agent")].spec.containers[?(@.name=="flux-agent")].args[*]}') -if [[ $args != *"--git-url=git@github.com:weaveworks/example --git-path=k8s/example --git-branch=example --git-label=example"* ]]; then - echo "Missing existing flux args" +expected="--git-url=git@github.com:weaveworks/example --git-path=k8s/example --git-branch=example --git-label=example" +if [[ $args != *"$expected"* ]]; then + echo "Missing existing flux args: \"$expected\" not found in \"$args\"" exit 1 fi diff --git a/service/static/install.sh b/service/static/install.sh index 0b594be5..682f8f64 100644 --- a/service/static/install.sh +++ b/service/static/install.sh @@ -50,8 +50,9 @@ else fi # Download the bootstrap binary -echo "Downloading the Weave Cloud installer... " -curl -Ls "{{.Scheme}}://{{.LauncherHostname}}/bootstrap?dist=$dist" >> "$TMPFILE" +installerurl="{{.Scheme}}://{{.LauncherHostname}}/bootstrap?dist=$dist" +echo "Downloading the Weave Cloud installer from $installerurl" +curl -Ls "$installerurl" >> "$TMPFILE" # Make the bootstrap binary executable chmod +x "$TMPFILE" From 891620dd91c796798a6c3c3de51e934ee054a0b6 Mon Sep 17 00:00:00 2001 From: Simon Howe Date: Wed, 5 Jun 2019 19:01:04 +0200 Subject: [PATCH 3/6] Fixes k8s upgrade test --- agent/flux.go | 4 ++++ integration-tests/tests/kube-system-migration.sh | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/agent/flux.go b/agent/flux.go index 315e7545..4ccf2236 100644 --- a/agent/flux.go +++ b/agent/flux.go @@ -112,5 +112,9 @@ func getFluxConfig(k kubectl.Client, namespace string) (*FluxConfig, error) { return nil, err } + if cfg.AsQueryParams() == "" { + return nil, nil + } + return cfg, nil } diff --git a/integration-tests/tests/kube-system-migration.sh b/integration-tests/tests/kube-system-migration.sh index 0d7d2ee6..c140cff1 100755 --- a/integration-tests/tests/kube-system-migration.sh +++ b/integration-tests/tests/kube-system-migration.sh @@ -35,6 +35,6 @@ while [ $(kubectl get pods -n kube-system -l 'app in (weave-flux, weave-cortex, echo "• Check old flux arguments have been applied to the new agent" args=$(kubectl get pod -n weave -l name=weave-flux-agent -o jsonpath='{.items[?(@.metadata.labels.name=="weave-flux-agent")].spec.containers[?(@.name=="flux-agent")].args[*]}') if [[ $args != *"--git-url=git@github.com:weaveworks/example --git-path=k8s/example --git-branch=master --git-label=example"* ]]; then - echo "Missing existing flux args" + echo "Missing existing flux args: $args" exit 1 fi From 22175a0dfdef18aeb9afa4a5d458214d6dfd8f02 Mon Sep 17 00:00:00 2001 From: Simon Howe Date: Thu, 6 Jun 2019 15:17:47 +0200 Subject: [PATCH 4/6] Use same flag parser as flux uses - More tests - Don't need struct flag parse annotations --- agent/flux.go | 120 ++++++++++++++++++++++++++++++++++++--------- agent/main_test.go | 50 +++++++++++++++++++ 2 files changed, 146 insertions(+), 24 deletions(-) diff --git a/agent/flux.go b/agent/flux.go index 4ccf2236..434b8894 100644 --- a/agent/flux.go +++ b/agent/flux.go @@ -6,43 +6,43 @@ import ( "strings" "time" - flags "github.com/jessevdk/go-flags" + "github.com/spf13/pflag" "github.com/weaveworks/launcher/pkg/kubectl" ) // FluxConfig stores existing flux arguments which will be used when updating WC agents type FluxConfig struct { // git parameters - GitLabel *string `long:"git-label"` - GitURL *string `long:"git-url"` + GitLabel *string + GitURL *string // This arg is multi-valued, or can be passed as comma-separated // values. This accounts for either form. - GitPath []string `long:"git-path"` - GitBranch *string `long:"git-branch"` - GitTimeout *time.Duration `long:"git-timeout"` - GitPollInterval *time.Duration `long:"git-poll-interval"` - GitSetAuthor *bool `long:"git-set-author"` - GitCISkip *bool `long:"git-ci-skip"` + GitPath []string + GitBranch *string + GitTimeout *time.Duration + GitPollInterval *time.Duration + GitSetAuthor *bool + GitCISkip *bool // sync behaviour - GarbageCollection *bool `long:"sync-garbage-collection"` + GarbageCollection *bool // For specifying ECR region from outside AWS (fluxd detects it when inside AWS) - RegistryECRRegion []string `long:"registry-ecr-region"` + RegistryECRRegion []string // For requiring a particular registry to be accessible, else crash - RegistryRequire []string `long:"registry-require"` + RegistryRequire []string // Can be used to switch image registry scanning off for some or // all images (with glob patterns) - RegistryExcludeImage []string `long:"registry-exclude-image"` + RegistryExcludeImage []string // This is now hard-wired to empty in launch-generator, to tell flux // _not_ to use service discovery. But: maybe someone needs to use // service discovery. - MemcachedService *string `long:"memcached-service"` + MemcachedService *string // Just in case we more explicitly support restricting Weave // Cloud, or just Flux to particular namespaces - AllowNamespace []string `long:"k8s-allow-namepace"` + AllowNamespace []string } // AsQueryParams returns the configuration as a fragment of query @@ -51,6 +51,10 @@ func (c *FluxConfig) AsQueryParams() string { // Nothing clever here. vals := url.Values{} + if c == nil { + return "" + } + // String-valued arguments for arg, val := range map[string]*string{ "git-label": c.GitLabel, @@ -99,22 +103,90 @@ func (c *FluxConfig) AsQueryParams() string { return vals.Encode() } -func getFluxConfig(k kubectl.Client, namespace string) (*FluxConfig, error) { - out, err := k.Execute("get", "pod", "-n", namespace, "-l", "name=weave-flux-agent", "-o", "jsonpath='{.items[?(@.metadata.labels.name==\"weave-flux-agent\")].spec.containers[0].args[*]}'") - if err != nil { - return nil, err - } +func isFlagPassed(fs *pflag.FlagSet, name string) bool { + found := false + fs.Visit(func(f *pflag.Flag) { + if f.Name == name { + found = true + } + }) + return found +} +// ParseFluxArgs parses a string of args into a nice FluxConfig +func ParseFluxArgs(argString string) (*FluxConfig, error) { cfg := &FluxConfig{} - parser := flags.NewParser(cfg, flags.IgnoreUnknown) - _, err = parser.ParseArgs(strings.Split(out, " ")) - if err != nil { - return nil, err + + fs := pflag.NewFlagSet("default", pflag.ContinueOnError) + fs.ParseErrorsWhitelist.UnknownFlags = true + + // strings + cfg.GitLabel = fs.String("git-label", "", "") + cfg.GitURL = fs.String("git-url", "", "") + cfg.GitBranch = fs.String("git-branch", "", "") + cfg.MemcachedService = fs.String("memcached-service", "", "") + + // durations + cfg.GitTimeout = fs.Duration("git-timeout", time.Second, "") + cfg.GitPollInterval = fs.Duration("git-poll-interval", time.Minute, "") + + // bools + cfg.GitSetAuthor = fs.Bool("git-set-author", false, "") + cfg.GitCISkip = fs.Bool("git-ci-skip", false, "") + cfg.GarbageCollection = fs.Bool("sync-garbage-collection", false, "") + + // string slices + fs.StringSliceVar(&cfg.GitPath, "git-path", nil, "") + fs.StringSliceVar(&cfg.RegistryECRRegion, "registry-ecr-region", nil, "") + fs.StringSliceVar(&cfg.RegistryRequire, "registry-require", nil, "") + fs.StringSliceVar(&cfg.RegistryExcludeImage, "registry-exclude-image", nil, "") + fs.StringSliceVar(&cfg.AllowNamespace, "k8s-allow-namespace", nil, "") + + // Parse it all + fs.Parse(strings.Split(argString, " ")) + + // Cleanup anything that wasn't explicitly set + for arg, val := range map[string]**string{ + "git-label": &cfg.GitLabel, + "git-url": &cfg.GitURL, + "git-branch": &cfg.GitBranch, + "memcached-service": &cfg.MemcachedService, + } { + if !isFlagPassed(fs, arg) { + *val = nil + } + } + for arg, val := range map[string]**time.Duration{ + "git-timeout": &cfg.GitTimeout, + "git-poll-interval": &cfg.GitPollInterval, + } { + if !isFlagPassed(fs, arg) { + *val = nil + } + } + for arg, val := range map[string]**bool{ + "sync-garbage-collection": &cfg.GarbageCollection, + "git-set-author": &cfg.GitSetAuthor, + "git-ci-skip": &cfg.GitCISkip, + } { + if !isFlagPassed(fs, arg) { + *val = nil + } } + // Did we find anything? if cfg.AsQueryParams() == "" { return nil, nil } return cfg, nil } + +func getFluxConfig(k kubectl.Client, namespace string) (*FluxConfig, error) { + out, err := k.Execute("get", "pod", "-n", namespace, "-l", "name=weave-flux-agent", "-o", "jsonpath='{.items[?(@.metadata.labels.name==\"weave-flux-agent\")].spec.containers[0].args[*]}'") + if err != nil { + return nil, err + } + + return ParseFluxArgs(out) +} diff --git a/agent/main_test.go b/agent/main_test.go index 13104cdb..3cfb75ed 100644 --- a/agent/main_test.go +++ b/agent/main_test.go @@ -86,3 +86,53 @@ func TestAgentFluxURL(t *testing.T) { assert.Contains(t, manifestURL, v.Encode()) } + +func TestParseFluxArgs(t *testing.T) { + // nothing + argString := "" + fluxCfg, err := ParseFluxArgs(argString) + assert.Equal(t, nil, err) + assert.Equal(t, "", fluxCfg.AsQueryParams()) + + // Test handling boolean flags w/out `=true|false` + argString = "--git-ci-skip" + fluxCfg, err = ParseFluxArgs(argString) + assert.Equal(t, nil, err) + assert.Equal(t, true, *fluxCfg.GitCISkip) + + // Test handling boolean flags w `=true|false` + argString = "--git-ci-skip=true" + fluxCfg, err = ParseFluxArgs(argString) + assert.Equal(t, nil, err) + assert.Equal(t, true, *fluxCfg.GitCISkip) + + argString = "--git-ci-skip=false" + fluxCfg, err = ParseFluxArgs(argString) + assert.Equal(t, nil, err) + assert.Equal(t, false, *fluxCfg.GitCISkip) + + // Test we only serialize props that we provided + argString = "--git-label=foo --git-path=derp" + fluxCfg, err = ParseFluxArgs(argString) + assert.Equal(t, nil, err) + assert.Equal(t, "foo", *fluxCfg.GitLabel) + assert.Equal(t, "git-label=foo&git-path=derp", fluxCfg.AsQueryParams()) + + // string[] + argString = "--git-path=zing --git-path=derp" + fluxCfg, err = ParseFluxArgs(argString) + assert.Equal(t, nil, err) + assert.Equal(t, "git-path=zing&git-path=derp", fluxCfg.AsQueryParams()) + + // unknown + argString = "--token=derp" + fluxCfg, err = ParseFluxArgs(argString) + assert.Equal(t, nil, err) + assert.Equal(t, "", fluxCfg.AsQueryParams()) + + // some unknown + argString = "--git-path=zing --token=derp" + fluxCfg, err = ParseFluxArgs(argString) + assert.Equal(t, nil, err) + assert.Equal(t, "git-path=zing", fluxCfg.AsQueryParams()) +} From b6519f7ace99f0625e206ac67f0ef1dc95fe1582 Mon Sep 17 00:00:00 2001 From: Michael Bridgen Date: Mon, 10 Jun 2019 11:15:50 +0100 Subject: [PATCH 5/6] Simplify the flux flags parsing We can save the clean-up phase, which reset things to `nil` if they weren't passed as flags, by checking with the flag parser directly. This makes it trickier to construct a config (since it needs the flags parser state), but that's what `ParseFluxArgs` is for. The code that uses ParseFluxArgs is sensitive to whether it gets a `nil`, a value, or an error, so that behaviour must be preserved. --- agent/flux.go | 104 ++++++------------ agent/main.go | 4 +- agent/main_test.go | 26 ++--- .../tests/kube-system-migration.sh | 2 +- 4 files changed, 45 insertions(+), 91 deletions(-) diff --git a/agent/flux.go b/agent/flux.go index 434b8894..8aec8606 100644 --- a/agent/flux.go +++ b/agent/flux.go @@ -13,19 +13,19 @@ import ( // FluxConfig stores existing flux arguments which will be used when updating WC agents type FluxConfig struct { // git parameters - GitLabel *string - GitURL *string + GitLabel string + GitURL string // This arg is multi-valued, or can be passed as comma-separated // values. This accounts for either form. GitPath []string - GitBranch *string - GitTimeout *time.Duration - GitPollInterval *time.Duration - GitSetAuthor *bool - GitCISkip *bool + GitBranch string + GitTimeout time.Duration + GitPollInterval time.Duration + GitSetAuthor bool + GitCISkip bool // sync behaviour - GarbageCollection *bool + GarbageCollection bool // For specifying ECR region from outside AWS (fluxd detects it when inside AWS) RegistryECRRegion []string @@ -38,11 +38,13 @@ type FluxConfig struct { // This is now hard-wired to empty in launch-generator, to tell flux // _not_ to use service discovery. But: maybe someone needs to use // service discovery. - MemcachedService *string + MemcachedService string // Just in case we more explicitly support restricting Weave // Cloud, or just Flux to particular namespaces AllowNamespace []string + + fs *pflag.FlagSet } // AsQueryParams returns the configuration as a fragment of query @@ -56,14 +58,14 @@ func (c *FluxConfig) AsQueryParams() string { } // String-valued arguments - for arg, val := range map[string]*string{ + for arg, val := range map[string]string{ "git-label": c.GitLabel, "git-url": c.GitURL, "git-branch": c.GitBranch, "memcached-service": c.MemcachedService, } { - if val != nil { - vals.Add(arg, *val) + if c.fs.Changed(arg) { + vals.Add(arg, val) } } @@ -81,59 +83,48 @@ func (c *FluxConfig) AsQueryParams() string { } // duration-valued arguments - for arg, dur := range map[string]*time.Duration{ + for arg, dur := range map[string]time.Duration{ "git-timeout": c.GitTimeout, "git-poll-interval": c.GitPollInterval, } { - if dur != nil { + if c.fs.Changed(arg) { vals.Add(arg, dur.String()) } } - for arg, flag := range map[string]*bool{ + for arg, flag := range map[string]bool{ "sync-garbage-collection": c.GarbageCollection, "git-set-author": c.GitSetAuthor, "git-ci-skip": c.GitCISkip, } { - if flag != nil { - vals.Add(arg, strconv.FormatBool(*flag)) + if c.fs.Changed(arg) { + vals.Add(arg, strconv.FormatBool(flag)) } } return vals.Encode() } -func isFlagPassed(fs *pflag.FlagSet, name string) bool { - found := false - fs.Visit(func(f *pflag.Flag) { - if f.Name == name { - found = true - } - }) - return found -} - // ParseFluxArgs parses a string of args into a nice FluxConfig func ParseFluxArgs(argString string) (*FluxConfig, error) { - cfg := &FluxConfig{} - fs := pflag.NewFlagSet("default", pflag.ContinueOnError) fs.ParseErrorsWhitelist.UnknownFlags = true + cfg := &FluxConfig{fs: fs} // strings - cfg.GitLabel = fs.String("git-label", "", "") - cfg.GitURL = fs.String("git-url", "", "") - cfg.GitBranch = fs.String("git-branch", "", "") - cfg.MemcachedService = fs.String("memcached-service", "", "") + fs.StringVar(&cfg.GitLabel, "git-label", "", "") + fs.StringVar(&cfg.GitURL, "git-url", "", "") + fs.StringVar(&cfg.GitBranch, "git-branch", "", "") + fs.StringVar(&cfg.MemcachedService, "memcached-service", "", "") // durations - cfg.GitTimeout = fs.Duration("git-timeout", time.Second, "") - cfg.GitPollInterval = fs.Duration("git-poll-interval", time.Minute, "") + fs.DurationVar(&cfg.GitTimeout, "git-timeout", time.Second, "") + fs.DurationVar(&cfg.GitPollInterval, "git-poll-interval", time.Minute, "") // bools - cfg.GitSetAuthor = fs.Bool("git-set-author", false, "") - cfg.GitCISkip = fs.Bool("git-ci-skip", false, "") - cfg.GarbageCollection = fs.Bool("sync-garbage-collection", false, "") + fs.BoolVar(&cfg.GitSetAuthor, "git-set-author", false, "") + fs.BoolVar(&cfg.GitCISkip, "git-ci-skip", false, "") + fs.BoolVar(&cfg.GarbageCollection, "sync-garbage-collection", false, "") // string slices fs.StringSliceVar(&cfg.GitPath, "git-path", nil, "") @@ -145,41 +136,10 @@ func ParseFluxArgs(argString string) (*FluxConfig, error) { // Parse it all fs.Parse(strings.Split(argString, " ")) - // Cleanup anything that wasn't explicitly set - for arg, val := range map[string]**string{ - "git-label": &cfg.GitLabel, - "git-url": &cfg.GitURL, - "git-branch": &cfg.GitBranch, - "memcached-service": &cfg.MemcachedService, - } { - if !isFlagPassed(fs, arg) { - *val = nil - } + if fs.NFlag() > 0 { + return cfg, nil } - for arg, val := range map[string]**time.Duration{ - "git-timeout": &cfg.GitTimeout, - "git-poll-interval": &cfg.GitPollInterval, - } { - if !isFlagPassed(fs, arg) { - *val = nil - } - } - for arg, val := range map[string]**bool{ - "sync-garbage-collection": &cfg.GarbageCollection, - "git-set-author": &cfg.GitSetAuthor, - "git-ci-skip": &cfg.GitCISkip, - } { - if !isFlagPassed(fs, arg) { - *val = nil - } - } - - // Did we find anything? - if cfg.AsQueryParams() == "" { - return nil, nil - } - - return cfg, nil + return nil, nil } func getFluxConfig(k kubectl.Client, namespace string) (*FluxConfig, error) { diff --git a/agent/main.go b/agent/main.go index fe7e2c70..000afd2f 100644 --- a/agent/main.go +++ b/agent/main.go @@ -39,9 +39,7 @@ const ( defaultWCHostname = "cloud.weave.works" defaultWCPollURL = "https://{{.WCHostname}}/k8s.yaml" + "?k8s-version={{.KubernetesVersion}}&t={{.Token}}&omit-support-info=true" + - "{{if .FluxConfig}}" + - "&{{.FluxConfig.AsQueryParams}}" + - "{{end}}" + + "{{if .FluxConfig.AsQueryParams}}&{{.FluxConfig.AsQueryParams}}{{end}}" + "{{if .CRIEndpoint}}" + "&cri-endpoint={{.CRIEndpoint}}" + "{{end}}" + diff --git a/agent/main_test.go b/agent/main_test.go index 3cfb75ed..85ce53bb 100644 --- a/agent/main_test.go +++ b/agent/main_test.go @@ -2,10 +2,11 @@ package main import ( "errors" + "fmt" "net/url" "reflect" + "strings" "testing" - "time" "github.com/stretchr/testify/assert" ) @@ -59,16 +60,11 @@ func TestAgentManifestURL(t *testing.T) { func TestAgentFluxURL(t *testing.T) { // Not an exhaustive test; just representative gitPath := []string{"config/helloworld", "config/hej-world"} - memcachedService := "" - gitCISkip := false - gitTimeout := 40 * time.Second - - fluxCfg := &FluxConfig{ - GitPath: gitPath, - MemcachedService: &memcachedService, - GitCISkip: &gitCISkip, - GitTimeout: &gitTimeout, - } + + argsStr := fmt.Sprintf(`--git-path=%s --memcached-service= --git-ci-skip=false --git-timeout=40s`, strings.Join(gitPath, ",")) + + fluxCfg, err := ParseFluxArgs(argsStr) + assert.NoError(t, err) cfg := &agentConfig{ AgentPollURLTemplate: defaultWCPollURL, @@ -98,24 +94,24 @@ func TestParseFluxArgs(t *testing.T) { argString = "--git-ci-skip" fluxCfg, err = ParseFluxArgs(argString) assert.Equal(t, nil, err) - assert.Equal(t, true, *fluxCfg.GitCISkip) + assert.Equal(t, true, fluxCfg.GitCISkip) // Test handling boolean flags w `=true|false` argString = "--git-ci-skip=true" fluxCfg, err = ParseFluxArgs(argString) assert.Equal(t, nil, err) - assert.Equal(t, true, *fluxCfg.GitCISkip) + assert.Equal(t, true, fluxCfg.GitCISkip) argString = "--git-ci-skip=false" fluxCfg, err = ParseFluxArgs(argString) assert.Equal(t, nil, err) - assert.Equal(t, false, *fluxCfg.GitCISkip) + assert.Equal(t, false, fluxCfg.GitCISkip) // Test we only serialize props that we provided argString = "--git-label=foo --git-path=derp" fluxCfg, err = ParseFluxArgs(argString) assert.Equal(t, nil, err) - assert.Equal(t, "foo", *fluxCfg.GitLabel) + assert.Equal(t, "foo", fluxCfg.GitLabel) assert.Equal(t, "git-label=foo&git-path=derp", fluxCfg.AsQueryParams()) // string[] diff --git a/integration-tests/tests/kube-system-migration.sh b/integration-tests/tests/kube-system-migration.sh index c140cff1..f1d23c1b 100755 --- a/integration-tests/tests/kube-system-migration.sh +++ b/integration-tests/tests/kube-system-migration.sh @@ -35,6 +35,6 @@ while [ $(kubectl get pods -n kube-system -l 'app in (weave-flux, weave-cortex, echo "• Check old flux arguments have been applied to the new agent" args=$(kubectl get pod -n weave -l name=weave-flux-agent -o jsonpath='{.items[?(@.metadata.labels.name=="weave-flux-agent")].spec.containers[?(@.name=="flux-agent")].args[*]}') if [[ $args != *"--git-url=git@github.com:weaveworks/example --git-path=k8s/example --git-branch=master --git-label=example"* ]]; then - echo "Missing existing flux args: $args" + echo "Missing existing flux args: got $args" exit 1 fi From d1749ffff05d0ad90e7eba0c8fd352f4a7213d71 Mon Sep 17 00:00:00 2001 From: Simon Howe Date: Tue, 11 Jun 2019 11:04:22 +0200 Subject: [PATCH 6/6] Add test for parsing empty strings from flux args --- agent/main_test.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/agent/main_test.go b/agent/main_test.go index 85ce53bb..4aaa990b 100644 --- a/agent/main_test.go +++ b/agent/main_test.go @@ -131,4 +131,10 @@ func TestParseFluxArgs(t *testing.T) { fluxCfg, err = ParseFluxArgs(argString) assert.Equal(t, nil, err) assert.Equal(t, "git-path=zing", fluxCfg.AsQueryParams()) + + // Preserves empty values + argString = "--memcached-service=" + fluxCfg, err = ParseFluxArgs(argString) + assert.Equal(t, nil, err) + assert.Equal(t, "memcached-service=", fluxCfg.AsQueryParams()) }