AWS add user_path option for role. #6380
Conversation
|
|
There was a problem hiding this comment.
Hey @povils -- sorry for the delay here, was out of town for a few days. Mostly looking good!
I would also suggest adding some tests for this feature; I think it should be quick. Just update the existing acceptance tests to create a user in a particular path and then verify the user got created in the given path.
One small point -- there's a lot of noise in viewing the diff because it looks like your editor has made a bunch of whitespace changes to the CHANGELOG. It's much easier to review if these are kept separate, and I've also found the Vault team tends to update the CHANGELOG themselves (so, e.g., if a PR doesn't make a given release, the PR doesn't need to be updated to specify the new release).
|
Also, I want to add my normal disclaimer so I don't give anybody the wrong impression -- I am not a HashiCorp employee and I don't have merge power for PRs, so the opinions I express here are my own personal ones and don't represent official views of HashiCorp. I try to give feedback to make Vault a better product, but agreeing with me isn't a guarantee that your PR will get merged; likewise, disagreeing with me doesn't guarantee that it won't get merged. |
|
Hey @joelthompson, many thanks for your feedback! |
|
@povils this PR looks great! Thanks for submitting it. I'd be comfortable moving forward with it now if it just contained the fix for #2318 . We could either keep the two fixes together and wait until we've reached a good place with the other one, or if you don't want to wait, they could be split into separate PRs to get the one moving forward. Up to you! |
|
@tyrannosaurus-becks thank you for your input! |
There was a problem hiding this comment.
Hi @povils !
I tested this today! It's looking really good. I found that if you don't provide the user_path parameter, it behaves exactly as before. However, if you do provide user_path=/foo/user/, in the AWS console you get that exact path in the AWS console, as well as a user ARN like arn:aws:iam::<redacted account number>:user/foo/user/vault-root-my-role-1555613099-2800.
I do have one last request! Regex's are notorious for being on the more error-prone and hard-to-understand side. Would you be willing to add a couple tests of the regex pattern alone that simply show a couple of strings that should be accepted by it, as well as a couple of strings that should be rejected by it? Just to make sure it's easy to grok and maintain.
|
Hey @tyrannosaurus-becks ! Thanks for testing it. I added the test you asked for :) |
user_pathoption when creating role. Fixes feature request: make AWS IAM user prefix configurable #2318