From dcddfb375e30214a0c40482025ab979bbd6f2c37 Mon Sep 17 00:00:00 2001 From: Rogger Vasquez Date: Wed, 24 Jan 2024 11:41:43 -0800 Subject: [PATCH] rpk: fix mixed use of backcompat flags with -X We were giving priority to old back compatibility flags (like --user, --password) over the new configuration (-X) flags. So if a user used both, the old ones got priority. This introduced a bug in rpk acl user create where we have a flag that collides with an old flag name: --password. (cherry picked from commit 16835f692a3123f1d758709f1ea2b5100a35fb37) --- src/go/rpk/pkg/config/params.go | 30 +++++++++++++++--------------- tests/rptest/clients/rpk.py | 15 +++++++++++++++ tests/rptest/tests/rpk_acl_test.py | 24 ++++++++++++++++++++++++ 3 files changed, 54 insertions(+), 15 deletions(-) diff --git a/src/go/rpk/pkg/config/params.go b/src/go/rpk/pkg/config/params.go index 9fd95169d040..2c31a0540c64 100644 --- a/src/go/rpk/pkg/config/params.go +++ b/src/go/rpk/pkg/config/params.go @@ -822,52 +822,52 @@ func (p *Params) InstallFormatFlag(cmd *cobra.Command) { func (p *Params) backcompatFlagsToOverrides() { if len(p.brokers) > 0 { - p.FlagOverrides = append(p.FlagOverrides, fmt.Sprintf("%s=%s", xKafkaBrokers, strings.Join(p.brokers, ","))) + p.FlagOverrides = append([]string{fmt.Sprintf("%s=%s", xKafkaBrokers, strings.Join(p.brokers, ","))}, p.FlagOverrides...) } if p.user != "" { - p.FlagOverrides = append(p.FlagOverrides, fmt.Sprintf("%s=%s", xKafkaSASLUser, p.user)) + p.FlagOverrides = append([]string{fmt.Sprintf("%s=%s", xKafkaSASLUser, p.user)}, p.FlagOverrides...) } if p.password != "" { - p.FlagOverrides = append(p.FlagOverrides, fmt.Sprintf("%s=%s", xKafkaSASLPass, p.password)) + p.FlagOverrides = append([]string{fmt.Sprintf("%s=%s", xKafkaSASLPass, p.password)}, p.FlagOverrides...) } if p.saslMechanism != "" { - p.FlagOverrides = append(p.FlagOverrides, fmt.Sprintf("%s=%s", xKafkaSASLMechanism, p.saslMechanism)) + p.FlagOverrides = append([]string{fmt.Sprintf("%s=%s", xKafkaSASLMechanism, p.saslMechanism)}, p.FlagOverrides...) } if p.enableKafkaTLS { - p.FlagOverrides = append(p.FlagOverrides, fmt.Sprintf("%s=%t", xKafkaTLSEnabled, p.enableKafkaTLS)) + p.FlagOverrides = append([]string{fmt.Sprintf("%s=%t", xKafkaTLSEnabled, p.enableKafkaTLS)}, p.FlagOverrides...) } if p.kafkaCAFile != "" { - p.FlagOverrides = append(p.FlagOverrides, fmt.Sprintf("%s=%s", xKafkaCACert, p.kafkaCAFile)) + p.FlagOverrides = append([]string{fmt.Sprintf("%s=%s", xKafkaCACert, p.kafkaCAFile)}, p.FlagOverrides...) } if p.kafkaCertFile != "" { - p.FlagOverrides = append(p.FlagOverrides, fmt.Sprintf("%s=%s", xKafkaClientCert, p.kafkaCertFile)) + p.FlagOverrides = append([]string{fmt.Sprintf("%s=%s", xKafkaClientCert, p.kafkaCertFile)}, p.FlagOverrides...) } if p.kafkaKeyFile != "" { - p.FlagOverrides = append(p.FlagOverrides, fmt.Sprintf("%s=%s", xKafkaClientKey, p.kafkaKeyFile)) + p.FlagOverrides = append([]string{fmt.Sprintf("%s=%s", xKafkaClientKey, p.kafkaKeyFile)}, p.FlagOverrides...) } if len(p.adminURLs) > 0 { - p.FlagOverrides = append(p.FlagOverrides, fmt.Sprintf("%s=%s", xAdminHosts, strings.Join(p.adminURLs, ","))) + p.FlagOverrides = append([]string{fmt.Sprintf("%s=%s", xAdminHosts, strings.Join(p.adminURLs, ","))}, p.FlagOverrides...) } if p.enableAdminTLS { - p.FlagOverrides = append(p.FlagOverrides, fmt.Sprintf("%s=%t", xAdminTLSEnabled, p.enableAdminTLS)) + p.FlagOverrides = append([]string{fmt.Sprintf("%s=%t", xAdminTLSEnabled, p.enableAdminTLS)}, p.FlagOverrides...) } if p.adminCAFile != "" { - p.FlagOverrides = append(p.FlagOverrides, fmt.Sprintf("%s=%s", xAdminCACert, p.adminCAFile)) + p.FlagOverrides = append([]string{fmt.Sprintf("%s=%s", xAdminCACert, p.adminCAFile)}, p.FlagOverrides...) } if p.adminCertFile != "" { - p.FlagOverrides = append(p.FlagOverrides, fmt.Sprintf("%s=%s", xAdminClientCert, p.adminCertFile)) + p.FlagOverrides = append([]string{fmt.Sprintf("%s=%s", xAdminClientCert, p.adminCertFile)}, p.FlagOverrides...) } if p.adminKeyFile != "" { - p.FlagOverrides = append(p.FlagOverrides, fmt.Sprintf("%s=%s", xAdminClientKey, p.adminKeyFile)) + p.FlagOverrides = append([]string{fmt.Sprintf("%s=%s", xAdminClientKey, p.adminKeyFile)}, p.FlagOverrides...) } if p.cloudClientID != "" { - p.FlagOverrides = append(p.FlagOverrides, fmt.Sprintf("%s=%s", xCloudClientID, p.cloudClientID)) + p.FlagOverrides = append([]string{fmt.Sprintf("%s=%s", xCloudClientID, p.cloudClientID)}, p.FlagOverrides...) } if p.cloudClientSecret != "" { - p.FlagOverrides = append(p.FlagOverrides, fmt.Sprintf("%s=%s", xCloudClientSecret, p.cloudClientSecret)) + p.FlagOverrides = append([]string{fmt.Sprintf("%s=%s", xCloudClientSecret, p.cloudClientSecret)}, p.FlagOverrides...) } } diff --git a/tests/rptest/clients/rpk.py b/tests/rptest/clients/rpk.py index 98f05b969488..b1825d71ad2f 100644 --- a/tests/rptest/clients/rpk.py +++ b/tests/rptest/clients/rpk.py @@ -443,6 +443,21 @@ def sasl_create_user_basic(self, return self._run(cmd) + def sasl_create_user_basic_mix(self, + new_username, + auth_user="", + auth_password="", + new_password="", + mechanism="SCRAM-SHA-256"): + cmd = [ + "acl", "user", "create", new_username, "--password", new_password, + "--mechanism", mechanism, "-X", + "admin.hosts=" + self._redpanda.admin_endpoints() + ] + cmd += ["-X", "user=" + auth_user, "-X", "pass=" + auth_password] + + return self._run(cmd) + def sasl_update_user(self, user, new_password): cmd = [ "acl", "user", "update", user, "--new-password", new_password, diff --git a/tests/rptest/tests/rpk_acl_test.py b/tests/rptest/tests/rpk_acl_test.py index 506ad8d57e65..5605b7406629 100644 --- a/tests/rptest/tests/rpk_acl_test.py +++ b/tests/rptest/tests/rpk_acl_test.py @@ -178,3 +178,27 @@ def test_create_user_no_pass(self): auth_password="any_pw", mechanism=self.mechanism) assert "Automatically generated password" not in out + + @cluster(num_nodes=1) + def test_back_compat_flags(self): + """ + This test ensures that we can use old flags ("--password") + mixed with new flags (-X pass, and --new-password) + """ + user_1 = "foo_6" + # This uses --user, --password, and --new-password + out = self._rpk.sasl_create_user_basic(new_username=user_1, + new_password="my_pass", + auth_user=self.username, + auth_password=self.password, + mechanism=self.mechanism) + assert f'Created user "{user_1}"' in out + + user_2 = "foo_7" + # This uses -X user, --password, and -X pass + out = self._rpk.sasl_create_user_basic_mix(new_username=user_2, + new_password="my_pass", + auth_user=self.username, + auth_password=self.password, + mechanism=self.mechanism) + assert f'Created user "{user_2}"' in out