rpk: use redpanda.admin if rpk.admin_api is nil #6955

r-vasquez · 2022-10-26T17:15:54Z

Cover letter

Previously rpk will use rpk.admin_api.addresses to communicate with admin API, if that field isn't set, rpk will use the default 127.0.0.1:9644

That's a problem if we are running in non-default port or address, now, rpk will use redpanda.admin if rpk.admin_api.addresses is not set

Fixes #2752, Fixes #6860

Backport Required

papercut/not impactful enough to backport
v22.2.x
v22.1.x
v21.11.x

UX changes

Let's say you have this config file:

redpanda:
    ...
    admin:
        - address: 1.2.3.4   # non-default address
          port: 9645            # non-default port
    developer_mode:  true
    auto_create_topics_enabled: true
    group_topic_partitions: 3
    storage_min_free_bytes: 10485760
    topic_partitions_per_shard: 1000
rpk:
    tune_disk_irq: true
    ... # no rpk.admin_api set.

Running rpk cluster health will give you an error like this::

* unable to fetch metrics from the admin API: Get "http://127.0.0.1:9644/...

The above happened because rpk will use the default host:port instead of using the one set in redpanda.admin property, now, rpk will use redpanda.admin (1.2.3.4:9645) in this same scenario.

Release notes

Improvements

rpk now will use admin address from redpanda.admin in the redpanda.yaml file if rpk.admin_api.addresses is empty.
rpk will now use broker addresses from redpanda.kafka_api in the redpanda.yaml file if rpk.kafka_api.brokers is empty

jcsp · 2022-10-26T17:37:58Z

src/go/rpk/pkg/config/params.go

+	defer func() { r.AdminAPI.Addresses = addrs }()
+	if len(addrs) == 0 && len(c.Redpanda.AdminAPI) > 0 {
+		for _, adminAPI := range c.Redpanda.AdminAPI {
+			addrs = append(addrs, net.JoinHostPort(adminAPI.Address, strconv.Itoa(adminAPI.Port)))


This should probably have a preference about what listener to use.

The admin API can have mTLS on a per-listener basis: if someone has configured an insecure listener on 127.0.0.1, then that is probably the one that rpk wants to use.

If we want to go this route, we can use https://pkg.go.dev/net#IP.IsPrivate and sort that one first.

Check https://github.com/redpanda-data/redpanda/compare/f53770465bcc183c7108466d93f4799b73cb3cd0..1e169a554d00fc10045c90036a78e1eb635b56bd

src/go/rpk/pkg/config/params.go

r-vasquez · 2022-10-26T20:10:15Z

Force Push:

Sort admin API addresses
Unit test for this method

jcsp · 2022-10-26T20:54:37Z

src/go/rpk/pkg/config/params.go

+	if len(addrs) == 0 && len(c.Redpanda.AdminAPI) > 0 {
+		// We want to order the admin API addresses by:
+		// localhost -> loobpack -> private -> public.
+		var localhost, loopback, private, public []string


The address order looks right, I would recommend also ordering non-mTLS endpoints first

addressed -- non-TLS is sorted first, and if we see any non-TLS, we avoid TLS listeners entirely

src/go/rpk/pkg/config/params_test.go

jcsp

LGTM, looks like there is a go linter complaining though

Previously rpk will use rpk.admin_api.addresses to communicate with admin API, if that field isn't set, rpk will use the default 127.0.0.1:9644 That's a problem if we are running in non-default port or address, now, rpk will use redpanda.admin _if_ rpk.admin_api.addresses is not set

The prior commits introduce behavior to use redpanda.admin if rpk.admin_api is empty. We previously had this same behavior for rpk.kafka_api, but this only used redpanda.kafka_api[0]. We abstract the logic from the previous commit (and have a authn => no authn helper) to make using the prior commit just as simple for brokers. We also do *not* use TLS listeners if we have any non-TLS listeners: rpk likely cannot fill client certs, and they may not be on the host. The expected behavior is that a person ssh's into the node and then runs rpk; localhost non-TLS communication will work. The new abstraction is well setup for when we add support for pandaproxy and schemaregistry to rpk, both of which use the same types. This adds testing for all this behavior as well.

Improving on the prior commit, we also now prefer tls over mtls. *If* we have to use tls, we are more likely able to communicate with brokers that do not require client certs (because we cannot setup client certs).

admin api's PrometheusMetrics actually requires one host to send to (i.e. you must pick the one host you want metrics from). Our tests previously passed because rpk.admin_api was empty and then we used the default 127.0.0.1:9644. The tests now failed because redpanda.admin was non-empty and we picked all three brokers in it. Because we favor the most-likely-accessible host first, we can just pick the first host in the config.

See embedded comment. A server listening on 0.0.0.0 is not an IP we can dial, but it does result in us being able to dial 127.0.0.1.

twmb · 2022-10-28T18:05:59Z

Failure: #6745

r-vasquez requested a review from jcsp October 26, 2022 17:15

r-vasquez requested review from twmb and 0x5d as code owners October 26, 2022 17:15

github-actions bot added the area/rpk label Oct 26, 2022

twmb previously approved these changes Oct 26, 2022

View reviewed changes

jcsp reviewed Oct 26, 2022

View reviewed changes

r-vasquez dismissed twmb’s stale review via 1e169a5 October 26, 2022 20:09

r-vasquez force-pushed the discover-admin-api-in-redpanda branch from f537704 to 1e169a5 Compare October 26, 2022 20:09

r-vasquez force-pushed the discover-admin-api-in-redpanda branch 2 times, most recently from 8d6c0f3 to 250cd13 Compare October 26, 2022 20:19

r-vasquez requested review from jcsp and twmb October 26, 2022 20:20

jcsp reviewed Oct 26, 2022

View reviewed changes

twmb reviewed Oct 27, 2022

View reviewed changes

src/go/rpk/pkg/config/params_test.go Show resolved Hide resolved

twmb reviewed Oct 27, 2022

View reviewed changes

src/go/rpk/pkg/config/params_test.go Outdated Show resolved Hide resolved

jcsp previously approved these changes Oct 27, 2022

View reviewed changes

twmb dismissed jcsp’s stale review via b8e99b9 October 27, 2022 17:27

twmb force-pushed the discover-admin-api-in-redpanda branch 2 times, most recently from b8e99b9 to f551082 Compare October 27, 2022 21:39

r-vasquez and others added 2 commits October 27, 2022 15:41

config: sort non-TLS first when defaulting to redpanda configs

7770a24

jcsp previously approved these changes Oct 28, 2022

View reviewed changes

twmb dismissed jcsp’s stale review via fe0f808 October 28, 2022 15:16

twmb force-pushed the discover-admin-api-in-redpanda branch from ee8ceb0 to fe0f808 Compare October 28, 2022 15:16

twmb added 4 commits October 28, 2022 09:18

rpk config: prefer tls over mtls

2c08895

Improving on the prior commit, we also now prefer tls over mtls. *If* we have to use tls, we are more likely able to communicate with brokers that do not require client certs (because we cannot setup client certs).

rpk config: rewrite redpanda's 0.0.0.0 to rpk's 127.0.0.1

fe0f808

See embedded comment. A server listening on 0.0.0.0 is not an IP we can dial, but it does result in us being able to dial 127.0.0.1.

twmb approved these changes Oct 28, 2022

View reviewed changes

piyushredpanda merged commit 5d51cc6 into redpanda-data:dev Oct 28, 2022

r-vasquez deleted the discover-admin-api-in-redpanda branch November 9, 2022 18:44

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rpk: use redpanda.admin if rpk.admin_api is nil #6955

rpk: use redpanda.admin if rpk.admin_api is nil #6955

r-vasquez commented Oct 26, 2022 •

edited by twmb

Loading

jcsp Oct 26, 2022

twmb Oct 26, 2022

r-vasquez Oct 26, 2022

r-vasquez commented Oct 26, 2022

jcsp Oct 26, 2022

twmb Oct 27, 2022

jcsp left a comment

twmb commented Oct 28, 2022

rpk: use redpanda.admin if rpk.admin_api is nil #6955

rpk: use redpanda.admin if rpk.admin_api is nil #6955

Conversation

r-vasquez commented Oct 26, 2022 • edited by twmb Loading

Cover letter

Backport Required

UX changes

Release notes

Improvements

jcsp Oct 26, 2022

Choose a reason for hiding this comment

twmb Oct 26, 2022

Choose a reason for hiding this comment

r-vasquez Oct 26, 2022

Choose a reason for hiding this comment

r-vasquez commented Oct 26, 2022

jcsp Oct 26, 2022

Choose a reason for hiding this comment

twmb Oct 27, 2022

Choose a reason for hiding this comment

jcsp left a comment

Choose a reason for hiding this comment

twmb commented Oct 28, 2022

r-vasquez commented Oct 26, 2022 •

edited by twmb

Loading