Skip to content

Commit

Permalink
rpk: fix mixed use of backcompat flags with -X
Browse files Browse the repository at this point in the history 
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 16835f6)
  • Loading branch information
@r-vasquez @vbotbuildovich
r-vasquez authored and vbotbuildovich committed Jan 29, 2024
1 parent db9d704 commit cd9ce83
Show file tree
Hide file tree
Showing 3 changed files with 54 additions and 15 deletions.
30 changes: 15 additions & 15 deletions src/go/rpk/pkg/config/params.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -648,52 +648,52 @@ func (p *Params) InstallCloudFlags(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...)
}
}

Expand Down
15 changes: 15 additions & 0 deletions tests/rptest/clients/rpk.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -326,6 +326,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,
Expand Down
24 changes: 24 additions & 0 deletions tests/rptest/tests/rpk_acl_test.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

0 comments on commit cd9ce83

Please sign in to comment.