Rpk config: Support polymorphic advertised kafka api

[0x5d]

Redpanda supports 2 different structures for redpanda.advertised_kafka_api:

redpanda:
  advertised_kafka_api:
    address: "127.0.0.1"
    port: 9092

and

redpanda:
  advertised_kafka_api:
  - address: "127.0.0.1"
    port: 9092
  - address: "localhost"
    port: 9092

However, rpk only recognized the 1st one. This change set adds support for both.

It also introduces config versioning. From here on, whenever there's a change in the config structure, a new config version schema will be declared, to which the config file contents can be marshalled unto. The new schema should implement ToGeneric() (*Config, error), which should translate that schema to the "generic" one declared in config/schema.go.

Pending work:

• Support the different structures for redpanda.seed_servers
• Support a list for redpanda.kafka_api
• Support names for redpanda.kafka_api & redpanda.advertised_kafka_api items.

The above changes will be made in a follow-up PR, so that this one can be merged faster to unblock other work that depends on it.

Fix #524

[0x5d]

Set as draft while I write more tests.

[dotnwat]

Redpanda supports 2 different structures for redpanda.advertised_kafka_api:

However, rpk only recognized the 1st one. This change set adds support for both.

It also introduces config versioning. From here on, whenever there's a change in the config structure, a new config version schema will be declared, to which the config file contents can be marshalled unto. The new schema should implement ToGeneric() (*Config, error), which should translate that schema to the "generic" one declared in config/schema.go.

Pending work:

• Support the different structures for redpanda.seed_servers
• Support a list for redpanda.kafka_api
• Support names for redpanda.kafka_api & redpanda.advertised_kafka_api items.

The above changes will be made in a follow-up PR, so that this one can be merged faster to unblock other work that depends on it.

I'm not completely sure that this feature is useful without name: and support in kafka_api: but i may not be familiar enough with the use case.

If it helps I'm not sure there is any value in ever writing the original non-list format (newer versions of redpanda will read the older version) which might simplify compatibility handling in rpk.

[0x5d]

@dotnwat I hadn't seen 15795ed#diff-029da2340cdf2c65a04e8cda2e44dc6bea00e5be5d643429be7050bcfa408a43 when I started this, but it should be easy enough to support name in the config file.
For rpk redpanda start's --seeds flag. I will go with the format used by Kafka: <name>://<host>:<port>.

[BenPope]

For rpk redpanda start's --seeds flag. I will go with the format used by Kafka: <name>://<host>:<port>.

Nice. Any reason not to use that format for the named versions of kafka_[advertised_]api as well?

I think you'll be able to use an URI parser, which might simplify some parser code.

[0x5d]

For rpk redpanda start's --seeds flag. I will go with the format used by Kafka: <name>://<host>:<port>.

Nice. Any reason not to use that format for the named versions of kafka_[advertised_]api as well?

I'm sorry! I meant that exactly. Seeds don't have names 🤦🏽‍♂️

I think you'll be able to use an URI parser, which might simplify some parser code.

Done!

[BenPope]

I think it's worth considering:

func MakeNamedSocketAddress(name, host, port) NamedSocketAddress

Firstly to reduce the syntactic noise, and secondly to help potential refactoring, if, say, we wanted nice JSON serialisation and deserialisation the struct would have to be manually flattened. Maybe also a MakeUnnamedNamedSocket of some sort, but naming is hard, so maybe use the builder pattern:

sa := SocketAddress{"localhost", 80};
nsa1 := BuildNamedSocketAddress().Name("internal"),SocketAddress(sa).Build();
nsa2 := BuildNamedSocketAddress().HostAndPort("localhost",80).Build();

But that might not be helping much with the syntactic noise.

Maybe just manually flatten it and potentially add a convenience member if required:

func (nsa NamedSocketAddress) AsSocketAddress() SocketAddress

Probably worth validating []NamedSocketAddress for unique names.

But now that I think about it, perhaps a map[string]SocketAddress is a more appropriate internal structure than []NamedSocketAddress, but that makes serde a little more manual and verbose.

[dotnwat]

@0x5d has this moved passed the draft stage?

[0x5d]

@0x5d has this moved passed the draft stage?

@dotnwat Nope, I'm still fixing some tests!

[0x5d]

I removed all the reviewers while I tidy things up a bit.

[0x5d]

Closing this to prevent spamming the participants.