Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove viper and mapstructure from rpk #5061

Merged
merged 1 commit into from
Jun 29, 2022

Conversation

r-vasquez
Copy link
Contributor

@r-vasquez r-vasquez commented Jun 7, 2022

Cover letter

This PR removes the dependency of viper and mapstructure for our config file handling. Now we have control over Write and Read operations in our config file.

We introduced in #4894 the Weak Types, which allow us to handle weakly typed parameters.

It also includes the upgrade from Yaml.v2 to Yaml.v3, one of the key major changes in this library is the indentation of 4 instead of 2. So our config files will look different (but valid although)

Fixes #4531, Fixes #4176, Fixes #5202

Also, now rpk prevents the user to change ownership of config files, so with this it:

Fixes #4046, Fixes #3064, Fixes #1919

What is changing

Config File:

Now you will be able to detect if your configuration file contains some weak types that were allowed before but now we want to enforce strict typing. e.g:

redpanda:
    kafka_api:
        name: test
        address: 127.0.0.1
        port: 4664

will be parsed as a YAML list

redpanda:
    kafka_api:
        - name: test
          address: 127.0.0.1
          port: 4664

As you can see above, the indent is now with 4 spaces instead of 2, this is enforced in the new yaml.v3 package and still valid.

Old code will write redpanda.seed_servers.node_id, this property is no longer needed/supported. We are stripping it out from the config, so:

- node_id: 1
  host:
    address: 192.168.10.1
    port: 33145
- node_id: 2
  host:
    address: 192.168.10.2
    port: 33145
- node_id: 3
  host:
    address: 192.168.10.3
    port: 33145

Will become:

- host:
    address: 192.168.10.1
    port: 33145
- host:
    address: 192.168.10.2
    port: 33145
- host:
    address: 192.168.10.3
    port: 33145

Behavior changes/notes:

When running params.Load() method we are doing some backward compatibility changes, one of them is to copy rpk.tls to rpk.kafka_api.tls and rpk.amin_api.tls. This is already in place and working for some commands, with this PR will work for ALL commands that load the config file.

The old manager created a directory when flag --config was passed, we will keep this behavior for now.

Also removed an undocumented and unused way of changing config parameters in camelCase using --format json in rpk config set.

Ducktape Tests:

  • Changed expected configuration order and indentation.
  • Added logs for failure scenarios.
  • Changed expected value for redpanda.seed_servers (see above)

K8S Configurator:

  • Removed manager as a dependency to use only config.Params and config.Config methods to load and manipulate configuration.

RPK Go Code:

  • Removed Viper and Mapstructure
  • Upgrade yaml from v2 to v3
  • Removed config.Manager interface and it’s implementations.
  • Deleted old/unused config/license.go

Release notes

  • rpk preserves now the file ownership of the configuration file.
  • rpk will print warnings of errors in configuration file parameters.
  • rpk: bugfix, redpanda start no longer strips empty tls values.

@r-vasquez r-vasquez force-pushed the remove-viper-mapstructure branch 5 times, most recently from 20db5bc to 1a9585a Compare June 9, 2022 18:25
src/go/rpk/pkg/cli/cmd/container/start.go Outdated Show resolved Hide resolved
src/go/rpk/pkg/cli/cmd/redpanda/config.go Outdated Show resolved Hide resolved
src/go/rpk/pkg/cli/cmd/redpanda/mode.go Outdated Show resolved Hide resolved
src/go/rpk/pkg/cli/cmd/redpanda/mode.go Outdated Show resolved Hide resolved
src/go/rpk/pkg/config/params_test.go Outdated Show resolved Hide resolved
src/go/rpk/pkg/config/config.go Outdated Show resolved Hide resolved
src/go/rpk/pkg/config/config.go Outdated Show resolved Hide resolved
src/go/rpk/pkg/config/config_test.go Outdated Show resolved Hide resolved
src/go/rpk/pkg/config/config_test.go Show resolved Hide resolved
src/go/rpk/pkg/config/config_test.go Show resolved Hide resolved
@r-vasquez r-vasquez force-pushed the remove-viper-mapstructure branch 3 times, most recently from df1034e to a732ee6 Compare June 9, 2022 23:04
src/go/rpk/pkg/config/weak.go Outdated Show resolved Hide resolved
src/go/rpk/pkg/config/weak.go Outdated Show resolved Hide resolved
@r-vasquez r-vasquez force-pushed the remove-viper-mapstructure branch 5 times, most recently from 88afb49 to 42ea445 Compare June 11, 2022 00:00
@r-vasquez r-vasquez force-pushed the remove-viper-mapstructure branch 10 times, most recently from 352163f to 3cf5944 Compare June 15, 2022 23:03
@r-vasquez r-vasquez changed the title WIP: remove viper and mapstructure Remove viper and mapstructure from rpk Jun 23, 2022
@r-vasquez r-vasquez marked this pull request as ready for review June 23, 2022 21:35
@r-vasquez r-vasquez requested review from dotnwat, NyaliaLui, 0x5d and a team as code owners June 23, 2022 21:35
@r-vasquez r-vasquez requested a review from twmb June 23, 2022 21:36
Copy link
Contributor

