From 938f8020dda8917ec4a753ec15e35850c63eb6b4 Mon Sep 17 00:00:00 2001 From: Kimmo Lehto Date: Tue, 5 Nov 2024 13:00:54 +0200 Subject: [PATCH] Fix InstallFlags change detection with kubelet-extra-args Signed-off-by: Kimmo Lehto --- internal/shell/split.go | 62 ++++++ internal/shell/unquote.go | 80 ++++++++ phase/gather_k0s_facts.go | 11 +- phase/reinstall.go | 2 +- .../v1beta1/cluster/flags.go | 78 +++++++- .../v1beta1/cluster/flags_test.go | 33 ++++ .../v1beta1/cluster/host.go | 182 +++++------------- 7 files changed, 292 insertions(+), 156 deletions(-) create mode 100644 internal/shell/split.go create mode 100644 internal/shell/unquote.go diff --git a/internal/shell/split.go b/internal/shell/split.go new file mode 100644 index 00000000..b6a8fc56 --- /dev/null +++ b/internal/shell/split.go @@ -0,0 +1,62 @@ +package shell + +// this is borrowed as-is from rig v2 until k0sctl is updated to use it + +import ( + "fmt" + "strings" +) + +// Split splits the input string respecting shell-like quoted segments. +func Split(input string) ([]string, error) { //nolint:cyclop + var segments []string + + currentSegment, ok := builderPool.Get().(*strings.Builder) + if !ok { + currentSegment = &strings.Builder{} + } + defer builderPool.Put(currentSegment) + defer currentSegment.Reset() + + var inDoubleQuotes, inSingleQuotes, isEscaped bool + + for i := range len(input) { + currentChar := input[i] + + if isEscaped { + currentSegment.WriteByte(currentChar) + isEscaped = false + continue + } + + switch { + case currentChar == '\\' && !inSingleQuotes: + isEscaped = true + case currentChar == '"' && !inSingleQuotes: + inDoubleQuotes = !inDoubleQuotes + case currentChar == '\'' && !inDoubleQuotes: + inSingleQuotes = !inSingleQuotes + case currentChar == ' ' && !inDoubleQuotes && !inSingleQuotes: + // Space outside quotes; delimiter for a new segment + segments = append(segments, currentSegment.String()) + currentSegment.Reset() + default: + currentSegment.WriteByte(currentChar) + } + } + + if inDoubleQuotes || inSingleQuotes { + return nil, fmt.Errorf("split `%q`: %w", input, ErrMismatchedQuotes) + } + + if isEscaped { + return nil, fmt.Errorf("split `%q`: %w", input, ErrTrailingBackslash) + } + + // Add the last segment if present + if currentSegment.Len() > 0 { + segments = append(segments, currentSegment.String()) + } + + return segments, nil +} diff --git a/internal/shell/unquote.go b/internal/shell/unquote.go new file mode 100644 index 00000000..d4da95a5 --- /dev/null +++ b/internal/shell/unquote.go @@ -0,0 +1,80 @@ +package shell + +import ( + "errors" + "fmt" + "strings" + "sync" +) + +// This is borrowed from rig v2 until k0sctl is updated to use it + +var ( + builderPool = sync.Pool{ + New: func() interface{} { + return &strings.Builder{} + }, + } + + // ErrMismatchedQuotes is returned when the input string has mismatched quotes when unquoting. + ErrMismatchedQuotes = errors.New("mismatched quotes") + + // ErrTrailingBackslash is returned when the input string ends with a trailing backslash. + ErrTrailingBackslash = errors.New("trailing backslash") +) + +// Unquote is a mostly POSIX compliant implementation of unquoting a string the same way a shell would. +// Variables and command substitutions are not handled. +func Unquote(input string) (string, error) { //nolint:cyclop + sb, ok := builderPool.Get().(*strings.Builder) + if !ok { + sb = &strings.Builder{} + } + defer builderPool.Put(sb) + defer sb.Reset() + + var inDoubleQuotes, inSingleQuotes, isEscaped bool + + for i := range len(input) { + currentChar := input[i] + + if isEscaped { + sb.WriteByte(currentChar) + isEscaped = false + continue + } + + switch currentChar { + case '\\': + if !inSingleQuotes { // Escape works in double quotes or outside any quotes + isEscaped = true + } else { + sb.WriteByte(currentChar) // Treat as a regular character within single quotes + } + case '"': + if !inSingleQuotes { // Toggle double quotes only if not in single quotes + inDoubleQuotes = !inDoubleQuotes + } else { + sb.WriteByte(currentChar) // Treat as a regular character within single quotes + } + case '\'': + if !inDoubleQuotes { // Toggle single quotes only if not in double quotes + inSingleQuotes = !inSingleQuotes + } else { + sb.WriteByte(currentChar) // Treat as a regular character within double quotes + } + default: + sb.WriteByte(currentChar) + } + } + + if inDoubleQuotes || inSingleQuotes { + return "", fmt.Errorf("unquote `%q`: %w", input, ErrMismatchedQuotes) + } + + if isEscaped { + return "", fmt.Errorf("unquote `%q`: %w", input, ErrTrailingBackslash) + } + + return sb.String(), nil +} diff --git a/phase/gather_k0s_facts.go b/phase/gather_k0s_facts.go index 114b1229..62961975 100644 --- a/phase/gather_k0s_facts.go +++ b/phase/gather_k0s_facts.go @@ -7,7 +7,6 @@ import ( "net" "net/url" "path" - "regexp" "strings" "github.com/k0sproject/dig" @@ -183,8 +182,6 @@ func (p *GatherK0sFacts) listEtcdMembers(h *cluster.Host) error { return nil } -var escapedSpaceRe = regexp.MustCompile(`\\x20`) - func (p *GatherK0sFacts) investigateK0s(h *cluster.Host) error { output, err := h.ExecOutput(h.Configurer.K0sCmdf("version"), exec.Sudo(h)) if err != nil { @@ -279,14 +276,10 @@ func (p *GatherK0sFacts) investigateK0s(h *cluster.Host) error { if len(status.Args) > 2 { // status.Args contains the binary path and the role as the first two elements, which we can ignore here. for _, a := range status.Args[2:] { - parts := strings.SplitN(a, "=", 2) - key, value := parts[0], parts[1] - // status output has escaped spaces, replace them with real spaces - escapedSpaceRe.ReplaceAllString(value, " ") - args.Add(fmt.Sprintf("%s=%s", key, value)) + args.Add(a) } } - h.Metadata.K0sStatusArgs = []string(args) + h.Metadata.K0sStatusArgs = args log.Infof("%s: is running k0s %s version %s", h, h.Role, h.Metadata.K0sRunningVersion) if h.IsController() { diff --git a/phase/reinstall.go b/phase/reinstall.go index 1ef42163..b22b42e0 100644 --- a/phase/reinstall.go +++ b/phase/reinstall.go @@ -77,7 +77,7 @@ func (p *Reinstall) reinstall(h *cluster.Host) error { h.InstallFlags.AddOrReplace("--enable-dynamic-config") } - h.InstallFlags.AddOrReplace("--force") + h.InstallFlags.AddOrReplace("--force=true") cmd, err := h.K0sInstallCommand() if err != nil { diff --git a/pkg/apis/k0sctl.k0sproject.io/v1beta1/cluster/flags.go b/pkg/apis/k0sctl.k0sproject.io/v1beta1/cluster/flags.go index 5095a6e8..853f6beb 100644 --- a/pkg/apis/k0sctl.k0sproject.io/v1beta1/cluster/flags.go +++ b/pkg/apis/k0sctl.k0sproject.io/v1beta1/cluster/flags.go @@ -1,8 +1,12 @@ package cluster import ( + "fmt" "strconv" "strings" + + "github.com/alessio/shellescape" + "github.com/k0sproject/k0sctl/internal/shell" ) // Flags is a slice of strings with added functions to ease manipulating lists of command-line flags @@ -10,30 +14,42 @@ type Flags []string // Add adds a flag regardless if it exists already or not func (f *Flags) Add(s string) { + if ns, err := shell.Unquote(s); err == nil { + s = ns + } *f = append(*f, s) } // Add a flag with a value func (f *Flags) AddWithValue(key, value string) { - *f = append(*f, key+" "+value) + if nv, err := shell.Unquote(value); err == nil { + value = nv + } + *f = append(*f, key+"="+value) } // AddUnlessExist adds a flag unless one with the same prefix exists func (f *Flags) AddUnlessExist(s string) { + if ns, err := shell.Unquote(s); err == nil { + s = ns + } if f.Include(s) { return } - *f = append(*f, s) + f.Add(s) } // AddOrReplace replaces a flag with the same prefix or adds a new one if one does not exist func (f *Flags) AddOrReplace(s string) { + if ns, err := shell.Unquote(s); err == nil { + s = ns + } idx := f.Index(s) if idx > -1 { (*f)[idx] = s return } - *f = append(*f, s) + f.Add(s) } // Include returns true if a flag with a matching prefix can be found @@ -43,6 +59,9 @@ func (f Flags) Include(s string) bool { // Index returns an index to a flag with a matching prefix func (f Flags) Index(s string) int { + if ns, err := shell.Unquote(s); err == nil { + s = ns + } var flag string sepidx := strings.IndexAny(s, "= ") if sepidx < 0 { @@ -73,6 +92,9 @@ func (f Flags) GetValue(s string) string { if fl == "" { return "" } + if nfl, err := shell.Unquote(fl); err == nil { + fl = nfl + } idx := strings.IndexAny(fl, "= ") if idx < 0 { @@ -80,10 +102,6 @@ func (f Flags) GetValue(s string) string { } val := fl[idx+1:] - s, err := strconv.Unquote(val) - if err == nil { - return s - } return val } @@ -137,5 +155,49 @@ func (f *Flags) MergeAdd(b Flags) { // Join creates a string separated by spaces func (f *Flags) Join() string { - return strings.Join(*f, " ") + parts := make([]string, len(*f)) + f.Each(func(k, v string) { + parts = append(parts, fmt.Sprintf("%s=%s", k, shellescape.Quote(v))) + }) + return strings.Join(parts, " ") +} + +// Each iterates over each flag and calls the function with the flag key and value as arguments +func (f Flags) Each(fn func(string, string)) { + for _, flag := range f { + sepidx := strings.IndexAny(flag, "= ") + if sepidx < 0 { + fn(flag, "") + } else { + key, value := flag[:sepidx], flag[sepidx+1:] + if unq, err := shell.Unquote(value); err == nil { + value = unq + } + fn(key, value) + } + } +} + +// Map returns a map[string]string of the flags where the key is the flag and the value is the value +func (f Flags) Map() map[string]string { + res := make(map[string]string) + f.Each(func(k, v string) { + res[k] = v + }) + return res +} + +// Equals compares the flags with another Flags and returns true if they have the same flags and values, ignoring order +func (f Flags) Equals(b Flags) bool { + if len(f) != len(b) { + return false + } + for _, flag := range f { + ourValue := f.GetValue(flag) + theirValue := b.GetValue(flag) + if ourValue != theirValue { + return false + } + } + return true } diff --git a/pkg/apis/k0sctl.k0sproject.io/v1beta1/cluster/flags_test.go b/pkg/apis/k0sctl.k0sproject.io/v1beta1/cluster/flags_test.go index fd29610a..50a4998b 100644 --- a/pkg/apis/k0sctl.k0sproject.io/v1beta1/cluster/flags_test.go +++ b/pkg/apis/k0sctl.k0sproject.io/v1beta1/cluster/flags_test.go @@ -99,6 +99,39 @@ func TestGetBoolean(t *testing.T) { result, err := flags.GetBoolean("--flag3") require.NoError(t, err) require.Equal(t, result, false) + }) +} +func TestEach(t *testing.T) { + flags := Flags{"--flag1", "--flag2=foo", "--flag3=bar"} + var countF, countV int + flags.Each(func(flag string, value string) { + countF++ + if value != "" { + countV++ + } }) + require.Equal(t, 3, countF) + require.Equal(t, 2, countV) +} + +func TestMap(t *testing.T) { + flags := Flags{"--flag1", "--flag2=foo", "--flag3=bar"} + m := flags.Map() + require.Len(t, m, 3) + require.Equal(t, "", m["--flag1"]) + require.Equal(t, "foo", m["--flag2"]) + require.Equal(t, "bar", m["--flag3"]) +} + +func TestEquals(t *testing.T) { + flags1 := Flags{"--flag1", "--flag2=foo", "--flag3=bar"} + flags2 := Flags{"--flag1", "--flag2=foo", "--flag3=bar"} + require.True(t, flags1.Equals(flags2)) + + flags2 = Flags{"--flag1", "--flag2=foo"} + require.False(t, flags1.Equals(flags2)) + + flags2 = Flags{"--flag1", "--flag2=foo", "--flag3=baz"} + require.False(t, flags1.Equals(flags2)) } diff --git a/pkg/apis/k0sctl.k0sproject.io/v1beta1/cluster/host.go b/pkg/apis/k0sctl.k0sproject.io/v1beta1/cluster/host.go index 7c2f460c..dce6dc32 100644 --- a/pkg/apis/k0sctl.k0sproject.io/v1beta1/cluster/host.go +++ b/pkg/apis/k0sctl.k0sproject.io/v1beta1/cluster/host.go @@ -5,9 +5,6 @@ import ( "net/url" gos "os" gopath "path" - "regexp" - "sort" - "strconv" "strings" "time" @@ -16,6 +13,7 @@ import ( "github.com/go-playground/validator/v10" "github.com/jellydator/validation" "github.com/jellydator/validation/is" + "github.com/k0sproject/k0sctl/internal/shell" "github.com/k0sproject/rig" "github.com/k0sproject/rig/exec" "github.com/k0sproject/rig/os" @@ -184,7 +182,7 @@ type HostMetadata struct { K0sNewConfig string K0sJoinToken string K0sJoinTokenID string - K0sStatusArgs []string + K0sStatusArgs Flags Arch string IsK0sLeader bool Hostname string @@ -275,64 +273,64 @@ func (h *Host) K0sConfigPath() string { return h.Configurer.K0sConfigPath() } -// unquote + unescape a string -func unQE(s string) string { - unq, err := strconv.Unquote(s) - if err != nil { - return s +func (h *Host) K0sRole() string { + switch h.Role { + case "controller+worker": + return "controller" + case "single": + return "controller" } - - c := string(s[0]) // string was quoted, c now has the quote char - re := regexp.MustCompile(fmt.Sprintf(`(?:^|[^\\])\\%s`, c)) // replace \" with " (remove escaped quotes inside quoted string) - return string(re.ReplaceAllString(unq, c)) + return h.Role } -// K0sInstallCommand returns a full command that will install k0s service with necessary flags -func (h *Host) K0sInstallCommand() (string, error) { - role := h.Role +func (h *Host) K0sInstallFlags() (Flags, error) { flags := Flags(h.InstallFlags) - flags.AddOrReplace(fmt.Sprintf("--data-dir=%s", h.K0sDataDir())) + flags.AddOrReplace(fmt.Sprintf("--data-dir=%s", shellescape.Quote(h.K0sDataDir()))) - switch role { + switch h.Role { case "controller+worker": - role = "controller" flags.AddUnlessExist("--enable-worker") if h.NoTaints { flags.AddUnlessExist("--no-taints") } case "single": - role = "controller" flags.AddUnlessExist("--single") } if !h.Metadata.IsK0sLeader { - flags.AddUnlessExist(fmt.Sprintf(`--token-file "%s"`, h.K0sJoinTokenPath())) + flags.AddUnlessExist(fmt.Sprintf(`--token-file=%s`, shellescape.Quote(h.K0sJoinTokenPath()))) } if h.IsController() { - flags.AddUnlessExist(fmt.Sprintf(`--config "%s"`, h.K0sConfigPath())) + flags.AddUnlessExist(fmt.Sprintf(`--config=%s`, shellescape.Quote(h.K0sConfigPath()))) } if strings.HasSuffix(h.Role, "worker") { var extra Flags if old := flags.GetValue("--kubelet-extra-args"); old != "" { - extra = Flags{unQE(old)} + parts, err := shell.Split(old) + if err != nil { + return flags, fmt.Errorf("failed to split kubelet-extra-args: %w", err) + } + for _, part := range parts { + extra.Add(part) + } } // set worker's private address to --node-ip in --extra-kubelet-args if cloud ins't enabled enableCloudProvider, err := h.InstallFlags.GetBoolean("--enable-cloud-provider") if err != nil { - return "", fmt.Errorf("--enable-cloud-provider flag is set to invalid value: %s. (%v)", h.InstallFlags.GetValue("--enable-cloud-provider"), err) + return flags, fmt.Errorf("--enable-cloud-provider flag is set to invalid value: %s. (%v)", h.InstallFlags.GetValue("--enable-cloud-provider"), err) } if !enableCloudProvider && h.PrivateAddress != "" { - extra.AddUnlessExist(fmt.Sprintf("--node-ip=%s", h.PrivateAddress)) + extra.AddUnlessExist("--node-ip=" + h.PrivateAddress) } if h.HostnameOverride != "" { - extra.AddOrReplace(fmt.Sprintf("--hostname-override=%s", h.HostnameOverride)) + extra.AddOrReplace("--hostname-override=" + h.HostnameOverride) } if extra != nil { - flags.AddOrReplace(fmt.Sprintf("--kubelet-extra-args=%s", strconv.Quote(extra.Join()))) + flags.AddOrReplace(fmt.Sprintf("--kubelet-extra-args=%s", shellescape.Quote(extra.Join()))) } } @@ -341,7 +339,17 @@ func (h *Host) K0sInstallCommand() (string, error) { flags.Delete("--force") } - return h.Configurer.K0sCmdf("install %s %s", role, flags.Join()), nil + return flags, nil +} + +// K0sInstallCommand returns a full command that will install k0s service with necessary flags +func (h *Host) K0sInstallCommand() (string, error) { + flags, err := h.K0sInstallFlags() + if err != nil { + return "", err + } + + return h.Configurer.K0sCmdf("install %s %s", h.K0sRole(), flags.Join()), nil } // K0sBackupCommand returns a full command to be used as run k0s backup @@ -572,121 +580,19 @@ func (h *Host) ExpandTokens(input string, k0sVersion *version.Version) string { return builder.String() } -var flagParseRe = regexp.MustCompile(`--?([\w\-]+)(?:[=\s](\S+))?`) - // FlagsChanged returns true when the flags have changed by comparing the host.Metadata.K0sStatusArgs to what host.InstallFlags would produce func (h *Host) FlagsChanged() bool { - var formattedFlags []string - - cmd, err := h.K0sInstallCommand() - if err != nil { - log.Warnf("%s: could not get install command: %s", h, err) - return false - } - flags, err := shellSplit(cmd) + installFlags, err := h.K0sInstallFlags() if err != nil { - log.Warnf("%s: could not split install command: %s", h, err) - return false - } - if len(flags) < 4 { - // ["k0s", "install", , ] - log.Debugf("%s: no installFlags", h) - return false - } - flags = flags[3:] // discard the first 3 elements - - // format the flags the same way as spf13/cobra does in k0s - for i := 0; i < len(flags); i++ { - flag := flags[i] - var key string - var value string - match := flagParseRe.FindStringSubmatch(flag) - if len(match) < 2 { - log.Warnf("%s: could not parse flag: %s", h, flag) - continue - } - - key = match[1] - - if len(match) > 2 && len(match[2]) > 0 { - value = match[2] - } else if len(flags) > i+1 && !strings.HasPrefix(flags[i+1], "-") { - value = flags[i+1] - i++ - } else { - value = "true" - } - if s, err := strconv.Unquote(value); err == nil { - value = s - } - formattedFlags = append(formattedFlags, fmt.Sprintf("--%s=%s", key, value)) - } - - if len(h.Metadata.K0sStatusArgs) != len(formattedFlags) { - log.Debugf("%s: installFlags seem to have changed because of different length: %d %v vs %d %v", h, len(h.Metadata.K0sStatusArgs), h.Metadata.K0sStatusArgs, len(formattedFlags), formattedFlags) - return true + log.Warnf("%s: could not get install flags: %s", h, err) + installFlags = Flags{} } - var k0sArgs []string - copy(k0sArgs, h.Metadata.K0sStatusArgs) - sort.Strings(formattedFlags) - sort.Strings(k0sArgs) - for i := range formattedFlags { - if formattedFlags[i] != k0sArgs[i] { - log.Debugf("%s: installFlags seem to have changed because of different flags: %v vs %v", h, formattedFlags, k0sArgs) - return true - } - } - - log.Debugf("%s: installFlags have not changed", h) - - return false -} - -// Split splits the input string respecting shell-like quoted segments -- borrowed from rig v2 until migration -func shellSplit(input string) ([]string, error) { //nolint:cyclop - var segments []string - currentSegment := &strings.Builder{} - - var inDoubleQuotes, inSingleQuotes, isEscaped bool - - for i := range len(input) { - currentChar := input[i] - - if isEscaped { - currentSegment.WriteByte(currentChar) - isEscaped = false - continue - } - - switch { - case currentChar == '\\' && !inSingleQuotes: - isEscaped = true - case currentChar == '"' && !inSingleQuotes: - inDoubleQuotes = !inDoubleQuotes - case currentChar == '\'' && !inDoubleQuotes: - inSingleQuotes = !inSingleQuotes - case currentChar == ' ' && !inDoubleQuotes && !inSingleQuotes: - // Space outside quotes; delimiter for a new segment - segments = append(segments, currentSegment.String()) - currentSegment.Reset() - default: - currentSegment.WriteByte(currentChar) - } - } - - if inDoubleQuotes || inSingleQuotes { - return nil, fmt.Errorf("split `%q`: mismatched quotes", input) - } - - if isEscaped { - return nil, fmt.Errorf("split `%q`: trailing backslash", input) - } - - // Add the last segment if present - if currentSegment.Len() > 0 { - segments = append(segments, currentSegment.String()) + if installFlags.Equals(h.Metadata.K0sStatusArgs) { + log.Debugf("%s: installFlags have not changed", h) + return false } - return segments, nil + log.Debugf("%s: installFlags seem to have changed. existing: %+v new: %+v", h, h.Metadata.K0sStatusArgs.Map(), installFlags.Map()) + return true }