Return default value for role alias_name_source instead of empty string #134
Conversation
Field alias_name_source is missing if role was created with Vault prior version 1.9.0. When reading stored role back, the go struct entry gets initialized to "" and user will be presented role with "alias_name_source": "" in HTTP response, which is invalid value. This change returns the default value instead of empty string. Signed-off-by: Tero Saarni <tero.saarni@est.tech>
|
Cc @benashz |
path_role.go
Outdated
| if role.AliasNameSource != "" { | ||
| d["alias_name_source"] = role.AliasNameSource | ||
| } else { | ||
| d["alias_name_source"] = aliasNameSourceDefault | ||
| } |
There was a problem hiding this comment.
Very much a nit suggestion, but this is quite a common pattern when dealing with default values IME, so just makes it a tad easier to read.
| if role.AliasNameSource != "" { | |
| d["alias_name_source"] = role.AliasNameSource | |
| } else { | |
| d["alias_name_source"] = aliasNameSourceDefault | |
| } | |
| d["alias_name_source"] = aliasNameSourceDefault | |
| if role.AliasNameSource != "" { | |
| d["alias_name_source"] = role.AliasNameSource | |
| } |
| @@ -311,3 +311,70 @@ func TestPath_Delete(t *testing.T) { | |||
| t.Fatalf("Unexpected resp data: expected nil got %#v\n", resp.Data) | |||
| } | |||
| } | |||
|
|
|||
| func TestPath_Migration(t *testing.T) { | |||
| t.Fatalf("err:%s resp:%#v\n", err, resp) | ||
| } | ||
|
|
||
| // Writing the role back should succeed. |
There was a problem hiding this comment.
Just to tighten the test a little, is it maybe worth deserialising resp.Data into roleStorageEntry as well so that we can assert AliasNameSource is set to the default we would expect?
There was a problem hiding this comment.
Good suggestion, now fixed!
|
I've discussed this internally with the team, and there was some concern about returning data that doesn't match what's in storage. We're planning to open a different PR to make |
|
Yes, that would solve it as well |
|
OK great, sorry for the late change of tack. |
|
@tomhjp One question though: One alternative that I was pondering about while writing #133 was not to return diff --git a/path_role.go b/path_role.go
index ea64e69..2f6ed7d 100644
--- a/path_role.go
+++ b/path_role.go
@@ -182,7 +182,6 @@ func (b *kubeAuthBackend) pathRoleRead(ctx context.Context, req *logical.Request
if role.NumUses > 0 {
d["num_uses"] = role.NumUses
}
- d["alias_name_source"] = aliasNameSourceDefault
if role.AliasNameSource != "" {
d["alias_name_source"] = role.AliasNameSource
}In that case empty string would not necessarily need to be accepted, and the public documentation would not necessarily need to change. So in a way, maybe impacts are less but it still would fix my problem of reading and re-applying the config. But for me any approach is completely ok :) |
Overview
New field
alias_name_sourcewas introduced to Role in Vault 1.9.0. If Role was created with older version of Vault, the field is missing from the persisted config in storage. When user sends "Read Role" request to read the stored Role with new Vault version, it will be returned with invalid value"alias_name_source": "". The reason is that the corresponding Go struct gets initialized with empty string when deserializing the stored JSON without the field.Design of Change
This change returns the default value
"serviceaccount_uid"instead of empty string, which makes the returned config data valid (e.g. it can be applied back in "Create Role" request).Related Issues
Fixes #133
Contributor Checklist
[N/A] Add relevant docs to upstream Vault repository, or sufficient reasoning why docs won’t be added yet
[N/A] Add output for any tests not ran in CI to the PR description (eg, acceptance tests)
[X] Backwards compatible