@twmb twmb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some preliminary comments and questions!

src/go/rpk/pkg/cli/cmd/redpanda/config.go Outdated Show resolved Hide resolved
src/go/rpk/pkg/cli/cmd/redpanda/config.go Outdated Show resolved Hide resolved
src/go/rpk/pkg/cli/cmd/redpanda/config.go Show resolved Hide resolved
src/go/rpk/pkg/cli/cmd/redpanda/mode.go Show resolved Hide resolved
src/go/rpk/pkg/cli/cmd/redpanda/mode.go Outdated Show resolved Hide resolved
src/go/rpk/pkg/config/params.go Outdated Show resolved Hide resolved
src/go/rpk/pkg/config/params.go Outdated Show resolved Hide resolved
src/go/rpk/pkg/config/params.go Outdated Show resolved Hide resolved
src/go/rpk/pkg/config/params.go Show resolved Hide resolved
p := props[len(props)-1]
in = fmt.Sprintf("%s: %s", p, value)
}
err = yaml.Unmarshal([]byte(in), i)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why doesn't this properly call weak unmarshaling for socket addresses, etc? I think you brought up that setting redpanda.seed_servers is different than setting redpanda with a value containing seed servers?

Copy link
Contributor Author

@r-vasquez r-vasquez Jun 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't call weak unmarshaling for any of the one_or_many weak types, since we created other intermediary types for them:

[]NamedSocketAddress -> NamedSocketAddresses
[]SeedServer -> SeedServers
... 

We use the intermediary types in the general config.Config type, not in every field, e.g : https://github.com/redpanda-data/redpanda/blob/dev/src/go/rpk/pkg/config/weak.go#L316L335

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have an idea for how to solve this for the yaml case, but not the json. In the yaml case, when we return, we could have a huge type switch:

case []string => return weakStrings(xyz)
case []NamedSocketAddress => return namedSocketAddresses(xyz)

this only works for yaml because weak decoding is only implemented for yaml
this theoretically could be supported for json in a followup PR if we want,
but I think this should be tracked as a separate issue: allow setting slices with one element in set

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created: #5264

src/go/rpk/pkg/config/weak_test.go Outdated Show resolved Hide resolved
src/go/rpk/pkg/config/weak_test.go Outdated Show resolved Hide resolved
This commit removes viper and mapstructure
These are all the required changes to handle the
parsing, weak typing and writes to config file.

From now on we will print warning to users so they
know what needs to be changed in the config file
before we deprecate this weak typing.

This also include Yaml V3 upgrade, one notable
change is the indent in yaml files.
@r-vasquez
Copy link
Contributor Author

r-vasquez commented Jun 29, 2022

Failure in: https://buildkite.com/redpanda/redpanda/builds/11813#0181ad2c-892a-4d2e-a49e-3e5a71f4cc95

Possibly related to: #2406, checking if the same issue or if I should create a new one.

Copy link
Contributor

@twmb twmb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome

@emaxerrno
Copy link
Contributor

@r-vasquez awesome work!

r-vasquez added a commit to r-vasquez/redpanda that referenced this pull request Jul 12, 2022
In the viper removal PR (redpanda-data#5061) we introduced
a bug:

rpk redpanda start will load the file and update
some unset defaults such as rpk.kafka_api.

This breaks backward compatibility for some users
so we are correcting it in this commit.
r-vasquez added a commit to r-vasquez/redpanda that referenced this pull request Jul 13, 2022
In the viper removal PR (redpanda-data#5061) we introduced
a bug:

rpk redpanda start will load the file and update
some unset defaults such as rpk.kafka_api.

This breaks backward compatibility for some users
so we are correcting it in this commit.
r-vasquez added a commit to r-vasquez/redpanda that referenced this pull request Jul 14, 2022
In the viper removal PR (redpanda-data#5061) we introduced
a bug:

rpk redpanda start will load the file and update
some unset defaults such as rpk.kafka_api.

This breaks backward compatibility for some users
so we are correcting it in this commit.
@r-vasquez r-vasquez deleted the remove-viper-mapstructure branch August 11, 2022 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants