Fix for Issue 11863 - Panic when creating/updating approle role with token_type #11864
Conversation
…to add warning for default-service or default-batch token type. Also adding guard around code that sets resp to a new logical.Response further on in the function.
|
|
| "token_max_ttl": 5000, | ||
| "token_type": "default-service", | ||
| } | ||
| roleReq.Data = roleData |
There was a problem hiding this comment.
It would be nice to turn these into table-based tests so that the inputs for each test case can be treated as immutable and make it a bit clearer to see exactly what each test's inputs are.
There was a problem hiding this comment.
Do you mean for just the various cases within this test function? The table-based tests are what I'm more used to doing, so I don't mind giving it a go. Seems like this test function could be combined with the TestAppRole_RoleCRUD function at the very least.
changelog/11863.txt
Outdated
| @@ -0,0 +1,3 @@ | |||
| ```release-note:bug | |||
There was a problem hiding this comment.
Thanks for adding a changelog note - one small update though, the number in the file name should actually be the PR number.
There was a problem hiding this comment.
My bad. I'll fix that right away.
| } | ||
|
|
||
| var actualStruct roleStorageEntry | ||
| err = mapstructure.Decode(resp.Data, &actualStruct) |
There was a problem hiding this comment.
I may be missing something obvious, but can you skip decoding and just compare the two map[string]interface{} values? (along with setting the RoleID still)
There was a problem hiding this comment.
To be honest, this test function is just a copy of the above test function TestAppRole_RoleWithTokenBoundCIDRsCRUD and then I removed the unnecessary bits.
There was a problem hiding this comment.
I also don't really know why the tests go through that additional mapstructure decode operation for the comparison. If I were to venture a guess, I suppose it does ensure that the fields being compared (some fields are being compared directly aside from the DeepEquals comparison) are properly tagged in the struct definition. That said, since it's clearly the established pattern in the test file, it feels wrong to break from the norm in a single test function.
|
@marcboudreau The changes looks good. The comments Tom left are very valid. However, I'd understand if you prefer to work on refactoring as a subsequent new PR after this one gets in. Up to you. Let us know. |
|
@vishalnayak I'm willing to take a stab at refactoring the test code a bit. If it turns into a rabbit hole, we can backtrack and go with this original PR. |
|
On second thought @vishalnayak, I'd like to keep the PR as is. I've looked at the code and found that there is definitely some repetition among a few of the test functions, namely around creating the role, and updating it, but then there's also quite a bit of unique quarks. Like in the TestAppRole_RoleWithTokenBoundCIDRsCRUD. |
Totally understandable. I've seen that part of the code and had a feeling that you might hit the thoughts you did. Thanks for the submission! Hoping to see the refactoring PR from you as well, but no rush. |
This PR offers a fix for Issue #11863. The cause of the panic was that the variable resp was declared but not initialized. When the warning was added if the token_type provided to the create or update request was either
default-serviceordefault-batch.This change creates a new logical.Response instance and assigns its address to the resp variable prior to either cases where the warning is added because of the token_type value. Since those cases are exclusive, there's no possibility that the resp variable be anything but nil at either of those points.
The only other location in the function where the resp variable is used is to add the warning about the max_ttl exceeding the mount's tuned max_ttl. In this case, the code was creating a new logical.Response. I've added a condition around it, so that if a new instance has already been created and a warning about the token_type added, the same instance gets reused.