Support specifying period when creating EC2 roles #140
Conversation
|
This is exciting! I'll get #109 in for you, and see if I can see whats up with the tests |
|
Okay @otakup0pe I finally updated this branch and added testing around token lifespan. This should be 🚢-able if it looks good to you. |
hvac/v1/__init__.py
Outdated
| @@ -764,8 +765,18 @@ def create_ec2_role(self, role, bound_ami_id=None, bound_account_id=None, bound_ | |||
| params['bound_iam_instance_profile_arn'] = bound_iam_instance_profile_arn | |||
| if role_tag is not None: | |||
| params['role_tag'] = role_tag | |||
| if ttl is not None: | |||
There was a problem hiding this comment.
From what I've seen, Vault is going to treat a None (null once its dumped into JSON) value the same as its default value of 0. How would you feel about dropping all this "if is not None" boilerplate and instead just including them in the initial params dictionary declaration?
There was a problem hiding this comment.
Alternatively, just set the default values we're setting here in the method signature.
hvac/v1/__init__.py
Outdated
| @@ -744,8 +744,9 @@ def list_vault_ec2_certificate_configurations(self): | |||
| return self._get('/v1/auth/aws-ec2/config/certificates', params=params).json() | |||
|
|
|||
| def create_ec2_role(self, role, bound_ami_id=None, bound_account_id=None, bound_iam_role_arn=None, | |||
| bound_iam_instance_profile_arn=None, role_tag=None, max_ttl=None, policies=None, | |||
| allow_instance_migration=False, disallow_reauthentication=False, **kwargs): | |||
| bound_iam_instance_profile_arn=None, role_tag=None, ttl=None, max_ttl=None, | |||
There was a problem hiding this comment.
I don't have a obvious suggestion for how to avoid this, but its worth nothing that adding in these new kwargs in the middle of the method's parameters will break backwards compatibility in some cases. I'm suppose that is something we can just note in the changelog 🤷♂️.
|
I ended up feeling like this is good enough to ship as-is after inadvertently trying out some convergent changes in #195. I went ahead and merged in master to resolve some of the merge conflicts. I also went a tad bit further and finished filling in the remaining EC2-related AWS auth backend parameters for this route so we could drop the catchall I pushed those changes up to your branch @ianwestcott so we could keep it all in this existing PR, hope ya don't mind! |
|
Don't mind a bit. @jeffwecan thanks for picking up my slack and getting this merged! |
Note: This includes (and builds on) changes in #109, so that PR should be merged first.
Adds a
periodargument to thecreate_ec2_role()function, which allows for the creation of tokens without a max TTL (a.k.a. "periodic tokens". Also makes parameters for TTL, max TTL and period explicit to ensure they're properly written on update.TODO
Write tests for: