From 0604ee25d7cc93f27202abc6cc97076846545826 Mon Sep 17 00:00:00 2001 From: Travis Bischel Date: Tue, 19 Jul 2022 16:25:35 -0600 Subject: [PATCH] rpk redpanda config set: improve setting arrays Recent rpk improvements now initialize index 0 of a slice if the key we are setting is deeply inside the slice. e.g., foo.bar.baz would initialize bar[0] to set baz. However, it was not possible to set foo.bar[1]. This change fixes that. We also allow setting one past the end of an array to extend it: if foo.bar[0] exists, then foo.bar[1].baz adds to the array. Old rpk never had the ability to set an array key to a single value that was not wrapped in brackets. For example, `foo.bar = "baz"` would not work if bar was an array. Now, we use / initialize element 0 and set that. Fixes #2958 Fixes #5498 Fixes #5264 --- src/go/rpk/pkg/cli/cmd/redpanda/config.go | 30 ++- src/go/rpk/pkg/config/config_test.go | 76 ++++++++ src/go/rpk/pkg/config/params.go | 223 ++++++++++++++++------ src/go/rpk/pkg/config/weak.go | 8 +- 4 files changed, 267 insertions(+), 70 deletions(-) diff --git a/src/go/rpk/pkg/cli/cmd/redpanda/config.go b/src/go/rpk/pkg/cli/cmd/redpanda/config.go index c5be52368df2..64ccf0497f70 100644 --- a/src/go/rpk/pkg/cli/cmd/redpanda/config.go +++ b/src/go/rpk/pkg/cli/cmd/redpanda/config.go @@ -49,18 +49,36 @@ func set(fs afero.Fs) *cobra.Command { ) c := &cobra.Command{ Use: "set ", - Short: "Set configuration values, such as the node IDs or the list of seed servers", - Long: `Set configuration values, such as the node IDs or the list of seed servers + Short: "Set configuration values, such as the redpanda node ID or the list of seed servers", + Long: `Set configuration values, such as the redpanda node ID or the list of seed servers -You can pass a key that represents the configuration property name, if it's a -nested property you should pass the group/category where it belongs, e.g: +This command modifies the redpanda.yaml you have locally on disk. The first +argument is the key within the yaml representing a property / field that you +would like to set. Nested fields can be accessed through a dot: rpk redpanda config set redpanda.developer_mode true -if --format is not used, rpk will use yaml as default, you can also pass -partial json/yaml config objects: +The default format is to parse the value as yaml. Individual specific fields +can be set, or full structs: + + rpk redpanda config set rpk.tune_disk_irq true + rpk redpanda config set redpanda.rpc_server '{address: 3.250.158.1, port: 9092}' + +You can set an entire array by wrapping all items with braces, or by using one +struct: + + rpk redpanda config set redpanda.advertised_kafka_api '[{address: 0.0.0.0, port: 9092}]' + rpk redpanda config set redpanda.advertised_kafka_api '{address: 0.0.0.0, port: 9092}' # same + +Indexing can be used to set specific items in an array. You can index one past +the end of an array to extend it: + + rpk redpanda config set redpanda.advertised_kafka_api[1] '{address: 0.0.0.0, port: 9092}' + +The json format can be used to set values as json: rpk redpanda config set redpanda.rpc_server '{"address":"0.0.0.0","port":33145}' --format json + `, Args: cobra.ExactArgs(2), Run: func(cmd *cobra.Command, args []string) { diff --git a/src/go/rpk/pkg/config/config_test.go b/src/go/rpk/pkg/config/config_test.go index 1551a5450c9d..6fd86af2177f 100644 --- a/src/go/rpk/pkg/config/config_test.go +++ b/src/go/rpk/pkg/config/config_test.go @@ -257,6 +257,7 @@ tune_cpu: true`, require.Exactly(st, expected, c.Redpanda.AdvertisedKafkaAPI) }, }, + { name: "set value of a slice", key: "redpanda.admin.port", @@ -265,6 +266,24 @@ tune_cpu: true`, require.Exactly(st, 9641, c.Redpanda.AdminAPI[0].Port) }, }, + + { + name: "set value of a slice with an index", + key: "redpanda.admin[0].port", + value: "9641", + check: func(st *testing.T, c *Config) { + require.Exactly(st, 9641, c.Redpanda.AdminAPI[0].Port) + }, + }, + { + name: "set value of a slice with an index at end extends slice", + key: "redpanda.admin[1].port", + value: "9648", + check: func(st *testing.T, c *Config) { + require.Exactly(st, 9648, c.Redpanda.AdminAPI[1].Port) + }, + }, + { name: "set slice single values", key: "redpanda.seed_servers.host.address", @@ -274,6 +293,7 @@ tune_cpu: true`, require.Exactly(st, "foo", c.Redpanda.SeedServers[0].Host.Address) }, }, + { name: "set slice object", key: "redpanda.seed_servers.host", @@ -284,6 +304,37 @@ tune_cpu: true`, require.Exactly(st, 80, c.Redpanda.SeedServers[0].Host.Port) }, }, + + { + name: "set slice with object defaults to index 0", + key: "redpanda.advertised_kafka_api", + value: `{address: 3.250.158.1, port: 9092}`, + format: "yaml", + check: func(st *testing.T, c *Config) { + require.Exactly(st, "3.250.158.1", c.Redpanda.AdvertisedKafkaAPI[0].Address) + require.Exactly(st, 9092, c.Redpanda.AdvertisedKafkaAPI[0].Port) + }, + }, + + { + name: "slices with one element works", + key: "rpk.kafka_api.brokers", + value: `127.0.0.0:9092`, + format: "yaml", + check: func(st *testing.T, c *Config) { + require.Exactly(st, "127.0.0.0:9092", c.Rpk.KafkaAPI.Brokers[0]) + }, + }, + { + name: "slices with one element works with indexing", + key: "rpk.kafka_api.brokers[0]", + value: `127.0.0.0:9092`, + format: "yaml", + check: func(st *testing.T, c *Config) { + require.Exactly(st, "127.0.0.0:9092", c.Rpk.KafkaAPI.Brokers[0]) + }, + }, + { name: "fail if the value isn't well formatted (json)", key: "redpanda", @@ -318,6 +369,31 @@ tune_cpu: true`, value: "foo", expectErr: true, }, + + { + name: "invalid negative index", + key: "redpanda.admin[-1].port", + value: "9641", + expectErr: true, + }, + { + name: "invalid large index", + key: "redpanda.admin[12310293812093823094801].port", + value: "9641", + expectErr: true, + }, + { + name: "invalid out of bounds index", + key: "redpanda.admin[2].port", // 0 is default, 1 is valid (extends by one), 2 is invalid + value: "9641", + expectErr: true, + }, + { + name: "index into other (unknown) field", + key: "redpanda.fiz[0]", + value: "9641", + expectErr: true, + }, } for _, tt := range tests { diff --git a/src/go/rpk/pkg/config/params.go b/src/go/rpk/pkg/config/params.go index 3ed84d00ea7d..409043478b16 100644 --- a/src/go/rpk/pkg/config/params.go +++ b/src/go/rpk/pkg/config/params.go @@ -17,6 +17,7 @@ import ( "os" "path/filepath" "reflect" + "regexp" "strconv" "strings" "time" @@ -571,9 +572,9 @@ func (c *Config) addUnsetDefaults() { } } -// Set allow to set a single configuration property by passing a key value pair +// Set allow to set a single configuration field by passing a key value pair // -// Key: string containing the yaml property tag, e.g: 'rpk.admin_api'. +// Key: string containing the yaml field tags, e.g: 'rpk.admin_api'. // Value: string representation of the value, either single value or partial // representation. // Format: either json or yaml (default: yaml). @@ -581,87 +582,127 @@ func (c *Config) Set(key, value, format string) error { if key == "" { return fmt.Errorf("key field must not be empty") } - props := strings.Split(key, ".") - rv := reflect.ValueOf(c).Elem() - field, other, err := getField(props, rv) + tags := strings.Split(key, ".") + for _, tag := range tags { + if _, _, err := splitTagIndex(tag); err != nil { + return err + } + } + + field, other, err := getField(tags, "", reflect.ValueOf(c).Elem()) if err != nil { return err } isOther := other != reflect.Value{} + // For Other fields, we need to wrap the value in key:value format when + // unmarshaling, and we forbid indexing. + var finalTag string if isOther { + finalTag = tags[len(tags)-1] + if _, index, _ := splitTagIndex(finalTag); index >= 0 { + return fmt.Errorf("cannot index into unknown field %q", finalTag) + } field = other } - if field.CanAddr() { - i := field.Addr().Interface() - in := value - switch strings.ToLower(format) { - // single is deprecated, leaving it here for backward compatibility. - case "yaml", "single", "": - if isOther { - p := props[len(props)-1] - in = fmt.Sprintf("%s: %s", p, value) - } - err = yaml.Unmarshal([]byte(in), i) - if err != nil { - return err - } - return nil - case "json": - if isOther { - p := props[len(props)-1] - in = fmt.Sprintf("{%q: %s}", p, value) - } - err = json.Unmarshal([]byte(in), i) - if err != nil { - return err - } - return nil - default: - return fmt.Errorf("unsupported format %s", format) + if !field.CanAddr() { + return errors.New("rpk bug, please describe how you encountered this at https://github.com/redpanda-data/redpanda/issues/new?assignees=&labels=kind%2Fbug&template=01_bug_report.md") + } + + var unmarshal func([]byte, interface{}) error + switch strings.ToLower(format) { + case "yaml", "single", "": // single is deprecated; it is kept for backcompat + if isOther { + value = fmt.Sprintf("%s: %s", finalTag, value) } + unmarshal = yaml.Unmarshal + case "json": + if isOther { + value = fmt.Sprintf("{%q: %s}", finalTag, value) + } + unmarshal = json.Unmarshal + default: + return fmt.Errorf("unsupported format %s", format) } - return errors.New("rpk bug, please describe how you encountered this at https://github.com/redpanda-data/redpanda/issues/new?assignees=&labels=kind%2Fbug&template=01_bug_report.md") -} -// getField deeply search in p for the value that reflect property props. -func getField(props []string, p reflect.Value) (reflect.Value, reflect.Value, error) { - if len(props) == 0 { - return p, reflect.Value{}, nil + // If we cannot unmarshal, but our error looks like we are trying to + // unmarshal a single element into a slice, we index[0] into the slice + // and try unmarshaling again. + if err := unmarshal([]byte(value), field.Addr().Interface()); err != nil { + if elem0, ok := tryValueAsSlice0(field, format, err); ok { + return unmarshal([]byte(value), elem0.Addr().Interface()) + } + return err } - if p.Kind() == reflect.Slice { - if p.Len() == 0 { - p.Set(reflect.Append(p, reflect.Indirect(reflect.New(p.Type().Elem())))) + return nil +} + +// getField deeply search in v for the value that reflect field tags. +// +// The parentRawTag is the previous tag, and includes an index if there is one. +func getField(tags []string, parentRawTag string, v reflect.Value) (reflect.Value, reflect.Value, error) { + // *At* the last element, we check if it is a slice. The final tag can + // still index into the slice and if that happens, we want to return + // the index: + // + // rpk.kafka_api.brokers[0] => return first broker. + // + if v.Kind() == reflect.Slice { + index := -1 + if parentRawTag != "" { + _, index, _ = splitTagIndex(parentRawTag) + } + if index < 0 { + // If there is no index and if there are no additional + // tags, we return the field itself (the slice). If + // there are more tags or there is an index, we index. + if len(tags) == 0 { + return v, reflect.Value{}, nil + } + index = 0 } - p = p.Index(0) + if index > v.Len() { + return reflect.Value{}, reflect.Value{}, fmt.Errorf("field %q: unable to modify index %d of %d elements", parentRawTag, index, v.Len()) + } else if index == v.Len() { + v.Set(reflect.Append(v, reflect.Indirect(reflect.New(v.Type().Elem())))) + } + v = v.Index(index) + } + + if len(tags) == 0 { + // Now, either this is not a slice and we return the field, or + // we indexed into the slice and we return the indexed value. + return v, reflect.Value{}, nil } - // If is a nil pointer we assign the zero value, and we reassign p to the - // value that p points to - if p.Kind() == reflect.Ptr { - if p.IsNil() { - p.Set(reflect.New(p.Type().Elem())) + tag, _, _ := splitTagIndex(tags[0]) // err already checked at the start in Set + + // If is a nil pointer we assign the zero value, and we reassign v to the + // value that v points to + if v.Kind() == reflect.Ptr { + if v.IsNil() { + v.Set(reflect.New(v.Type().Elem())) } - p = reflect.Indirect(p) + v = reflect.Indirect(v) } - if p.Kind() == reflect.Struct { - newP, other, err := getFieldByTag(props[0], p) + if v.Kind() == reflect.Struct { + newP, other, err := getFieldByTag(tag, v) if err != nil { return reflect.Value{}, reflect.Value{}, err } // if is "Other" map field, we stop the recursion and return if (other != reflect.Value{}) { - // user may try to set deep unmanaged property: + // user may try to set deep unmanaged field: // rpk.unmanaged.name = "name" - if len(props) > 1 { - return reflect.Value{}, reflect.Value{}, fmt.Errorf("unable to set property %q using rpk", strings.Join(props, ".")) + if len(tags) > 1 { + return reflect.Value{}, reflect.Value{}, fmt.Errorf("unable to set field %q using rpk", strings.Join(tags, ".")) } return reflect.Value{}, other, nil } - return getField(props[1:], newP) + return getField(tags[1:], tags[0], newP) } - return reflect.Value{}, reflect.Value{}, fmt.Errorf("unable to set field of type %v", p.Type()) + return reflect.Value{}, reflect.Value{}, fmt.Errorf("unable to set field of type %v", v.Type()) } // getFieldByTag finds a field with a given yaml tag and returns 3 parameters: @@ -669,11 +710,11 @@ func getField(props []string, p reflect.Value) (reflect.Value, reflect.Value, er // 1. if tag is found within the struct, return the field. // 2. if tag is not found _but_ the struct has "Other" field, return Other. // 3. Error if it can't find the given tag and "Other" field is unavailable. -func getFieldByTag(tag string, p reflect.Value) (reflect.Value, reflect.Value, error) { - t := p.Type() +func getFieldByTag(tag string, v reflect.Value) (reflect.Value, reflect.Value, error) { + t := v.Type() var other bool // Loop struct to get the field that match tag. - for i := 0; i < p.NumField(); i++ { + for i := 0; i < v.NumField(); i++ { // rpk allows blindly setting unknown configuration parameters in // Other map[string]interface{} fields if t.Field(i).Name == "Other" { @@ -688,14 +729,76 @@ func getFieldByTag(tag string, p reflect.Value) (reflect.Value, reflect.Value, e ft := strings.Split(yt, ",")[0] if ft == tag { - return p.Field(i), reflect.Value{}, nil + return v.Field(i), reflect.Value{}, nil } } // If we can't find the tag but the struct has an 'Other' map field: if other { - return reflect.Value{}, p.FieldByName("Other"), nil + return reflect.Value{}, v.FieldByName("Other"), nil } return reflect.Value{}, reflect.Value{}, fmt.Errorf("unable to find field %q", tag) } + +// All valid tags in redpanda.yaml are alphabetic_with_underscores. The k8s +// tests use dashes in and numbers in places for AdditionalConfiguration, and +// theoretically, a key may be added to redpanda in the future with a dash or a +// number. We will accept alphanumeric with any case, as well as dashes or +// underscores. That is plenty generous. +// +// 0: entire match +// 1: tag name +// 2: index, if present +var tagIndexRe = regexp.MustCompile(`^([_a-zA-Z0-9-]+)(?:\[(\d+)\])?$`) + +// We accept tags with indices such as foo[1]. This splits the index and +// returns it if present, or -1 if not present. +func splitTagIndex(tag string) (string, int, error) { + m := tagIndexRe.FindStringSubmatch(tag) + if len(m) == 0 { + return "", 0, fmt.Errorf("invalid field %q", tag) + } + + field := m[1] + + if m[2] != "" { + index, err := strconv.Atoi(m[2]) + if err != nil { + return "", 0, fmt.Errorf("invalid field %q index: %v", field, err) + } + return field, index, nil + } + + return field, -1, nil +} + +// If a value is a slice and our error indicates we are decoding a single +// element into the slice, we create index 0 and return that to be unmarshaled +// into. +// +// For json this is nice, the error is explicit. For yaml, we have to string +// match and it is a bit rough. +func tryValueAsSlice0(v reflect.Value, format string, err error) (reflect.Value, bool) { + if v.Kind() != reflect.Slice { + return v, false + } + + switch format { + case "json": + if te := (*json.UnmarshalTypeError)(nil); !errors.As(err, &te) || te.Type.Kind() != reflect.Slice { + return v, false + } + case "yaml": + if !strings.Contains(err.Error(), "cannot unmarshal !!") { + return v, false + } + } + if v.Len() == 0 { + v.Set(reflect.Append(v, reflect.Indirect(reflect.New(v.Type().Elem())))) + } + // We are setting an entire array with one item; we always clear what + // existed previously. + v.Set(v.Slice(0, 1)) + return v.Index(0), true +} diff --git a/src/go/rpk/pkg/config/weak.go b/src/go/rpk/pkg/config/weak.go index 0c9b6a2c4cca..84405930b7a2 100644 --- a/src/go/rpk/pkg/config/weak.go +++ b/src/go/rpk/pkg/config/weak.go @@ -466,7 +466,7 @@ func (k *KafkaClient) UnmarshalYAML(n *yaml.Node) error { SASLMechanism *weakString `yaml:"sasl_mechanism"` SCRAMUsername *weakString `yaml:"scram_username"` SCRAMPassword *weakString `yaml:"scram_password"` - Other map[string]interface{} `yaml:",inline" mapstructure:",remain"` + Other map[string]interface{} `yaml:",inline"` } if err := n.Decode(&internal); err != nil { return err @@ -504,7 +504,7 @@ func (s *ServerTLS) UnmarshalYAML(n *yaml.Node) error { TruststoreFile weakString `yaml:"truststore_file"` Enabled weakBool `yaml:"enabled"` RequireClientAuth weakBool `yaml:"require_client_auth"` - Other map[string]interface{} `yaml:",inline" mapstructure:",remain"` + Other map[string]interface{} `yaml:",inline"` } if err := n.Decode(&internal); err != nil { return err @@ -574,8 +574,8 @@ func (sa *SocketAddress) UnmarshalYAML(n *yaml.Node) error { func (nsa *NamedSocketAddress) UnmarshalYAML(n *yaml.Node) error { var internal struct { Name weakString `yaml:"name"` - Address weakString `yaml:"address" mapstructure:"address"` - Port weakInt `yaml:"port" mapstructure:"port"` + Address weakString `yaml:"address"` + Port weakInt `yaml:"port"` } if err := n.Decode(&internal); err != nil {